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

Make sure app has correct SignInType after upgrading #930

Merged
merged 5 commits into from
May 5, 2023

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented May 4, 2023

Warning
I think this PR merits a hotfix, so I have it targeting a new hotfix branch: release/7.37.1.

Description

This avoids scenarios where users migrating from a password SignInType to the token SIgnInType can get stuck in a bad state where the app will always fail to retrieve an updated access token because the app still thought the user was using a password SignInType so it would try to use the stored token as a password, which would fail. This causes the app to present notifications saying the user has been signed out, but the app still indicates the user is signed in apart from this error (and all the API calls would fail).

image

This PR essentially has two fixes for this issue...

  1. d13c31b makes it so that when a user signs in after getting one of these errors, the SignInType is updated appropriately. Before, the SignInType would not get updated because when we called addAccountExplicitly(...) the account already existed. In other words, this makes it so that even if a user gets one of these errors, the issue will be resolved when they sign in.
  2. db1fb96 avoids the issue arising in the first place by making sure that any time we store a refresh token instead of a password, we update the SignInType to be Tokens.

This PR also includes some minor improvements to our logging in this area.

Testing Instructions

1. Test that logging in clears the error

  1. Install a debugProd build from the 7.36 tag
  2. Log into the app
  3. Install a debugProd build from the release/7.37.1 branch, applying this patch to make the issue easy to recreate.
  4. Tap refresh from the "Profile" tab
  5. Observe a sign-in error notification is presented
  6. Tap on the sign-in error notification and sign into the same account you signed into on the 7.36 build
  7. Observe a new sign-in error notification is presented
  8. Install a debugProd build from the first commit in this PR (d13c31b) applying this patch. The reason for using this commit is because it will still allow the error to occur (the fix that avoids the error entirely is db1fb96) and allow you to test that signing in will now fix the error.
  9. Install that build
  10. Tap refresh from the "Profile" tab
  11. Observe there is a sign-in error notification
  12. Tap on the error notification and sign into the app using the same account you signed into on the 7.36 build
  13. ✅ Observe there is no sign-in error notification and that you can refresh the app from the "Profile" tab without any issues

2. Test that error is avoided entirely

  1. Install a debugProd build from the 7.36 tag
  2. Log into the app
  3. Install a debugProd build from this PR applying [this patch (https://gist.github.com/mchowning/0d4edda7247b536a5abccd20357cebb0).
  4. Start the app and perform a refresh.
  5. ✅ Observe there are no error notifications

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes

@@ -419,9 +419,8 @@ class SyncManagerImpl @Inject constructor(
private suspend fun downloadTokens(
email: String,
refreshToken: RefreshToken,
syncServerManager: SyncServerManager,
Copy link
Contributor Author

@mchowning mchowning May 4, 2023

Choose a reason for hiding this comment

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

This doesn't need to be passed because the class already has the SyncServerManager stored as a field.

@mchowning mchowning force-pushed the fix/sign-out-issues branch from d9c0e2f to ed52969 Compare May 4, 2023 20:25
@mchowning mchowning marked this pull request as ready for review May 4, 2023 20:31
@mchowning mchowning requested a review from a team as a code owner May 4, 2023 20:31
@ashiagr
Copy link
Contributor

ashiagr commented May 5, 2023

Looks good! It was a tricky one, great job in investigating and providing clear testing steps.

@ashiagr ashiagr merged commit e026e9a into release/7.37.1 May 5, 2023
@ashiagr ashiagr deleted the fix/sign-out-issues branch May 5, 2023 04:47
@ashiagr ashiagr added this to the 7.39 milestone May 5, 2023
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