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

fix: fix the refresh token issue #14

Merged
merged 3 commits into from
Apr 12, 2024
Merged

fix: fix the refresh token issue #14

merged 3 commits into from
Apr 12, 2024

Conversation

wood-push-melon
Copy link

@wood-push-melon wood-push-melon commented Apr 8, 2024

This pull request tries to fix the refresh token not issued problem in device flow. Also tries to make some corrections in the tests.

Question:

  • Not sure when the client will be a ClientWithCustomTokenLifespans. But from this line, do we need to also add something for device flow grant type in here?

@wood-push-melon wood-push-melon marked this pull request as ready for review April 8, 2024 21:27
@wood-push-melon wood-push-melon requested a review from a team April 9, 2024 02:36
Copy link

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

I tried this with hydra and the refresh token is still not issued, is this supposed to fix the bug or is there work that needs to be done in hydra for this?

@wood-push-melon
Copy link
Author

I tried this with hydra and the refresh token is still not issued, is this supposed to fix the bug or is there work that needs to be done in hydra for this?

Took another look at the Hydra and Fosite. From what I understand, I think the endpoint /oauth2/device/verify missed granting the scopes to the authorize request. It did update the granted scopes at this line. However, it does not store the updated authorize request.

With the regular OAuth2 flow, the updated authorize request is stored at this line call. So the access token request handler can use the granted scopes in the authorize request to see if a refresh token should be issued.

We can sit together after the daily to have a walkthrough.

nsklikas
nsklikas previously approved these changes Apr 11, 2024
Copy link

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Actually I have one minor comment

handler/oauth2/flow_authorize_code_token_test.go Outdated Show resolved Hide resolved
@wood-push-melon wood-push-melon merged commit 7fe9b89 into canonical Apr 12, 2024
5 checks passed
@wood-push-melon wood-push-melon deleted the IAM-786 branch April 12, 2024 17:03
nsklikas pushed a commit that referenced this pull request Sep 12, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Sep 12, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Sep 25, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Sep 25, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Sep 26, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Oct 16, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Oct 16, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Oct 16, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Oct 18, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Oct 23, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Nov 1, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
nsklikas pushed a commit that referenced this pull request Nov 6, 2024
* fix: fix the refresh token issue

* fix: fix the OIDC token missing issue
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.

3 participants