replace MapClaims with structured ArgoClaims

Signed-off-by: Atif Ali <[email protected]>

cmd/argocd/commands/login.go

@@ -197,9 +197,21 @@ func userDisplayName(claims jwt.MapClaims) string {
 	if name := jwtutil.StringField(claims, "name"); name != "" {
 		return name
 	}
-	return utils.GetUserIdentifier(claims)
+	argoClaims := &utils.ArgoClaims{
+		RegisteredClaims: jwt.RegisteredClaims{
+			Subject: claims["sub"].(string),
+		},
+	}
+	if fedClaims, ok := claims["federated_claims"].(map[string]interface{}); ok {
+		argoClaims.FederatedClaims = &utils.FederatedClaims{
+			ConnectorID: fedClaims["connector_id"].(string),
+			UserID:      fedClaims["user_id"].(string),
+		}
+	}
+	return utils.GetUserIdentifier(argoClaims)
 }
 
+// oauth2Login opens a browser, runs a temporary HTTP server to delegate OAuth2 login flow and
 // oauth2Login opens a browser, runs a temporary HTTP server to delegate OAuth2 login flow and
 // returns the JWT token and a refresh token (if supported)
 func oauth2Login(

cmd/argocd/commands/login_test.go

@@ -62,3 +62,18 @@ func Test_ssoAuthFlow_ssoLaunchBrowser_false(t *testing.T) {
 
 	assert.Contains(t, out, "To authenticate, copy-and-paste the following URL into your preferred browser: http://test-sso-browser-flow.com")
 }
+
+func Test_userDisplayName_federatedClaims(t *testing.T) {
+	claims := jwt.MapClaims{
+		"iss":    "qux",
+		"sub":    "foo",
+		"groups": []string{"baz"},
+		"federated_claims": map[string]interface{}{
+			"connector_id": "dex",
+			"user_id":      "ldap-123",
+		},
+	}
+	actualName := userDisplayName(claims)
+	expectedName := "ldap-123"
+	assert.Equal(t, expectedName, actualName)
+}

cmd/argocd/commands/project_role.go

@@ -279,6 +279,22 @@ func tokenTimeToString(t int64) string {
 	return tokenTimeToString
 }
 
+func mapClaimsToArgoClaims(claims jwtgo.MapClaims) *utils.ArgoClaims {
+	sub := jwt.StringField(claims, "sub")
+	argoClaims := &utils.ArgoClaims{
+		RegisteredClaims: jwtgo.RegisteredClaims{
+			Subject: sub,
+		},
+	}
+	if fedClaims, ok := claims["federated_claims"].(map[string]interface{}); ok {
+		argoClaims.FederatedClaims = &utils.FederatedClaims{
+			ConnectorID: fmt.Sprint(fedClaims["connector_id"]),
+			UserID:      fmt.Sprint(fedClaims["user_id"]),
+		}
+	}
+	return argoClaims
+}
+
 // NewProjectRoleCreateTokenCommand returns a new instance of an `argocd proj role create-token` command
 func NewProjectRoleCreateTokenCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command {
 	var (
@@ -332,7 +348,7 @@ Create token succeeded for proj:test-project:test-role.
 			issuedAt, _ := jwt.IssuedAt(claims)
 			expiresAt := int64(jwt.Float64Field(claims, "exp"))
 			id := jwt.StringField(claims, "jti")
-			subject := utils.GetUserIdentifier(claims)
+			subject := utils.GetUserIdentifier(mapClaimsToArgoClaims(claims))
 
 			if !outputTokenOnly {
 				fmt.Printf("Create token succeeded for %s.\n", subject)

cmd/argocd/commands/utils/claims.go

@@ -1,25 +1,48 @@
 package utils
 
 import (
+	"fmt"
+
 	"github.com/golang-jwt/jwt/v4"
 )
 
-const (
-	federatedClaimsKey = "federated_claims"
-	userIDKey          = "user_id"
-	subKey             = "sub"
-)
+// ArgoClaims defines the claims structure based on Dex's documented claims
+type ArgoClaims struct {
+	jwt.RegisteredClaims
+	Email         string   `json:"email,omitempty"`
+	EmailVerified bool     `json:"email_verified,omitempty"`
+	Name          string   `json:"name,omitempty"`
+	Groups        []string `json:"groups,omitempty"`
+	// As per Dex docs, federated_claims has a specific structure
+	FederatedClaims *FederatedClaims `json:"federated_claims,omitempty"`
+}
 
-// GetUserIdentifier returns a consistent user identifier, checking federated_claims.user_id when Dex is in use
-func GetUserIdentifier(claims jwt.MapClaims) string {
-	if federatedClaims, ok := claims[federatedClaimsKey].(map[string]interface{}); ok {
-		if userID, exists := federatedClaims[userIDKey].(string); exists && userID != "" {
-			return userID
+// FederatedClaims represents the structure documented by Dex
+type FederatedClaims struct {
+	ConnectorID string `json:"connector_id"`
+	UserID      string `json:"user_id"`
+}
+
+// GetFederatedClaims extracts federated claims from jwt.MapClaims
+func GetFederatedClaims(claims jwt.MapClaims) *FederatedClaims {
+	if federated, ok := claims["federated_claims"].(map[string]interface{}); ok {
+		return &FederatedClaims{
+			ConnectorID: fmt.Sprint(federated["connector_id"]),
+			UserID:      fmt.Sprint(federated["user_id"]),
 		}
 	}
+	return nil
+}
+
+// GetUserIdentifier returns a consistent user identifier, checking federated_claims.user_id when Dex is in use
+func GetUserIdentifier(claims *ArgoClaims) string {
+	// Check federated claims first
+	if claims.FederatedClaims != nil && claims.FederatedClaims.UserID != "" {
+		return claims.FederatedClaims.UserID
+	}
 	// Fallback to sub
-	if sub, ok := claims[subKey].(string); ok && sub != "" {
-		return sub
+	if claims.Subject != "" {
+		return claims.Subject
 	}
 	return ""
 }

cmd/argocd/commands/utils/claims_test.go

@@ -10,39 +10,45 @@ import (
 func TestGetUserIdentifier(t *testing.T) {
 	tests := []struct {
 		name   string
-		claims jwt.MapClaims
+		claims *ArgoClaims
 		want   string
 	}{
 		{
-			name: "federated claims present",
-			claims: jwt.MapClaims{
-				"sub": "ignored:login",
-				"federated_claims": map[string]interface{}{
-					"user_id": "dex-user",
+			name: "when both dex and sub defined - prefer dex user_id",
+			claims: &ArgoClaims{
+				RegisteredClaims: jwt.RegisteredClaims{
+					Subject: "ignored:login",
+				},
+				FederatedClaims: &FederatedClaims{
+					UserID: "dex-user",
 				},
 			},
 			want: "dex-user",
 		},
 		{
-			name: "empty federated claims falls back to sub",
-			claims: jwt.MapClaims{
-				"sub": "test:apiKey",
-				"federated_claims": map[string]interface{}{
-					"user_id": "",
+			name: "when both dex and sub defined but dex user_id empty - fallback to sub",
+			claims: &ArgoClaims{
+				RegisteredClaims: jwt.RegisteredClaims{
+					Subject: "test:apiKey",
+				},
+				FederatedClaims: &FederatedClaims{
+					UserID: "",
 				},
 			},
 			want: "test:apiKey",
 		},
 		{
-			name: "no federated claims uses sub",
-			claims: jwt.MapClaims{
-				"sub": "admin:login",
+			name: "when only sub is defined (no dex) - use sub",
+			claims: &ArgoClaims{
+				RegisteredClaims: jwt.RegisteredClaims{
+					Subject: "admin:login",
+				},
 			},
 			want: "admin:login",
 		},
 		{
-			name:   "empty claims",
-			claims: jwt.MapClaims{},
+			name:   "when neither dex nor sub defined - return empty",
+			claims: &ArgoClaims{},
 			want:   "",
 		},
 	}

server/account/account.go

@@ -38,7 +38,7 @@ func NewServer(sessionMgr *session.SessionManager, settingsMgr *settings.Setting
 // UpdatePassword updates the password of the currently authenticated account or the account specified in the request.
 func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRequest) (*account.UpdatePasswordResponse, error) {
 	issuer := session.Iss(ctx)
-	username := session.Sub(ctx)
+	username := session.GetUserIdentifier(ctx)
 	updatedUsername := username
 
 	if q.Name != "" {
@@ -170,7 +170,7 @@ func toApiAccount(name string, a settings.Account) *account.Account {
 
 func (s *Server) ensureHasAccountPermission(ctx context.Context, action string, account string) error {
 	// account has always has access to itself
-	if session.Sub(ctx) == account && session.Iss(ctx) == session.SessionManagerClaimsIssuer {
+	if session.GetUserIdentifier(ctx) == account && session.Iss(ctx) == session.SessionManagerClaimsIssuer {
 		return nil
 	}
 	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, action, account); err != nil {

server/rbacpolicy/rbacpolicy.go

@@ -116,8 +116,14 @@ func (p *RBACPolicyEnforcer) EnforceClaims(claims jwt.Claims, rvals ...interface
 	if err != nil {
 		return false
 	}
+	argoClaims := &utils.ArgoClaims{
+		RegisteredClaims: jwt.RegisteredClaims{
+			Subject: jwtutil.StringField(mapClaims, "sub"),
+		},
+		FederatedClaims: utils.GetFederatedClaims(mapClaims),
+	}
 
-	subject := utils.GetUserIdentifier(mapClaims)
+	subject := utils.GetUserIdentifier(argoClaims)
 	// Check if the request is for an application resource. We have special enforcement which takes
 	// into consideration the project's token and group bindings
 	var runtimePolicy string

server/server.go

@@ -1534,6 +1534,15 @@ func (a *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, string, error
 			groupClaims = *tmpClaims
 		}
 	}
+
+	// Convert to ArgoClaims for user identifier comparison
+	argoClaims := &utils.ArgoClaims{
+		RegisteredClaims: jwt.RegisteredClaims{
+			Subject: jwtutil.StringField(groupClaims, "sub"),
+		},
+		FederatedClaims: utils.GetFederatedClaims(groupClaims),
+	}
+
 	iss := jwtutil.StringField(groupClaims, "iss")
 	if iss != util_session.SessionManagerClaimsIssuer && a.settings.UserInfoGroupsEnabled() && a.settings.UserInfoPath() != "" {
 		userInfo, unauthorized, err := a.ssoClientApp.GetUserInfo(groupClaims, a.settings.IssuerURL(), a.settings.UserInfoPath())
@@ -1545,7 +1554,13 @@ func (a *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, string, error
 			log.Errorf("error fetching user info endpoint: %v", err)
 			return claims, "", status.Errorf(codes.Internal, "invalid userinfo response")
 		}
-		if utils.GetUserIdentifier(groupClaims) != utils.GetUserIdentifier(userInfo) {
+		userInfoClaims := &utils.ArgoClaims{
+			RegisteredClaims: jwt.RegisteredClaims{
+				Subject: jwtutil.StringField(userInfo, "sub"),
+			},
+			FederatedClaims: utils.GetFederatedClaims(userInfo),
+		}
+		if utils.GetUserIdentifier(argoClaims) != utils.GetUserIdentifier(userInfoClaims) {
 			return claims, "", status.Error(codes.Unknown, "subject of claims from user info endpoint didn't match subject of idToken, see https://openid.net/specs/openid-connect-core-1_0.html#UserInfo")
 		}
 		groupClaims["groups"] = userInfo["groups"]

server/server_test.go

@@ -1644,3 +1644,115 @@ func Test_enforceContentTypes(t *testing.T) {
 		assert.Equal(t, http.StatusUnsupportedMediaType, resp.StatusCode, "should not have passed, since a disallowed content type was provided")
 	})
 }
+
+func TestGetClaimsWithFederatedIdentity(t *testing.T) {
+	defaultExpiry := jwt.NewNumericDate(time.Now().Add(time.Hour * 24))
+
+	tests := []struct {
+		name                  string
+		claims                jwt.MapClaims
+		expectedIdentifier    string
+		expectedErrorContains string
+	}{
+		{
+			name: "federated claims present - should use federated user_id",
+			claims: jwt.MapClaims{
+				"aud": "argo-cd",
+				"exp": defaultExpiry,
+				"sub": "different-id",
+				"federated_claims": map[string]interface{}{
+					"connector_id": "github",
+					"user_id":      "federated-user-12345",
+				},
+			},
+			expectedIdentifier:    "federated-user-12345",
+			expectedErrorContains: "",
+		},
+		{
+			name: "no federated claims - should fallback to sub",
+			claims: jwt.MapClaims{
+				"aud": "argo-cd",
+				"exp": defaultExpiry,
+				"sub": "fallback-sub-id",
+			},
+			expectedIdentifier:    "fallback-sub-id",
+			expectedErrorContains: "",
+		},
+		{
+			name: "empty federated claims - should fallback to sub",
+			claims: jwt.MapClaims{
+				"aud":              "argo-cd",
+				"exp":              defaultExpiry,
+				"sub":              "fallback-sub-id",
+				"federated_claims": map[string]interface{}{},
+			},
+			expectedIdentifier:    "fallback-sub-id",
+			expectedErrorContains: "",
+		},
+		{
+			name: "federated claims without user_id - should fallback to sub",
+			claims: jwt.MapClaims{
+				"aud": "argo-cd",
+				"exp": defaultExpiry,
+				"sub": "fallback-sub-id",
+				"federated_claims": map[string]interface{}{
+					"connector_id": "github",
+				},
+			},
+			expectedIdentifier:    "fallback-sub-id",
+			expectedErrorContains: "",
+		},
+		{
+			name: "no sub and no federated claims",
+			claims: jwt.MapClaims{
+				"aud": "argo-cd",
+				"exp": defaultExpiry,
+			},
+			expectedIdentifier:    "",
+			expectedErrorContains: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create test server
+			argocd, oidcURL := getTestServer(t, false, true, false, settings_util.OIDCConfig{})
+
+			// Add issuer to claims
+			tt.claims["iss"] = oidcURL
+
+			// Create token and context
+			token := jwt.NewWithClaims(jwt.SigningMethodRS512, tt.claims)
+			key, err := jwt.ParseRSAPrivateKeyFromPEM(testutil.PrivateKey)
+			require.NoError(t, err)
+			tokenString, err := token.SignedString(key)
+			require.NoError(t, err)
+			ctx := metadata.NewIncomingContext(context.Background(),
+				metadata.Pairs(apiclient.MetaDataTokenKey, tokenString))
+
+			// Test claims retrieval
+			gotClaims, _, err := argocd.getClaims(ctx)
+
+			if tt.expectedErrorContains != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.expectedErrorContains)
+				return
+			}
+			require.NoError(t, err)
+			mapClaims, ok := gotClaims.(jwt.MapClaims)
+			require.True(t, ok)
+
+			var actualIdentifier string
+			if fedClaims, ok := mapClaims["federated_claims"].(map[string]interface{}); ok {
+				if userID, exists := fedClaims["user_id"]; exists && userID != "" {
+					actualIdentifier = userID.(string)
+				}
+			}
+			if actualIdentifier == "" && mapClaims["sub"] != nil {
+				actualIdentifier = mapClaims["sub"].(string)
+			}
+
+			assert.Equal(t, tt.expectedIdentifier, actualIdentifier)
+		})
+	}
+}