-
Notifications
You must be signed in to change notification settings - Fork 361
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
Allow redirect_uri override in loginWithRedirect #66
Conversation
response_mode: 'query', | ||
state: TEST_ENCODED_STATE, | ||
nonce: TEST_RANDOM_STRING, | ||
redirect_uri: REDIRECT_OPTIONS.redirect_uri, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be redirect_uri: redirect_uri
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lbalmaceda I don't think so. The test determines that the options passed into loginWithRedirect
take precedence over the options passed when creating the client. redirect_uri
here represents the options passed into the client.
@@ -111,7 +111,7 @@ export default class Auth0Client { | |||
stateIn, | |||
nonceIn, | |||
code_challenge, | |||
window.location.origin | |||
this.options.redirect_uri || window.location.origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default redirect_uri
was window.location.origin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for web_message requests (that don't actually redirect anywhere)
@@ -64,7 +64,7 @@ export default class Auth0Client { | |||
response_mode: 'query', | |||
state, | |||
nonce, | |||
redirect_uri: this.options.redirect_uri || redirect_uri, | |||
redirect_uri: redirect_uri || this.options.redirect_uri, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this flipped? where do this.options
comes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lbalmaceda This is ultimately what was causing the bug. The options passed into createAuth0Client
(this.options
) was taking precedence over the options being passed into loginWithPopup
, getTokenSilently
, etc. It should be the other way around.
Description
The developer should be able to override the default redirect_uri when calling
loginWithRedirect
. Today, we were actually overriding anything passed in theloginWithRedirect
method with the default redirect_uri provided in thecreateAuth0Client
function, which doesn't make sense and it's a bug. This PR fixes that.References
Bug found in this thread: #52 (comment)
Testing