From 79b2f33848f298a4a49201ae5eb9fe406d791e68 Mon Sep 17 00:00:00 2001 From: opencloudeu Date: Tue, 3 Nov 2026 23:17:38 +0000 Subject: [PATCH 1/2] [tx] updated from transifex --- .../pkg/service/v0/l10n/locale/cs/LC_MESSAGES/settings.po | 2 +- .../userlog/pkg/service/l10n/locale/fr/LC_MESSAGES/userlog.po | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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" From 31722c878e85873fb30c80eac11bd45453201af8 Mon Sep 17 00:00:00 2001 From: Ryo Kusnadi Date: Wed, 4 Nov 2026 05:11:59 +0700 Subject: [PATCH 2/2] fix(proxy): answer 403 instead of 500 when the role claim is missing With PROXY_ROLE_ASSIGNMENT_DRIVER=oidc, a token whose claims give no OpenCloud role made the account resolver answer 500. Mark these claim errors with userroles.ErrRoleClaim and answer 403 for them. Failures of the settings or gateway service still answer 500. Signed-off-by: Ryo Kusnadi --- .../proxy/pkg/middleware/account_resolver.go | 5 ++ .../pkg/middleware/account_resolver_test.go | 54 +++++++++++++++++++ services/proxy/pkg/userroles/oidcroles.go | 6 ++- .../proxy/pkg/userroles/oidcroles_test.go | 50 +++++++++++++++++ services/proxy/pkg/userroles/userroles.go | 6 +++ 5 files changed, 119 insertions(+), 2 deletions(-) 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 {