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

Populate refresh token from a client credentials OAuth2 call #3794

Merged
merged 7 commits into from Jul 29, 2021
Merged

Populate refresh token from a client credentials OAuth2 call #3794

merged 7 commits into from Jul 29, 2021

Conversation

ihatemornings
Copy link
Contributor

@ihatemornings ihatemornings commented Jul 7, 2021

There’s a bug (#2602) with the client credentials OAuth2 flow: the refresh_token returned by the access token URL is ignored.

This change updates the response handling in the client credentials flow to match that of the authorization code flow, which populates the refresh token as expected. I assume the missing key was an oversight in the original code – I can’t think of a reason why the refresh token would be populated for one method and not the other.

I’ve tested this fix on MacOS (using the steps from the bug report) and the refresh token is now populated and can be used to repeatedly refresh the access token.

This is my first pull request to the project – let me know if there’s anything I’ve missed or anything I could add to improve it or make it easier to review and accept!

Closes #2602.

@aeri
Copy link
Contributor

aeri commented Jul 24, 2021

Although the standard recommends not to issue refresh tokens in the Client Credentials flow, it is possible to find OAuth 2.0 server implementations that do issue this token since it is not restricted, only discouraged. #2602 (comment)

Since Insomnia is a software for API development and testing, it could be interesting to include this feature (knowing that the OAuth 2.0 server used is not following the practices recommended by the standard).

@develohpanda
Copy link
Contributor

@ihatemornings thank you for the PR and welcome to the project! One thing of use would be a gif/video/loom recording of the feature working. 😄

Although the standard recommends not to issue refresh tokens in the Client Credentials flow, it is possible to find OAuth 2.0 server implementations that do issue this token since it is not restricted, only discouraged. #2602 (comment)

Since Insomnia is a software for API development and testing, it could be interesting to include this feature (knowing that the OAuth 2.0 server used is not following the practices recommended by the standard).

This is a very good point, I just read through the RFC section and it clearly states that the client credentials flow should not return a refresh_token. In saying that, Insomnia is also a generic tool and does not strictly enforce any standards, so I'm not sure where to stand with this particular workflow. @wdawson what are your thoughts?

@ihatemornings
Copy link
Contributor Author

I see! I have since realised that the API I’m working with is not following the recommendation of the RFC, so I understand now why the code was written to ignore any refresh_token value.

Given that there are indeed APIs out there that return a refresh_token in the client credentials flow (or even that a developer might include a refresh_token during development of an API without fully understanding the RFC), I still think the user experience was confusing. The refresh token field is enabled and empty in the Insomnia OAuth UI, so there’s nothing to indicate to the user that the token is being explicitly ignored.

On balance I think I would still prefer to have the field included and leave it to the user and/or the API designer to build a compliant API. That’s just my two cents as a user. Thanks for taking the time to respond!

@wdawson
Copy link
Contributor

wdawson commented Jul 26, 2021

Although the standard recommends not to issue refresh tokens in the Client Credentials flow, it is possible to find OAuth 2.0 server implementations that do issue this token since it is not restricted, only discouraged. #2602 (comment)

Since Insomnia is a software for API development and testing, it could be interesting to include this feature (knowing that the OAuth 2.0 server used is not following the practices recommended by the standard).

This is a very good point, I just read through the RFC section and it clearly states that the client credentials flow should not return a refresh_token. In saying that, Insomnia is also a generic tool and does not strictly enforce any standards, so I'm not sure where to stand with this particular workflow. @wdawson what are your thoughts?

Yeah, non-compliant OAuth and OIDC implementations abound. Given that it's a SHOULD NOT, however, I think we should not ignore it if it comes back in the response. Thanks for the PR @ihatemornings !

Copy link
Contributor

@develohpanda develohpanda 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 haven't been able to QA this as I don't have a server setup in this way but the code & tests look great

@vercel vercel bot temporarily deployed to Preview July 26, 2021 23:47 Inactive
@develohpanda develohpanda requested a review from DMarby July 26, 2021 23:48
@ihatemornings
Copy link
Contributor Author

@develohpanda here’s a quick recording of the branch doing its thing. The client credentials call to the OAuth token endpoint returns a refresh_token, it’s displayed in the UI and used to refresh the access token.

CleanShot 2021-07-27 at 10 30 16

@develohpanda
Copy link
Contributor

Awesome! Thanks for adding in the recording 😄

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

cool beans.

@dimitropoulos dimitropoulos enabled auto-merge (squash) July 29, 2021 04:31
@vercel vercel bot temporarily deployed to Preview July 29, 2021 04:57 Inactive
@dimitropoulos dimitropoulos merged commit b8cc881 into Kong:develop Jul 29, 2021
@ihatemornings ihatemornings deleted the oauth2-client-credentials-refresh-token-fix branch July 29, 2021 08:08
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.

OAuth 2 "refresh_token" is not populated
5 participants