From 8cea8c8cfd2f596caa5601e89546eab96ffe33ac Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 24 Sep 2025 17:18:56 +0200 Subject: [PATCH] feat(proxy): account_resolver multi-tenancy Make the account resolve reject users without a tenantid, when multi-tenancy is enabled. --- services/proxy/pkg/command/server.go | 1 + .../proxy/pkg/middleware/account_resolver.go | 17 +++++ .../pkg/middleware/account_resolver_test.go | 63 ++++++++++++++++--- services/proxy/pkg/middleware/options.go | 13 +++- 4 files changed, 82 insertions(+), 12 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index c129fe027..912843b46 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -370,6 +370,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, middleware.UserOIDCClaim(cfg.UserOIDCClaim), middleware.UserCS3Claim(cfg.UserCS3Claim), middleware.AutoprovisionAccounts(cfg.AutoprovisionAccounts), + middleware.MultiTenantEnabled(cfg.Commons.MultiTenantEnabled), middleware.EventsPublisher(publisher), ), middleware.SelectorCookie( diff --git a/services/proxy/pkg/middleware/account_resolver.go b/services/proxy/pkg/middleware/account_resolver.go index 90f730ecb..0ad054192 100644 --- a/services/proxy/pkg/middleware/account_resolver.go +++ b/services/proxy/pkg/middleware/account_resolver.go @@ -43,6 +43,7 @@ func AccountResolver(optionSetters ...Option) func(next http.Handler) http.Handl userCS3Claim: options.UserCS3Claim, userRoleAssigner: options.UserRoleAssigner, autoProvisionAccounts: options.AutoprovisionAccounts, + multiTenantEnabled: options.MultiTenantEnabled, lastGroupSyncCache: lastGroupSyncCache, eventsPublisher: options.EventsPublisher, } @@ -56,6 +57,7 @@ type accountResolver struct { userProvider backend.UserBackend userRoleAssigner userroles.UserRoleAssigner autoProvisionAccounts bool + multiTenantEnabled bool userOIDCClaim string userCS3Claim string // lastGroupSyncCache is used to keep track of when the last sync of group @@ -159,6 +161,14 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { return } + // if this is a multi-tenant setup, make sure the resolved user has a tenant id set + if m.multiTenantEnabled && user.GetId().GetTenantId() == "" { + m.logger.Error().Str("userid", user.Id.OpaqueId).Msg("User does not have a tenantId assigned") + w.WriteHeader(http.StatusUnauthorized) + return + } + + // update user if needed if m.autoProvisionAccounts { if err = m.userProvider.UpdateUserIfNeeded(req.Context(), user, claims); err != nil { m.logger.Error().Err(err).Str("userid", user.GetId().GetOpaqueId()).Interface("claims", claims).Msg("Failed to update autoprovisioned user") @@ -201,6 +211,13 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { m.logger.Debug().Interface("claims", claims).Interface("user", user).Msg("associated claims with user") } else if user != nil && !hasToken { + // if this is a multi-tenant setup, make sure the resolved user has a tenant id set + if m.multiTenantEnabled && user.GetId().GetTenantId() == "" { + m.logger.Error().Str("userid", user.Id.OpaqueId).Msg("User does not have a tenantId assigned") + w.WriteHeader(http.StatusUnauthorized) + return + } + // If we already have a token (e.g. the app auth middleware adds the token to the context) there is no need // to get yet another one here. var err error diff --git a/services/proxy/pkg/middleware/account_resolver_test.go b/services/proxy/pkg/middleware/account_resolver_test.go index e2aa7457a..b279e869f 100644 --- a/services/proxy/pkg/middleware/account_resolver_test.go +++ b/services/proxy/pkg/middleware/account_resolver_test.go @@ -24,7 +24,7 @@ func TestTokenIsAddedWithMailClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, Mail: "foo@example.com", - }, nil, oidc.Email, "mail") + }, nil, oidc.Email, "mail", false) req, rw := mockRequest(map[string]interface{}{ oidc.Iss: "https://idx.example.com", @@ -42,7 +42,7 @@ func TestTokenIsAddedWithUsernameClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, Mail: "foo@example.com", - }, nil, oidc.PreferredUsername, "username") + }, nil, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ oidc.Iss: "https://idx.example.com", @@ -61,7 +61,7 @@ func TestTokenIsAddedWithDotUsernamePathClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, Mail: "foo@example.com", - }, nil, "li.un", "username") + }, nil, "li.un", "username", false) // This is how lico adds the username to the access token req, rw := mockRequest(map[string]interface{}{ @@ -83,7 +83,7 @@ func TestTokenIsAddedWithDotEscapedUsernameClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, Mail: "foo@example.com", - }, nil, "li\\.un", "username") + }, nil, "li\\.un", "username", false) // This tests the . escaping of the readUserIDClaim req, rw := mockRequest(map[string]interface{}{ @@ -103,7 +103,7 @@ func TestTokenIsAddedWithDottedUsernameClaimFallback(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, Mail: "foo@example.com", - }, nil, "li.un", "username") + }, nil, "li.un", "username", false) // This tests the . escaping fallback of the readUserIDClaim req, rw := mockRequest(map[string]interface{}{ @@ -120,7 +120,7 @@ func TestTokenIsAddedWithDottedUsernameClaimFallback(t *testing.T) { } func TestNSkipOnNoClaims(t *testing.T) { - sut := newMockAccountResolver(nil, backend.ErrAccountDisabled, oidc.Email, "mail") + sut := newMockAccountResolver(nil, backend.ErrAccountDisabled, oidc.Email, "mail", false) req, rw := mockRequest(nil) sut.ServeHTTP(rw, req) @@ -131,7 +131,7 @@ func TestNSkipOnNoClaims(t *testing.T) { } func TestUnauthorizedOnUserNotFound(t *testing.T) { - sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.PreferredUsername, "username") + sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ oidc.Iss: "https://idx.example.com", oidc.PreferredUsername: "foo", @@ -145,7 +145,7 @@ func TestUnauthorizedOnUserNotFound(t *testing.T) { } func TestUnauthorizedOnUserDisabled(t *testing.T) { - sut := newMockAccountResolver(nil, backend.ErrAccountDisabled, oidc.PreferredUsername, "username") + sut := newMockAccountResolver(nil, backend.ErrAccountDisabled, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ oidc.Iss: "https://idx.example.com", oidc.PreferredUsername: "foo", @@ -159,7 +159,7 @@ func TestUnauthorizedOnUserDisabled(t *testing.T) { } func TestInternalServerErrorOnMissingMailAndUsername(t *testing.T) { - sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.Email, "mail") + sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.Email, "mail", false) req, rw := mockRequest(map[string]interface{}{ oidc.Iss: "https://idx.example.com", }) @@ -171,7 +171,49 @@ func TestInternalServerErrorOnMissingMailAndUsername(t *testing.T) { assert.Equal(t, http.StatusInternalServerError, rw.Code) } -func newMockAccountResolver(userBackendResult *userv1beta1.User, userBackendErr error, oidcclaim, cs3claim string) http.Handler { +func TestUnauthorizedOnMissingTenantId(t *testing.T) { + sut := newMockAccountResolver( + &userv1beta1.User{ + Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Username: "foo", + }, + nil, oidc.PreferredUsername, "username", true) + req, rw := mockRequest(map[string]any{ + oidc.Iss: "https://idx.example.com", + oidc.PreferredUsername: "foo", + }) + + sut.ServeHTTP(rw, req) + + token := req.Header.Get(revactx.TokenHeader) + assert.Empty(t, token) + assert.Equal(t, http.StatusUnauthorized, rw.Code) +} + +func TestTokenIsAddedWhenUserHasTenantId(t *testing.T) { + sut := newMockAccountResolver( + &userv1beta1.User{ + Id: &userv1beta1.UserId{ + Idp: "https://idx.example.com", + OpaqueId: "123", + TenantId: "tenant1", + }, + Username: "foo", + }, + nil, oidc.PreferredUsername, "username", true) + req, rw := mockRequest(map[string]any{ + oidc.Iss: "https://idx.example.com", + oidc.PreferredUsername: "foo", + }) + + sut.ServeHTTP(rw, req) + + token := req.Header.Get(revactx.TokenHeader) + assert.NotEmpty(t, token) + assert.Contains(t, token, "eyJ") +} + +func newMockAccountResolver(userBackendResult *userv1beta1.User, userBackendErr error, oidcclaim, cs3claim string, multiTenant bool) http.Handler { tokenManager, _ := jwt.New(map[string]interface{}{ "secret": "change-me", "expires": int64(60), @@ -198,6 +240,7 @@ func newMockAccountResolver(userBackendResult *userv1beta1.User, userBackendErr UserOIDCClaim(oidcclaim), UserCS3Claim(cs3claim), AutoprovisionAccounts(false), + MultiTenantEnabled(multiTenant), )(mockHandler{}) } diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index 6fda3f730..e87afeaff 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -70,8 +70,10 @@ type Options struct { // TraceProvider sets the tracing provider. TraceProvider trace.TracerProvider // SkipUserInfo prevents the oidc middleware from querying the userinfo endpoint and read any claims directly from the access token instead - SkipUserInfo bool - EventsPublisher events.Publisher + SkipUserInfo bool + // MultiTenantEnabled causes the account resolve middleware to reject users that don't have a tenant id assigned + MultiTenantEnabled bool + EventsPublisher events.Publisher } // newOptions initializes the available default options. @@ -239,6 +241,13 @@ func SkipUserInfo(val bool) Option { } } +// MultiTenantEnabled sets the MultiTenantEnabled flag. +func MultiTenantEnabled(val bool) Option { + return func(o *Options) { + o.MultiTenantEnabled = val + } +} + // EventsPublisher sets the events publisher. func EventsPublisher(ep events.Publisher) Option { return func(o *Options) {