ory / kratos

Next-gen identity server replacing your Auth0, Okta, Firebase with hardened security and PassKeys, SMS, OIDC, Social Sign In, MFA, FIDO, TOTP and OTP, WebAuthn, passwordless and much more. Golang, headless, API-first. Available as a worry-free SaaS with the fairest pricing on the market!
https://www.ory.sh/kratos/?utm_source=github&utm_medium=banner&utm_campaign=kratos
Apache License 2.0
10.81k stars 935 forks source link

feat: pass OIDC claims into post-login flow to include in web hook context #3922

Open fenech opened 1 month ago

fenech commented 1 month ago

The login flow doesn't trigger a refresh of the identity when the OIDC claims have changed. By passing the claims through to the web hook context, this means that an external handler can be configured to update the identity as appropriate, when there are changes.

Related issue(s)

2898

Checklist

Further Comments

The PR looks a lot bigger due to the fact that the Claims type has been moved into its own separate package, to resolve a circular dependency. I suspect that it is not in the right place and should be moved elsewhere.

The linked issue refers to refreshing the identity directly as part of the login flow, to handle the case where the OIDC Claims have changed.

However, my follow-up proposal is different: if instead of directly updating the identity, we just pass the OIDC Claims through to the web hook context, we allow users to write their own web hook handler to update the identity as they would like. I think it's a much less invasive change to Kratos (just passing an extra object through the login/post-login flow).

fenech commented 3 weeks ago

I'm periodically coming back to check the status of this PR and clicking "update" to rebase it, but I don't know if you'd prefer for me to just leave it as it is :smile:

I'm happy to contribute to some docs as well, just wanted to make sure the approach was good before starting to do that.

kghost commented 3 weeks ago

Yes, this is crutial, otherwise we can't keep SSO information up to date.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 78.00000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 78.19%. Comparing base (b192c92) to head (a717bfa).

Files Patch % Lines
selfservice/strategy/oidc/provider_discord.go 0.00% 3 Missing :warning:
selfservice/strategy/oidc/provider_x.go 0.00% 3 Missing :warning:
selfservice/strategy/oidc/provider_apple.go 50.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_github.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_github_app.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_lark.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_patreon.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_slack.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_spotify.go 0.00% 2 Missing :warning:
selfservice/strategy/oidc/provider_dingtalk.go 66.66% 1 Missing :warning:
... and 1 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3922 +/- ## ========================================== + Coverage 78.17% 78.19% +0.02% ========================================== Files 363 365 +2 Lines 25453 25459 +6 ========================================== + Hits 19898 19908 +10 + Misses 4032 4030 -2 + Partials 1523 1521 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zepatrik commented 2 weeks ago

I just checked and the overall issue with this approach is that the webhook target cannot update the identity on login:

Modifying the identity is currently only possible during the registration and settings flows.

However, I think that adding the claims to the webhook is an OK solution, but not necessarily for the root problem. It is useful to notify other systems, but there will always be some delay between the login and update of the identity through another API call. This will be especially a problem for integrations using session to JWT or some kind of short-term cache.

IMO we should still merge this PR. I would however consider either allowing the webhook to update the identity in the response (adds latency to login), and/or add some other "quick" path for cases where one only wants to mirror claims into the identity (i.e. jsonnet).

robinknaapen commented 2 weeks ago

@zepatrik

Another PR is open that implements the jsonnet flow: https://github.com/ory/kratos/pull/3788

Not sure if that has been a consideration down the line