From ff3977aeb532fcd284fbd65b3944e277721068f6 Mon Sep 17 00:00:00 2001 From: Noste <83548733+Noooste@users.noreply.github.com> Date: Tue, 12 May 2026 23:31:11 +0200 Subject: [PATCH] feat: enhance admin role checks to support multiple roles configuration (#43) Signed-off-by: Noste <83548733+Noooste@users.noreply.github.com> --- backend/internal/auth/auth.go | 11 ++++-- backend/internal/auth/auth_test.go | 32 ++++++++++----- backend/internal/config/config.go | 36 ++++++++++++++--- backend/internal/config/config_test.go | 55 +++++++++++++++++++++++++- backend/internal/routes/routes.go | 7 ++-- config.example.yaml | 7 ++++ helm/garage-ui/values.yaml | 6 ++- 7 files changed, 130 insertions(+), 24 deletions(-) diff --git a/backend/internal/auth/auth.go b/backend/internal/auth/auth.go index ee268b1..787cb6a 100644 --- a/backend/internal/auth/auth.go +++ b/backend/internal/auth/auth.go @@ -252,15 +252,18 @@ func (a *Service) ExtractRolesFromAccessToken(accessToken string) []string { return extractRoles(claims, a.authConfig.OIDC.RoleAttributePath) } -// IsAdmin checks if the user has admin role +// IsAdmin checks if the user has any of the configured admin roles. func (a *Service) IsAdmin(userInfo *UserInfo) bool { - if a.authConfig.OIDC.AdminRole == "" { + adminRoles := a.authConfig.OIDC.EffectiveAdminRoles() + if len(adminRoles) == 0 { return false } for _, role := range userInfo.Roles { - if role == a.authConfig.OIDC.AdminRole { - return true + for _, adminRole := range adminRoles { + if role == adminRole { + return true + } } } diff --git a/backend/internal/auth/auth_test.go b/backend/internal/auth/auth_test.go index 84041e2..ec891bc 100644 --- a/backend/internal/auth/auth_test.go +++ b/backend/internal/auth/auth_test.go @@ -609,23 +609,35 @@ func TestExtractRolesFromAccessToken_BadJSONInPayload(t *testing.T) { func TestIsAdmin(t *testing.T) { tests := []struct { - name string - adminRole string - userRoles []string - want bool + name string + adminRole string + adminRoles []string + userRoles []string + want bool }{ - {"empty admin role config returns false", "", []string{"admin"}, false}, - {"user has admin role", "admin", []string{"viewer", "admin"}, true}, - {"user lacks admin role", "admin", []string{"viewer"}, false}, - {"user has no roles", "admin", nil, false}, - {"role match is exact (case-sensitive)", "admin", []string{"Admin"}, false}, + {"empty admin role config returns false", "", nil, []string{"admin"}, false}, + {"user has admin role", "admin", nil, []string{"viewer", "admin"}, true}, + {"user lacks admin role", "admin", nil, []string{"viewer"}, false}, + {"user has no roles", "admin", nil, nil, false}, + {"role match is exact (case-sensitive)", "admin", nil, []string{"Admin"}, false}, + + // admin_roles list + {"user matches first entry in admin_roles", "", []string{"group1", "group2"}, []string{"group1"}, true}, + {"user matches second entry in admin_roles", "", []string{"group1", "group2"}, []string{"group2"}, true}, + {"user matches none of admin_roles", "", []string{"group1", "group2"}, []string{"group3"}, false}, + {"empty admin_roles list returns false", "", []string{}, []string{"group1"}, false}, + + // admin_role + admin_roles merge + {"matches single admin_role when admin_roles set too", "admin", []string{"ops"}, []string{"admin"}, true}, + {"matches admin_roles entry when admin_role set too", "admin", []string{"ops"}, []string{"ops"}, true}, + {"matches neither admin_role nor admin_roles", "admin", []string{"ops"}, []string{"viewer"}, false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { svc := &Service{ authConfig: &config.AuthConfig{ - OIDC: config.OIDCConfig{AdminRole: tc.adminRole}, + OIDC: config.OIDCConfig{AdminRole: tc.adminRole, AdminRoles: tc.adminRoles}, }, } if got := svc.IsAdmin(&UserInfo{Roles: tc.userRoles}); got != tc.want { diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 82de209..63164d5 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -78,6 +78,7 @@ type OIDCConfig struct { NameAttribute string `mapstructure:"name_attribute"` RoleAttributePath string `mapstructure:"role_attribute_path"` AdminRole string `mapstructure:"admin_role"` + AdminRoles []string `mapstructure:"admin_roles"` TLSSkipVerify bool `mapstructure:"tls_skip_verify"` SessionMaxAge int `mapstructure:"session_max_age"` CookieName string `mapstructure:"cookie_name"` @@ -86,6 +87,29 @@ type OIDCConfig struct { CookieSameSite string `mapstructure:"cookie_same_site"` } +// EffectiveAdminRoles returns the deduplicated list of admin roles drawn from +// both admin_role (legacy single-value) and admin_roles (list). A user is +// considered an admin if any of their roles matches any entry in this list. +func (o OIDCConfig) EffectiveAdminRoles() []string { + seen := make(map[string]struct{}, len(o.AdminRoles)+1) + var roles []string + add := func(r string) { + if r == "" { + return + } + if _, ok := seen[r]; ok { + return + } + seen[r] = struct{}{} + roles = append(roles, r) + } + add(o.AdminRole) + for _, r := range o.AdminRoles { + add(r) + } + return roles +} + // CORSConfig contains CORS settings for frontend communication type CORSConfig struct { Enabled bool `mapstructure:"enabled"` @@ -231,6 +255,7 @@ func bindEnvVars() { viper.BindEnv("auth.oidc.name_attribute", "GARAGE_UI_AUTH_OIDC_NAME_ATTRIBUTE") viper.BindEnv("auth.oidc.role_attribute_path", "GARAGE_UI_AUTH_OIDC_ROLE_ATTRIBUTE_PATH") viper.BindEnv("auth.oidc.admin_role", "GARAGE_UI_AUTH_OIDC_ADMIN_ROLE") + viper.BindEnv("auth.oidc.admin_roles", "GARAGE_UI_AUTH_OIDC_ADMIN_ROLES") viper.BindEnv("auth.oidc.tls_skip_verify", "GARAGE_UI_AUTH_OIDC_TLS_SKIP_VERIFY") viper.BindEnv("auth.oidc.session_max_age", "GARAGE_UI_AUTH_OIDC_SESSION_MAX_AGE") viper.BindEnv("auth.oidc.cookie_name", "GARAGE_UI_AUTH_OIDC_COOKIE_NAME") @@ -291,11 +316,12 @@ func (c *Config) Validate() error { return fmt.Errorf("oidc scopes are required when oidc is enabled") } // Every authenticated route on this service grants full admin - // access — there is no separate authorization layer. An empty - // admin_role would therefore promote every user in the IdP realm - // to cluster admin. Require operators to opt in explicitly. - if c.Auth.OIDC.AdminRole == "" { - return fmt.Errorf("oidc admin_role is required when oidc is enabled: leaving it empty would grant cluster-admin access to any authenticated IdP user") + // access — there is no separate authorization layer. Empty + // admin role configuration would therefore promote every user + // in the IdP realm to cluster admin. Require operators to opt + // in explicitly via admin_role or admin_roles. + if len(c.Auth.OIDC.EffectiveAdminRoles()) == 0 { + return fmt.Errorf("oidc admin_role or admin_roles is required when oidc is enabled: leaving them empty would grant cluster-admin access to any authenticated IdP user") } } diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go index af96ecf..c1716fc 100644 --- a/backend/internal/config/config_test.go +++ b/backend/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -292,7 +293,33 @@ func TestValidate(t *testing.T) { applyValidOIDC(c) c.Auth.OIDC.AdminRole = "" }, - wantErrContains: "oidc admin_role is required", + wantErrContains: "oidc admin_role or admin_roles is required", + }, + { + name: "oidc enabled with admin_roles only is valid", + mutate: func(c *Config) { + applyValidOIDC(c) + c.Auth.OIDC.AdminRole = "" + c.Auth.OIDC.AdminRoles = []string{"group1", "group2"} + }, + wantErrContains: "", + }, + { + name: "oidc enabled with both admin_role and admin_roles is valid", + mutate: func(c *Config) { + applyValidOIDC(c) + c.Auth.OIDC.AdminRoles = []string{"group2", "group3"} + }, + wantErrContains: "", + }, + { + name: "oidc enabled with empty admin_role and empty admin_roles rejected", + mutate: func(c *Config) { + applyValidOIDC(c) + c.Auth.OIDC.AdminRole = "" + c.Auth.OIDC.AdminRoles = []string{} + }, + wantErrContains: "oidc admin_role or admin_roles is required", }, { name: "oidc fully configured is valid", @@ -498,6 +525,32 @@ func TestValidate_TokenAuthExplicitlyEnabled(t *testing.T) { } } +func TestEffectiveAdminRoles(t *testing.T) { + tests := []struct { + name string + adminRole string + adminRoles []string + want []string + }{ + {"both empty", "", nil, nil}, + {"single only", "admin", nil, []string{"admin"}}, + {"list only", "", []string{"a", "b"}, []string{"a", "b"}}, + {"merge single + list", "admin", []string{"viewer", "ops"}, []string{"admin", "viewer", "ops"}}, + {"dedupes overlap", "admin", []string{"admin", "ops"}, []string{"admin", "ops"}}, + {"dedupes within list", "", []string{"a", "a", "b"}, []string{"a", "b"}}, + {"skips empty strings in list", "admin", []string{"", "ops", ""}, []string{"admin", "ops"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + o := OIDCConfig{AdminRole: tc.adminRole, AdminRoles: tc.adminRoles} + got := o.EffectiveAdminRoles() + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("EffectiveAdminRoles() = %v, want %v", got, tc.want) + } + }) + } +} + func TestIsProduction(t *testing.T) { tests := []struct { env string diff --git a/backend/internal/routes/routes.go b/backend/internal/routes/routes.go index e9079bf..f4d6be6 100644 --- a/backend/internal/routes/routes.go +++ b/backend/internal/routes/routes.go @@ -236,7 +236,8 @@ func SetupRoutes( // ID token and the userinfo endpoint (Keycloak emits resource_access // only in the access token by default), so fall back to the access // token and then the userinfo endpoint before denying access. - if cfg.Auth.OIDC.AdminRole != "" { + adminRoles := cfg.Auth.OIDC.EffectiveAdminRoles() + if len(adminRoles) > 0 { if !authService.IsAdmin(userInfo) { if roles := authService.ExtractRolesFromAccessToken(token.AccessToken); len(roles) > 0 { userInfo.Roles = roles @@ -250,9 +251,9 @@ func SetupRoutes( if !authService.IsAdmin(userInfo) { logger.Warn(). Str("username", userInfo.Username). - Str("required_role", cfg.Auth.OIDC.AdminRole). + Strs("required_roles", adminRoles). Strs("roles", userInfo.Roles). - Msg("OIDC login denied: user does not have required admin role") + Msg("OIDC login denied: user does not have any required admin role") return c.Status(fiber.StatusForbidden).JSON(fiber.Map{ "error": "User does not have the required admin role", }) diff --git a/config.example.yaml b/config.example.yaml index 24df37a..29a7314 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -75,7 +75,14 @@ auth: # Role-based access (optional) role_attribute_path: "resource_access.garage-ui.roles" + # Single admin role (backward-compatible). admin_role: "admin" + # Multiple admin roles: a user is granted admin if ANY of their roles + # matches ANY entry below. Values from admin_role and admin_roles are + # merged, so you can set either, both, or only admin_roles. + # admin_roles: + # - "garage-admins" + # - "platform-team" # TLS configuration tls_skip_verify: false # Only set to true for testing, not recommended for production diff --git a/helm/garage-ui/values.yaml b/helm/garage-ui/values.yaml index 96951d0..05d4b09 100644 --- a/helm/garage-ui/values.yaml +++ b/helm/garage-ui/values.yaml @@ -84,9 +84,13 @@ config: email_attribute: "email" username_attribute: "preferred_username" name_attribute: "name" - # Role-based access control + # Role-based access control. + # Set admin_role (single value, backward-compatible) and/or admin_roles + # (list). Values from both are merged: a user is granted admin if ANY + # of their roles matches ANY entry across these two fields. role_attribute_path: "resource_access.garage-ui.roles" admin_role: "admin" + admin_roles: [] # TLS settings tls_skip_verify: false # Session settings