fix(proxy): let users without a matching role claim log in (#11467)

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

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}

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}

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{}:

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)
+		}
+	})
+}

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:
 

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),
 		)

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

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)

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))
+}