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

Support grant types other than "authorization_code" #56

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndrewDryga
Copy link
Collaborator

@AndrewDryga AndrewDryga commented Mar 16, 2024

This is a replacement of #52 because that PR was based off our master branch and extra changes piled up on top of intended ones.


I think this is another thing that we might want to clean up before pushing 1.0 to hex.pm.

This PR:
Removes the hardcoded "authorization_code" grant type from fetch_tokens/2.

Before you could override the grant_type using params but because the code assumed this grant type by default it always added redirect_uri to the params too, which can not be removed.

There are two reasons to remove it:

  • typically redirect_uri is retrieved in a Phoenix controller, but if you want to use a refresh_token grant in a background job you don't know that redirect_uri beforehand unless you have access to phoenix endpoint (which is not true for umbrella apps), and storing it doesn't make a lot of sense just to work around this function limitations;
  • we really should not send parameters that are not part of the spec to the token endpoint because if some provider starts to validate our requests against the spec the payload will be invalid.

Additionally, I completely removed redirect_uri from the config map and made it a function argument for authorization_uri/3.

Again, this is because it's the only required argument there, which should not be hidden behind a map, and it doesn't make a ton of sense to store this URI in the memory or database. Especially when the route can be changed and then you will have to migrate the data.

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

Successfully merging this pull request may close these issues.

None yet

1 participant