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.

Sign up for GitHub

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

Jump to bottom

feat: handle concurrent refreshes and improve graceful refreshing #3895

Merged
merged 5 commits into from
Dec 17, 2024
Merged

feat: handle concurrent refreshes and improve graceful refreshing #3895

merged 5 commits into from
Dec 17, 2024

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Nov 25, 2024 •
edited
Loading

This patch improves Ory Hydra's ability to deal with refresh flows which, for example, concurrently refresh the same token. Furthermore, graceful token refresh has been improved to handle a variety of edge cases and scenarios.

Additionally, serializability errors in CockroachDB are now correctly retried.

Related issue(s)

See https://github.com/ory-corp/cloud/issues/7311
Closes #3895

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@aeneasr aeneasr requested review from hperl and alnr as code owners November 25, 2024 10:15
alnr
alnr approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

difficult to test programatically I guess

@aeneasr
Copy link
Member Author

aeneasr commented Nov 25, 2024

We do have tests in network for this so if it passes there it's fine here too

@aeneasr
Copy link
Member Author

aeneasr commented Nov 28, 2024 •
edited
Loading

difficult to test programatically I guess

It actually wasn't that hard other than that the test tooling isn't great :) I've added a test case

@aeneasr aeneasr force-pushed the fix-concurrent-retry branch 3 times, most recently from 34f492e to e6534c2 Compare December 4, 2024 11:30
@aeneasr aeneasr force-pushed the fix-concurrent-retry branch from e6534c2 to 812b199 Compare December 4, 2024 12:25
@aeneasr aeneasr changed the title feat: retry crdb serializable errors feat: handle concurrent refreshes and improve graceful refreshing Dec 4, 2024
aeneasr
aeneasr commented Dec 4, 2024
View reviewed changes
.schema/config.schema.json Show resolved Hide resolved
aead/aead_test.go Show resolved Hide resolved
consent/test/manager_test_helpers.go Show resolved Hide resolved
cypress/integration/oauth2/refresh_token.js Show resolved Hide resolved
driver/config/provider.go Show resolved Hide resolved
persistence/sql/persister_oauth2.go Outdated Show resolved Hide resolved
persistence/sql/persister_oauth2.go Show resolved Hide resolved
persistence/sql/persister_oauth2.go Show resolved Hide resolved
persistence/sql/persister_oauth2.go Show resolved Hide resolved
spec/config.json Show resolved Hide resolved
@aeneasr aeneasr requested a review from alnr December 4, 2024 13:10
@aeneasr
Copy link
Member Author

aeneasr commented Dec 4, 2024

ory/fosite#838

@aeneasr aeneasr force-pushed the fix-concurrent-retry branch 10 times, most recently from 269fbb6 to cdf6c19 Compare December 9, 2024 10:25
alnr
alnr approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

This is a dense change. I did not see anything wrong with it.

@aeneasr aeneasr requested a review from zepatrik December 10, 2024 13:49
zepatrik
zepatrik reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

It's super hard to review with all the parallel refactoring. I know that you already put in a lot of work, but maybe you could still pull out the refactoring into a separate PR? Especially refactoring and changing of relevant test cases at the same time gets super hard to review 😅

...ce/sql/migrations/20241129111700000000_add_refresh_token_access_token_link.autocommit.up.sql Show resolved Hide resolved
@aeneasr
Copy link
Member Author

aeneasr commented Dec 11, 2024

It's super hard to review with all the parallel refactoring. I know that you already put in a lot of work, but maybe you could still pull out the refactoring into a separate PR? Especially refactoring and changing of relevant test cases at the same time gets super hard to review 😅

I had to change the tests because I was not able to test parallel refresh grants in SQLite. So I had to change the test suite to use cockroach etc.

If you review it in IntelliJ and hide whitespace changes it should be much easier to review

@aeneasr
Copy link
Member Author

aeneasr commented Dec 11, 2024

And the graceful refresh token rotation tests I had to anyways rewrite because the test suite was not effective.

@aeneasr
feat: handle concurrent refreshes and improve graceful refreshing
62a1e6f 
This patch improves Ory Hydra's ability to deal with refresh flows which, for example, concurrently refresh the same token. Furthermore, graceful token refresh has been improved to handle a variety of edge cases and scenarios.
@aeneasr aeneasr force-pushed the fix-concurrent-retry branch from 1faf44d to 62a1e6f Compare December 11, 2024 09:44
@aeneasr aeneasr mentioned this pull request Dec 11, 2024
Open
5 tasks
zepatrik
zepatrik approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

All LGTM, especially test improvements 👍

hperl
hperl approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

LGTM! The review comments were quite helpful in understanding the change, thanks!

@aeneasr aeneasr force-pushed the fix-concurrent-retry branch from 6aac1b3 to 1e9420d Compare December 17, 2024 08:24
@aeneasr
Copy link
Member Author

aeneasr commented Dec 17, 2024

I confirmed that a refresh token created before this patch also works after this patch.

@aeneasr aeneasr merged commit 0a6c966 into master Dec 17, 2024
29 checks passed
@aeneasr aeneasr deleted the fix-concurrent-retry branch December 17, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zepatrik zepatrik zepatrik approved these changes

@alnr alnr alnr approved these changes

@hperl hperl hperl approved these changes

Assignees

@aeneasr aeneasr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aeneasr @hperl @alnr @zepatrik