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

Allow authorize options to override default values #177

Merged
merged 2 commits into from
Nov 6, 2018
Merged

Allow authorize options to override default values #177

merged 2 commits into from
Nov 6, 2018

Conversation

kenzic
Copy link
Contributor

@kenzic kenzic commented Sep 25, 2018

No description provided.

@cocojoe
Copy link
Member

cocojoe commented Oct 1, 2018

@kenzic please can you provide more information in your description, what issue did you run into that required this PR.

@kenzic
Copy link
Contributor Author

kenzic commented Oct 1, 2018

@cocojoe - I want to change the value of the redirectUri via options passed to the authorize method. Based on the current order there's no way to do that:

        let query = {
          ...options,
          clientId,
          responseType: 'code',
          redirectUri,
          state: expectedState,
          ...defaults,
        };

By switching the order, if options.redirectUri exists then it will override the value set inside the method (${bundleIdentifier.toLowerCase()}://${domain}/${Platform.OS}/${bundleIdentifier}/callback).

@kenzic
Copy link
Contributor Author

kenzic commented Oct 10, 2018

@cocojoe - Is there anything else you need from me?

@cocojoe
Copy link
Member

cocojoe commented Oct 11, 2018

@kenzic Yes, why do you need to do this? The callback URL is standardised across our native platforms.

@kenzic
Copy link
Contributor Author

kenzic commented Oct 22, 2018

@cocojoe I'm running into issues using this along side a framework I'm using. But that's more of an aside. It seems strange that the defaults override the options. Is this intentional?

@kenzic
Copy link
Contributor Author

kenzic commented Oct 26, 2018

@cocojoe following up on this

@cocojoe
Copy link
Member

cocojoe commented Oct 26, 2018

@kenzic well the previous comment still stands. #177 (comment)

This is not something you can change (intentional) in the dedicated iOS or Android Auth0 libraries, so I would like to know what is the use case here for changing the callback URL. What is your scenario, what framework etc Thx

@kenzic
Copy link
Contributor Author

kenzic commented Oct 26, 2018

The framework I'm using is Expo. Due to a bug in Expo I can't have a custom scheme URL and have the bundle identifier handle linking properly.

For example, if I have a bundle identifier com.test.myapp and a scheme myapp, only one will work. The other will have it's protocol changed to exp. I'm working with the expo team to address this.

Right now, if my app supports the custom URL scheme, the react-native-auth fails. All this being said, it does seem like there are other reasons someone might want to have a custom redirectUri.

My specific issue aside, the reason for my request comes down to:

  • Should defaults override options?
  • And/or should the API allow for changing the redirectUri?

defaults overriding options is seems like either a bug or a bit of a misnomer, and being able to modify the redirectUri seems like it would be pretty useful.

@cocojoe
Copy link
Member

cocojoe commented Oct 27, 2018

Thanks for this answer, it's much clearer.

So I agree with you that user applied options should override defaults, otherwise there is not much value in the options.
I was hesitant around the callback. However, I can't see a good story around not having flexibility around this.

So please rebase your PR against latest master and add some tests. 👍

@kenzic
Copy link
Contributor Author

kenzic commented Oct 30, 2018

@cocojoe - rebased and pushed

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

LGTM

@cocojoe cocojoe added this to the v1-Next milestone Oct 31, 2018
@cocojoe
Copy link
Member

cocojoe commented Oct 31, 2018

@kenzic This could be an issue with anyone who has set weird options but as these would never have been set due to the parameter order merging it would have worked okay. Is a bit of a weird scenario but wanted to voice it.

@cocojoe cocojoe changed the title correct order so options override default values Allow options to override default values Nov 6, 2018
@cocojoe cocojoe changed the title Allow options to override default values Allow authorize options to override default values Nov 6, 2018
@cocojoe cocojoe merged commit 673ccc4 into auth0:master Nov 6, 2018
@@ -54,12 +54,12 @@ export default class WebAuth {
}/${bundleIdentifier}/callback`;
const expectedState = options.state || state;
let query = {
...options,
...defaults
Copy link
Member

Choose a reason for hiding this comment

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

That missing comma!!

@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.4.0 Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants