Log out user account after account deletion success #1395
Conversation
| guard let deletionController = accountDeletionController, | ||
| deletionController.hasValidDeletionRequest, | ||
| let user = simperium.user, | ||
| user.authenticated() else { |
There was a problem hiding this comment.
user.authenticated() will be false because we come here when user token is invalid, which means user is not authenticated.
We don't need this condition here and can safely remove it.
There was a problem hiding this comment.
After I removed user.authenticated() and tested the account deletion flow, I found another issue.
Calling [self.simperium signOutAndRemoveLocalData:YES completion:^{}]; doesn't do anything because the first line there is checking if user is authenticated. It will return false in our case because the token is not valid and singOut won't do anything because of that. This means that all the data is still stored in the core data and we failed to remove it.
https://github.com/Simperium/simperium-ios/blob/develop/Simperium/Simperium.m#L589
- (void)signOutAndRemoveLocalData:(BOOL)remove completion:(SimperiumSignoutCompletion)completion {
// Don't proceed, if the user isn't logged in
if (!self.user.authenticated) {
return;
}
...
The issue here is that simperium checks for self.user.authenticated which can be false, but we will still have the user object and credentials stored in the keychain and on the next app launch Simperium will try to use the token from the keychain. The check is quite misleading.
I suggest that we change if (!self.user.authenticated) to if (!self.user) or even fully remove the line. This should allow us to fully clean the state even if the user is not authenticated.
@jleandroperez WDYT?
There was a problem hiding this comment.
Simperium updated here. TY Evgeny!!
+1 yes let's just check if there's a valid deletion request pending.
Plus: we should point to the new Simperium branch (I'll make sure to release a new version once we know it's good!)
| guard let deletionController = accountDeletionController, | ||
| deletionController.hasValidDeletionRequest, | ||
| let user = simperium.user, | ||
| user.authenticated() else { |
There was a problem hiding this comment.
Simperium updated here. TY Evgeny!!
+1 yes let's just check if there's a valid deletion request pending.
Plus: we should point to the new Simperium branch (I'll make sure to release a new version once we know it's good!)
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
Alright, tested the simperium changes, and that does work. It created a new issue where the onboarding screen was loaded for a moment then it is dismissed, and then it is displayed again. This happens because of simperium.authenticationDidFail schedules an onboarding VC to appear and logout calls dismissAllModals and then also calls authenticateIfNecessary which schedules a new onboarding VC. I removed the call to dismiss all modals to fix the issue. Also I added a change that fixes #1393 (adding that in so it will auto close when this is merged) To test these changes with the simperium changes in the SNiOS Pod file comment out and add
|
| switch simperiumError { | ||
| case .invalidToken: | ||
| logOutIfAccountDeletionRequested() | ||
| break |
There was a problem hiding this comment.
No break needed here
|
I checked it out as well and it looks good to me @jleandroperez |
sir!
Fix
Per some discussions with in app account deletion for MacOS, we decided that it would be best to log out a user once the account deletion is complete, instead of simply displaying the onboarding. We did this on macOS to deal with some windowing bugs, which were not apparent on iOS, but could lead to being able to navigate the app after an account was deleted making it seem like the deletion had failed.
By logging the user out the local data is removed and there is no way to navigate the app.
Test
Review
(Required) Add instructions for reviewers. For example:
Release