-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
fix: Override sub with federated_claims.user_id when dex is used #20683
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
3f5b5c7
to
dee232e
Compare
eb08b64
to
e374df5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20683 +/- ##
==========================================
- Coverage 55.59% 55.50% -0.09%
==========================================
Files 339 340 +1
Lines 57346 57417 +71
==========================================
- Hits 31881 31870 -11
- Misses 22784 22847 +63
- Partials 2681 2700 +19 ☔ View full report in Codecov by Sentry. |
4cf2359
to
d131b41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall code change looks good!!
@jannfis @pasha-codefresh do you see any concerns with the change?
4e8b021
to
1393382
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from RegisterClaims to MapClaims makes this PR a lot more impactful and the blast radius is hard to grasp, especially that not the same claims seems to be added in the context object.
68b2da0
to
df53a24
Compare
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Hey @aali309. Thanks for the changes. The logic look good. I will push some style refactor and changes to the unit tests so we can merge this in the next version 🚀 🎉 |
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Fixes: #17908
Checklist: