feat: add ability to allow token refresh from hook without overriding the session claims #3146
Conversation
This file contains 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
6 tasks
Codecov Report
@@ Coverage Diff @@
## v2.x #3146 +/- ##
=======================================
Coverage ? 78.86%
=======================================
Files ? 113
Lines ? 8489
Branches ? 0
=======================================
Hits ? 6695
Misses ? 1398
Partials ? 396 Continue to review full report at Codecov.
|
… the session claims
zachabney
force-pushed
the
refresh-token-hook-without-update
branch
from
bc9d317
to
f8eacb8
Compare
aeneasr
requested changes
Jun 13, 2022
aeneasr
requested changes
Jun 14, 2022
oauth2/oauth2_auth_code_test.go
Outdated
| @@ -969,21 +969,40 @@ func TestAuthCodeWithMockStrategy(t *testing.T) { | |||
| }) | |||
|
|
|||
| t.Run("should not override session data if token refresh hook returns no content", func(t *testing.T) { | |||
| if strat.d != "jwt" { | |||
| t.Skip() | |||
There was a problem hiding this comment.
This test would work for both JWT and opaque if you use:
accessTokenClaims := testhelpers.IntrospectToken(t, oauthConfig, &refreshedToken, ts)
82a919e#diff-6d883efffdabd9715dc9872121018df30a5843c81e25dc6c4af2c3edc13fb21cR957
The problem is that the JWT strategy works a bit different so we need to ensure this works as intended for both strategies :)
There was a problem hiding this comment.
Ah, that's what I was missing. Thanks! Should be updated now.
aeneasr
approved these changes
Jun 17, 2022
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.
The refresh token hook currently requires a 200 response code with an updated session payload. There is no way to permit the token refresh without overriding the session data. It's useful, however, to be able to use the token refresh hook in order to accept/reject token refreshes without the need to update the session data.
This adds the ability for the token refresh hook to return a
204 No Contentresponse code to indicate that the token refresh is permitted, but not to update the session data.The documentation has also been updated with PR ory/docs#863
Related issue(s)
#3082
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the
maintainers to push the changes.
Further Comments
In the future, the token refresh hook request payload could be expanded to include the existing session data and allow for the
sidto be set on the refreshed id token as described in #3082, but this is a quick workaround that'll continue to be useful even if that is implemented.