Skip to content

Conversation

@mattcreaser
Copy link
Member

@mattcreaser mattcreaser commented May 9, 2025

Issue #, if available: #235

Description of changes:
Authenticator now reacts to SIGNED_IN events it receives from Amplify. Historically it didn't do this because the idea was that you would only use Authenticator for Sign In, and not use Amplify APIs directly, but #235 rightly points out that you currently would use Amplify for social sign in via Cognito's web UI, which would leave Authenticator in an incorrect state.

In this PR Authenticator listens to events from Amplify for when the user signs in. If the event is not expected (i.e. Authenticator is not currently in the process of calling a sign in API) then it reacts to the sign in by going directly to the SignedInState.

How did you test these changes?

  • Unit tests
  • Test app with programmatic sign in (Amplify.Auth.signIn)
  • Test app with WebUi sign in (Amplify.Auth.signInWithWebUI)
  • Test app with social sign in (Amplify.Auth.signInWithSocialWebUI)

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)
  • Ensure commit message has the appropriate scope (e.g fix(liveness): message, fix(authenticator): message, fix(all): message)

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

@mattcreaser
Copy link
Member Author

mattcreaser commented May 9, 2025

This is a Draft PR pending verification of the web UI and social web UI use cases. All test cases have been validated.

@codecov
Copy link

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 53.33333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 26.11%. Comparing base (8682849) to head (b5f1861).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
+ Coverage   25.90%   26.11%   +0.21%     
==========================================
  Files          95       95              
  Lines        4458     4468      +10     
  Branches      865      869       +4     
==========================================
+ Hits         1155     1167      +12     
+ Misses       3113     3109       -4     
- Partials      190      192       +2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mattcreaser mattcreaser marked this pull request as ready for review May 12, 2025 16:49
@mattcreaser mattcreaser requested review from a team as code owners May 12, 2025 16:49
@mattcreaser mattcreaser enabled auto-merge (squash) May 12, 2025 17:00
@mattcreaser mattcreaser merged commit 93c1245 into main May 12, 2025
4 checks passed
@mattcreaser mattcreaser deleted the mattcreaser/amplify-sign-in branch May 12, 2025 17:04
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