fix(server): avoid unnecessary claims restrictions (#22973)#23202
fix(server): avoid unnecessary claims restrictions (#22973)#23202crenshaw-dev merged 3 commits intoargoproj:masterfrom
Conversation
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23202 +/- ##
==========================================
+ Coverage 60.01% 60.09% +0.07%
==========================================
Files 343 342 -1
Lines 57890 57845 -45
==========================================
+ Hits 34744 34763 +19
+ Misses 20362 20315 -47
+ Partials 2784 2767 -17 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR reverts the previous use of the ArgoClaims struct for extracting JWT claims and replaces it with a simpler approach that directly works with jwt.MapClaims. The changes are focused on:
- Removing the claimsutil.MapClaimsToArgoClaims conversion in favor of using the new utility function claimsutil.GetUserIdentifier.
- Updating various modules (session, server, rbac, and CLI commands) to work directly with jwt.MapClaims.
- Simplifying test cases by no longer relying on the ArgoClaims struct.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| util/session/sessionmanager_test.go | Removed dependency on ArgoClaims conversion in tests. |
| util/session/sessionmanager.go | Dropped ArgoClaims in favor of jwt.RegisteredClaims and GetUserIdentifier. |
| util/rbac/rbac.go | Replaced MapClaimsToArgoClaims with GetUserIdentifier for clarity. |
| util/claims/claims.go | Removed the ArgoClaims and FederatedClaims structs; consolidated GetUserIdentifier. |
| server/server.go | Updated claims retrieval logic to work directly with jwt.MapClaims. |
| server/rbacpolicy/rbacpolicy.go | Updated to use claimsutil.GetUserIdentifier. |
| cmd/argocd/commands/project_role.go | Updated token parsing and claims extraction logic. |
| cmd/argocd/commands/login_test.go | Modified tests to use jwt.MapClaims directly. |
| cmd/argocd/commands/login.go | Simplified userDisplayName to use jwtutil.StringField and GetUserIdentifier. |
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
agaudreault
left a comment
There was a problem hiding this comment.
Removing ArgoClaims type so we dont have an additional type until we want to refactor the claims to use our own type instead of a mix of MapClaims/RegisteredClaims with utility methods #21715
…rgoproj#23202) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
…rgoproj#23202) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
…rgoproj#23202) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
…rgoproj#23202) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: enneitex <etienne.divet@gmail.com>
#20683 introduced a new ArgoClaims struct to hold JWT information, specifically information about federated claims.
The struct enforces a particular JWT format. One problematic expectation is that the
groupsclaim be represented as an array rather than as a string (see #22973).The struct isn't necessary to accomplish the key task (retrieving information from the
federated_claimsclaim).I've reverted the parts of #20683 that relied on the new struct. Instead, I use a new utility function to pull info from
federated_claimsand fall back tosubwhen necessary.We can consider cherry-picking a smaller change (possibly just dropping the
Groupsfield from theArgoClaimsstruct. But I think introducing the struct was an error, and we should favor a simpler implementation going forward.