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

feat(auth): add option to disable idp oauth flow #13373

Open
wants to merge 3 commits into
base: v5-stable
Choose a base branch
from

Conversation

ashwinkumar6
Copy link
Contributor

@ashwinkumar6 ashwinkumar6 commented May 13, 2024

Description of changes

Amplify supports two types of oauth flows in V5 "IDP (initiated by provider)" and "SP (initiated by client)". Since IDP flow is initiated outside of Amplify we uses a global urlListener to handle the oauth response when the url query params contain code,access_token or error.

However even if the app isn't redirected from HostedUI but contains the the url query params code,access_token or error, the global urlListener is hit and the url is replaced with the redirectSignIn url

Solution

  • Add an option to Amplify.configure() to optionally disable IDP initiated flow to avoid calling the global urlListener
  • Detect if a SP initiated flow is in flight using a amplify-sp-initiated-oauth-inFlight flag in local storage
  • If SP initiated flow is in flight OR IDP initiated flow is enabled call the global urlListener to handle oauth response

Issue #, if available

Description of how you validated changes

// on sampleApp
import { Amplify } from "aws-amplify";
import awsconfig from "./aws-exports";

Amplify.configure({
  ...awsconfig,
  oauth: {
    ...awsconfig.oauth,
    idpEnabled: false, // defaults to true to avoid breaking change
  },
});

Tested the following flows

  • SP flow works as expected irrespective of setting idpEnabled in Amplify.configure
  • IDP flow works by default, idpEnabled: true and ignored when idpEnabled: false
  • The url query params aren't swallowed when idpEnabled: false

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.

@ashwinkumar6 ashwinkumar6 requested a review from a team as a code owner May 13, 2024 20:32
@ashwinkumar6 ashwinkumar6 marked this pull request as draft May 13, 2024 20:35
@@ -2437,6 +2444,7 @@ export class AuthClass {
? this._config.oauth.redirectSignIn
: this._config.oauth.redirectUri;

this._storage.setItem('amplify-sp-initiated-oauth-inFlight', 'true');
Copy link
Contributor

@israx israx May 14, 2024

Choose a reason for hiding this comment

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

is this flag set when Auth.federatedSignIn is called ?

if this issue is web specific, we probably can exclude RN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this flag is set when Auth.federatedSignIn is called. This is to determine if an SP initiated oauth is in progress.

The following flow is common for web and RN

  1. add a flag when Auth.federatedSignIn is called
  2. When redirected back from HostedUI, if ^^ flag is present allow urlListener to get fired, hence handling oauth
  3. remove flag from storage

Comment on lines +2529 to +2530
if (this._storage.getItem('amplify-sp-initiated-oauth-inFlight')) {
this._storage.removeItem('amplify-sp-initiated-oauth-inFlight');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if it make sense to clean any keys after the user has finished the oauth flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When _handleAuthResponse is called we handle Oauth only when the url contains code | error. Hence clearing the flag specifically here. Irrespective of success/failure clearing the flag early on.
WDYT

@ashwinkumar6 ashwinkumar6 marked this pull request as ready for review May 29, 2024 20:57
@ashwinkumar6 ashwinkumar6 requested a review from israx May 29, 2024 22:32
@ashwinkumar6
Copy link
Contributor Author

NOTE: The actual naming of the flag (idpEnabled) is pending approval
rest of all the code changes are ready for review

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.

None yet

2 participants