fix(auth): Allow Web Urls on signInWithRedirect in Mobile Apps #12969
Conversation
packages/auth/src/providers/cognito/utils/oauth/getRedirectUrl.native.ts
Outdated
Show resolved
Hide resolved
| throw invalidRedirectException; | ||
| } else { | ||
| // fallsback to available URL https://github.com/aws-amplify/amplify-js/issues/12890 | ||
| return redirects[0]; |
There was a problem hiding this comment.
Always return the first url? What if they want to call a particular url instead?
There was a problem hiding this comment.
how a customer would select that?
There was a problem hiding this comment.
By re-directing to the right url is understanding please correct me
if and only if I am wrong or not
@elorzafe 😕
| // fallsback to available URL https://github.com/aws-amplify/amplify-js/issues/12890 | ||
| return redirects[0]; | ||
| } | ||
| } | ||
| return redirect; |
There was a problem hiding this comment.
These changes will still return the mobile url right?
Ex: Giving ["mobile://", "http://"] as input the output is still "mobile://" url
In the issue linked, it seems like they want to hit "http://" first then "mobile://"
There was a problem hiding this comment.
Good call, the issue is how a customer would choose which url to choose?
There was a problem hiding this comment.
This will still choose first mobile url because is filtered. Now if you want to force the web url, that seems something that could be address by an API change
There was a problem hiding this comment.
Correct, with the current API there's no way to actively select which URL gets used. We (along with other platforms) use a heuristic to determine which URL to use.
….native.ts Co-authored-by: ManojNB <manojnb@amazon.com>
| @@ -10,7 +10,12 @@ export function getRedirectUrl(redirects: string[]): string { | |||
| !redirect.startsWith('http://') && !redirect.startsWith('https://') | |||
| ); | |||
| if (!redirect) { | |||
| throw invalidRedirectException; | |||
| } | |||
| if (redirects.length === 0) { | |||
There was a problem hiding this comment.
Nit: Looks like a tabbing error with this block
| // fallsback to available URL https://github.com/aws-amplify/amplify-js/issues/12890 | ||
| return redirects[0]; | ||
| } | ||
| } | ||
| return redirect; |
There was a problem hiding this comment.
Correct, with the current API there's no way to actively select which URL gets used. We (along with other platforms) use a heuristic to determine which URL to use.
|
I just realised: this fixes #12890 but ignores #12956 - which was closed in favour of the former. While the 2 issues have a similar cause, they're not equal. In my case I have a Capacitor (not RN) app - which behaves entirely like a web app - which has to redirect to a native app. So it's more like the other way around - and the misbehaving implementation is Can this be addressed here too? Or should I open another PR on top? |
Description of changes
Adds fallback URL in case customer wants to redirect native app to a website
Issue #, if available
fixes: #12890
Description of how you validated changes
React-Native CLI App
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.