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.

Sign up for GitHub

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

Jump to bottom

feat(auth): standalone enable oauth listener for MPAs #12731

Merged
merged 5 commits into from
Dec 23, 2023
Merged

feat(auth): standalone enable oauth listener for MPAs #12731

merged 5 commits into from
Dec 23, 2023

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Dec 20, 2023 •
edited
Loading

Description of changes

This PR introduces an importable and standalone side effect for enabling the OAuth listener for signInWithRedirect API in multipage app use cases.

Developers should be able to import 'aws-amplify/auth/enable-oauth-listener' in a route (as the signInWithRedirect redirect url) to complete the OAuth sign in flow.

  • Refactored the original signInWithRedirect implementation
    • Modularized the implementation to make the side effect part can be used independently
    • Mostly copy out and paste into new files
    • Fixed issues in the original impl. caught by writing new unit tests
  • Refactored the way how the listener is being triggered
    • Attached the listener to the Amplify singleton where whether the singleton has a valid OAuth config is the single trigger of the listener - this prevents the listener to be executed multiple times on repetitive calls of Amplify.configure when someone tries manual configurations
  • Refactored the auth unit tests suite
    • To avoid enabling the side effect in irrelevant tests
    • Rewrote unit tests for signInWithRedirect and side effect implementation

Issue #, if available

Description of how you validated changes

  • Unit tests
  • Local tests with
    • Next.js sample app
    • react-native sample app

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@HuiSF HuiSF requested review from a team as code owners December 20, 2023 14:45
@github-actions GitHub Actions
Copy link

⚠️ This PR includes manual changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

  • Does not modify "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library.

A repository administrator is required to review & merge this change.

HuiSF
HuiSF commented Dec 20, 2023
View reviewed changes
packages/core/src/singleton/Amplify.ts

/** @internal */
[ADD_OAUTH_LISTENER](listener: (authConfig: AuthConfig['Cognito']) => void) {
if (this.resourcesConfig.Auth?.Cognito.loginWith?.oauth) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't use the assertion function here to reduce bundle size impact...

packages/core/src/singleton/Amplify.ts

private notifyOAuthListener() {
if (
!this.resourcesConfig.Auth?.Cognito.loginWith?.oauth ||
Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto, Bundle size impact.

AllanZhengYP
AllanZhengYP reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

It looks good! Thank you @HuiSF

@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch from dc3e30b to 4a5313a Compare December 20, 2023 18:45
@HuiSF HuiSF requested a review from AllanZhengYP December 20, 2023 19:04
@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch from 4a5313a to e69f132 Compare December 20, 2023 20:15
AllanZhengYP
AllanZhengYP previously approved these changes Dec 20, 2023
View reviewed changes
@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch from e69f132 to cebf03d Compare December 20, 2023 23:00
@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch 2 times, most recently from 6711470 to 5d64565 Compare December 21, 2023 19:22
israx
israx previously approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@israx israx left a comment

Choose a reason for hiding this comment

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

LGTM! I smoked test these changes with a React App and works fine!

@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch from 5d64565 to b93ceb4 Compare December 21, 2023 20:29
@HuiSF HuiSF force-pushed the hui/fix/auth/oauth-map-app branch from b93ceb4 to df1957e Compare December 21, 2023 22:00
@HuiSF HuiSF requested a review from AllanZhengYP December 21, 2023 22:55
Ashish-Nanda
Ashish-Nanda approved these changes Dec 23, 2023
View reviewed changes
Copy link
Contributor

@Ashish-Nanda Ashish-Nanda left a comment

Choose a reason for hiding this comment

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

Approved. Lets release to the preview tag first for further testing and validation of this change.

@HuiSF HuiSF merged commit 94d1fb2 into main Dec 23, 2023
30 checks passed
@HuiSF HuiSF deleted the hui/fix/auth/oauth-map-app branch December 23, 2023 00:51
AllanZhengYP added a commit to AllanZhengYP/amplify-js that referenced this pull request Dec 26, 2023
@AllanZhengYP
Revert "feat(auth): standalone enable oauth listener for MPAs (aws-am…
e4f53e3 
…plify#12731)"

This reverts commit 94d1fb2.
AllanZhengYP added a commit that referenced this pull request Dec 26, 2023
@AllanZhengYP
revert(auth): standalone enable oauth listener for MPAs (#12753)
0462761 
* Revert "fix(auth): oAuthStore is used before a valid OAuth config is confirmed (#12748)"

This reverts commit ae64386.

* Revert "feat(auth): standalone enable oauth listener for MPAs (#12731)"

This reverts commit 94d1fb2.
@HuiSF HuiSF mentioned this pull request Dec 29, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllanZhengYP AllanZhengYP AllanZhengYP approved these changes

@Ashish-Nanda Ashish-Nanda Ashish-Nanda approved these changes

@kvramyasri7 kvramyasri7 kvramyasri7 approved these changes

@israx israx israx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HuiSF @AllanZhengYP @Ashish-Nanda @kvramyasri7 @israx