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 issue where unauthenticated logins caused 401 because refresh caused new credentials to be created #2752

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

marcoatblueowl
Copy link
Contributor

@marcoatblueowl marcoatblueowl commented Apr 3, 2024

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:
#2751

Description of changes:
These changes were based off how amplify-swift state machine behaves for same scenario.
For the iOS SDK:
When a RefreshSession event occurs for an IdentityPool only, then a RefeshUnAuthAwsCredentials event is created with the identity ID. Refer to code here

Then when the event is processed in the state machine, it send an action to FetchAuthAWSCredentials with the passed identity ID and an empty login map. Refer to code here

However on Android SDK:
When a RefreshSession event occurs for an IdentityPool only, then a RefreshUnAuthSession event is created but the identity ID that the session needs to be refreshed for is not passed in. This is essentially where I believe we lose track of the anonymous user so when we create credentials, it creates it for a new user. Refer to code here

Then when the event is processed in the state machine, it sends an action to RefreshAuthSessionAction instead of FetchAWSCredentialsAction with the identity ID and empty login map. Refer to code here

The updates to this are to reflect the same logic that is expressed on the iOS SDK as going thru the same scenario there the correct identity ID is being kept and session is being refreshed as well. Please let me know if I misunderstood the logic of both SDKs and if more information is needed. Thanks

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.

@marcoatblueowl marcoatblueowl requested a review from a team as a code owner April 3, 2024 00:53
@mattcreaser
Copy link
Member

Thanks for posting this PR @marcoatblueowl! A member of our team will have a look and help move this PR along.

@joon-won joon-won self-assigned this Apr 11, 2024
@joon-won joon-won merged commit fd2fff9 into aws-amplify:main Apr 11, 2024
3 checks passed
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