feat: add token request hooks for all grant types by sgal · Pull Request #3427 · ory/hydra

sgal · 2023-01-29T19:52:55Z

Added a generic token hook that is called for all grant types and includes payload with a single allowed value - assertion to cover the jwt-bearer grant type customization.

Existing refresh hook is left unchanged and will be deprecated in favor of the new hook.

Related issue(s)

#3244
ory/fosite#729

This change completes and extends the work done in #3254

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
[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 have added or changed the documentation.

Further Comments

Docs update PR - ory/docs#1241

codecov · 2023-01-29T20:12:49Z

Codecov Report

Merging #3427 (4b341d8) into master (a663927) will decrease coverage by 0.01%.
The diff coverage is 71.76%.

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

@@            Coverage Diff             @@
##           master    #3427      +/-   ##
==========================================
- Coverage   76.83%   76.82%   -0.01%     
==========================================
  Files         123      124       +1     
  Lines        9125     9160      +35     
==========================================
+ Hits         7011     7037      +26     
- Misses       1666     1673       +7     
- Partials      448      450       +2

Impacted Files	Coverage Δ
oauth2/refresh_hook.go	`67.64% <60.00%> (ø)`
oauth2/token_hook.go	`71.05% <71.05%> (ø)`
driver/config/provider.go	`81.85% <100.00%> (ø)`
driver/registry_base.go	`85.98% <100.00%> (+0.05%)`	⬆️

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

oauth2/hook.go

oauth2/oauth2_auth_code_test.go

hperl

This looks very good! I just had some minor questions & suggestions.

driver/config/provider.go

oauth2/hook.go

sgal · 2023-03-07T15:42:00Z

@hperl I fixed the comments, please have a look!

hperl

LGTM! 🎉

aeneasr

Very nice changes, just two minor remarks

oauth2/hook.go

sgal · 2023-03-24T09:14:00Z

@aeneasr I fixed the following parts:

Remove client_secret from the payload that is sent to the webhook
Only send the payload in the jwt-bearer grant type
Remove the data duplication in the webhook request

Please have a look.

sgal · 2023-03-24T09:14:29Z

Docs are also updated - ory/docs#1241

...ion=should_pass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured.json

aeneasr · 2023-03-24T10:57:46Z

spec/config.json

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


Do we need all of these config elements? Maybe it makes more sense to have one endpoint and pass the grant type and let the webhook deal with that?

Yeah, that makes much more sense. Let me take a stab at it.

Awesome thanks :)

To keep backwards compatibility we could also just add a new key "hook" and deprecate the refresh token hook at some point. That way we don't break BC?

oauth2/hook.go

sgal · 2023-03-24T15:46:23Z

@aeneasr Changed to having a generic token hook config, along with the existing one. Cleaned up the request payload for the new hook to remove duplication.

aeneasr

Very nice

sgal · 2023-03-26T10:56:39Z

@fehrnah FYI, it's finally in! Thanks for laying the foundation for this feature 👍

fehrnah · 2023-03-26T10:57:21Z

@fehrnah FYI, it's finally in! Thanks for laying the foundation for this feature 👍

Congratulations!

aeneasr · 2023-03-26T11:08:30Z

Thank you both :)

Added a generic token hook that is called for all grant types and includes `payload` with a single allowed value - `assertion` to cover the `jwt-bearer` grant type customization. The existing `refresh token hook` is left unchanged and is considered to be deprecated in favor of the new hook logic. The `refresh token hook` will at some point be removed. Closes ory#3244 Closes ory/fosite#729

sgal requested a review from aeneasr as a code owner January 29, 2023 19:52

sgal commented Jan 30, 2023

View reviewed changes

oauth2/hook.go Outdated Show resolved Hide resolved

sgal commented Jan 30, 2023

View reviewed changes

oauth2/oauth2_auth_code_test.go Show resolved Hide resolved

This was referenced Jan 30, 2023

feat: add token hooks for all grants types #3254

Closed

feat: update webhook documentation with more grant types ory/docs#1241

Merged

hperl self-requested a review March 6, 2023 09:53

hperl requested changes Mar 7, 2023

View reviewed changes

driver/config/provider.go Outdated Show resolved Hide resolved

oauth2/hook.go Outdated Show resolved Hide resolved

oauth2/hook.go Outdated Show resolved Hide resolved

hperl previously approved these changes Mar 10, 2023

View reviewed changes

aeneasr reviewed Mar 23, 2023

View reviewed changes

oauth2/hook.go Outdated Show resolved Hide resolved

oauth2/hook.go Outdated Show resolved Hide resolved

sgal added 6 commits March 23, 2023 19:55

feat: add token hooks for all grant types

2ca2980

Add tests for webhook in client_credentials grant_type

1e419f3

Add tests for webhook in jwt_bearer grant_type

05606f9

Add tests for hook in authorization_code grant_type

923557c

Fix openapi description fro payload field

4d5fb14

fix: address review comments

fb5bd4b

sgal dismissed hperl’s stale review via 3f75efd March 23, 2023 20:28

fix: strip sensitive values from the hook payload

272b1a7

aeneasr reviewed Mar 24, 2023

View reviewed changes

oauth2/hook.go Outdated Show resolved Hide resolved

sgal added 2 commits March 24, 2023 16:29

fix:make a generic token hook config that is run for all grant types

8a37a90

fix:add hook tests for authorization code grant

d06cdf0

aeneasr approved these changes Mar 26, 2023

View reviewed changes

aeneasr merged commit 9bdf225 into ory:master Mar 26, 2023

sgal deleted the feat-add-request-hooks-for-all-grant-types branch March 26, 2023 10:55

terev mentioned this pull request Jul 2, 2024

feat: pass associated oauth client data to token hook #3790

Open

7 tasks

Conversation

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

Uh oh!

Related issue(s)

Checklist

Further Comments

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

hperl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgal commented Mar 7, 2023

Uh oh!

hperl left a comment

Choose a reason for hiding this comment

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sgal commented Mar 24, 2023

Uh oh!

sgal commented Mar 24, 2023

Uh oh!

Uh oh!

aeneasr Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

sgal Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgal commented Mar 24, 2023

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

sgal commented Mar 26, 2023

Uh oh!

fehrnah commented Mar 26, 2023

Uh oh!

aeneasr commented Mar 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sgal commented Jan 29, 2023 •

edited

Loading

codecov bot commented Jan 29, 2023 •

edited

Loading