feat: add session and requester to refresh token webhook data #3204

sgal · 2022-07-27T06:10:09Z

Related issue(s)

Feature: This patch adds session and requester data to the refresh webhook for session tracking by integrators. Fixes #3203.

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 green light (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. - see feat: add docs for session and requester fields in Hydra refresh webhook docs#932

Further Comments

Changes outside of hook.go are created by running make sdk.

CLAassistant · 2022-07-27T06:10:13Z

All committers have signed the CLA.

aeneasr

Epic :) I think we can send the whole session in the payload!

To ensure that the change has the desired effect, can you please add the appropriate assertions to the test case:

hydra/oauth2/oauth2_auth_code_test.go

Line 916 in ceada19

t.Run("should call refresh token hook if configured", func(t *testing.T) {

The documentation will also need to explain the new key, the appropriate document for that should be: https://github.com/ory/docs/blob/45a1f9268d693b164d169e16272a30327860c4e9/docs/hydra/guides/updating-claims-at-refresh.mdx

oauth2/hook.go

sgal · 2022-07-27T20:14:15Z

@aeneasr Is there a good approach to make Session into a JSON and Swagger-friendly representation? I tried to use it as is, but it generates a lot of new files for fosite.DefaultSession and IDTokenClaims to map them to Swagger. I feel like there should be a better way to do it, but not sure where to look for examples. Could you please point me in the right direction?
Same question for requester.

aeneasr · 2022-07-27T21:36:17Z

A common practice, if the types don't work in swagger, is to create a replacement / patch like here:

hydra/.schema/openapi/patches/oauth2.yaml

Lines 3 to 4 in c9be891

    
           - op: remove 
        
             path: /components/schemas/consentRequestSession/properties/access_token/type

Basically, this will patch the given path in

https://github.com/ory/hydra/blob/master/spec/api.json

with the value you provide. This can be helpful in scenarios where go-swagger creates types that just aren't correct.

Regarding the files generated, that is fine and sometimes expected with OpenAPI generator - feel free to push your changes if you want me to take a look.

oauth2/hook.go

oauth2/oauth2_auth_code_test.go

internal/httpclient/models/default_session.go

go.mod

oauth2/oauth2_auth_code_test.go

codecov · 2022-07-29T13:05:34Z

Codecov Report

Merging #3204 (0c50c15) into master (e66ba3c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 0c50c15 differs from pull request most recent head 6508ceb. Consider uploading reports for the commit 6508ceb to get more accurate results

@@            Coverage Diff             @@
##           master    #3204      +/-   ##
==========================================
+ Coverage   79.28%   79.32%   +0.04%     
==========================================
  Files         111      111              
  Lines        8077     8084       +7     
==========================================
+ Hits         6404     6413       +9     
+ Misses       1259     1258       -1     
+ Partials      414      413       -1

Impacted Files	Coverage Δ
oauth2/session.go	`89.74% <ø> (ø)`
oauth2/hook.go	`68.11% <100.00%> (+3.59%)`	⬆️
persistence/sql/persister_oauth2.go	`79.91% <0.00%> (+0.81%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

aeneasr

Great, this looks really good already! :)

Once finished, could you please also update the docs PR accordingly? Thank you!

oauth2/hook.go

oauth2/oauth2_auth_code_test.go

aeneasr · 2022-07-29T13:12:45Z

oauth2/oauth2_auth_code_test.go

+							require.Equal(t, hookReq.Requester.ClientID, oauthConfig.ClientID)
+							require.ElementsMatch(t, hookReq.Requester.GrantedScopes, expectedGrantedScopes)
+
+							assertx.EqualAsJSONExcept(t, hookReq.Session, json.RawMessage(`{


If you use snapshotx. it will create a JSON file on the disk which makes the content a bit easier to read:

snapshotx.SnapshotT(t, json.RawMessage("..."), snapshotx.ExceptPaths("...") )

To update snapshots set env var UPDATE_SNAPSHOTS=true

It looks like snapshotx.SnapshotT is not available in the version of snapshotx that hydra uses (0.0.368). Tried to bump to lastest - 0.0.450 - a lot of tests fail. So I used SnapshotTExcept.

Also, it generated 6 snapshots with the same content - probably because tests are run in parallel.

sgal · 2022-07-29T16:59:38Z

oauth2/oauth2_auth_code_test.go

+								"session.id_token.id_token_claims.rat",
+								"session.id_token.id_token_claims.auth_time",
+							}
+							snapshotx.SnapshotTExcept(t, hookReq, except)


This is indeed tidier, thanks for pointing me into snapshotx!

sgal · 2022-07-29T19:55:03Z

Docs PR is also updated with the latest schema.

vinckr · 2022-08-01T18:43:57Z

Hello @sgal
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

mih-kopylov · 2022-08-25T09:46:18Z

Hey @aeneasr
I've reaslized that oauth2/.snapshots/TestAuthCodeWithMockStrategy-strategy=jwt-case=2-description=should_pass_because_prompt=none_and_max_age_<_auth_time-should_call_refresh_token_hook_if_configured.json file, that is added in this pull request, breaks ability to checkout the repository on Windows OS.

Is there a chance to rename the files so that Windows contributors are not blocked?

aeneasr · 2022-08-25T09:59:37Z

Yes it’s already fixed on master!

mih-kopylov · 2022-08-25T10:11:07Z

Sorry @aeneasr, but these two files are still in master:

Their names contain < symbol which is reserved in Windows

TestAuthCodeWithMockStrategy-strategy=jwt-case=2-description=should_pass_because_prompt=none_and_max_age_<_auth_time-should_call_refresh_token_hook_if_configured.json
TestAuthCodeWithMockStrategy-strategy=opaque-case=2-description=should_pass_because_prompt=none_and_max_age_<_auth_time-should_call_refresh_token_hook_if_configured.json

Am I missing anything?

aeneasr · 2022-08-25T10:11:48Z

Oh I see, then it's only fixed on the v2.x branch!

mih-kopylov · 2022-08-25T10:14:12Z

Is there a chance to get the changes backported to master?

aeneasr · 2022-08-25T10:14:51Z

Sure, can you maybe just cherry-pick the commit and make a PR?

mih-kopylov · 2022-08-25T10:15:34Z

Sure, will do.

mih-kopylov · 2022-08-25T10:57:38Z

@aeneasr #3229 here it is

aeneasr · 2022-08-25T11:05:15Z

👍

aeneasr reviewed Jul 27, 2022

View reviewed changes

oauth2/hook.go Outdated Show resolved Hide resolved

aeneasr reviewed Jul 27, 2022

View reviewed changes

oauth2/hook.go Outdated Show resolved Hide resolved

sgal mentioned this pull request Jul 27, 2022

feat: add docs for session and requester fields in Hydra refresh webhook ory/docs#932

Merged

6 tasks

sgal commented Jul 28, 2022

View reviewed changes

oauth2/hook.go Show resolved Hide resolved

sgal commented Jul 28, 2022

View reviewed changes

oauth2/oauth2_auth_code_test.go Show resolved Hide resolved

sgal commented Jul 28, 2022

View reviewed changes

internal/httpclient/models/default_session.go Outdated Show resolved Hide resolved

This was referenced Jul 28, 2022

feat: add json mappings to default session and its contents ory/fosite#687

Merged

feat: add json mappings to default session and its contents ory/fosite#688

Merged

sgal added 3 commits July 29, 2022 13:44

feat: add id_token_extra to refresh token webhook data

49834d9

feat: use entire session and requester in the refresh hook request body

f69035d

feat: use new fosite and add tests for refresh webhook

6159ee1

sgal commented Jul 29, 2022

View reviewed changes

go.mod Show resolved Hide resolved

sgal commented Jul 29, 2022

View reviewed changes

oauth2/oauth2_auth_code_test.go Outdated Show resolved Hide resolved

aeneasr reviewed Jul 29, 2022

View reviewed changes

sgal commented Jul 29, 2022

View reviewed changes

fix: use snapshot in the refresh webhook test for readability

6508ceb

sgal changed the title ~~feat: add id_token_extra to refresh token webhook data~~ feat: add session and requester to refresh token webhook data Jul 29, 2022

sgal marked this pull request as ready for review July 29, 2022 19:45

aeneasr approved these changes Aug 1, 2022

View reviewed changes

aeneasr merged commit 6d23859 into ory:master Aug 1, 2022

sgal deleted the id-token-extra-in-refresh-webhook branch August 1, 2022 11:44

mih-kopylov mentioned this pull request Aug 25, 2022

fix: remove special char from snapshot symbols #3229

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add session and requester to refresh token webhook data #3204

feat: add session and requester to refresh token webhook data #3204

sgal commented Jul 27, 2022 •

edited

Loading

CLAassistant commented Jul 27, 2022 •

edited

Loading

aeneasr left a comment

sgal commented Jul 27, 2022

aeneasr commented Jul 27, 2022

codecov bot commented Jul 29, 2022 •

edited

Loading

aeneasr left a comment

aeneasr Jul 29, 2022

sgal Jul 29, 2022

sgal Jul 29, 2022

sgal commented Jul 29, 2022

vinckr commented Aug 1, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

feat: add session and requester to refresh token webhook data #3204

feat: add session and requester to refresh token webhook data #3204

Conversation

sgal commented Jul 27, 2022 • edited Loading

Related issue(s)

Checklist

Further Comments

CLAassistant commented Jul 27, 2022 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

sgal commented Jul 27, 2022

aeneasr commented Jul 27, 2022

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jul 29, 2022

Choose a reason for hiding this comment

sgal Jul 29, 2022

Choose a reason for hiding this comment

sgal Jul 29, 2022

Choose a reason for hiding this comment

sgal commented Jul 29, 2022

vinckr commented Aug 1, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

mih-kopylov commented Aug 25, 2022

aeneasr commented Aug 25, 2022

sgal commented Jul 27, 2022 •

edited

Loading

CLAassistant commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 29, 2022 •

edited

Loading