feat: add token hooks for all grants types by fehrnah · Pull Request #3254 · ory/hydra

fehrnah · 2022-09-14T13:12:45Z

add client_credentials token hook
add authorization_code token hook
add jwt_profile token hook
add relevant configuration

Related issue(s)

#3244

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
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works. => I am unsure how to test hooks, the refresh hook doesn't have tests I could look at
I have added or changed the documentation.

codecov · 2022-09-14T13:34:15Z

Codecov Report

Merging #3254 (d400ffa) into master (16831c5) will increase coverage by 0.02%.
The diff coverage is 71.15%.

❗ Current head d400ffa differs from pull request most recent head fbc185c. Consider uploading reports for the commit fbc185c to get more accurate results

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   76.75%   76.77%   +0.02%     
==========================================
  Files         123      123              
  Lines        9076     9111      +35     
==========================================
+ Hits         6966     6995      +29     
- Misses       1668     1674       +6     
  Partials      442      442

Impacted Files	Coverage Δ
oauth2/hook.go	`71.87% <69.66%> (+3.45%)`	⬆️
driver/config/provider.go	`82.22% <75.00%> (-0.34%)`	⬇️
driver/registry_base.go	`86.09% <100.00%> (+0.15%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aeneasr

Great stuff! Could you please also add some tests for this?

aeneasr · 2022-09-14T13:13:57Z

oauth2/hook.go

+}
+
+// AuthorizationCodeHook is an AccessRequestHook called for `authorization_code` grant type.
+func AuthorizationCodeHook(reg interface {


Could you please DRY up the code a bit, it looks like we're re-using most of it across the functions!

I am unsure about how this kind os factorisation should be done.
My first instinct is to abuse generics. Is that OK with you ?

@fehrnah Every function here has 2 points that are different - get a hookURL and check the grant type from the requester. The rest of the function body is the same for all new functions. Basically lines 213 - 299 could be moved into a separate function and reused.

With that approach you could also get rid of repeating struct definitions around *Request and *Response since they are the same (at least for now)

Thank you for the input.
This has not been my priority for the las few moth, but I'm still interested in getting things through.

I made the changes you suggested and things are much better looking !

@fehrnah Awesome work! Could please add a hook for https://www.ory.sh/docs/hydra/guides/jwt#exchanging-jwts-for-access-tokens?

@fehrnah Awesome work! Could please add a hook for https://www.ory.sh/docs/hydra/guides/jwt#exchanging-jwts-for-access-tokens?

I think this is what I called JWTProfile.
I renamed it and related conf to JWTBearer to be consistant.

So after this, tthere are hooks for the following grant_types:

refresh_token

authorization_code

client_credentials

urn:ietf:params:oauth:grant-type:jwt-bearer

Are there more I should support ?

The only case not supported would be the implicit flow, but IIRC it is considered legacy

No, I think it is fine to support the ones you added and skip the implicit flow. @aeneasr what do you think?

sgal · 2022-09-20T15:30:35Z

driver/config/provider.go

 	KeyOAuth2GrantJWTMaxDuration                 = "oauth2.grant.jwt.max_ttl"
-	KeyRefreshTokenHookURL                       = "oauth2.refresh_token_hook" // #nosec G101
+	KeyRefreshTokenHookURL                       = "oauth2.refresh_token_hook"      // #nosec G101
+	KeyAuthorizationCodeTokenURL                 = "oauth2.authorization_code_hook" // #nosec G101


Suggested change

KeyAuthorizationCodeTokenURL = "oauth2.authorization_code_hook" // #nosec G101

KeyAuthorizationCodeHookURL = "oauth2.authorization_code_hook" // #nosec G101

Not sure if it is a typo, but it should be consistent with other keys that represent hook URLs

Was a typo, should be fixed.

sgal · 2022-12-13T10:05:32Z

spec/config.json

+          "format": "uri",
+          "examples": ["https://my-example.app/token-client-credentials-hook"]
+        },
+        "jwt_profile_hook": {


For consistency with other places

Suggested change

"jwt_profile_hook": {

"jwt_bearer_hook": {

sgal · 2022-12-13T12:12:26Z

spec/config.json

+        },
+        "jwt_bearer_hook": {
+          "type": "string",
+          "description": "Sets the client credentials hook endpoint. If set it will be called during token refresh to receive updated token claims.",


This needs to be updated - seems like a copy-paste from client_credentials section

sgal · 2022-12-13T12:12:42Z

spec/config.json

+          "type": "string",
+          "description": "Sets the client credentials hook endpoint. If set it will be called during token refresh to receive updated token claims.",
+          "format": "uri",
+          "examples": ["https://my-example.app/token-jwt-profile-hook"]


Suggested change

"examples": ["https://my-example.app/token-jwt-profile-hook"]

"examples": ["https://my-example.app/token-jwt-bearer-hook"]

add client_credentials token hook add authorization_code token hook add jwt_profile token hook add relevant configuration

sgal · 2022-12-21T14:08:46Z

@fehrnah Do you have anything else that you would like to add to this?

fehrnah · 2022-12-21T14:34:11Z

@fehrnah Do you have anything else that you would like to add to this?

No, this is all the functionality I wanted to add.

sgal · 2023-01-09T13:32:19Z

@fehrnah Could you please add some tests to cover new cases? I think you can use oauth2_auth_code_test.go as an example.

fehrnah · 2023-01-10T12:03:17Z

@fehrnah Could you please add some tests to cover new cases? I think you can use oauth2_auth_code_test.go as an example.

Sure, I'll try to do it for the end of next week.

sgal · 2023-01-30T12:34:50Z

@fehrnah In my company, we are currently relying on this change to be able to run the native factors login flow. With that, I took the liberty to complete your change (along with an extra thing) in a separate PR - #3427.

I assumed that you are tied up with work on other things to focus on this, so I took over. I hope it's ok. I did keep all your changes intact and added tests to make sure they are working as expected. Would love to have your review in my PR!

fehrnah · 2023-02-01T07:49:27Z

That's great !

I'll take a bit of time to have a look.

aeneasr · 2023-05-01T20:55:28Z

The follow up was merged: #3427

fehrnah requested a review from aeneasr as a code owner September 14, 2022 13:12

fehrnah changed the title ~~feat: Add token hooks for all grants types~~ feat: add token hooks for all grants types Sep 14, 2022

aeneasr requested changes Sep 14, 2022

View reviewed changes

sgal reviewed Sep 20, 2022

View reviewed changes

fehrnah force-pushed the dev/v2.x/token_hooks branch 2 times, most recently from 7da54c4 to aa4a43e Compare November 29, 2022 12:57

fehrnah force-pushed the dev/v2.x/token_hooks branch 3 times, most recently from a2b248f to 606bea0 Compare December 13, 2022 09:22

sgal reviewed Dec 13, 2022

View reviewed changes

fehrnah force-pushed the dev/v2.x/token_hooks branch from 606bea0 to 53f0588 Compare December 13, 2022 10:13

sgal reviewed Dec 13, 2022

View reviewed changes

fehrnah force-pushed the dev/v2.x/token_hooks branch from 53f0588 to f57f44c Compare December 13, 2022 12:46

Add extra token hooks

fbc185c

add client_credentials token hook add authorization_code token hook add jwt_profile token hook add relevant configuration

fehrnah force-pushed the dev/v2.x/token_hooks branch from f57f44c to fbc185c Compare December 14, 2022 09:51

sgal mentioned this pull request Dec 29, 2022

RFC7523: Store the payload of the supplied JWT for later use in token hook in Hydra ory/fosite#729

Closed

6 tasks

sgal mentioned this pull request Jan 29, 2023

feat: add token request hooks for all grant types #3427

Merged

7 tasks

aeneasr closed this May 1, 2023

	KeyAuthorizationCodeTokenURL = "oauth2.authorization_code_hook" // #nosec G101
	KeyAuthorizationCodeHookURL = "oauth2.authorization_code_hook" // #nosec G101

	"examples": ["https://my-example.app/token-jwt-profile-hook"]
	"examples": ["https://my-example.app/token-jwt-bearer-hook"]

Conversation

fehrnah commented Sep 14, 2022

Related issue(s)

Checklist

Uh oh!

codecov bot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fehrnah Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fehrnah Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgal commented Dec 21, 2022

Uh oh!

fehrnah commented Dec 21, 2022

Uh oh!

sgal commented Jan 9, 2023

Uh oh!

fehrnah commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgal commented Jan 30, 2023

Uh oh!

fehrnah commented Feb 1, 2023

Uh oh!

aeneasr commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 14, 2022 •

edited

Loading

fehrnah Dec 12, 2022 •

edited

Loading

fehrnah Dec 13, 2022 •

edited

Loading

fehrnah commented Jan 10, 2023 •

edited

Loading