fix(auth): handle multiple redirect links #12415
Conversation
packages/auth/src/providers/cognito/utils/oauth/getSignInRedirect.native.ts
Outdated
Show resolved
Hide resolved
| /** @internal */ | ||
| export function getSignInRedirect(redirects: string[]): string { | ||
| const redirect = redirects.find(redirect => { | ||
| return redirect.startsWith(String(window.location.origin + '/')); |
There was a problem hiding this comment.
QQ: If there's no match with the current location.. does it necessarily invalidate the redirect url? Is it possible they want to redirect to a different url? I'm wondering if we should start with this and additionally fallback to a best guess rather than throw an error
There was a problem hiding this comment.
I think that makes, sense what is flutter doing?
There was a problem hiding this comment.
Flutter is matching the current window location and also throws. I added a fallback case but I kept the throw logic in case nothing is found in the signInRedirect array config option. Otherwise it will return undefined and customers will still face an error while in the oauth flow.
There was a problem hiding this comment.
Thinking twice I think it is not a good idea to add a fallback because the signInRedirect should be coming from the same origin, v6 doesn't support oauth flows coming outside of Amplify, thus customers can have their redirectSignIn url with tokens hanging in the browser.
There was a problem hiding this comment.
added an additional fallback to match the same domain
israx
force-pushed
the
fix/multiple-links
branch
from
cb39d7b
to
b7fb118
Compare
| export function getRedirectUrl(redirects: string[]): string { | ||
| const redirect = redirects?.find( | ||
| redirect => | ||
| !redirect.startsWith('http://') && !redirect.startsWith('https://') | ||
| ); | ||
| if (!redirect) { | ||
| throw invalidRedirectException; | ||
| } | ||
| return redirect; | ||
| } |
There was a problem hiding this comment.
This will always use the first non http https url, this means if you have more than the rest will be ignored
There was a problem hiding this comment.
Is it possible to detect the app name?
There was a problem hiding this comment.
yeah this is specific for RN, I don't think there is a way to distinguish more than 1 non http or non https. The conditions here are less restrictive
| // origin + pathname => https://example.com/app | ||
| const isSameOriginAndPathName = (redirect: string) => | ||
| redirect.startsWith( | ||
| String(window.location.origin + window.location.pathname ?? '/') |
There was a problem hiding this comment.
Nit: Could you use string interpolation to avoid the String() cast? More importantly - it might make the expression itself a little clearer (I think this means if there's no pathname then use '/') and doing something like
`${window.location.origin}${window.location.pathname ?? '/'}`might make that marginally clearer?
| ); | ||
| // domain => outlook.live.com, github.com | ||
| const isTheSameDomain = (redirect: string) => | ||
| redirect.includes(String(window.location.hostname)); |
There was a problem hiding this comment.
Nit: Would it be worth using URL to make sure the part of the string that matches is in fact the hostname and not some part of the path or is that something we don't really need to worry about?
Also, is matching the full hostname (i.e. including subdomain) still too strict? Or do we consider that best practice?
There was a problem hiding this comment.
Yeah I think if we want to be less restrictive, the next case basically wouldn't match an url coming from the same origin, thus not something that Amplify supports at the moment
Description of changes
Web:
use cases supported by Amplify:
Websites with different origins don't share elements in storage, thus Amplify is unable to access oauth keys.
RN:
Issue #, if available
Description of how you validated changes
Test in a sample app with code and token responseType
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.