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..132f70c79e3 --- /dev/null +++ b/changelog/unreleased/bugfix-proxy-oidc-default-role.md @@ -0,0 +1,22 @@ +Bugfix: Users without a matching role claim were stuck in a login loop + +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 +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 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/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/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/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/README.md b/services/proxy/README.md index 3d01ef255b1..6d7e78a4bab 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -259,7 +259,113 @@ 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 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 +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` + +`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: 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..6bf9a486577 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -3,7 +3,10 @@ package userroles import ( "context" "errors" + "fmt" + "maps" "regexp" + "slices" "sync" "time" @@ -15,9 +18,24 @@ 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") + +// 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 } @@ -44,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 { + // 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 } 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{}{} @@ -67,7 +95,7 @@ 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 @@ -108,16 +136,27 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c roleIDFromClaim := roleNamesToRoleIDs[overwriteRole] if overwriteRole == "" { + // 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) - 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("") + switch { + 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") } // the roleMapping config is supposed to have the role mappings ordered from the highest privileged role @@ -132,9 +171,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 +245,40 @@ 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) { + if ra.defaultRole == "" { + logger.Error(). + Str("claim", ra.rolesClaim). + 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.") + return "", ErrNoRoleAssigned + } + + roleID := roleNamesToRoleIDs[ra.defaultRole] + if roleID == "" { + err := fmt.Errorf("the configured default role %q does not exist", ra.defaultRole) + logger.Error().Err(err). + 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.Warn(). + Str("claim", ra.rolesClaim). + Strs("claimValues", slices.Sorted(maps.Keys(claimRoles))). + Str("ocisRole", ra.defaultRole). + 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 +} + // 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..d2c1a7c80a3 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,365 @@ 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) + // 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 { + 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) + } +} + +// 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) + } + }) +} + +// 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) + } + }) +} 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) { 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' })