Update coinbase - #334
Open
cpruijsen wants to merge 1 commit into
Open
Update coinbase#334cpruijsen wants to merge 1 commit into
cpruijsen wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update the built-in
coinbaseauthorize and token URLs tohttps://login.coinbase.com/oauth2/authandhttps://login.coinbase.com/oauth2/token, and document in Provider Quirks that Coinbase rejects a redirect URI containing the wordcoinbase, soredirect_urimust be set explicitly.Fixes #318.
Provenance
Both parts follow what is already established on the issue:
redirect_urineeds to be specified explicitly."The provider key stays
coinbase, so existing/connect/coinbaseusers are unaffected. Renaming it to something likecoinwould break them, and you noted the default name usually tracks the provider domain.Verification
npx mocha test/config.jspasses; thector coinbasecase asserts the new endpoints.What I have not verified myself is a live authorization round trip against Coinbase, which needs a Coinbase developer account. If you would rather that be confirmed before merging, say so and I will register one and report back. I did not want to claim an end-to-end check I had not run.
Optional
The Quirks note ends with a sentence about redirecting the user on to Grant's
/connect/coinbase/callbackroute. Happy to drop it and leave only "specifyredirect_uriexplicitly" if you find it clearer.