Page MenuHome

Implement account deletion in Blender ID (step 1/3)
ClosedPublic

Authored by Anna Sirota (railla) on Jan 12 2021, 6:29 PM.

Details

Summary

The goal

Step 1 <- we are here

Having a user-facing UI for requesting account deletion to save time on doing this manually every time people ask for their Blender accounts to be deleted.
After deletion is requested, the following should happen Blender ID:

  1. user is deactivated → it's no longer possible to login into Blender ID or via Blender ID;
  2. existing account sessions are deleted (not implemented yet);
  3. date of deletion is saved for a possibility of a grace period, both for the sake of the person requesting deletion and the dependant applications;
  4. user-modified webhook is called and its payload contains the non-null deletion date.

Applications subscribing to the webhook handle this event at their own discretion.

Special considerations

Blender ID is an OAuth server, and it has no direct control over how applications, connected to it, store and delete their account data. Deleting Blender ID ≠ deleting all account data in all connected applications.
We'll be able to automate account deletion in *some* of them, starting with Blender Cloud, but definitely not most of them.

Step 2

Having a background task or a cronjob that handles deletion requests by anonymizing/deleting relevant data.

After the grace period is over, Blender ID can process deletion of its own data, e.g. delete all tokens and anonymize or delete the user record.

Step 3

Having Clouds handle deletion as well:

  • via a command in Cloudv3
  • automatically on user-modified in Cloudv4

What does it do

Frontend

Adds a "Danger zone" section at the bottom of the profile page, with a "Delete account" link to a "Delete account" page.

Changes to the database

bid_main_user.date_deletetion_requested column is added
bid_main_user.deletetion_requested column is removed

Backend

Adds a form with a confirmation checkbox, that, when submitted and valid, will

  • save current date-time as bid_main_user.date_deletetion_requested
  • deactivate current user's account
  • log out

date_deletetion_requested field is also added to the API's userinfo and user-modified webhook payload.

(task in progress: https://developer.blender.org/T84610)

Diff Detail

Repository
rBID Blender ID

Event Timeline

Anna Sirota (railla) requested review of this revision.Jan 12 2021, 6:29 PM
Anna Sirota (railla) created this revision.
Anna Sirota (railla) edited the summary of this revision. (Show Details)
Anna Sirota (railla) edited the summary of this revision. (Show Details)
Anna Sirota (railla) edited the summary of this revision. (Show Details)Jan 12 2021, 6:34 PM
Anna Sirota (railla) edited the summary of this revision. (Show Details)

Tests added

API/webhook tests updated with the new field

Minor fixes to templates

Anna Sirota (railla) edited the summary of this revision. (Show Details)Jan 13 2021, 5:38 PM
poetry.lock
1–2

This diff to poetry.lock was generated after adding freezegun, nothing else.

Anna Sirota (railla) edited the summary of this revision. (Show Details)Jan 14 2021, 10:35 AM
Anna Sirota (railla) edited the summary of this revision. (Show Details)
Anna Sirota (railla) edited the summary of this revision. (Show Details)Jan 14 2021, 10:38 AM
Sybren A. Stüvel (sybren) requested changes to this revision.Jan 14 2021, 10:43 AM

Nice work! Just a few tiny nags, apart from those it all looks good.

bid_api/signals.py
146–147

What's the reason the date_deletion_requested field is included at all when user.date_deletion_requested is None? Wouldn't it be nicer to just leave out the field entirely in that case?

bid_api/tests/test_webhooks.py
205

I didn't know freezegun, seems like a really useful tool!

It's easy to make mistakes with timezones, so I'd always suggest testing with non-zero ones. According to their docs freezegun.freeze_time('2021-01-01 01:02:03', tz_offset=-9) would do the trick.

bid_main/admin.py
124–126

I don't think you have to define this function above the regular attributes. If possible, move it down to the other functions.

bid_main/urls.py
21

There are multiple things deletable in Blender ID (like auth tokens / app associations). I don't really feel strongly about this, but maybe delete-my-account is a more explicit one? It'll also pop up on DuckDuckGo searches when people search for "blender delete account"; search engines love matches on the URL itself.

21

By inserting this rule here, the switch and switch/<switch_to> URLs are further apart from each other. It's because the latter exists that the former has an optional slash. I think the delete rule should either be above the two switch rules, or below them, but not in between. I also don't think there needs to be an optional slash in the URL, as there is no delete/xxx rule.

This revision now requires changes to proceed.Jan 14 2021, 10:43 AM
Anna Sirota (railla) marked 3 inline comments as done and an inline comment as not done.Jan 14 2021, 11:47 AM
Anna Sirota (railla) added inline comments.
bid_api/signals.py
146–147

I think I'm used to treating these as APIs, that is having a fixed schema, so adding or removing a field in the payload depending on data alone didn't cross my mind.
Other than not having to change tests, not sure what would be the benefit of this

bid_api/tests/test_webhooks.py
205

Tried that, but it gives strange results that don't match the offset from UTC (the formatted time is the same):

"date_deletion_requested": "2021-01-01T01:02:03+00:00",

Offset given as part of the time string @freeze_time('2021-01-01T01:02:03+02:00') works as expected, however:

"date_deletion_requested": "2020-12-31T23:02:03+00:00",

(There are a few of open issues with tz_offset)

bid_api/signals.py
146–147

The benefit is that the payload doesn't change w.r.t. older versions of Blender ID. The downside of this is that it requires better documentation (which wouldn't be bad in itself at all) to tell receivers of this data that there is an optional field.

Such optional fields in JSON come quite natural in Golang, where there is even an option called "omitempty" that causes this exact behaviour.

Now that we have dataclasses in Python, I tend to gravitate towards using those to define what's in the JSON document, and then feed dataclasses.asdict(theinstance) to json.dumps(). One of the advantages of this is that you have a clear place where these JSON document structures are defined, rather than passing along dicts. It also helps for type declarations. I think that a dataclass would also just include the None value, though.

I'll leave it up to you to make a final decision here, I don't feel that strongly about this.

bid_api/tests/test_webhooks.py
205

Eek, well I guess that shows that it's a good idea to test with timezones ;-)
Your current solution looks fine to me.

Anna Sirota (railla) marked 2 inline comments as done.Jan 14 2021, 12:07 PM
bid_api/signals.py
146–147

I'd prefer to keep it as it is now, considering that adding a new field usually doesn't cause problems: JSON validators/loaders, like Python's marshmallow or Golang's json.Unmarshal tend to ignore unknown fields in the payload. And it would indeed save having to explain why this behaviour exists in the first place.

This revision is now accepted and ready to land.Jan 14 2021, 12:30 PM