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

fix(auth): Fix Device Metadata migration if alised userId was used #2963

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

tylerjroach
Copy link
Member

  • PR title and description conform to Pull Request guidelines.

Amplify v1 (or standalone AWS Android SDK) migrations to Amplify v2 do not migrate all required data in some cases. This results in token refresh failures.

Requirements for Issue:

  1. Device Tracking Enabled
  2. Sign in is completed with an alias that does not match the real Cognito assigned userId (ex: sign in with email)
  3. Amplify v1 Sign In completed with sign in method that supports device tracking (ex: SRP, not HostedUI)

Cause of Bug

Amplify v2 pulls "LastAuthUserId" from the legacy (v1) credential store. This value is the alias (ex: email address), not necessarily the actual userId. This results in a failed lookup for device metadata. Device metadata will not be migrated to the new credential store format, and the existing file remains untouched on the device.

Additional Bug Discovered

We are only migrating device metadata for the "LastAuthUserId". We shouldn't have this restriction.

Issue #, if available:
#2929

Description of changes:

We can pull a list of userIds (not aliased) with device metadata stored on the device, by scanning shared preference files.
We then iterate through each of those userIds, and lookup the legacy device metadata. If we do not currently have device metadata for the given userId in the Amplify v2 credential store, we will now properly migrate the device metadata from the legacy keystore.

How did you test these changes?
(Please add a line here how the changes were tested)

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Member

@harsh62 harsh62 left a comment

Choose a reason for hiding this comment

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

LGTM

@tylerjroach tylerjroach marked this pull request as ready for review December 13, 2024 13:33
@tylerjroach tylerjroach requested a review from a team as a code owner December 13, 2024 13:33
@tylerjroach tylerjroach merged commit de22fa8 into main Dec 13, 2024
4 checks passed
@tylerjroach tylerjroach deleted the tjroach/device-metadata-migration-fix branch December 13, 2024 13:33
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.

3 participants