Skip to content

Add usePKCE Flag for Dropbox #232

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

Merged
merged 5 commits into from
Feb 1, 2019

Conversation

awmichel
Copy link
Contributor

This allows providers like Dropbox who don't support PKCE and go so far as to error is you provide unknown parameters in the OAuth2 request to function correctly.

By specifying usePKCE: false in the auth options the code_challenge parameter will not be sent to the OAuth2 provider and code verification will not run against the response.

Related to #231 and #169. See comment #169 (comment) as well.

The redirect URL issue with Dropbox can be resolved using universal links or a redirect service.

@kadikraman
Copy link
Contributor

Hi @awmichel - thanks for the PR.

I've just merged a PR that's now causing you some conflicts in RNAppAuth.m, but they relate to your question so it might not be too bad.

Basically, the authorize response is kind of a mishmash of token and authorization responses. The map that you refer to actually came from another PR I merged yesterday and was intending to tidy it up today.

Essentially, there are three additionalParameters to choose from

  1. the one passed into the authorize request
  2. the one returned from tokenResponse
  3. the one returned from authorizeResponse

We can disregard 1. because the user will know what they passed in.

So there's a choice between 2 and 3. I'm currently thinking we should return both, but under different keys. So tokenAdditionalParameters and authorizeAdditionalParameters.

If you would rather not implement this yourself (it'll have to be done on Android as well) - resolve your conflicts so I can merge your PR and I'll tidy it up :)

@kadikraman kadikraman added this to the Next Release 🚀 milestone Feb 1, 2019
@kadikraman kadikraman added the enhancement A new feature label Feb 1, 2019
@awmichel
Copy link
Contributor Author

awmichel commented Feb 1, 2019

@kadikraman Thanks for taking a look. I resolved the merge commits and separated the additionalParameters for the authorize and token responses.

It looks like the Android library was also merging the additional parameters from the authorize call into the additionalParameters it returned. I went ahead and removed that code so iOS and Android are now consistent.

Let me know if there are any other tweaks you'd like. There are a couple breaking changes in here now to signal out.

I also added a section to the README for configuring Dropbox.

Copy link
Contributor

@kadikraman kadikraman left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - I especially appreciate that you added docs in the readme for Dropbox 🙏
Regarding timelines - this will be in the next release, which I'm aiming to land at the end of next week.

@kadikraman kadikraman merged commit 0563f3b into FormidableLabs:master Feb 1, 2019
@kadikraman kadikraman modified the milestones: Next Major Release 🚀, Next Minor Release 🎉 Feb 18, 2019
@kadikraman
Copy link
Contributor

Thanks for your help! This has been published in v4.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants