Skip to content
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

Account self-deletion #652

Merged
merged 11 commits into from
Nov 12, 2024
Merged

Account self-deletion #652

merged 11 commits into from
Nov 12, 2024

Conversation

jsharkey13
Copy link
Member

This PR adds two new endpoints:

  • an endpoint to allow a logged in student with an account email address that is not DELIVERY_FAILED to request an account deletion link be sent to their email address;
  • a second endpoint to take the random token sent to the account email address and delete the user's account.

I have several questions about this implementation:

  • Tokens are valid for 7 days; I didn't want open-ended validity, but 24 hours like password resets seemed unnecessarily short. Is this okay?
  • You have to be logged in to use the deletion token; rather than include the user ID in the URL, just force the user to be logged in. If the email is sent right away, this is likely not an issue?
  • Is the email facade the right place for these endpoints to live? Could the manager level code be better located?

We always use the method from the password authenticator whenever we
do this check; there is no need for this method in this class.
This adds a new table for account deletion tokens, a new persistence
manager, an endpoint to request the token via, and adds assorted other
methods for generating secure random tokens and creating the emails.

There are still questions about whether the endpoint is in the right
place, and whether the methods should live elsewhere in the tree of
managers.
The endpoint confirms the deletion token is valid and then deletes the
logged-in user's account and clears the login cookies.
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 35.59322% with 76 lines in your changes missing coverage. Please review.

Project coverage is 34.39%. Comparing base (586d96e) to head (e94e293).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...e/dao/users/PgDeletionTokenPersistenceManager.java 37.83% 21 Missing and 2 partials ⚠️
...n/java/uk/ac/cam/cl/dtg/segue/api/EmailFacade.java 29.16% 17 Missing ⚠️
.../segue/api/managers/UserAuthenticationManager.java 31.57% 12 Missing and 1 partial ⚠️
.../cl/dtg/segue/api/managers/UserAccountManager.java 26.66% 11 Missing ⚠️
...m/cl/dtg/isaac/dos/users/AccountDeletionToken.java 52.94% 8 Missing ⚠️
...cam/cl/dtg/segue/auth/SegueLocalAuthenticator.java 0.00% 2 Missing ⚠️
...e/configuration/SegueGuiceConfigurationModule.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #652      +/-   ##
==========================================
+ Coverage   34.16%   34.39%   +0.23%     
==========================================
  Files         519      521       +2     
  Lines       23264    23378     +114     
  Branches     2854     2863       +9     
==========================================
+ Hits         7947     8042      +95     
- Misses      14515    14528      +13     
- Partials      802      808       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mwtrew mwtrew left a comment

Choose a reason for hiding this comment

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

Pair-reviewed with @axlewin.

The general approach looks good. I think I would lean towards a shorter expiry time, especially since we're requiring login on the basis that the user will probably act on it quickly.

I don't have a strong opinion about where the code lives, I would perhaps lean towards the user facade/manager so it's in the same place as the account creation logic but this is also fine.

Some integration tests for various scenarios (happy path, using another user's token, expired token) would be nice.

@jsharkey13
Copy link
Member Author

Let's bring the time down to 1 day, as for password resets. Then I will have a go at adding some integration tests.

@jsharkey13 jsharkey13 marked this pull request as ready for review November 11, 2024 16:18
@jsharkey13 jsharkey13 requested a review from mwtrew November 11, 2024 16:18
# Conflicts:
#	src/main/resources/db_scripts/postgres-rutherford-create-script.sql
#	src/test/resources/test-postgres-rutherford-data-dump.sql
@mwtrew mwtrew merged commit 65a72f8 into master Nov 12, 2024
3 checks passed
@mwtrew mwtrew deleted the feature/account-self-deletion branch November 12, 2024 09:51
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.

2 participants