diff --git a/services/proxy/pkg/middleware/account_resolver.go b/services/proxy/pkg/middleware/account_resolver.go index 04b57b6105..b457f7744b 100644 --- a/services/proxy/pkg/middleware/account_resolver.go +++ b/services/proxy/pkg/middleware/account_resolver.go @@ -223,6 +223,11 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { user, err = m.userRoleAssigner.UpdateUserRoleAssignment(ctx, user, claims) if err != nil { m.logger.Error().Err(err).Msg("Could not get user roles") + if errors.Is(err, userroles.ErrRoleClaim) { + // the token has no usable role claim: the user is not authorized, OpenCloud did not fail + w.WriteHeader(http.StatusForbidden) + return + } w.WriteHeader(http.StatusInternalServerError) return } diff --git a/services/proxy/pkg/middleware/account_resolver_test.go b/services/proxy/pkg/middleware/account_resolver_test.go index f81e639ccb..cbeb042cb5 100644 --- a/services/proxy/pkg/middleware/account_resolver_test.go +++ b/services/proxy/pkg/middleware/account_resolver_test.go @@ -2,6 +2,8 @@ package middleware import ( "context" + "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -16,6 +18,7 @@ import ( "github.com/opencloud-eu/opencloud/services/proxy/pkg/router" "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend" "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend/mocks" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/userroles" userRoleMocks "github.com/opencloud-eu/opencloud/services/proxy/pkg/userroles/mocks" "github.com/opencloud-eu/reva/v2/pkg/auth/scope" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" @@ -466,3 +469,54 @@ func TestTenantIDMapping(t *testing.T) { }) } } + +func TestRoleAssignmentErrors(t *testing.T) { + tests := []struct { + name string + roleErr error + wantStatusCode int + }{ + { + name: "role claim missing is forbidden", + roleErr: fmt.Errorf("%w: unsupported type ''", userroles.ErrRoleClaim), + wantStatusCode: http.StatusForbidden, + }, + { + name: "settings service failure stays an internal error", + roleErr: errors.New("settings service unavailable"), + wantStatusCode: http.StatusInternalServerError, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + user := &userv1beta1.User{Id: &userv1beta1.UserId{OpaqueId: "user-id"}} + + ub := mocks.UserBackend{} + ub.On("GetUserByClaims", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(user, "", nil) + + ra := userRoleMocks.UserRoleAssigner{} + ra.On("UpdateUserRoleAssignment", mock.Anything, mock.Anything, mock.Anything).Return(nil, tc.roleErr) + + called := false + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { called = true }) + sut := AccountResolver( + Logger(log.NewLogger()), + UserProvider(&ub), + UserRoleAssigner(&ra), + SkipUserInfo(false), + UserOIDCClaim(oidc.PreferredUsername), + UserCS3Claim("username"), + AutoprovisionAccounts(false), + )(next) + + req, rw := mockRequest(map[string]any{ + oidc.Iss: testIdP, + oidc.PreferredUsername: "foo", + }) + sut.ServeHTTP(rw, req) + + assert.Equal(t, tc.wantStatusCode, rw.Code) + assert.False(t, called, "the request must not reach the next handler") + }) + } +} diff --git a/services/proxy/pkg/userroles/oidcroles.go b/services/proxy/pkg/userroles/oidcroles.go index 409f67f422..055f16e95e 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -3,6 +3,7 @@ package userroles import ( "context" "errors" + "fmt" "sync" "time" @@ -81,12 +82,13 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c claimRoles, err := extractRoles(ra.rolesClaim, claims) if err != nil { + err = fmt.Errorf("%w: %w", ErrRoleClaim, err) logger.Error().Err(err).Msg("Error mapping role names to role ids") return nil, err } if len(claimRoles) == 0 { - err := errors.New("no roles set in claim") + err := fmt.Errorf("%w: no roles set in claim", ErrRoleClaim) logger.Error().Err(err).Msg("") return nil, err } @@ -104,7 +106,7 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c } if roleIDFromClaim == "" { - err := errors.New("no role in claim maps to an OpenCloud role") + err := fmt.Errorf("%w: no role in claim maps to an OpenCloud role", ErrRoleClaim) logger.Error().Err(err).Msg("") return nil, err } diff --git a/services/proxy/pkg/userroles/oidcroles_test.go b/services/proxy/pkg/userroles/oidcroles_test.go index 81548e5ae7..427b15b277 100644 --- a/services/proxy/pkg/userroles/oidcroles_test.go +++ b/services/proxy/pkg/userroles/oidcroles_test.go @@ -1,8 +1,15 @@ package userroles import ( + "context" "encoding/json" + "errors" "testing" + "time" + + cs3 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" ) func TestExtractRolesArray(t *testing.T) { @@ -118,3 +125,46 @@ func TestNoRoles(t *testing.T) { t.Fatal("length of roles mut be 0") } } + +func TestUpdateUserRoleAssignmentClaimErrors(t *testing.T) { + // pre-fill the role name cache so no settings service is needed + roleNameToID.lock.Lock() + roleNameToID.roleNameToID = map[string]string{"admin": "admin-id", "user": "user-id"} + roleNameToID.lastRead = time.Now() + roleNameToID.lock.Unlock() + t.Cleanup(func() { + roleNameToID.lock.Lock() + roleNameToID.roleNameToID = nil + roleNameToID.lastRead = time.Time{} + roleNameToID.lock.Unlock() + }) + + ra := NewOIDCRoleAssigner( + WithLogger(log.NewLogger()), + WithRolesClaim("organization.roles"), + WithRoleMapping([]config.RoleMapping{{RoleName: "admin", ClaimValue: "oc-admin"}}), + ) + user := &cs3.User{Id: &cs3.UserId{OpaqueId: "user-id"}} + + tests := []struct { + name string + claims map[string]any + }{ + {"claim missing", map[string]any{"sub": "x"}}, + {"claim is null", map[string]any{"organization": map[string]any{"roles": nil}}}, + {"claim is empty", map[string]any{"organization": map[string]any{"roles": []any{}}}}, + {"claim has a non-string role", map[string]any{"organization": map[string]any{"roles": []any{1}}}}, + {"claim maps to no role", map[string]any{"organization": map[string]any{"roles": []any{"other"}}}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := ra.UpdateUserRoleAssignment(context.Background(), user, tc.claims) + if got != nil { + t.Fatal("must not return a user") + } + if !errors.Is(err, ErrRoleClaim) { + t.Fatalf("want ErrRoleClaim, got %v", err) + } + }) + } +} diff --git a/services/proxy/pkg/userroles/userroles.go b/services/proxy/pkg/userroles/userroles.go index 4ebaee18ee..1a71df5b29 100644 --- a/services/proxy/pkg/userroles/userroles.go +++ b/services/proxy/pkg/userroles/userroles.go @@ -2,6 +2,7 @@ package userroles import ( "context" + "errors" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" cs3 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -11,6 +12,11 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" ) +// ErrRoleClaim is returned (wrapped) by a UserRoleAssigner when the user's claims +// do not yield an OpenCloud role: the claim is missing, malformed, empty or maps to no role. +// It is a problem with the user's token, not a failure of OpenCloud itself. +var ErrRoleClaim = errors.New("role claim does not yield an OpenCloud role") + // UserRoleAssigner allows providing different implementations for how users get their default roles // assigned by the proxy during authentication type UserRoleAssigner interface { diff --git a/services/settings/pkg/service/v0/l10n/locale/cs/LC_MESSAGES/settings.po b/services/settings/pkg/service/v0/l10n/locale/cs/LC_MESSAGES/settings.po index be876ae831..4c2667da1a 100644 --- a/services/settings/pkg/service/v0/l10n/locale/cs/LC_MESSAGES/settings.po +++ b/services/settings/pkg/service/v0/l10n/locale/cs/LC_MESSAGES/settings.po @@ -13,7 +13,7 @@ msgid "" msgstr "" "Project-Id-Version: \n" "Report-Msgid-Bugs-To: EMAIL\n" -"POT-Creation-Date: 2026-10-14 23:16+0000\n" +"POT-Creation-Date: 2026-11-03 23:16+0000\n" "PO-Revision-Date: 2025-01-27 10:17+0000\n" "Last-Translator: Frantisek Gössel, 2026\n" "Language-Team: Czech (https://app.transifex.com/opencloud-eu/teams/204053/cs/)\n" diff --git a/services/userlog/pkg/service/l10n/locale/fr/LC_MESSAGES/userlog.po b/services/userlog/pkg/service/l10n/locale/fr/LC_MESSAGES/userlog.po index 9f555793dc..d85c4915ce 100644 --- a/services/userlog/pkg/service/l10n/locale/fr/LC_MESSAGES/userlog.po +++ b/services/userlog/pkg/service/l10n/locale/fr/LC_MESSAGES/userlog.po @@ -12,7 +12,7 @@ msgid "" msgstr "" "Project-Id-Version: \n" "Report-Msgid-Bugs-To: EMAIL\n" -"POT-Creation-Date: 2026-10-14 23:16+0000\n" +"POT-Creation-Date: 2026-11-03 23:16+0000\n" "PO-Revision-Date: 2025-01-27 10:17+0000\n" "Last-Translator: eric_G , 2026\n" "Language-Team: French (https://app.transifex.com/opencloud-eu/teams/204053/fr/)\n"