Skip to content

11 fix delete account flow #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Nov 28, 2024
Merged

Conversation

chriscarrollsmith
Copy link
Contributor

@chriscarrollsmith chriscarrollsmith commented Nov 24, 2024

The User model has a deleted column that we set to True if the user has deleted their account. So we need to make sure that in our auth endpoints and helper functions, we don't return deleted users (or allow deleted users to perform account actions). This approach is more complicated than actually deleting the user, but it has the following advantages:

  • Allows easier recovery of deleted data
  • Allows auditing of database history
  • Allows us to preserve rather than cascade-delete records with user_id as a required foreign key

However, here's the part I don't love:

  • If the user deletes their account data, they expect it to actually be deleted
  • If an attacker compromises deleted account data, users who thought they deleted that data are going to be mad
  • We could always just make user_id optional for any records we want to retain after user deletion

So, it's possible this is the wrong approach.

@chriscarrollsmith chriscarrollsmith linked an issue Nov 24, 2024 that may be closed by this pull request
@chriscarrollsmith
Copy link
Contributor Author

Changed my mind and went with a flow where we actually delete the user record.

  • Reverted my previous changes
  • Removed deleted attributes from all database models
  • Performed an actual delete in the delete_user endpoint
  • Cascaded the delete to dependent RolePermissionLink and PasswordResetToken errors

Additionally, in this PR:

  • Added Pydantic request models for a couple endpoints in user.py
  • Added tests for the user.py endpoints as well as a couple in main.py
  • Replaced the client fixture in the test suite with two separate fixtures, and authed_client and an unauthed_client
  • Changed get_authenticated_user in utils/auth.py so it uses a 303 rather than 307 HTTPException (because 303 changes POST requests to GET, whereas 307 redirects the original POST request to a different endpoint, which is not what we want for unauthorized POSTS)

@chriscarrollsmith
Copy link
Contributor Author

@AkanshuS, want to try your hand at code review? Good skill to know when applying for jobs!

Copy link
Collaborator

@AkanshuS AkanshuS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage is comprehensive, comments are easy to understand.

@chriscarrollsmith chriscarrollsmith merged commit 49b4468 into main Nov 28, 2024
2 checks passed
@chriscarrollsmith chriscarrollsmith deleted the 11-fix-delete-account-flow branch November 28, 2024 02:32
@chriscarrollsmith chriscarrollsmith self-assigned this Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "delete account" flow
2 participants