From fbe8a6889b6bb84bbc09e1acddfb4c7061a8da67 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Tue, 2 Jun 2026 11:57:02 +0000 Subject: [PATCH] Auth: Add cluster_admin role with admin-tier and federation helpers Adds the Portal-only cluster_admin operator role (declared in CE, registered only on Portal builds), the acl.AdminRoles/IsAdminRole admin-tier set, and the acl.IsFederatedRole/FederatedRoleUpdate helpers that keep cluster_admin and visitor out of externally mapped account roles. SuperAdmin resolves to cluster_admin on registered builds, SaveForm keeps an admin-level role on a self-save, and UpdateUser rejects self-role changes. RoleStrings.Strings hides visitor and none from the CLI role help. --- internal/api/users_update.go | 16 ++++ internal/api/users_update_test.go | 23 ++++++ internal/auth/acl/const.go | 26 +++--- internal/auth/acl/roles.go | 61 +++++++++++--- internal/auth/acl/roles_test.go | 103 ++++++++++++++++++++---- internal/commands/clients_flags_test.go | 15 ++-- internal/commands/users_flags_test.go | 17 ++-- internal/entity/auth_user.go | 48 ++++++++--- 8 files changed, 249 insertions(+), 60 deletions(-) diff --git a/internal/api/users_update.go b/internal/api/users_update.go index ddb7c01b5..131f2f417 100644 --- a/internal/api/users_update.go +++ b/internal/api/users_update.go @@ -64,6 +64,13 @@ func UpdateUser(router *gin.RouterGroup) { return } + // System accounts (Unknown id=-1, Visitor id=-2) must not be modified. + if m.ID < 0 { + event.AuditErr([]string{ClientIP(c), "session %s", "users", clean.Log(uid), "update", status.Denied}, s.RefID) + AbortForbidden(c) + return + } + // Initialize form with model values. f, err := m.Form() @@ -98,6 +105,15 @@ func UpdateUser(router *gin.RouterGroup) { // Get user from session. u := s.GetUser() + // Prevent users from changing their own account role, which could lock + // an operator out of the admin UI (e.g. a cluster_admin demoting + // themselves). Other own-profile fields remain editable. + if u != nil && u.UserUID == m.UserUID && f.UserRole != "" && clean.Role(f.UserRole) != clean.Role(m.UserRole) { + event.AuditErr([]string{ClientIP(c), "session %s", "users", m.UserName, "update own role", status.Denied}, s.RefID) + AbortForbidden(c) + return + } + // Persist form values. if err = m.SaveForm(f, u); err != nil { event.AuditErr([]string{ClientIP(c), "session %s", "users", m.UserName, "update", err.Error()}, s.RefID) diff --git a/internal/api/users_update_test.go b/internal/api/users_update_test.go index 57e70febc..0db5a344e 100644 --- a/internal/api/users_update_test.go +++ b/internal/api/users_update_test.go @@ -202,3 +202,26 @@ func TestUpdateUser(t *testing.T) { assert.Equal(t, http.StatusRequestEntityTooLarge, r.Code) }) } + +func TestUpdateUser_Guards(t *testing.T) { + t.Run("SelfRoleChangeForbidden", func(t *testing.T) { + app, router, conf := NewApiTest() + conf.SetAuthMode(config.AuthModePasswd) + defer conf.SetAuthMode(config.AuthModePublic) + UpdateUser(router) + sessId := AuthenticateUser(app, router, "alice", "Alice123!") + body, _ := json.Marshal(form.User{UserName: "alice", UserRole: "user"}) + r := AuthenticatedRequestWithBody(app, "PUT", "/api/v1/users/uqxetse3cy5eo9z2", string(body), sessId) + assert.Equal(t, http.StatusForbidden, r.Code) + }) + t.Run("SystemAccountForbidden", func(t *testing.T) { + app, router, conf := NewApiTest() + conf.SetAuthMode(config.AuthModePasswd) + defer conf.SetAuthMode(config.AuthModePublic) + UpdateUser(router) + sessId := AuthenticateUser(app, router, "alice", "Alice123!") + body, _ := json.Marshal(form.User{DisplayName: "Hacked"}) + r := AuthenticatedRequestWithBody(app, "PUT", "/api/v1/users/"+entity.UnknownUser.UserUID, string(body), sessId) + assert.Equal(t, http.StatusForbidden, r.Code) + }) +} diff --git a/internal/auth/acl/const.go b/internal/auth/acl/const.go index 86162e537..6128548e0 100644 --- a/internal/auth/acl/const.go +++ b/internal/auth/acl/const.go @@ -8,17 +8,21 @@ const RoleAliasNone = "none" // Roles that can be granted Permissions to use a Resource. const ( - RoleDefault Role = "default" - RoleAdmin Role = "admin" - RoleUser Role = "user" - RoleViewer Role = "viewer" - RoleGuest Role = "guest" - RoleVisitor Role = "visitor" - RoleInstance Role = "instance" - RoleService Role = "service" - RolePortal Role = "portal" - RoleClient Role = "client" - RoleNone Role = "" + RoleDefault Role = "default" + RoleAdmin Role = "admin" + // RoleClusterAdmin is a Portal-only operator role for the cluster admin UI. + // It is registered (and thus assignable and granted permissions) only on + // Portal builds; CE/Pro leave it absent from acl.UserRoles and acl.Rules. + RoleClusterAdmin Role = "cluster_admin" + RoleUser Role = "user" + RoleViewer Role = "viewer" + RoleGuest Role = "guest" + RoleVisitor Role = "visitor" + RoleInstance Role = "instance" + RoleService Role = "service" + RolePortal Role = "portal" + RoleClient Role = "client" + RoleNone Role = "" ) // Permissions to use a Resource that can be granted to a Role. diff --git a/internal/auth/acl/roles.go b/internal/auth/acl/roles.go index 54bc86524..b000982ca 100644 --- a/internal/auth/acl/roles.go +++ b/internal/auth/acl/roles.go @@ -29,29 +29,66 @@ var ClientRoles = RoleStrings{ RoleAliasNone: RoleNone, } -// Strings returns the roles as string slice. +// AdminRoles maps the roles that grant administrative privileges. The +// Portal-only cluster_admin is treated as an admin-tier role everywhere admin +// privileges are checked (e.g. user-management self-lockout protection), so a +// cluster_admin owner is not forced or downgraded to the plain admin role. +var AdminRoles = RoleStrings{ + string(RoleAdmin): RoleAdmin, + string(RoleClusterAdmin): RoleClusterAdmin, +} + +// IsAdminRole reports whether role is an administrative role (admin or cluster_admin). +func IsAdminRole(role Role) bool { + _, ok := AdminRoles[string(role)] + return ok +} + +// IsFederatedRole reports whether role may be assigned to a user account through +// an external identity provider (OIDC group/role claims or LDAP group/attribute +// mapping). The Portal-only operator role cluster_admin and the anonymous +// visitor role are never federated, and the empty role is rejected so a missing +// mapping cannot clear an account's role. Federation therefore neither grants +// nor revokes a non-federatable role: a compromised IdP/AD cannot escalate an +// account to operator access (the Portal Admin UI), and an existing +// cluster_admin/visitor account is never changed by a directory sync. +func IsFederatedRole(role Role) bool { + switch role { + case RoleNone, RoleClusterAdmin, RoleVisitor: + return false + default: + return true + } +} + +// FederatedRoleUpdate reports the account role an external identity provider may +// apply to an existing user and whether to apply it. Federation neither grants +// nor revokes a non-federatable role, so it returns ok=false when the current +// role is non-federatable (cluster_admin/visitor must not be touched), when the +// mapped role is non-federatable (no escalation to operator/visitor), or when +// the role is unchanged. Both the OIDC and LDAP sync paths share this decision. +func FederatedRoleUpdate(current, mapped Role) (Role, bool) { + if !IsFederatedRole(current) || !IsFederatedRole(mapped) || current == mapped { + return RoleNone, false + } + + return mapped, true +} + +// Strings returns the roles as string slice for display, e.g. CLI help. func (m RoleStrings) Strings() []string { result := make([]string, 0, len(m)) - includesNone := false for r := range m { - if r == "app" { + if r == "" || r == RoleAliasNone || r == "app" || r == RoleVisitor.String() { continue } - if r == RoleAliasNone { - includesNone = true - } else if r != string(RoleNone) { - result = append(result, r) - } + result = append(result, r) } sort.Strings(result) - if includesNone { - result = append(result, RoleAliasNone) - } - return result } diff --git a/internal/auth/acl/roles_test.go b/internal/auth/acl/roles_test.go index 2d10409d6..5a090077d 100644 --- a/internal/auth/acl/roles_test.go +++ b/internal/auth/acl/roles_test.go @@ -18,8 +18,10 @@ func TestRoleStrings_Strings_SortedAndNoEmpty(t *testing.T) { got := m.Strings() - // Expect deterministic, sorted output and no empty entries. - assert.Equal(t, []string{"admin", "guest", "visitor"}, got) + // Expect deterministic, sorted output, no empty entries, and visitor + // excluded (reserved for anonymous/link-share access, never offered). + assert.Equal(t, []string{"admin", "guest"}, got) + assert.NotContains(t, got, "visitor") assert.True(t, sort.StringsAreSorted(got)) } @@ -47,8 +49,12 @@ func TestRoleStrings_CliUsageString(t *testing.T) { assert.Equal(t, "admin, or guest", m.CliUsageString()) }) t.Run("Three", func(t *testing.T) { + m := RoleStrings{"user": RoleUser, "guest": RoleGuest, "admin": RoleAdmin} + assert.Equal(t, "admin, guest, or user", m.CliUsageString()) + }) + t.Run("ExcludesVisitor", func(t *testing.T) { m := RoleStrings{"visitor": RoleVisitor, "guest": RoleGuest, "admin": RoleAdmin} - assert.Equal(t, "admin, guest, or visitor", m.CliUsageString()) + assert.Equal(t, "admin, or guest", m.CliUsageString()) }) } @@ -84,36 +90,43 @@ func TestRoles_Allow(t *testing.T) { } func TestRoleStrings_GlobalMaps_AliasNoneAndUsage(t *testing.T) { - t.Run("ClientRolesStringsIncludeAliasNoneExcludeEmpty", func(t *testing.T) { + t.Run("ClientRolesStringsExcludeAliasNoneAndEmpty", func(t *testing.T) { got := ClientRoles.Strings() - // Contains exactly the expected elements, order not enforced. - assert.ElementsMatch(t, []string{"admin", "instance", "client", "none", "portal", "service"}, got) - // Does not include empty string + // Contains exactly the expected elements, order not enforced; the "none" + // alias and the empty role are excluded from display. + assert.ElementsMatch(t, []string{"admin", "instance", "client", "portal", "service"}, got) + assert.NotContains(t, got, "none") + // Does not include empty string. for _, s := range got { assert.NotEqual(t, "", s) } }) - t.Run("UserRolesStringsIncludeAliasNoneExcludeEmpty", func(t *testing.T) { + t.Run("UserRolesStringsExcludeAliasNoneEmptyAndVisitor", func(t *testing.T) { got := UserRoles.Strings() - assert.ElementsMatch(t, []string{"admin", "guest", "none", "visitor"}, got) + assert.ElementsMatch(t, []string{"admin", "guest"}, got) + assert.NotContains(t, got, "none") + assert.NotContains(t, got, "visitor") for _, s := range got { assert.NotEqual(t, "", s) } }) - t.Run("ClientRolesCliUsageStringIncludesNoneAndOrBeforeLast", func(t *testing.T) { + t.Run("ClientRolesCliUsageStringExcludesNoneAndOrBeforeLast", func(t *testing.T) { u := ClientRoles.CliUsageString() - // Should list known roles and end with "or none" (alias present). - for _, s := range []string{"admin", "client", "instance", "portal", "service", "none"} { + // Should list known roles and end with "or service"; the "none" alias is excluded. + for _, s := range []string{"admin", "client", "instance", "portal", "service"} { assert.Contains(t, u, s) } - assert.Regexp(t, `, or none$`, u) + assert.NotContains(t, u, "none") + assert.Regexp(t, `, or service$`, u) }) - t.Run("UserRolesCliUsageStringIncludesNoneAndOrBeforeLast", func(t *testing.T) { + t.Run("UserRolesCliUsageStringExcludesNoneVisitorAndOrBeforeLast", func(t *testing.T) { u := UserRoles.CliUsageString() - for _, s := range []string{"admin", "guest", "visitor", "none"} { + for _, s := range []string{"admin", "guest"} { assert.Contains(t, u, s) } - assert.Regexp(t, `, or none$`, u) + assert.NotContains(t, u, "none") + assert.NotContains(t, u, "visitor") + assert.Regexp(t, `, or guest$`, u) }) t.Run("AliasNoneMapsToRoleNone", func(t *testing.T) { assert.Equal(t, RoleNone, ClientRoles[RoleAliasNone]) @@ -177,3 +190,61 @@ func TestResourceNames_ContainsCore(t *testing.T) { assert.Truef(t, found, "resource %s not found in ResourceNames", w) } } + +func TestIsAdminRole(t *testing.T) { + assert.True(t, IsAdminRole(RoleAdmin)) + assert.True(t, IsAdminRole(RoleClusterAdmin)) + assert.False(t, IsAdminRole(RoleUser)) + assert.False(t, IsAdminRole(RoleViewer)) + assert.False(t, IsAdminRole(RoleGuest)) + assert.False(t, IsAdminRole(RoleVisitor)) + assert.False(t, IsAdminRole(RoleNone)) + assert.False(t, IsAdminRole(Role("manager"))) +} + +func TestIsFederatedRole(t *testing.T) { + t.Run("Federatable", func(t *testing.T) { + assert.True(t, IsFederatedRole(RoleAdmin)) + assert.True(t, IsFederatedRole(RoleUser)) + assert.True(t, IsFederatedRole(RoleViewer)) + assert.True(t, IsFederatedRole(RoleGuest)) + assert.True(t, IsFederatedRole(Role("manager"))) + assert.True(t, IsFederatedRole(Role("contributor"))) + }) + t.Run("NotFederatable", func(t *testing.T) { + // cluster_admin is the Portal operator role and visitor is anonymous; + // neither may be granted or revoked via an external IdP/AD, and an empty + // role must never be applied by a sync. + assert.False(t, IsFederatedRole(RoleClusterAdmin)) + assert.False(t, IsFederatedRole(RoleVisitor)) + assert.False(t, IsFederatedRole(RoleNone)) + }) +} + +func TestFederatedRoleUpdate(t *testing.T) { + t.Run("AppliesChangedFederatableRole", func(t *testing.T) { + role, ok := FederatedRoleUpdate(RoleUser, RoleViewer) + assert.True(t, ok) + assert.Equal(t, RoleViewer, role) + }) + t.Run("UnchangedRoleNotApplied", func(t *testing.T) { + _, ok := FederatedRoleUpdate(RoleUser, RoleUser) + assert.False(t, ok) + }) + t.Run("NeverDowngradesClusterAdmin", func(t *testing.T) { + // An existing cluster_admin/visitor account must not be touched. + _, ok := FederatedRoleUpdate(RoleClusterAdmin, RoleAdmin) + assert.False(t, ok) + _, ok = FederatedRoleUpdate(RoleVisitor, RoleGuest) + assert.False(t, ok) + }) + t.Run("NeverEscalatesToNonFederatable", func(t *testing.T) { + // The directory must not promote to cluster_admin/visitor. + _, ok := FederatedRoleUpdate(RoleUser, RoleClusterAdmin) + assert.False(t, ok) + _, ok = FederatedRoleUpdate(RoleAdmin, RoleVisitor) + assert.False(t, ok) + _, ok = FederatedRoleUpdate(RoleUser, RoleNone) + assert.False(t, ok) + }) +} diff --git a/internal/commands/clients_flags_test.go b/internal/commands/clients_flags_test.go index 7f6d73472..c4a39e4a4 100644 --- a/internal/commands/clients_flags_test.go +++ b/internal/commands/clients_flags_test.go @@ -7,8 +7,8 @@ import ( "github.com/urfave/cli/v2" ) -func TestClientRoleFlagUsage_IncludesNoneAlias(t *testing.T) { - t.Run("AddCommandRoleFlagIncludesNone", func(t *testing.T) { +func TestClientRoleFlagUsage_ExcludesNoneAlias(t *testing.T) { + t.Run("AddCommandRoleFlagExcludesNone", func(t *testing.T) { var roleFlag *cli.StringFlag for _, f := range ClientsAddCommand.Flags { if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { @@ -19,9 +19,12 @@ func TestClientRoleFlagUsage_IncludesNoneAlias(t *testing.T) { if roleFlag == nil { t.Fatal("role flag not found on ClientsAddCommand") } - assert.Contains(t, roleFlag.Usage, "none") + // Real client roles are listed; the "none" alias is excluded. + assert.Contains(t, roleFlag.Usage, "client") + assert.Contains(t, roleFlag.Usage, "service") + assert.NotContains(t, roleFlag.Usage, "none") }) - t.Run("ModCommandRoleFlagIncludesNone", func(t *testing.T) { + t.Run("ModCommandRoleFlagExcludesNone", func(t *testing.T) { var roleFlag *cli.StringFlag for _, f := range ClientsModCommand.Flags { if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { @@ -32,6 +35,8 @@ func TestClientRoleFlagUsage_IncludesNoneAlias(t *testing.T) { if roleFlag == nil { t.Fatal("role flag not found on ClientsModCommand") } - assert.Contains(t, roleFlag.Usage, "none") + assert.Contains(t, roleFlag.Usage, "client") + assert.Contains(t, roleFlag.Usage, "service") + assert.NotContains(t, roleFlag.Usage, "none") }) } diff --git a/internal/commands/users_flags_test.go b/internal/commands/users_flags_test.go index 77e7ec7d8..77eebe82e 100644 --- a/internal/commands/users_flags_test.go +++ b/internal/commands/users_flags_test.go @@ -7,8 +7,8 @@ import ( "github.com/urfave/cli/v2" ) -func TestUserRoleFlagUsage_IncludesNoneAlias(t *testing.T) { - t.Run("AddCommandUserRoleFlagIncludesNone", func(t *testing.T) { +func TestUserRoleFlagUsage_ExcludesNoneAndVisitor(t *testing.T) { + t.Run("AddCommandUserRoleFlagExcludesNoneAndVisitor", func(t *testing.T) { var roleFlag *cli.StringFlag for _, f := range UsersAddCommand.Flags { if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { @@ -19,9 +19,13 @@ func TestUserRoleFlagUsage_IncludesNoneAlias(t *testing.T) { if roleFlag == nil { t.Fatal("role flag not found on UsersAddCommand") } - assert.Contains(t, roleFlag.Usage, "none") + // Offered roles are listed; the "none" alias and "visitor" are excluded. + assert.Contains(t, roleFlag.Usage, "admin") + assert.Contains(t, roleFlag.Usage, "guest") + assert.NotContains(t, roleFlag.Usage, "none") + assert.NotContains(t, roleFlag.Usage, "visitor") }) - t.Run("ModCommandUserRoleFlagIncludesNone", func(t *testing.T) { + t.Run("ModCommandUserRoleFlagExcludesNoneAndVisitor", func(t *testing.T) { var roleFlag *cli.StringFlag for _, f := range UsersModCommand.Flags { if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { @@ -32,6 +36,9 @@ func TestUserRoleFlagUsage_IncludesNoneAlias(t *testing.T) { if roleFlag == nil { t.Fatal("role flag not found on UsersModCommand") } - assert.Contains(t, roleFlag.Usage, "none") + assert.Contains(t, roleFlag.Usage, "admin") + assert.Contains(t, roleFlag.Usage, "guest") + assert.NotContains(t, roleFlag.Usage, "none") + assert.NotContains(t, roleFlag.Usage, "visitor") }) } diff --git a/internal/entity/auth_user.go b/internal/entity/auth_user.go index 2d7b7c168..6d1e9c138 100644 --- a/internal/entity/auth_user.go +++ b/internal/entity/auth_user.go @@ -777,6 +777,12 @@ func (m *User) AclRole() acl.Role { switch { case m.SuperAdmin: + // SuperAdmins map to the strongest operator role available. On Portal + // builds that is cluster_admin (registered in acl.UserRoles); CE/Pro + // leave it unregistered, so super admins resolve to RoleAdmin there. + if r, ok := acl.UserRoles[acl.RoleClusterAdmin.String()]; ok { + return r + } return acl.RoleAdmin case role == "": return acl.RoleNone @@ -878,13 +884,23 @@ func (m *User) Equal(u *User) bool { return m.UserUID == u.UserUID } -// IsAdmin checks if the user is an admin with username. +// IsAdmin checks if the user is an admin with username. The Portal-only +// cluster_admin operator role counts as an admin so account-management logic +// (e.g. SaveForm role changes) treats it like a regular admin. func (m *User) IsAdmin() bool { if m == nil { return false } - return m.IsSuperAdmin() || m.IsRegistered() && m.AclRole() == acl.RoleAdmin + if !m.IsSuperAdmin() && !m.IsRegistered() { + return false + } + + if m.IsSuperAdmin() { + return true + } + + return acl.IsAdminRole(m.AclRole()) } // IsSuperAdmin checks if the user is a super admin. @@ -1328,7 +1344,9 @@ func (m *User) PrivilegeLevelChange(frm form.User) bool { func (m *User) SaveForm(frm form.User, u *User) error { if m.UserName == "" || m.ID <= 0 { return fmt.Errorf("system users cannot be modified") - } else if (m.ID == 1 || frm.SuperAdmin) && acl.RoleAdmin.NotEqual(frm.Role()) { + } else if (m.ID == 1 || frm.SuperAdmin) && !acl.IsAdminRole(acl.Role(frm.Role())) { + // Super admins must keep an admin-level role. cluster_admin is the + // Portal operator role and counts as admin-level (it is unused on CE/Pro). return fmt.Errorf("super admin must not have a non-admin role") } else if frm.BasePath != "" && clean.UserPath(frm.BasePath) == "" { return fmt.Errorf("invalid base folder") @@ -1372,9 +1390,11 @@ func (m *User) SaveForm(frm form.User, u *User) error { m.DeletedAt = nil } - // Prevent admins from locking themselves out. + // Prevent admins from locking themselves out: keep their current role + // and keep login enabled on a self-save. Forcing RoleAdmin here would + // silently demote a Portal cluster_admin; an explicit self role change + // is already rejected by the update handler. if u.Equal(m) { - m.SetRole(acl.RoleAdmin.String()) m.CanLogin = true } else { m.SetRole(frm.Role()) @@ -1402,16 +1422,22 @@ func (m *User) SaveForm(frm form.User, u *User) error { m.SetUploadPath(frm.UploadPath) } - // Ensure super admins never have a non-admin role. - if m.SuperAdmin { - m.SetRole(acl.RoleAdmin.String()) + // Ensure super admins keep an admin-level role (admin, or cluster_admin on + // the Portal). Anything else is normalized to the configured default admin + // role, derived from Admin.UserRole (admin on CE/Pro, cluster_admin on the + // Portal). + if m.SuperAdmin && !acl.IsAdminRole(acl.Role(m.UserRole)) { + m.SetRole(Admin.UserRole) } - // Make sure that the initial admin user cannot lock itself out. - if m.ID == Admin.ID && (m.AclRole() != acl.RoleAdmin || !m.SuperAdmin || !m.CanLogin) { - m.SetRole(acl.RoleAdmin.String()) + // Make sure the initial admin user cannot lock itself out: it stays a super + // admin that can log in with an admin-level role. + if m.ID == Admin.ID { m.SuperAdmin = true m.CanLogin = true + if !acl.IsAdminRole(acl.Role(m.UserRole)) { + m.SetRole(Admin.UserRole) + } } return m.Save()