Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fenech
Copy link
Contributor

@fenech fenech commented May 14, 2024

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

  • 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

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 fenech changed the title Pass OIDC claims into post-login flow to include in web hook context feat: Pass OIDC claims into post-login flow to include in web hook context May 14, 2024
@fenech fenech changed the title feat: Pass OIDC claims into post-login flow to include in web hook context feat: pass OIDC claims into post-login flow to include in web hook context May 14, 2024
@fenech fenech force-pushed the feature/oidc-claims-web-hook-context branch from 9f37d04 to 4b99a8c Compare May 23, 2024 11:14
@fenech fenech force-pushed the feature/oidc-claims-web-hook-context branch from 4b99a8c to 80bdfb0 Compare June 4, 2024 13:14
@fenech
Copy link
Contributor Author

fenech commented Jun 4, 2024

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 😄

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
Copy link

kghost commented Jun 6, 2024

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

Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, this is going into the right direction.

We're going to need docs as well.

selfservice/flow/login/hook.go Outdated Show resolved Hide resolved
@fenech fenech force-pushed the feature/oidc-claims-web-hook-context branch 3 times, most recently from 68cb3d9 to a717bfa Compare June 14, 2024 05:05
Copy link

codecov bot commented Jun 14, 2024

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 ⚠️
selfservice/strategy/oidc/provider_x.go 0.00% 3 Missing ⚠️
selfservice/strategy/oidc/provider_apple.go 50.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_github.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_github_app.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_lark.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_patreon.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_slack.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_spotify.go 0.00% 2 Missing ⚠️
selfservice/strategy/oidc/provider_dingtalk.go 66.66% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonas-jonas
jonas-jonas previously approved these changes Jun 14, 2024
Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. AFAICT, the missing test coverage is because we didn't have tests for the lines in the first place, and the changes are just renames.

LGTM, @zepatrik PTAL.

zepatrik
zepatrik previously approved these changes Jun 17, 2024
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff 🎉

@zepatrik
Copy link
Member

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
Copy link

@zepatrik

Another PR is open that implements the jsonnet flow: #3788

Not sure if that has been a consideration down the line

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking merge as I want to review this before merge :)

@fenech
Copy link
Contributor Author

fenech commented Jul 23, 2024

Anything I can do to help move this along, @aeneasr (aside from pinging you 😄 )?

@spijs
Copy link

spijs commented Jul 23, 2024

I did not look into the implementation in depth, but it seems like the settings flow has not been considered? This would probably solve the issue mentioned here #3816

As it is possible to set up new OIDC connections via the settings flow as well, would it make sense to also include the claims in the webhook there? For us it makes sense to have similar information as with registration to perform for example some sort of validation. While the login flow definitely makes sense to keep everything up to date, it does not fix the need for validation BEFORE accepting the new OIDC provider.

If that would already be the case, I would be very happy. Otherwise I feel like it could be a nice enhancement to this PR.

@fenech fenech dismissed stale reviews from zepatrik and jonas-jonas via 0c6372a August 26, 2024 15:28
@fenech fenech force-pushed the feature/oidc-claims-web-hook-context branch 3 times, most recently from 8d69e61 to 20db06f Compare August 30, 2024 09:43
@fenech
Copy link
Contributor Author

fenech commented Aug 30, 2024

As it is possible to set up new OIDC connections via the settings flow as well, would it make sense to also include the claims in the webhook there? For us it makes sense to have similar information as with registration to perform for example some sort of validation. While the login flow definitely makes sense to keep everything up to date, it does not fix the need for validation BEFORE accepting the new OIDC provider.

This isn't something that I'd considered, as our use case only involves self-registration and then login. Since we are already using a version of Kratos based on this branch, I'd quite like to keep the scope minimal to increase its chances of getting merged, but if the project members see it as a useful addition then I'm happy to make an update.

To be honest, this was always intended as a workaround for our use case, and for us it would be sufficient to just do the same Jsonnet mapping onto the identity at both registration and login time, as @zepatrik mentioned earlier in the thread.

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.
@fenech fenech force-pushed the feature/oidc-claims-web-hook-context branch from 20db06f to 3d6656f Compare October 1, 2024 09:38
@fenech
Copy link
Contributor Author

fenech commented Oct 1, 2024

I've rebased this onto latest master and resolved the conflicts. There are some e2e tests that are failing, but as far as I can tell, they're not related to these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants