From 0c23020ecefdc42dc176d980c27ef8f11c9ddd1a Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Thu, 20 Aug 2026 20:24:45 +0000 Subject: [PATCH 1/6] fix(proxy): let users without a matching role claim log in A user whose OIDC role claim is missing, unreadable, or matches no entry in the proxy role_mapping could not log in: the proxy answered with a bare 500, so the web UI showed an access denied page whose 'log in again' button returned to the same page, because the login itself had succeeded. Clearing the browser cookies was the only way out. This is the normal state for users federated into the IDP from an external user directory, who have no role attached at all. Adds PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE, naming an ocis role to assign to those users. It is empty by default, so no existing deployment changes behaviour, and a matching role mapping always wins over it. It covers all three ways the claim can yield nothing, not just an unmatched value, because the reported case never reaches the mapping loop. When no default role is configured the login is still refused, but with ErrNoRoleAssigned, which the account resolver answers with 403 instead of 500, and the logged error names the claim, the values read from it and the configured mapping. Closes #11467. Signed-off-by: Sujeito Operator --- .../bugfix-proxy-oidc-default-role.md | 18 ++ .../examples/ocis_keycloak/docker-compose.yml | 6 + services/proxy/README.md | 35 ++- services/proxy/pkg/command/server.go | 1 + services/proxy/pkg/config/config.go | 5 +- .../proxy/pkg/middleware/account_resolver.go | 11 + .../pkg/middleware/account_resolver_test.go | 70 ++++++ services/proxy/pkg/userroles/oidcroles.go | 82 ++++++- .../proxy/pkg/userroles/oidcroles_test.go | 206 ++++++++++++++++++ services/proxy/pkg/userroles/userroles.go | 10 + 10 files changed, 429 insertions(+), 15 deletions(-) create mode 100644 changelog/unreleased/bugfix-proxy-oidc-default-role.md diff --git a/changelog/unreleased/bugfix-proxy-oidc-default-role.md b/changelog/unreleased/bugfix-proxy-oidc-default-role.md new file mode 100644 index 00000000000..56d1276579b --- /dev/null +++ b/changelog/unreleased/bugfix-proxy-oidc-default-role.md @@ -0,0 +1,18 @@ +Bugfix: Users without a matching role claim were stuck in a login loop + +Users whose OIDC role claim was missing, unreadable, or matched no entry in the proxy's +`role_mapping` could not log in. The proxy answered with a bare `500 Internal Server +Error`, so the web UI showed an access denied page whose "log in again" button returned +to the same page, because the login itself had succeeded. The only way out was clearing +the browser's cookies. This is common when users are federated into the IDP from an +external user directory and simply have no role attached. + +Such a request is now answered with `403 Forbidden`, and the logged error names the +claim that was read, the values found in it and the configured role mapping. + +We've also added `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE`, which names an ocis role to +assign to those users instead of refusing them. It applies when the claim is missing, +when it cannot be read, and when it matches no mapping; a mapping that does match always +wins over it. It is empty by default, so no existing deployment changes behavior. + +https://github.com/owncloud/ocis/issues/11467 diff --git a/deployments/examples/ocis_keycloak/docker-compose.yml b/deployments/examples/ocis_keycloak/docker-compose.yml index 7b3a885af96..859a29786e1 100644 --- a/deployments/examples/ocis_keycloak/docker-compose.yml +++ b/deployments/examples/ocis_keycloak/docker-compose.yml @@ -61,6 +61,12 @@ services: # Keycloak IDP specific configuration PROXY_AUTOPROVISION_ACCOUNTS: "true" PROXY_ROLE_ASSIGNMENT_DRIVER: "oidc" + # With the "oidc" driver a user needs a role claim that matches one of the + # configured role mappings, otherwise the login is refused with 403. The users + # this example creates have one. Users federated into Keycloak from an external + # user directory usually do not, so uncomment the line below to give them a + # low-privilege role instead of refusing them. + # PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE: "user-light" OCIS_OIDC_ISSUER: https://${KEYCLOAK_DOMAIN:-keycloak.owncloud.test}/realms/${KEYCLOAK_REALM:-oCIS} PROXY_OIDC_REWRITE_WELLKNOWN: "true" WEB_OIDC_CLIENT_ID: ${OCIS_OIDC_CLIENT_ID:-web} diff --git a/services/proxy/README.md b/services/proxy/README.md index 3d01ef255b1..3e71ae82b42 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -259,7 +259,40 @@ to the user. So if e.g. a user's `ocisRoles` claim has the values `myUserRole` a appears before `user` in the above sample configuration). If a user's claim values don't match any of the configured role mappings an error will be logged and -the user will not be able to login. +the user will not be able to login. The proxy answers such a request with `403 Forbidden`, and the +logged error names the claim it read, the values it found and the configured mapping. See +[Default Role](#default-role) for how to let those users log in instead. + +#### Default Role + +Users that reach Infinite Scale without any usable role claim cannot log in at all. This is common +when users are federated into the IDP from an external user directory: they authenticate correctly, +but no role is attached to them, so no `role_mapping` entry can match. The web UI then shows an +access denied page, and using its "log in again" button returns to the same page, because the login +itself succeeded. + +Setting `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE` (or `default_role` in the `oidc_role_mapper` +section) gives those users a role instead of refusing them: + +```yaml +role_assignment: + driver: oidc + oidc_role_mapper: + role_claim: ocisRoles + default_role: user-light + role_mapping: + - role_name: admin + claim_value: myAdminRole + - role_name: user + claim_value: myUserRole +``` + +The default role applies when the role claim is missing entirely, when it cannot be read, and when +it is present but matches no `role_mapping` entry. A mapping that does match always wins over it. + +This setting is empty by default, which keeps the behavior described above: such logins are refused. +Because the default role is handed to everyone the mappings do not cover, prefer a low-privilege role +such as `user-light` over `user` or `admin`. The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The default `role_mapping` is: diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 6f9d2482073..6401e26ad3e 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -263,6 +263,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, userroles.WithLogger(logger), userroles.WithRolesClaim(cfg.RoleAssignment.OIDCRoleMapper.RoleClaim), userroles.WithRoleMapping(cfg.RoleAssignment.OIDCRoleMapper.RolesMap), + userroles.WithDefaultRole(cfg.RoleAssignment.OIDCRoleMapper.DefaultRole), userroles.WithRevaGatewaySelector(gatewaySelector), userroles.WithServiceAccount(cfg.ServiceAccount), ) diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 3587b189ddf..b53f82cd67a 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -159,8 +159,9 @@ type RoleAssignment struct { // OIDCRoleMapper contains the configuration for the "oidc" role assignment driver type OIDCRoleMapper struct { - RoleClaim string `yaml:"role_claim" env:"PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM" desc:"The OIDC claim used to create the users role assignment." introductionVersion:"pre5.0"` - RolesMap []RoleMapping `yaml:"role_mapping" desc:"A list of mappings of ocis role names to PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM claim values. This setting can only be configured in the configuration file and not via environment variables."` + RoleClaim string `yaml:"role_claim" env:"PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM" desc:"The OIDC claim used to create the users role assignment." introductionVersion:"pre5.0"` + RolesMap []RoleMapping `yaml:"role_mapping" desc:"A list of mappings of ocis role names to PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM claim values. This setting can only be configured in the configuration file and not via environment variables."` + DefaultRole string `yaml:"default_role" env:"PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE" desc:"The name of the ocis role to assign when the user's PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM claim is missing or matches no entry in 'role_mapping'. Empty by default, which means such a login is refused. Useful when users are federated into the IDP without a role, e.g. from an external user directory." introductionVersion:"NEXT"` } // RoleMapping defines which ocis role matches a specific claim value diff --git a/services/proxy/pkg/middleware/account_resolver.go b/services/proxy/pkg/middleware/account_resolver.go index ea6755b5b42..42ae2364c8e 100644 --- a/services/proxy/pkg/middleware/account_resolver.go +++ b/services/proxy/pkg/middleware/account_resolver.go @@ -190,6 +190,17 @@ func (m accountResolver) resolveUserFromClaims(w http.ResponseWriter, req *http. role = m.guestRoleName } user, err = m.userRoleAssigner.UpdateUserRoleAssignment(ctx, user, claims, role) + if errors.Is(err, userroles.ErrNoRoleAssigned) { + // The user authenticated successfully but has no role in this instance. That is + // a property of their account, not a server error, so report it as one: a 500 + // here is indistinguishable from a broken deployment and leaves the user + // clicking "log in again" in a loop with nothing actionable anywhere. + m.logger.Error().Err(err). + Str("userid", user.GetId().GetOpaqueId()). + Msg("User has no role in this instance. Check the proxy role assignment configuration.") + w.WriteHeader(http.StatusForbidden) + return + } if err != nil { m.logger.Error().Err(err).Msg("Could not get user roles") w.WriteHeader(http.StatusInternalServerError) diff --git a/services/proxy/pkg/middleware/account_resolver_test.go b/services/proxy/pkg/middleware/account_resolver_test.go index f89e88f63be..64888c7936e 100644 --- a/services/proxy/pkg/middleware/account_resolver_test.go +++ b/services/proxy/pkg/middleware/account_resolver_test.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -11,6 +12,7 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/oidc" "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend" "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend/mocks" + "github.com/owncloud/ocis/v2/services/proxy/pkg/userroles" userRoleMocks "github.com/owncloud/ocis/v2/services/proxy/pkg/userroles/mocks" "github.com/owncloud/reva/v2/pkg/auth/scope" revactx "github.com/owncloud/reva/v2/pkg/ctx" @@ -472,3 +474,71 @@ func TestResolveUserType(t *testing.T) { }) } } + +// newMockAccountResolverWithRoleError builds the same middleware as +// newMockAccountResolver but lets the role assigner fail, which is the branch these two +// tests are about. It is a separate constructor rather than an extra parameter on the +// existing one so the other tests keep reading as they did. +func newMockAccountResolverWithRoleError(roleErr error) http.Handler { + user := &userv1beta1.User{ + Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Mail: "foo@example.com", + } + + tokenManager, _ := jwt.New(map[string]interface{}{ + "secret": "change-me", + "expires": int64(60), + }) + s, _ := scope.AddOwnerScope(nil) + token, _ := tokenManager.MintToken(context.Background(), user, s) + + ub := mocks.UserBackend{} + ub.On("GetUserByClaims", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(user, token, nil) + ub.On("GetUserRoles", mock.Anything, mock.Anything).Return(user, nil) + + ra := userRoleMocks.UserRoleAssigner{} + ra.On("UpdateUserRoleAssignment", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, roleErr) + + return AccountResolver( + Logger(log.NewLogger()), + UserProvider(&ub), + UserRoleAssigner(&ra), + SkipUserInfo(false), + UserOIDCClaim(oidc.Email), + UserCS3Claim("mail"), + AutoprovisionAccounts(false), + )(mockHandler{}) +} + +// TestForbiddenWhenTheUserHasNoRole is the user-visible half of the fix. A user who +// authenticated but holds no role in this instance used to get a bare 500, which is +// indistinguishable from a broken deployment and leaves them clicking "log in again" +// with nothing actionable anywhere. +func TestForbiddenWhenTheUserHasNoRole(t *testing.T) { + sut := newMockAccountResolverWithRoleError(userroles.ErrNoRoleAssigned) + req, rw := mockRequest(map[string]interface{}{ + oidc.Iss: "https://idx.example.com", + oidc.Email: "foo@example.com", + }) + + sut.ServeHTTP(rw, req) + + assert.Equal(t, http.StatusForbidden, rw.Code) + assert.Empty(t, req.Header.Get(revactx.TokenHeader)) +} + +// TestInternalServerErrorOnOtherRoleAssignmentErrors is the control. Only +// ErrNoRoleAssigned changes status; every other failure of the role assigner is still a +// server error, so the new branch cannot quietly turn real outages into 403s. +func TestInternalServerErrorOnOtherRoleAssignmentErrors(t *testing.T) { + sut := newMockAccountResolverWithRoleError(errors.New("settings service unreachable")) + req, rw := mockRequest(map[string]interface{}{ + oidc.Iss: "https://idx.example.com", + oidc.Email: "foo@example.com", + }) + + sut.ServeHTTP(rw, req) + + assert.Equal(t, http.StatusInternalServerError, rw.Code) + assert.Empty(t, req.Header.Get(revactx.TokenHeader)) +} diff --git a/services/proxy/pkg/userroles/oidcroles.go b/services/proxy/pkg/userroles/oidcroles.go index 645f17fb66b..85e2b8d0d40 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -3,7 +3,9 @@ package userroles import ( "context" "errors" + "fmt" "regexp" + "sort" "sync" "time" @@ -15,9 +17,16 @@ import ( "github.com/owncloud/ocis/v2/services/graph/pkg/identity" revactx "github.com/owncloud/reva/v2/pkg/ctx" "github.com/owncloud/reva/v2/pkg/utils" + "github.com/rs/zerolog" "go-micro.dev/v4/metadata" ) +// ErrNoRoleAssigned is returned by UpdateUserRoleAssignment when the user's claims +// yield no ocis role and no default role is configured. It is a property of the +// user's account rather than a server-side failure, so callers should report it as +// such instead of as a generic internal error. +var ErrNoRoleAssigned = errors.New("no role in claim maps to an ocis role and no default role is configured") + type oidcRoleAssigner struct { Options } @@ -108,16 +117,20 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c roleIDFromClaim := roleNamesToRoleIDs[overwriteRole] if overwriteRole == "" { + // A user whose claims yield no usable role fails in one of three places below. + // All three are the same situation for the person logging in, so they share a + // single exit: fall back to the configured default role, or return + // ErrNoRoleAssigned so the caller can report something better than a 500. claimRoles, err := extractRoles(ra.rolesClaim, claims) - if err != nil { - logger.Error().Err(err).Str("Claim", ra.rolesClaim).Interface("claims", claims).Msg("Error mapping role names to role ids") - return nil, err - } - - if len(claimRoles) == 0 { - err := errors.New("no roles set in claim") - logger.Error().Err(err).Msg("") - return nil, err + switch { + case err != nil: + // The claim is absent or unusable. This is the common case when users are + // federated into the IDP from an external directory and simply have no + // role attached. + logger.Debug().Err(err).Str("Claim", ra.rolesClaim).Msg("Could not extract roles from claims") + claimRoles = nil + case len(claimRoles) == 0: + logger.Debug().Str("Claim", ra.rolesClaim).Msg("No roles set in claim") } // the roleMapping config is supposed to have the role mappings ordered from the highest privileged role @@ -132,9 +145,10 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c } if roleIDFromClaim == "" { - err := errors.New("no role in claim maps to an ocis role") - logger.Error().Err(err).Msg("") - return nil, err + roleIDFromClaim, err = ra.fallbackRoleID(logger, roleNamesToRoleIDs, claimRoles) + if err != nil { + return nil, err + } } } @@ -205,6 +219,50 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c return user, nil } +// fallbackRoleID resolves the configured default role for a user whose claims matched +// no role mapping. It returns ErrNoRoleAssigned when no default role is configured, and +// a descriptive error when one is configured but does not exist in the settings +// service - a misconfiguration is worth reporting differently from a user without a +// role, because only one of the two is fixed by editing the deployment. +func (ra oidcRoleAssigner) fallbackRoleID(logger zerolog.Logger, roleNamesToRoleIDs map[string]string, claimRoles map[string]struct{}) (string, error) { + seen := make([]string, 0, len(claimRoles)) + for cr := range claimRoles { + seen = append(seen, cr) + } + sort.Strings(seen) + + if ra.defaultRole == "" { + logger.Error(). + Str("claim", ra.rolesClaim). + Strs("claimValues", seen). + Interface("roleMapping", ra.roleMapping). + Msg("No role mapping matched the user's claim and PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE is not set. " + + "Add a matching entry to the role mapping, or set a default role to let such users log in.") + return "", ErrNoRoleAssigned + } + + roleID := roleNamesToRoleIDs[ra.defaultRole] + if roleID == "" { + known := make([]string, 0, len(roleNamesToRoleIDs)) + for name := range roleNamesToRoleIDs { + known = append(known, name) + } + sort.Strings(known) + err := fmt.Errorf("the configured default role %q does not exist", ra.defaultRole) + logger.Error().Err(err). + Strs("knownRoles", known). + Msg("PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE names a role that the settings service does not know") + return "", err + } + + logger.Debug(). + Str("claim", ra.rolesClaim). + Strs("claimValues", seen). + Str("ocisRole", ra.defaultRole). + Msg("No role mapping matched, assigning the configured default role") + return roleID, nil +} + // ApplyUserRole it looks up the user's role in the settings service and adds it // user's opaque data func (ra oidcRoleAssigner) ApplyUserRole(ctx context.Context, user *cs3user.User) (*cs3user.User, error) { diff --git a/services/proxy/pkg/userroles/oidcroles_test.go b/services/proxy/pkg/userroles/oidcroles_test.go index 0fba777b5fb..340a87eeace 100644 --- a/services/proxy/pkg/userroles/oidcroles_test.go +++ b/services/proxy/pkg/userroles/oidcroles_test.go @@ -3,6 +3,8 @@ package userroles import ( "context" "encoding/json" + "errors" + "strings" "testing" "time" @@ -270,3 +272,207 @@ func TestUpdateUserRoleAssignmentFailsClosedOnInconclusivePermission(t *testing. return req.GetRoleId() == oldRoleID }), mock.Anything) } + +// newRoleAssignerFixture builds an oidcRoleAssigner whose settings service knows the +// roles in knownRoles and reports the user as already holding assignedRoleID. Holding +// the role the assigner is about to pick short-circuits the re-assignment branch, which +// keeps these tests on the claim-to-role resolution they are about rather than on the +// personal-space reconciliation that follows it. +func newRoleAssignerFixture(t *testing.T, opts Options, knownRoles map[string]string, assignedRoleID string) oidcRoleAssigner { + t.Helper() + + // the role-name cache is a package global with a 5 minute TTL; reset it so tests + // cannot leak role maps into one another + roleNameToID.lock.Lock() + roleNameToID.roleNameToID = nil + roleNameToID.lastRead = time.Time{} + roleNameToID.lock.Unlock() + + gatewayClient := &cs3mocks.GatewayAPIClient{} + selectorName := "GatewaySelector" + t.Name() + gatewaySelector := pool.GetSelector[gateway.GatewayAPIClient]( + selectorName, + "com.owncloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return gatewayClient + }, + ) + t.Cleanup(func() { pool.RemoveSelector(selectorName + "com.owncloud.api.gateway") }) + + gatewayClient.On("Authenticate", mock.Anything, mock.Anything).Return(&gateway.AuthenticateResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_OK}, + Token: "service-token", + }, nil) + + bundles := make([]*settingsmsg.Bundle, 0, len(knownRoles)) + for name, id := range knownRoles { + bundles = append(bundles, &settingsmsg.Bundle{Id: id, Name: name}) + } + roleService := &graphmocks.RoleService{} + roleService.On("ListRoles", mock.Anything, mock.Anything, mock.Anything).Return( + &settingssvc.ListBundlesResponse{Bundles: bundles}, nil) + roleService.On("ListRoleAssignments", mock.Anything, mock.Anything, mock.Anything).Return( + &settingssvc.ListRoleAssignmentsResponse{Assignments: []*settingsmsg.UserRoleAssignment{ + {Id: "assignment-id", AccountUuid: "user-1", RoleId: assignedRoleID}, + }}, nil) + // Mocked so that an unexpected re-assignment shows up as a failed assertion rather + // than as a panic in an unrelated place. + roleService.On("AssignRoleToUser", mock.Anything, mock.Anything, mock.Anything).Return( + &settingssvc.AssignRoleToUserResponse{Assignment: &settingsmsg.UserRoleAssignment{ + Id: "assignment-id", AccountUuid: "user-1", RoleId: assignedRoleID, + }}, nil) + + opts.logger = log.NopLogger() + opts.gatewaySelector = gatewaySelector + opts.roleService = roleService + opts.serviceAccount = config.ServiceAccount{ServiceAccountID: "service-account", ServiceAccountSecret: "secret"} + return oidcRoleAssigner{Options: opts} +} + +func assignedRoleFromOpaque(t *testing.T, user *cs3user.User) []string { + t.Helper() + entry := user.GetOpaque().GetMap()["roles"] + if entry == nil { + t.Fatal("the user's opaque data carries no roles entry") + } + var got []string + if err := json.Unmarshal(entry.GetValue(), &got); err != nil { + t.Fatalf("could not decode the roles opaque entry: %v", err) + } + return got +} + +// TestUpdateUserRoleAssignmentFallsBackToDefaultRoleOnUnmatchedClaim covers the case the +// issue names directly: the claim is present but its value matches no role mapping. +func TestUpdateUserRoleAssignmentFallsBackToDefaultRoleOnUnmatchedClaim(t *testing.T) { + const defaultRoleID = "user-light-id" + + ra := newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + defaultRole: "user-light", + }, + map[string]string{"admin": "admin-id", "user-light": defaultRoleID}, + defaultRoleID, + ) + + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, map[string]interface{}{"roles": "somethingElse"}, "") + if err != nil { + t.Fatalf("expected the default role to be applied, got error: %v", err) + } + if roles := assignedRoleFromOpaque(t, got); len(roles) != 1 || roles[0] != defaultRoleID { + t.Fatalf("expected the default role id %q, got %v", defaultRoleID, roles) + } +} + +// TestUpdateUserRoleAssignmentFallsBackToDefaultRoleWithoutAnyClaim is the case that +// actually reproduces the reported bug. Users federated into the IDP from an external +// directory have no role claim at all, so extractRoles fails outright - a fallback that +// only covered "claim present but unmatched" would not fix the report. +func TestUpdateUserRoleAssignmentFallsBackToDefaultRoleWithoutAnyClaim(t *testing.T) { + const defaultRoleID = "user-light-id" + + ra := newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + defaultRole: "user-light", + }, + map[string]string{"admin": "admin-id", "user-light": defaultRoleID}, + defaultRoleID, + ) + + // no "roles" key whatsoever + claims := map[string]interface{}{"sub": "abcd", "email": "federated@example.org"} + + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, claims, "") + if err != nil { + t.Fatalf("expected the default role to be applied, got error: %v", err) + } + if roles := assignedRoleFromOpaque(t, got); len(roles) != 1 || roles[0] != defaultRoleID { + t.Fatalf("expected the default role id %q, got %v", defaultRoleID, roles) + } +} + +// TestUpdateUserRoleAssignmentPrefersAMatchingMappingOverTheDefaultRole guards the +// fallback against swallowing normal operation. +func TestUpdateUserRoleAssignmentPrefersAMatchingMappingOverTheDefaultRole(t *testing.T) { + const adminRoleID = "admin-id" + + ra := newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + defaultRole: "user-light", + }, + map[string]string{"admin": adminRoleID, "user-light": "user-light-id"}, + adminRoleID, + ) + + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, map[string]interface{}{"roles": "ocisAdmin"}, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if roles := assignedRoleFromOpaque(t, got); len(roles) != 1 || roles[0] != adminRoleID { + t.Fatalf("expected the mapped role id %q, got %v", adminRoleID, roles) + } +} + +// TestUpdateUserRoleAssignmentWithoutDefaultRoleReturnsErrNoRoleAssigned pins the +// behaviour when no default role is configured. It must stay an error - this change does +// not hand out a role to anybody who was previously refused - but it must be one the +// caller can recognise, which is what turns the opaque 500 into a 403. +func TestUpdateUserRoleAssignmentWithoutDefaultRoleReturnsErrNoRoleAssigned(t *testing.T) { + ra := newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + }, + map[string]string{"admin": "admin-id"}, + "admin-id", + ) + + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + for name, claims := range map[string]map[string]interface{}{ + "unmatched claim": {"roles": "somethingElse"}, + "no claim at all": {"sub": "abcd"}, + } { + t.Run(name, func(t *testing.T) { + _, err := ra.UpdateUserRoleAssignment(context.Background(), user, claims, "") + if !errors.Is(err, ErrNoRoleAssigned) { + t.Fatalf("expected ErrNoRoleAssigned, got %v", err) + } + }) + } +} + +// TestUpdateUserRoleAssignmentReportsAnUnknownDefaultRole separates a deployment +// mistake from a user without a role: only one of the two is fixed by editing the +// configuration, so they must not return the same error. +func TestUpdateUserRoleAssignmentReportsAnUnknownDefaultRole(t *testing.T) { + ra := newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + defaultRole: "no-such-role", + }, + map[string]string{"admin": "admin-id"}, + "admin-id", + ) + + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + _, err := ra.UpdateUserRoleAssignment(context.Background(), user, map[string]interface{}{"roles": "somethingElse"}, "") + if err == nil { + t.Fatal("expected an error for a default role that does not exist") + } + if errors.Is(err, ErrNoRoleAssigned) { + t.Fatalf("a misconfigured default role must not be reported as ErrNoRoleAssigned, got %v", err) + } + if !strings.Contains(err.Error(), "no-such-role") { + t.Fatalf("the error should name the offending role, got %v", err) + } +} diff --git a/services/proxy/pkg/userroles/userroles.go b/services/proxy/pkg/userroles/userroles.go index 7ec62472e29..a8067425d07 100644 --- a/services/proxy/pkg/userroles/userroles.go +++ b/services/proxy/pkg/userroles/userroles.go @@ -28,6 +28,7 @@ type Options struct { roleService settingssvc.RoleService rolesClaim string roleMapping []config.RoleMapping + defaultRole string serviceAccount config.ServiceAccount logger log.Logger } @@ -63,6 +64,15 @@ func WithRoleMapping(roleMap []config.RoleMapping) Option { } } +// WithDefaultRole configures the ocis role to fall back to when the user's claims +// yield no role that maps to an ocis role. An empty value keeps the previous +// behaviour of refusing the login. +func WithDefaultRole(role string) Option { + return func(o *Options) { + o.defaultRole = role + } +} + // WithRevaGatewaySelector set the gatewaySelector option func WithRevaGatewaySelector(selectable pool.Selectable[gateway.GatewayAPIClient]) Option { return func(o *Options) { From e16c8bb5e811ba53fc3870f7b7c2c7d618129e70 Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Thu, 20 Aug 2026 21:31:50 +0000 Subject: [PATCH 2/6] test(e2e): cover login without a matching role claim Adds specs/keycloak/defaultRole.spec.ts, the Playwright scenario requested on the pull request. A Keycloak user created with no ocis realm role carries nothing the proxy role_mapping can match, which is the state #11467 reports; the spec asserts that such a user can log in and lands on an empty personal space and an empty project space list. Alice, who does have the ocisUser realm role, runs first in the same test as the control that a matching role_mapping entry still wins over the new fallback. createUserWithoutRealmRole skips the assignRole call that createUser makes, and deliberately does not call initializeUser: that first login is what the test exercises, and until it happens the user does not exist in ocis at all. run-e2e.py sets PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE for the keycloak suite. The value is a settings service role name, 'user', not the display name 'User'. The ocis_full example gets the same commented-out line the ocis_keycloak example already carries. Signed-off-by: Sujeito Operator --- deployments/examples/ocis_full/keycloak.yml | 6 ++ tests/acceptance/run-e2e.py | 8 +++ .../e2e/specs/keycloak/defaultRole.spec.ts | 61 +++++++++++++++++++ web/tests/e2e/steps/api/api.ts | 18 ++++++ web/tests/e2e/steps/ui/spaces.ts | 18 ++++++ web/tests/e2e/support/api/keycloak/user.ts | 47 ++++++++++++++ .../objects/app-files/page/spaces/personal.ts | 8 ++- .../objects/app-files/page/spaces/projects.ts | 9 ++- 8 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 web/tests/e2e/specs/keycloak/defaultRole.spec.ts diff --git a/deployments/examples/ocis_full/keycloak.yml b/deployments/examples/ocis_full/keycloak.yml index 05d75316bc3..589f6c0fe49 100644 --- a/deployments/examples/ocis_full/keycloak.yml +++ b/deployments/examples/ocis_full/keycloak.yml @@ -11,6 +11,12 @@ services: # Keycloak IDP specific configuration PROXY_AUTOPROVISION_ACCOUNTS: "true" PROXY_ROLE_ASSIGNMENT_DRIVER: "oidc" + # With the "oidc" driver a user needs a role claim that matches one of the + # configured role mappings, otherwise the login is refused with 403. The users + # this example creates have one. Users federated into Keycloak from an external + # user directory usually do not, so uncomment the line below to give them a + # low-privilege role instead of refusing them. + # PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE: "user-light" OCIS_OIDC_ISSUER: https://${KEYCLOAK_DOMAIN:-keycloak.owncloud.test}/realms/${KEYCLOAK_REALM:-oCIS} PROXY_OIDC_REWRITE_WELLKNOWN: "true" WEB_OIDC_CLIENT_ID: ${OCIS_OIDC_CLIENT_ID:-web} diff --git a/tests/acceptance/run-e2e.py b/tests/acceptance/run-e2e.py index 3d5fdaffe86..b8daecbd675 100755 --- a/tests/acceptance/run-e2e.py +++ b/tests/acceptance/run-e2e.py @@ -314,6 +314,14 @@ def _patch_urls(obj, old, new): "PROXY_AUTOPROVISION_CLAIM_GROUPS": "groups", "PROXY_AUTOPROVISION_GROUP_CREATE": "true", "PROXY_ROLE_ASSIGNMENT_DRIVER": "oidc", + # Fallback for users whose token matches no role_mapping entry, e.g. + # users federated into the realm without an ocis role. Empty by + # default; specs/keycloak/defaultRole.spec.ts needs it set. A + # matching role_mapping entry still wins, so the other keycloak + # specs are unaffected. The value is a settings-service role NAME + # ("user"), not a display name ("User") -- see + # services/settings/pkg/store/defaults/defaults.go. + "PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE": "user", "OCIS_OIDC_ISSUER": "https://localhost:8443/realms/oCIS", "PROXY_OIDC_REWRITE_WELLKNOWN": "true", "WEB_OIDC_CLIENT_ID": "web", diff --git a/web/tests/e2e/specs/keycloak/defaultRole.spec.ts b/web/tests/e2e/specs/keycloak/defaultRole.spec.ts new file mode 100644 index 00000000000..40e0165e394 --- /dev/null +++ b/web/tests/e2e/specs/keycloak/defaultRole.spec.ts @@ -0,0 +1,61 @@ +import { test } from '../../environment/test' +import * as api from '../../steps/api/api.js' +import * as ui from '../../steps/ui/index' + +// Regression test for https://github.com/owncloud/ocis/issues/11467. +// +// A Keycloak user who carries no ocis realm role produces a token the proxy +// role_mapping cannot match. Before the fix the proxy answered that login with a +// bare 500 and the web UI showed an access denied page whose "log in again" button +// returned to the same page, because the login itself had succeeded -- clearing the +// browser cookies was the only way out. That is the normal state for users +// federated into the realm from an external user directory. +// +// PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE names the role to fall back to. It is +// empty by default; tests/acceptance/run-e2e.py sets it to "User" for the keycloak +// suite. A matching role_mapping entry still wins over it, which is why Alice -- +// created the normal way, with the ocisUser realm role -- is exercised first in the +// same test. +test.describe('oidc default role', () => { + test('user with no ocis role in the token can log in', async () => { + // Given "Admin" creates following user using API + // | id | + // | Alice | + await api.usersHaveBeenCreated({ stepUser: 'Admin', users: ['Alice'] }) + + // When "Alice" logs in + // Alice has the ocisUser realm role, so she is assigned through the role + // mapping and never touches the default role. + await ui.userLogsIn({ stepUser: 'Alice' }) + // And "Alice" navigates to the personal space page + await ui.userNavigatesToPersonalSpacePage({ stepUser: 'Alice' }) + // Then "Alice" should see an empty personal space + await ui.userShouldSeeEmptyPersonalSpace({ stepUser: 'Alice' }) + // And "Alice" logs out + await ui.userLogsOut({ stepUser: 'Alice' }) + + // When "Admin" creates the following user in Keycloak without any ocis realm role + // | id | + // | Brian | + await api.keycloakUsersWithoutRoleHaveBeenCreated({ stepUser: 'Admin', users: ['Brian'] }) + + // Then "Brian" can log in + // This is the assertion the issue is about. Without the fallback this step + // fails: the login succeeds at the IDP and the proxy then refuses to resolve + // the account, so #web-content never appears. + await ui.userLogsIn({ stepUser: 'Brian' }) + + // And "Brian" navigates to the personal space page + await ui.userNavigatesToPersonalSpacePage({ stepUser: 'Brian' }) + // And "Brian" should see an empty personal space + await ui.userShouldSeeEmptyPersonalSpace({ stepUser: 'Brian' }) + + // And "Brian" navigates to the project spaces page + await ui.userNavigatesToSpacesPage({ stepUser: 'Brian' }) + // And "Brian" should see no project space + await ui.userShouldSeeNoSpaces({ stepUser: 'Brian' }) + + // And "Brian" logs out + await ui.userLogsOut({ stepUser: 'Brian' }) + }) +}) diff --git a/web/tests/e2e/steps/api/api.ts b/web/tests/e2e/steps/api/api.ts index aa766ff6ef5..b4fb8367172 100644 --- a/web/tests/e2e/steps/api/api.ts +++ b/web/tests/e2e/steps/api/api.ts @@ -23,6 +23,24 @@ export async function usersHaveBeenCreated({ } } +// Creates a Keycloak user with no ocis realm role, i.e. one whose token the proxy +// role_mapping cannot match. Keycloak-only; there is no graph equivalent, because +// with the graph driver the role is assigned by ocis itself and cannot be absent. +export async function keycloakUsersWithoutRoleHaveBeenCreated({ + stepUser, + users +}: { + stepUser: string + users: Array +}): Promise { + const world = getWorld() + const admin = world.usersEnvironment.getUser({ key: stepUser }) + for (const userToBeCreated of users) { + const user = world.usersEnvironment.getUser({ key: userToBeCreated }) + await api.keycloak.createUserWithoutRealmRole({ user, admin }) + } +} + export async function userHasCreatedFolder({ stepUser, folderName diff --git a/web/tests/e2e/steps/ui/spaces.ts b/web/tests/e2e/steps/ui/spaces.ts index 064bed325ff..084243dd429 100644 --- a/web/tests/e2e/steps/ui/spaces.ts +++ b/web/tests/e2e/steps/ui/spaces.ts @@ -24,6 +24,24 @@ export async function userNavigatesToSpacesPage({ stepUser }: { stepUser: string await pageObject.navigate() } +export async function userShouldSeeEmptyPersonalSpace({ + stepUser +}: { + stepUser: string +}): Promise { + const world = getWorld() + const { page } = world.actorsEnvironment.getActor({ key: stepUser }) + const pageObject = new objects.applicationFiles.page.spaces.Personal({ page }) + await pageObject.expectToBeEmpty() +} + +export async function userShouldSeeNoSpaces({ stepUser }: { stepUser: string }): Promise { + const world = getWorld() + const { page } = world.actorsEnvironment.getActor({ key: stepUser }) + const pageObject = new objects.applicationFiles.page.spaces.Projects({ page }) + await pageObject.expectToBeEmpty() +} + export async function userNavigatesToSpace({ stepUser, space diff --git a/web/tests/e2e/support/api/keycloak/user.ts b/web/tests/e2e/support/api/keycloak/user.ts index 71947f943e3..8dcdf464a53 100644 --- a/web/tests/e2e/support/api/keycloak/user.ts +++ b/web/tests/e2e/support/api/keycloak/user.ts @@ -75,6 +75,53 @@ export const createUser = async ({ user, admin }: { user: User; admin: User }): return user } +// Creates a Keycloak user that is given NO ocis realm role at all, which is the +// normal state for users federated into the realm from an external user directory. +// Their token carries no value the proxy role_mapping can match, so the proxy has +// to fall back to PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE. +// +// Unlike createUser() this deliberately does not call initializeUser(): that first +// login IS what the test exercises, and until it happens the user does not exist in +// oCIS at all, so there is no graph id to read either. The user is stored without a +// uuid so the login step can find its credentials; the cleanup hook deletes the oCIS +// user by name and tolerates a 404. +export const createUserWithoutRealmRole = async ({ + user, + admin +}: { + user: User + admin: User +}): Promise => { + const fullName = user.displayName.split(' ') + const body = JSON.stringify({ + username: user.id, + credentials: [{ value: user.password, type: 'password' }], + firstName: fullName[0], + lastName: fullName[1] ?? '', + email: user.email, + emailVerified: true, + enabled: true + }) + + const creationRes = await request({ + method: 'POST', + path: join(realmBasePath, 'users'), + body, + user: admin, + header: { 'Content-Type': 'application/json' } + }) + checkResponseStatus(creationRes, 'Failed while creating user without realm role') + + const keycloakUUID = getUserIdFromResponse(creationRes) + + const usersEnvironment = new UsersEnvironment() + usersEnvironment.storeCreatedKeycloakUser({ user: { ...user, uuid: keycloakUUID } }) + + const ocisUserKey = user.originalId || user.id + usersEnvironment.storeCreatedUser(ocisUserKey, { ...user }) + return user +} + export const assignRole = async ({ admin, uuid, diff --git a/web/tests/e2e/support/objects/app-files/page/spaces/personal.ts b/web/tests/e2e/support/objects/app-files/page/spaces/personal.ts index 4a44ffbc167..38b2176aa7c 100644 --- a/web/tests/e2e/support/objects/app-files/page/spaces/personal.ts +++ b/web/tests/e2e/support/objects/app-files/page/spaces/personal.ts @@ -1,6 +1,8 @@ -import { Page } from '@playwright/test' +import { expect, Page } from '@playwright/test' const personalSpaceNavSelector = '//a[@data-nav-name="files-spaces-generic"]' +// rendered by GenericSpace.vue when the current folder holds nothing +const emptySpaceSelector = '#files-space-empty' export class Personal { #page: Page @@ -12,4 +14,8 @@ export class Personal { async navigate(): Promise { await this.#page.locator(personalSpaceNavSelector).click() } + + async expectToBeEmpty(): Promise { + await expect(this.#page.locator(emptySpaceSelector)).toBeVisible() + } } diff --git a/web/tests/e2e/support/objects/app-files/page/spaces/projects.ts b/web/tests/e2e/support/objects/app-files/page/spaces/projects.ts index 0cf31e4f9fa..8a5b66d0634 100644 --- a/web/tests/e2e/support/objects/app-files/page/spaces/projects.ts +++ b/web/tests/e2e/support/objects/app-files/page/spaces/projects.ts @@ -1,6 +1,9 @@ -import { Page } from '@playwright/test' +import { expect, Page } from '@playwright/test' import { objects } from '../../../../index' +// rendered by Projects.vue when the user has access to no project space +const emptySpacesSelector = '#files-spaces-empty' + export class Projects { #page: Page @@ -8,6 +11,10 @@ export class Projects { this.#page = page } + async expectToBeEmpty(): Promise { + await expect(this.#page.locator(emptySpacesSelector)).toBeVisible() + } + async navigate(): Promise { await this.#page.locator('//a[@data-nav-name="files-spaces-projects"]').click() await this.#page.locator('#app-loading-spinner').waitFor({ state: 'detached' }) From ac747cee1aff8d5285ba6530b1e21543a2e9406c Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Mon, 24 Aug 2026 13:59:16 +0000 Subject: [PATCH 3/6] fix(proxy): tell a silent roles claim apart from an unreadable one Review raised that letting a user in on a broken or invalid role claim is different from letting in a user whose token simply carries no roles, and that the code should distinguish them. It did not: extractRoles returned a bare "no roles in user claims" error for an absent claim, a null claim and a claim holding a number alike, so the caller could only treat all three the same way. extractRoles now returns ErrRolesClaimNotSet when the token is silent about roles - the claim is absent, null, or the configured path does not exist - and a descriptive error when the claim is present but cannot be read. UpdateUserRoleAssignment falls back to the default role only on the former and reports the latter, so a broken claim mapping no longer hides behind a working login. Also from review: raise the "no mapping matched, using the default role" line from debug to warn so operators see it, and replace the hand-rolled key-sorting loops with slices.Sorted(maps.Keys(...)), which drops the intermediate allocation in fallbackRoleID and makes it obvious at a glance that claimRoles is a map. The README and changelog both promised the default role applied to an unreadable claim; both now describe the split. Signed-off-by: Sujeito Operator --- .../bugfix-proxy-oidc-default-role.md | 12 ++- services/proxy/README.md | 9 +- services/proxy/pkg/userroles/oidcroles.go | 102 +++++++++++++----- .../proxy/pkg/userroles/oidcroles_test.go | 101 +++++++++++++++++ 4 files changed, 190 insertions(+), 34 deletions(-) diff --git a/changelog/unreleased/bugfix-proxy-oidc-default-role.md b/changelog/unreleased/bugfix-proxy-oidc-default-role.md index 56d1276579b..132f70c79e3 100644 --- a/changelog/unreleased/bugfix-proxy-oidc-default-role.md +++ b/changelog/unreleased/bugfix-proxy-oidc-default-role.md @@ -1,6 +1,6 @@ Bugfix: Users without a matching role claim were stuck in a login loop -Users whose OIDC role claim was missing, unreadable, or matched no entry in the proxy's +Users whose OIDC role claim was missing or matched no entry in the proxy's `role_mapping` could not log in. The proxy answered with a bare `500 Internal Server Error`, so the web UI showed an access denied page whose "log in again" button returned to the same page, because the login itself had succeeded. The only way out was clearing @@ -11,8 +11,12 @@ Such a request is now answered with `403 Forbidden`, and the logged error names claim that was read, the values found in it and the configured role mapping. We've also added `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE`, which names an ocis role to -assign to those users instead of refusing them. It applies when the claim is missing, -when it cannot be read, and when it matches no mapping; a mapping that does match always -wins over it. It is empty by default, so no existing deployment changes behavior. +assign to those users instead of refusing them. It applies when the claim is missing and +when it matches no mapping; a mapping that does match always wins over it. It is empty by +default, so no existing deployment changes behavior. + +A roles claim that is present but cannot be read is treated as a fault rather than as a +user without a role: it is reported instead of being assigned the default role, so a +broken claim mapping does not hide behind a working login. https://github.com/owncloud/ocis/issues/11467 diff --git a/services/proxy/README.md b/services/proxy/README.md index 3e71ae82b42..136d3b9652f 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -287,8 +287,13 @@ role_assignment: claim_value: myUserRole ``` -The default role applies when the role claim is missing entirely, when it cannot be read, and when -it is present but matches no `role_mapping` entry. A mapping that does match always wins over it. +The default role applies when the role claim is missing entirely and when it is present but matches +no `role_mapping` entry. A mapping that does match always wins over it. + +A role claim that is present but cannot be read — it holds a number, or a list with a non-string in +it, or `role_claim` points through a value that is not an object — does *not* get the default role. +That is a fault in the token or in `role_claim` rather than a user without a role, and it is +reported so it does not hide behind a working login. This setting is empty by default, which keeps the behavior described above: such logins are refused. Because the default role is handed to everyone the mappings do not cover, prefer a low-privilege role diff --git a/services/proxy/pkg/userroles/oidcroles.go b/services/proxy/pkg/userroles/oidcroles.go index 85e2b8d0d40..b236543da99 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -4,8 +4,9 @@ import ( "context" "errors" "fmt" + "maps" "regexp" - "sort" + "slices" "sync" "time" @@ -27,6 +28,14 @@ import ( // such instead of as a generic internal error. var ErrNoRoleAssigned = errors.New("no role in claim maps to an ocis role and no default role is configured") +// ErrRolesClaimNotSet is returned by extractRoles when the token carries no roles for +// this user at all: the configured claim is absent, or present and null. The token is +// well formed and simply says nothing about roles, which is the normal shape for users +// federated into the IDP from an external directory. It is deliberately distinct from +// the errors returned for a claim that is present and unreadable - only one of the two +// is a sign that something is wrong. +var ErrRolesClaimNotSet = errors.New("no roles claim in user claims") + type oidcRoleAssigner struct { Options } @@ -53,12 +62,22 @@ func extractRoles(rolesClaim string, claims map[string]interface{}) (map[string] return claimRoles, nil } - claim, err := oidc.WalkSegments(oidc.SplitWithEscaping(rolesClaim, ".", "\\"), claims) + segments := oidc.SplitWithEscaping(rolesClaim, ".", "\\") + claim, err := oidc.WalkSegments(segments, claims) if err != nil { + // WalkSegments reports a missing intermediate segment and a segment that is + // present but is not an object with the same "unsupported type" error. Only + // the first is a token that is merely silent about roles. + if claimPathAbsent(segments, claims) { + return nil, ErrRolesClaimNotSet + } return nil, err } switch v := claim.(type) { + case nil: + // The path resolved, but there is nothing at the end of it. + return nil, ErrRolesClaimNotSet case []string: for _, cr := range v { claimRoles[cr] = struct{}{} @@ -76,12 +95,42 @@ func extractRoles(rolesClaim string, claims map[string]interface{}) (map[string] case string: claimRoles[v] = struct{}{} default: - return nil, errors.New("no roles in user claims") + return nil, fmt.Errorf("roles claim %q holds an unusable %T", rolesClaim, claim) } return claimRoles, nil } +// claimPathAbsent reports whether the dotted claim path is simply not present in the +// token, as opposed to present and holding something we cannot read. Every segment +// before the last must exist and be an object for the walk to continue: a missing +// segment means the token is silent about roles, while a segment that exists but is +// not an object means the token says something about them that we cannot make sense +// of. It mirrors the traversal in oidc.WalkSegments, which reports both as one error. +func claimPathAbsent(segments []string, claims map[string]interface{}) bool { + for i := 0; i < len(segments)-1; i++ { + switch next := claims[segments[i]].(type) { + case nil: + return true + case map[string]interface{}: + claims = next + case map[interface{}]interface{}: + converted := make(map[string]interface{}, len(next)) + for k, v := range next { + s, ok := k.(string) + if !ok { + return false + } + converted[s] = v + } + claims = converted + default: + return false + } + } + return claims[segments[len(segments)-1]] == nil +} + // matchesClaimMapping returns true if the provided mapping pattern matches at least // one of the values present in claimRoles. It supports: // - exact match when ClaimValue is a literal equal to a claim value @@ -117,18 +166,25 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c roleIDFromClaim := roleNamesToRoleIDs[overwriteRole] if overwriteRole == "" { - // A user whose claims yield no usable role fails in one of three places below. - // All three are the same situation for the person logging in, so they share a - // single exit: fall back to the configured default role, or return - // ErrNoRoleAssigned so the caller can report something better than a 500. + // A token that says nothing about roles and a token we cannot read are two + // different situations and are handled differently: the first falls back to + // the configured default role, the second is reported. claimRoles, err := extractRoles(ra.rolesClaim, claims) switch { - case err != nil: - // The claim is absent or unusable. This is the common case when users are - // federated into the IDP from an external directory and simply have no - // role attached. - logger.Debug().Err(err).Str("Claim", ra.rolesClaim).Msg("Could not extract roles from claims") + case errors.Is(err, ErrRolesClaimNotSet): + // The token is well formed and carries no roles for this user. This is the + // common case when users are federated into the IDP from an external + // directory and simply have no role attached, and it is what this login + // path exists to serve: fall through to the role mapping and the default + // role below. + logger.Debug().Str("Claim", ra.rolesClaim).Msg("No roles claim in user claims") claimRoles = nil + case err != nil: + // The claim is present and unreadable, so something is wrong with the + // token or with PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM. Signing the user in on + // the default role would paper over it, so report it instead. + logger.Error().Err(err).Str("Claim", ra.rolesClaim).Msg("Could not extract roles from claims") + return nil, err case len(claimRoles) == 0: logger.Debug().Str("Claim", ra.rolesClaim).Msg("No roles set in claim") } @@ -225,16 +281,10 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c // service - a misconfiguration is worth reporting differently from a user without a // role, because only one of the two is fixed by editing the deployment. func (ra oidcRoleAssigner) fallbackRoleID(logger zerolog.Logger, roleNamesToRoleIDs map[string]string, claimRoles map[string]struct{}) (string, error) { - seen := make([]string, 0, len(claimRoles)) - for cr := range claimRoles { - seen = append(seen, cr) - } - sort.Strings(seen) - if ra.defaultRole == "" { logger.Error(). Str("claim", ra.rolesClaim). - Strs("claimValues", seen). + Strs("claimValues", slices.Sorted(maps.Keys(claimRoles))). Interface("roleMapping", ra.roleMapping). Msg("No role mapping matched the user's claim and PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE is not set. " + "Add a matching entry to the role mapping, or set a default role to let such users log in.") @@ -243,23 +293,19 @@ func (ra oidcRoleAssigner) fallbackRoleID(logger zerolog.Logger, roleNamesToRole roleID := roleNamesToRoleIDs[ra.defaultRole] if roleID == "" { - known := make([]string, 0, len(roleNamesToRoleIDs)) - for name := range roleNamesToRoleIDs { - known = append(known, name) - } - sort.Strings(known) err := fmt.Errorf("the configured default role %q does not exist", ra.defaultRole) logger.Error().Err(err). - Strs("knownRoles", known). + Strs("knownRoles", slices.Sorted(maps.Keys(roleNamesToRoleIDs))). Msg("PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE names a role that the settings service does not know") return "", err } - logger.Debug(). + logger.Warn(). Str("claim", ra.rolesClaim). - Strs("claimValues", seen). + Strs("claimValues", slices.Sorted(maps.Keys(claimRoles))). Str("ocisRole", ra.defaultRole). - Msg("No role mapping matched, assigning the configured default role") + Msg("No role mapping matched the user's claim, assigning the configured default role. " + + "Add a matching entry to PROXY_ROLE_ASSIGNMENT_OIDC_ROLE_MAPPING if this user should get a different role.") return roleID, nil } diff --git a/services/proxy/pkg/userroles/oidcroles_test.go b/services/proxy/pkg/userroles/oidcroles_test.go index 340a87eeace..8d77e061ee3 100644 --- a/services/proxy/pkg/userroles/oidcroles_test.go +++ b/services/proxy/pkg/userroles/oidcroles_test.go @@ -476,3 +476,104 @@ func TestUpdateUserRoleAssignmentReportsAnUnknownDefaultRole(t *testing.T) { t.Fatalf("the error should name the offending role, got %v", err) } } + +// TestExtractRolesSeparatesASilentTokenFromAnUnreadableOne pins the distinction the +// fallback rests on. A token that carries no roles for this user is a normal token +// from an IDP that federates users in without role information; a token whose roles +// claim is present but unreadable is a sign that something is misconfigured. Before +// ErrRolesClaimNotSet existed both shapes came back as a bare "no roles in user +// claims" error, so the caller could not tell them apart and had to treat every +// unreadable claim as a user without a role. +func TestExtractRolesSeparatesASilentTokenFromAnUnreadableOne(t *testing.T) { + silent := map[string]map[string]interface{}{ + "claim absent": {"sub": "abcd"}, + "claim present but null": {"roles": nil}, + "nested path, parent absent": {"sub": "abcd"}, + "nested path, leaf absent": {"resource_access": map[string]interface{}{"ocis": map[string]interface{}{}}}, + } + for name, claims := range silent { + t.Run("silent/"+name, func(t *testing.T) { + claim := "roles" + if strings.HasPrefix(name, "nested") { + claim = "resource_access.ocis.roles" + } + roles, err := extractRoles(claim, claims) + if !errors.Is(err, ErrRolesClaimNotSet) { + t.Fatalf("expected ErrRolesClaimNotSet, got %v", err) + } + if len(roles) != 0 { + t.Fatalf("expected no roles, got %v", roles) + } + }) + } + + unreadable := map[string]struct { + claim string + claims map[string]interface{} + }{ + "claim holds a number": {"roles", map[string]interface{}{"roles": 42}}, + "claim holds a non-string entry": {"roles", map[string]interface{}{"roles": []interface{}{"a", 7}}}, + "path runs through a string": {"resource_access.ocis.roles", map[string]interface{}{"resource_access": "not-an-object"}}, + } + for name, tc := range unreadable { + t.Run("unreadable/"+name, func(t *testing.T) { + roles, err := extractRoles(tc.claim, tc.claims) + if err == nil { + t.Fatal("expected an error for an unreadable roles claim") + } + if errors.Is(err, ErrRolesClaimNotSet) { + t.Fatalf("an unreadable claim must not report as a silent token, got %v", err) + } + if len(roles) != 0 { + t.Fatalf("expected no roles, got %v", roles) + } + }) + } +} + +// TestUpdateUserRoleAssignmentRejectsAnUnreadableRolesClaim is the behaviour asked for +// in review: a claim we cannot read must not be quietly rounded down to "this user has +// no roles" and signed in on the default role. That would hide a broken IDP mapping +// behind a working login. A token that is merely silent about roles still falls back, +// which is what #11467 asks for; the two paths are covered together so neither can be +// changed without the other being considered. +func TestUpdateUserRoleAssignmentRejectsAnUnreadableRolesClaim(t *testing.T) { + const defaultRoleID = "user-light-id" + + newFixture := func(t *testing.T) oidcRoleAssigner { + return newRoleAssignerFixture(t, + Options{ + rolesClaim: "roles", + roleMapping: []config.RoleMapping{{RoleName: "admin", ClaimValue: "ocisAdmin"}}, + defaultRole: "user-light", + }, + map[string]string{"admin": "admin-id", "user-light": defaultRoleID}, + defaultRoleID, + ) + } + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + + t.Run("unreadable claim is reported", func(t *testing.T) { + ra := newFixture(t) + _, err := ra.UpdateUserRoleAssignment(context.Background(), user, + map[string]interface{}{"sub": "abcd", "roles": 42}, "") + if err == nil { + t.Fatal("expected an unreadable roles claim to be reported, not defaulted") + } + if errors.Is(err, ErrNoRoleAssigned) { + t.Fatalf("an unreadable claim is not a user without a role, got %v", err) + } + }) + + t.Run("silent token still falls back", func(t *testing.T) { + ra := newFixture(t) + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, + map[string]interface{}{"sub": "abcd"}, "") + if err != nil { + t.Fatalf("expected the default role to be applied, got error: %v", err) + } + if roles := assignedRoleFromOpaque(t, got); len(roles) != 1 || roles[0] != defaultRoleID { + t.Fatalf("expected the default role id %q, got %v", defaultRoleID, roles) + } + }) +} From 0bdd8aefc325dee1934e75f8000c391752609a46 Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Mon, 24 Aug 2026 14:24:27 +0000 Subject: [PATCH 4/6] docs(proxy): describe how the default role relates to the graph one GRAPH_ASSIGN_DEFAULT_USER_ROLE and PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE both hand out a role when nothing else does, so the proxy README now says in one table when each applies, to which users, and which one survives. They only ever meet with PROXY_AUTOPROVISION_ACCOUNTS enabled, where a first login creates the user through the graph service and resolves their role in the same request, in that order. The proxy runs second, so the role it resolves replaces the one the graph service assigned at creation - and when it resolves none, the login is refused even though the user was given the 'user' role a moment earlier. That second half is why GRAPH_ASSIGN_DEFAULT_USER_ROLE is not already a fix for #11467, so it is pinned by a test rather than only described. The fixture gained the two gateway mocks a changing role assignment needs; tests that leave the role unchanged never reach them. Signed-off-by: Sujeito Operator --- services/proxy/README.md | 28 +++++++++ .../proxy/pkg/userroles/oidcroles_test.go | 57 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/services/proxy/README.md b/services/proxy/README.md index 136d3b9652f..b08b055d342 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -299,6 +299,34 @@ This setting is empty by default, which keeps the behavior described above: such Because the default role is handed to everyone the mappings do not cover, prefer a low-privilege role such as `user-light` over `user` or `admin`. +##### Relation to `GRAPH_ASSIGN_DEFAULT_USER_ROLE` + +`GRAPH_ASSIGN_DEFAULT_USER_ROLE` and `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE` both hand out a role +when nothing else does, but they act at different moments and on different things: + +| | `GRAPH_ASSIGN_DEFAULT_USER_ROLE` | `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE` | +|---|---|---| +| when | once, when a user is **created** through the libregraph users API | on **every login**, after the role claim has been read | +| which role | always `user`, not configurable | any role name you configure | +| applies to | every user the graph service creates | only `role_assignment.driver: oidc` | +| default | `true` | empty, meaning "refuse such logins" | + +The two only meet when `PROXY_AUTOPROVISION_ACCOUNTS` is enabled — which additionally requires a +write-enabled libregraph user backend. A first login then creates the user through the graph service +and resolves their role in the same request, in that order. **The proxy runs second and its result +is the one that survives**, so on a login where the role claim is missing or matches no mapping: + +* with a default role configured, the user ends up on the default role, replacing whatever the graph + service assigned at creation; +* with no default role configured, the login is refused even though the graph service already + assigned the `user` role a moment earlier — `GRAPH_ASSIGN_DEFAULT_USER_ROLE` does not, on its own, + let a user without a role claim in. + +The same applies to a role assigned by hand through the graph API or the admin UI: with the `oidc` +driver the claim, or this default role, is re-applied at the next login and overwrites it. That is +pre-existing behavior of the `oidc` driver and is not changed by this setting; use the `default` +driver if roles are meant to be managed inside Infinite Scale rather than in the IDP. + The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The default `role_mapping` is: ```yaml diff --git a/services/proxy/pkg/userroles/oidcroles_test.go b/services/proxy/pkg/userroles/oidcroles_test.go index 8d77e061ee3..d2c1a7c80a3 100644 --- a/services/proxy/pkg/userroles/oidcroles_test.go +++ b/services/proxy/pkg/userroles/oidcroles_test.go @@ -303,6 +303,16 @@ func newRoleAssignerFixture(t *testing.T, opts Options, knownRoles map[string]st Status: &rpc.Status{Code: rpc.Code_CODE_OK}, Token: "service-token", }, nil) + // Only reached when the resolved role differs from the assigned one and the + // assignment is therefore rewritten. Tests that keep the role unchanged never + // call these. + gatewayClient.On("CheckPermission", mock.Anything, mock.Anything).Return(&permissions.CheckPermissionResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_OK}, + }, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&storageprovider.ListStorageSpacesResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_OK}, + StorageSpaces: []*storageprovider.StorageSpace{{Id: &storageprovider.StorageSpaceId{OpaqueId: "personal-space-id"}}}, + }, nil) bundles := make([]*settingsmsg.Bundle, 0, len(knownRoles)) for name, id := range knownRoles { @@ -577,3 +587,50 @@ func TestUpdateUserRoleAssignmentRejectsAnUnreadableRolesClaim(t *testing.T) { } }) } + +// TestUpdateUserRoleAssignmentOverridesTheGraphAssignedDefaultRole pins how this +// setting relates to GRAPH_ASSIGN_DEFAULT_USER_ROLE, which is the other place a role +// is handed out when nothing else does. With PROXY_AUTOPROVISION_ACCOUNTS enabled the +// two run in the same login request: the graph service creates the user and gives them +// the "user" role, and the proxy resolves their role immediately afterwards. The proxy +// runs second, so its answer is the one that survives - including when its answer is +// to refuse the login. Both halves are asserted together so neither can be changed +// without the other being considered. +func TestUpdateUserRoleAssignmentOverridesTheGraphAssignedDefaultRole(t *testing.T) { + const ( + graphAssignedRoleID = "graph-user-role-id" + defaultRoleID = "user-light-id" + ) + knownRoles := map[string]string{"user": graphAssignedRoleID, "user-light": defaultRoleID} + user := &cs3user.User{Id: &cs3user.UserId{OpaqueId: "user-1"}} + // a token that says nothing about roles, i.e. the federated user in the report + claims := map[string]interface{}{"sub": "abcd"} + + t.Run("the proxy default role replaces it", func(t *testing.T) { + ra := newRoleAssignerFixture(t, + Options{rolesClaim: "roles", defaultRole: "user-light"}, + knownRoles, + graphAssignedRoleID, + ) + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, claims, "") + if err != nil { + t.Fatalf("expected the default role to be applied, got error: %v", err) + } + if roles := assignedRoleFromOpaque(t, got); len(roles) != 1 || roles[0] != defaultRoleID { + t.Fatalf("expected the proxy default role %q to replace the graph assigned %q, got %v", + defaultRoleID, graphAssignedRoleID, roles) + } + }) + + t.Run("without one the login is still refused", func(t *testing.T) { + ra := newRoleAssignerFixture(t, + Options{rolesClaim: "roles", defaultRole: ""}, + knownRoles, + graphAssignedRoleID, + ) + _, err := ra.UpdateUserRoleAssignment(context.Background(), user, claims, "") + if !errors.Is(err, ErrNoRoleAssigned) { + t.Fatalf("a role assigned by the graph service must not stand in for a role claim, got %v", err) + } + }) +} From 3c8c7039df9e934639e0647810cdefcb1868418b Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Tue, 25 Aug 2026 09:30:11 +0000 Subject: [PATCH 5/6] refactor(oidc): let WalkSegments report a missing claim path itself Review asked for the absent-path check to live in WalkSegments rather than be re-derived beside it. WalkSegments now returns an error wrapping ErrMissingClaim when an intermediate segment is not present, so extractRoles can tell a token that is silent about roles from one whose roles claim it cannot read with a plain errors.Is, and the copy of the traversal goes away. The partition is unchanged. Both loops walked the same indices with the same arms, so wherever WalkSegments failed, claimPathAbsent returned at that same index: nil meant absent, anything else meant present-but-unreadable. claimPathAbsent's final line was unreachable from its only call site, since WalkSegments only fails inside the intermediate loop. An absent leaf never reached it either: the walk succeeds and yields nil there, which extractRoles already handles. A test pins that boundary. The only other caller, ReadStringClaim, now reports a missing intermediate segment as `claim path segment not present: "x"` rather than `unsupported type ''`. Nothing in the tree asserts on the old text. --- ocis-pkg/oidc/claims.go | 10 ++++ ocis-pkg/oidc/claims_test.go | 63 +++++++++++++++++++++++ services/proxy/pkg/userroles/oidcroles.go | 38 ++------------ 3 files changed, 77 insertions(+), 34 deletions(-) diff --git a/ocis-pkg/oidc/claims.go b/ocis-pkg/oidc/claims.go index 2eadcee2986..471a2222f22 100644 --- a/ocis-pkg/oidc/claims.go +++ b/ocis-pkg/oidc/claims.go @@ -1,6 +1,7 @@ package oidc import ( + "errors" "fmt" "strings" ) @@ -18,6 +19,13 @@ const ( OcisRoutingPolicy = "ocis.routing.policy" ) +// ErrMissingClaim is wrapped by WalkSegments when an intermediate segment of the +// path is not present in the claims at all, as opposed to being present and +// holding something the walk cannot descend into. Callers that need to tell "the +// token is silent about this" apart from "the token says something here that we +// cannot read" can check for it with errors.Is. +var ErrMissingClaim = errors.New("claim path segment not present") + // SplitWithEscaping splits s into segments using separator which can be escaped using the escape string // See https://codereview.stackexchange.com/a/280193 func SplitWithEscaping(s string, separator string, escapeString string) []string { @@ -37,6 +45,8 @@ func WalkSegments(segments []string, claims map[string]interface{}) (interface{} i := 0 for ; i < len(segments)-1; i++ { switch castedClaims := claims[segments[i]].(type) { + case nil: + return nil, fmt.Errorf("%w: %q", ErrMissingClaim, segments[i]) case map[string]interface{}: claims = castedClaims case map[interface{}]interface{}: diff --git a/ocis-pkg/oidc/claims_test.go b/ocis-pkg/oidc/claims_test.go index 5a9c60df093..bf8829e51fb 100644 --- a/ocis-pkg/oidc/claims_test.go +++ b/ocis-pkg/oidc/claims_test.go @@ -2,7 +2,9 @@ package oidc_test import ( "encoding/json" + "errors" "reflect" + "strings" "testing" "github.com/owncloud/ocis/v2/ocis-pkg/oidc" @@ -180,3 +182,64 @@ func TestWalkSegments(t *testing.T) { t.Run(test.name, test.run) } } + +// TestWalkSegmentsDistinguishesAnAbsentSegmentFromAnUnreadableOne pins the +// distinction ErrMissingClaim exists for. A path segment that is not in the claims +// at all means the token is simply silent about whatever lives under it; a segment +// that is present but cannot be descended into means the token says something there +// that we cannot make sense of. Both used to come back as the same "unsupported +// type" error, so a caller could not tell them apart without re-walking the claims +// itself. +func TestWalkSegmentsDistinguishesAnAbsentSegmentFromAnUnreadableOne(t *testing.T) { + t.Run("absent intermediate segment wraps ErrMissingClaim", func(t *testing.T) { + _, err := oidc.WalkSegments( + []string{"resource_access", "ocis", "roles"}, + map[string]interface{}{"sub": "abcd"}, + ) + if !errors.Is(err, oidc.ErrMissingClaim) { + t.Fatalf("expected ErrMissingClaim, got %v", err) + } + if !strings.Contains(err.Error(), "resource_access") { + t.Fatalf("the error should name the missing segment, got %v", err) + } + }) + + unreadable := map[string]map[string]interface{}{ + "intermediate segment is a string": {"resource_access": "not-an-object"}, + "intermediate segment is a number": {"resource_access": 42}, + "intermediate map has a non-string key": { + "resource_access": map[interface{}]interface{}{7: "ocis"}, + }, + } + for name, claims := range unreadable { + t.Run(name+" does not wrap ErrMissingClaim", func(t *testing.T) { + _, err := oidc.WalkSegments([]string{"resource_access", "ocis", "roles"}, claims) + if err == nil { + t.Fatal("expected an error for a segment that cannot be descended into") + } + if errors.Is(err, oidc.ErrMissingClaim) { + t.Fatalf("a present but unreadable segment must not report as missing, got %v", err) + } + }) + } + + // The boundary: only intermediate segments are walked. An absent *leaf* is not an + // error at all - the walk succeeds and yields nil, and it is the caller's job to + // decide what an empty claim means. + t.Run("absent leaf segment is not an error", func(t *testing.T) { + got, err := oidc.WalkSegments( + []string{"resource_access", "ocis", "roles"}, + map[string]interface{}{ + "resource_access": map[string]interface{}{ + "ocis": map[string]interface{}{}, + }, + }, + ) + if err != nil { + t.Fatalf("expected no error for an absent leaf, got %v", err) + } + if got != nil { + t.Fatalf("expected a nil claim, got %v", got) + } + }) +} diff --git a/services/proxy/pkg/userroles/oidcroles.go b/services/proxy/pkg/userroles/oidcroles.go index b236543da99..6bf9a486577 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -65,10 +65,10 @@ func extractRoles(rolesClaim string, claims map[string]interface{}) (map[string] segments := oidc.SplitWithEscaping(rolesClaim, ".", "\\") claim, err := oidc.WalkSegments(segments, claims) if err != nil { - // WalkSegments reports a missing intermediate segment and a segment that is - // present but is not an object with the same "unsupported type" error. Only - // the first is a token that is merely silent about roles. - if claimPathAbsent(segments, claims) { + // A segment that is simply not in the token means the token is silent about + // roles. A segment that is present but cannot be descended into means it says + // something about them that we cannot make sense of, which is a real error. + if errors.Is(err, oidc.ErrMissingClaim) { return nil, ErrRolesClaimNotSet } return nil, err @@ -101,36 +101,6 @@ func extractRoles(rolesClaim string, claims map[string]interface{}) (map[string] return claimRoles, nil } -// claimPathAbsent reports whether the dotted claim path is simply not present in the -// token, as opposed to present and holding something we cannot read. Every segment -// before the last must exist and be an object for the walk to continue: a missing -// segment means the token is silent about roles, while a segment that exists but is -// not an object means the token says something about them that we cannot make sense -// of. It mirrors the traversal in oidc.WalkSegments, which reports both as one error. -func claimPathAbsent(segments []string, claims map[string]interface{}) bool { - for i := 0; i < len(segments)-1; i++ { - switch next := claims[segments[i]].(type) { - case nil: - return true - case map[string]interface{}: - claims = next - case map[interface{}]interface{}: - converted := make(map[string]interface{}, len(next)) - for k, v := range next { - s, ok := k.(string) - if !ok { - return false - } - converted[s] = v - } - claims = converted - default: - return false - } - } - return claims[segments[len(segments)-1]] == nil -} - // matchesClaimMapping returns true if the provided mapping pattern matches at least // one of the values present in claimRoles. It supports: // - exact match when ClaimValue is a literal equal to a claim value From 1727469c6b6dc1d5117cd15a3cc9c50aa5cd4a09 Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Sat, 29 Aug 2026 22:53:17 +0200 Subject: [PATCH 6/6] docs(proxy): record which deployments the oidc default role reaches Writes down what jvillafanez established by testing: the setting is read by the oidc role assigner only, and a default deployment without Keycloak runs the default assigner and ignores it entirely. Adds two measured details for the oidc-assigner-with-built-in-IDP case, both pre-existing behaviour of that assigner rather than anything the default role introduces: - UpdateUserRoleAssignment overwrites on every login, so an administrator's role is replaced rather than preserved. - user-light is the only one of the four built-in roles without Drives.Create, so it is the one default_role value for which the proxy takes the DisablePersonalSpace branch after assigning. Also notes that recovery has no CLI path and that reverting the driver to default does not undo an assignment, since that assigner only assigns to accounts holding no role. Documentation only; no behaviour change. --- services/proxy/README.md | 42 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/services/proxy/README.md b/services/proxy/README.md index b08b055d342..6d7e78a4bab 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -297,7 +297,47 @@ reported so it does not hide behind a working login. This setting is empty by default, which keeps the behavior described above: such logins are refused. Because the default role is handed to everyone the mappings do not cover, prefer a low-privilege role -such as `user-light` over `user` or `admin`. +such as `user-light` over `user` or `admin` — but read +[Which deployments this affects](#which-deployments-this-affects) first, because `user-light` is the +one built-in role without the `Drives.Create` permission and that has a further consequence. + +##### Which deployments this affects + +`default_role` is read by the `oidc` role assigner only — the one selected by +`role_assignment.driver: oidc`, which is what a deployment fronted by Keycloak or another external +IDP uses. + +* **The default deployment, without Keycloak, is not affected.** It uses the `default` role + assigner, which never reads `default_role`; users there keep being assigned the `user` role + exactly as before. `PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE` is ignored on such a deployment, + whether or not it is set. +* **With an external IDP** the setting behaves as described above: it is consulted after the roles + claim has been read, and only for the users that no `role_mapping` entry matched. +* **The `oidc` assigner in front of the built-in IDP is the combination to be careful with.** The + `idp` service has no setting that populates a roles claim, so unless something else adds one every + token reaching the assigner is silent about roles. Every account then matches no mapping and takes + the same fallback — administrators included. Without `default_role` that setup refuses every + login; with it, everyone is signed in on the one configured role and nobody is left holding + `admin`. + +Two details are worth knowing before picking the role for that last case, both of them pre-existing +behavior of the `oidc` assigner rather than anything this setting introduces: + +* **The assignment overwrites, on every login.** `UpdateUserRoleAssignment` compares the role it + resolved against the one the account currently holds and reassigns whenever they differ, so an + administrator's `admin` role is replaced rather than preserved. +* **A demotion to `user-light` also disables the personal space.** After assigning the role the + proxy checks `Drives.Create` and, when the account does not have it, calls + `DisablePersonalSpace`. Of the four built-in roles only `user-light` lacks that permission + (`services/settings/pkg/store/defaults/defaults.go`), so `user-light` is the one value of + `default_role` for which this branch is taken. The space is disabled, not deleted, and a later + login on a role that has the permission restores it. + +Recovering from that state is a matter of assigning a role back to some account, and there is no +command-line path for it — the `settings` service ships no CLI beyond `health`/`server`/`version`, +so it takes an account that still holds `Account Management`, or direct access to the settings +store. Switching `role_assignment.driver` back to `default` does not undo it either: the `default` +assigner only assigns a role to an account that has none, and a demoted account has one. ##### Relation to `GRAPH_ASSIGN_DEFAULT_USER_ROLE`