diff --git a/internal/auth/acl/const.go b/internal/auth/acl/const.go index dd8f8df77..3c83bcafc 100644 --- a/internal/auth/acl/const.go +++ b/internal/auth/acl/const.go @@ -1,5 +1,8 @@ package acl +// RoleAliasNone is a more explicit, user-friendly alias for RoleNone. +const RoleAliasNone = "none" + // Roles that can be granted Permissions to use a Resource. const ( RoleDefault Role = "default" diff --git a/internal/auth/acl/role.go b/internal/auth/acl/role.go index 2c109534f..9b24d3de7 100644 --- a/internal/auth/acl/role.go +++ b/internal/auth/acl/role.go @@ -17,7 +17,7 @@ func (r Role) String() string { // Pretty returns the type in an easy-to-read format. func (r Role) Pretty() string { - if r == RoleNone { + if r == RoleNone || r == RoleAliasNone { return "None" } diff --git a/internal/auth/acl/roles.go b/internal/auth/acl/roles.go index 745248b31..0aa03f133 100644 --- a/internal/auth/acl/roles.go +++ b/internal/auth/acl/roles.go @@ -14,6 +14,7 @@ var UserRoles = RoleStrings{ string(RoleGuest): RoleGuest, string(RoleVisitor): RoleVisitor, string(RoleNone): RoleNone, + RoleAliasNone: RoleNone, } // ClientRoles maps valid API client roles. @@ -24,17 +25,28 @@ var ClientRoles = RoleStrings{ string(RolePortal): RolePortal, string(RoleClient): RoleClient, string(RoleNone): RoleNone, + RoleAliasNone: RoleNone, } // Strings returns the roles as string slice. func (m RoleStrings) Strings() []string { result := make([]string, 0, len(m)) + includesNone := false + for r := range m { - if r != "" { + if r == RoleAliasNone { + includesNone = true + } else if r != string(RoleNone) { 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 cb798b800..39e138a52 100644 --- a/internal/auth/acl/roles_test.go +++ b/internal/auth/acl/roles_test.go @@ -87,3 +87,45 @@ func TestRoles_Allow(t *testing.T) { assert.False(t, roles.Allow(RoleUser, ActionView)) }) } + +func TestRoleStrings_GlobalMaps_AliasNoneAndUsage(t *testing.T) { + t.Run("ClientRoles Strings include alias none, exclude empty", func(t *testing.T) { + got := ClientRoles.Strings() + // Contains exactly the expected elements, order not enforced. + assert.ElementsMatch(t, []string{"admin", "client", "instance", "none", "portal", "service"}, got) + // Does not include empty string + for _, s := range got { + assert.NotEqual(t, "", s) + } + }) + + t.Run("UserRoles Strings include alias none, exclude empty", func(t *testing.T) { + got := UserRoles.Strings() + assert.ElementsMatch(t, []string{"admin", "guest", "none", "visitor"}, got) + for _, s := range got { + assert.NotEqual(t, "", s) + } + }) + + t.Run("ClientRoles CliUsageString includes none and or before last", 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"} { + assert.Contains(t, u, s) + } + assert.Regexp(t, `, or none$`, u) + }) + + t.Run("UserRoles CliUsageString includes none and or before last", func(t *testing.T) { + u := UserRoles.CliUsageString() + for _, s := range []string{"admin", "guest", "visitor", "none"} { + assert.Contains(t, u, s) + } + assert.Regexp(t, `, or none$`, u) + }) + + t.Run("Alias none maps to RoleNone", func(t *testing.T) { + assert.Equal(t, RoleNone, ClientRoles[RoleAliasNone]) + assert.Equal(t, RoleNone, UserRoles[RoleAliasNone]) + }) +} diff --git a/internal/commands/clients_add_test.go b/internal/commands/clients_add_test.go index 48665ae33..0b8374388 100644 --- a/internal/commands/clients_add_test.go +++ b/internal/commands/clients_add_test.go @@ -20,3 +20,16 @@ func TestClientsAddCommand(t *testing.T) { assert.Contains(t, output, "Client Secret") }) } + +func TestClientsAddCommand_AddWithRoleAndUser(t *testing.T) { + t.Run("AddClientWithRolePortalAndUserAlice", func(t *testing.T) { + output, err := RunWithTestContext(ClientsAddCommand, []string{"add", "--name=Roly Poly", "--scope=vision", "--role=portal", "alice"}) + + assert.NoError(t, err) + assert.Contains(t, output, "Roly Poly") + assert.Contains(t, output, "portal") + assert.Contains(t, output, "vision") + assert.Contains(t, output, "alice") + assert.Contains(t, output, "Client Secret") + }) +} diff --git a/internal/commands/clients_flags_test.go b/internal/commands/clients_flags_test.go new file mode 100644 index 000000000..696780673 --- /dev/null +++ b/internal/commands/clients_flags_test.go @@ -0,0 +1,38 @@ +package commands + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/urfave/cli/v2" +) + +func TestClientRoleFlagUsage_IncludesNoneAlias(t *testing.T) { + t.Run("AddCommand role flag includes none", func(t *testing.T) { + var roleFlag *cli.StringFlag + for _, f := range ClientsAddCommand.Flags { + if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { + roleFlag = rf + break + } + } + if roleFlag == nil { + t.Fatal("role flag not found on ClientsAddCommand") + } + assert.Contains(t, roleFlag.Usage, "none") + }) + + t.Run("ModCommand role flag includes none", func(t *testing.T) { + var roleFlag *cli.StringFlag + for _, f := range ClientsModCommand.Flags { + if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { + roleFlag = rf + break + } + } + if roleFlag == nil { + t.Fatal("role flag not found on ClientsModCommand") + } + assert.Contains(t, roleFlag.Usage, "none") + }) +} diff --git a/internal/commands/clients_mod_test.go b/internal/commands/clients_mod_test.go index eb14c2b4d..8a07167f0 100644 --- a/internal/commands/clients_mod_test.go +++ b/internal/commands/clients_mod_test.go @@ -65,3 +65,43 @@ func TestClientsModCommand(t *testing.T) { assert.Contains(t, output, "Client Secret") }) } + +func TestClientsModCommand_ModRoleScopeLimits(t *testing.T) { + // Modify existing fixture client "analytics" (cs7pvt5h8rw9aaqj). + out0, err := RunWithTestContext(ClientsShowCommand, []string{"show", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + assert.Contains(t, out0, "ClientRole") + + // Apply changes. + _, err = RunWithTestContext(ClientsModCommand, []string{"mod", "--role=portal", "--scope=audit metrics", "--expires=600", "--tokens=3", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + + // Verify via show. + out1, err := RunWithTestContext(ClientsShowCommand, []string{"show", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + assert.Contains(t, out1, "ClientRole │ \"portal\"") + assert.Contains(t, out1, "AuthScope │ \"audit metrics\"") + assert.Contains(t, out1, "AuthExpires │ 600") + assert.Contains(t, out1, "AuthTokens │ 3") +} + +func TestClientsModCommand_ModRoleToNoneAndEmpty(t *testing.T) { + // Set to explicit none + _, err := RunWithTestContext(ClientsModCommand, []string{"mod", "--role=none", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + out1, err := RunWithTestContext(ClientsShowCommand, []string{"show", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + // Expect empty string value for ClientRole in report output + assert.Contains(t, out1, "ClientRole │ \"\"") + + // Set to explicit empty string (treated as none) + _, err = RunWithTestContext(ClientsModCommand, []string{"mod", "--role=", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + out2, err := RunWithTestContext(ClientsShowCommand, []string{"show", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) + assert.Contains(t, out2, "ClientRole │ \"\"") + + // Restore to client for other tests + _, err = RunWithTestContext(ClientsModCommand, []string{"mod", "--role=client", "cs7pvt5h8rw9aaqj"}) + assert.NoError(t, err) +} diff --git a/internal/commands/users_flags_test.go b/internal/commands/users_flags_test.go new file mode 100644 index 000000000..8f7884d5f --- /dev/null +++ b/internal/commands/users_flags_test.go @@ -0,0 +1,38 @@ +package commands + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/urfave/cli/v2" +) + +func TestUserRoleFlagUsage_IncludesNoneAlias(t *testing.T) { + t.Run("AddCommand user role flag includes none", func(t *testing.T) { + var roleFlag *cli.StringFlag + for _, f := range UsersAddCommand.Flags { + if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { + roleFlag = rf + break + } + } + if roleFlag == nil { + t.Fatal("role flag not found on UsersAddCommand") + } + assert.Contains(t, roleFlag.Usage, "none") + }) + + t.Run("ModCommand user role flag includes none", func(t *testing.T) { + var roleFlag *cli.StringFlag + for _, f := range UsersModCommand.Flags { + if rf, ok := f.(*cli.StringFlag); ok && rf.Name == "role" { + roleFlag = rf + break + } + } + if roleFlag == nil { + t.Fatal("role flag not found on UsersModCommand") + } + assert.Contains(t, roleFlag.Usage, "none") + }) +} diff --git a/internal/entity/auth_client.go b/internal/entity/auth_client.go index 7e9a6e609..21e75ca7d 100644 --- a/internal/entity/auth_client.go +++ b/internal/entity/auth_client.go @@ -154,8 +154,13 @@ func (m *Client) SetName(s string) *Client { // SetRole sets the client role specified as string. func (m *Client) SetRole(role string) *Client { - if role != "" { - m.ClientRole = acl.ClientRoles[clean.Role(role)].String() + r := clean.Role(role) + + // Map known roles (includes aliases like "none" or empty); fall back to client if unknown. + if mapped, ok := acl.ClientRoles[r]; ok { + m.ClientRole = mapped.String() + } else { + m.ClientRole = acl.RoleClient.String() } return m diff --git a/internal/entity/auth_client_add_test.go b/internal/entity/auth_client_add_test.go index 4308eea74..e63de57e8 100644 --- a/internal/entity/auth_client_add_test.go +++ b/internal/entity/auth_client_add_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" + "github.com/photoprism/photoprism/internal/auth/acl" "github.com/photoprism/photoprism/internal/form" ) @@ -55,3 +56,50 @@ func Test_AddClient(t *testing.T) { assert.Equal(t, "Monitoring", c.ClientName) }) } + +func Test_AddClient_WithRole(t *testing.T) { + t.Run("AdminRole", func(t *testing.T) { + frm := form.Client{ + ClientID: "cs5cpu17n6gj9r10", + ClientName: "Role Admin", + ClientRole: "admin", + AuthProvider: "client_credentials", + AuthMethod: "oauth2", + AuthScope: "all", + } + + c, err := AddClient(frm) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, "admin", c.ClientRole) + assert.True(t, c.HasRole(acl.RoleAdmin)) + + // Verify persisted role via lookup. + persisted := FindClientByUID("cs5cpu17n6gj9r10") + if persisted == nil { + t.Fatal("persisted client not found") + } + assert.Equal(t, "admin", persisted.ClientRole) + }) + + t.Run("InvalidRoleDefaultsToClient", func(t *testing.T) { + frm := form.Client{ + ClientID: "cs5cpu17n6gj9r11", + ClientName: "Role Invalid", + ClientRole: "superuser", + AuthProvider: "client_credentials", + AuthMethod: "oauth2", + AuthScope: "all", + } + + c, err := AddClient(frm) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, "client", c.ClientRole) + assert.True(t, c.HasRole(acl.RoleClient)) + }) +} diff --git a/internal/entity/auth_client_test.go b/internal/entity/auth_client_test.go index 65e6b88e0..12470cf31 100644 --- a/internal/entity/auth_client_test.go +++ b/internal/entity/auth_client_test.go @@ -3,6 +3,7 @@ package entity import ( "testing" + "github.com/photoprism/photoprism/internal/auth/acl" "github.com/photoprism/photoprism/internal/form" "github.com/photoprism/photoprism/pkg/authn" "github.com/photoprism/photoprism/pkg/txt/report" @@ -619,6 +620,157 @@ func TestClient_SetFormValues(t *testing.T) { }) } +func TestClient_SetFormValues_Role(t *testing.T) { + t.Run("SetValidRoleFromForm", func(t *testing.T) { + m := Client{ClientName: "RoleTest", ClientUID: "cs5cpu17n6gj9r01"} + + // Persist once to align with other tests, though not required for SetFormValues. + if err := m.Save(); err != nil { + t.Fatal(err) + } + + // Apply role via form. + c := m.SetFormValues(form.Client{ClientRole: "portal"}) + + assert.Equal(t, "portal", c.ClientRole) + assert.True(t, c.HasRole(acl.RolePortal)) + assert.False(t, c.HasRole(acl.RoleClient)) + }) + + t.Run("InvalidRoleFromFormDefaultsToClient", func(t *testing.T) { + m := Client{ClientName: "InvalidRole", ClientUID: "cs5cpu17n6gj9r02"} + if err := m.Save(); err != nil { + t.Fatal(err) + } + + // Unknown role → default to client. + c := m.SetFormValues(form.Client{ClientRole: "superuser"}) + + assert.Equal(t, "client", c.ClientRole) + assert.True(t, c.HasRole(acl.RoleClient)) + }) + + t.Run("ChangeRoleFromClientToAdmin", func(t *testing.T) { + m := NewClient() + m.ClientName = "ChangeRole" + m.ClientUID = "cs5cpu17n6gj9r03" + + // Default role is "client". + if err := m.Save(); err != nil { + t.Fatal(err) + } + assert.True(t, m.HasRole(acl.RoleClient)) + + // Change to admin via form. + m.SetFormValues(form.Client{ClientRole: "admin"}) + assert.True(t, m.HasRole(acl.RoleAdmin)) + assert.Equal(t, "admin", m.ClientRole) + }) +} + +func TestClient_SetFormValues_AuthEnabledToggle(t *testing.T) { + // Start enabled; attempt to disable via form should NOT flip to false. + m := NewClient() + m.ClientName = "ToggleEnabled" + m.ClientUID = "cs5cpu17n6gj9r04" + m.AuthEnabled = true + if err := m.Save(); err != nil { + t.Fatal(err) + } + + m.SetFormValues(form.Client{AuthEnabled: false}) + assert.True(t, m.AuthEnabled, "SetFormValues should not disable when AuthEnabled=false") + + // Now explicitly enable from false → true + m.AuthEnabled = false + m.SetFormValues(form.Client{AuthEnabled: true}) + assert.True(t, m.AuthEnabled) +} + +func TestClient_SetFormValues_SetUser(t *testing.T) { + t.Run("ByUID", func(t *testing.T) { + m := NewClient() + m.ClientName = "SetUserByUID" + m.ClientUID = "cs5cpu17n6gj9r05" + if err := m.Save(); err != nil { + t.Fatal(err) + } + + uid := UserFixtures.Pointer("friend").UserUID + c := m.SetFormValues(form.Client{UserUID: uid}) + + assert.Equal(t, uid, c.UserUID) + assert.Equal(t, uid, c.User().UserUID) + }) + + t.Run("ByUserName", func(t *testing.T) { + m := NewClient() + m.ClientName = "SetUserByName" + m.ClientUID = "cs5cpu17n6gj9r06" + if err := m.Save(); err != nil { + t.Fatal(err) + } + + c := m.SetFormValues(form.Client{UserName: "alice"}) + + assert.Equal(t, UserFixtures.Pointer("alice").UserUID, c.UserUID) + assert.Equal(t, "alice", c.UserName) + assert.Equal(t, "alice", c.User().UserName) + }) + + t.Run("UnknownUserNoChange", func(t *testing.T) { + // Seed with a known user, then attempt to change to an unknown one. + m := NewClient() + m.ClientName = "UnknownUserNoChange" + m.ClientUID = "cs5cpu17n6gj9r07" + m.SetUser(UserFixtures.Pointer("bob")) + if err := m.Save(); err != nil { + t.Fatal(err) + } + + prevUID := m.UserUID + c := m.SetFormValues(form.Client{UserUID: "u0000000000000xx", UserName: "nonexistent"}) + + assert.Equal(t, prevUID, c.UserUID) + assert.Equal(t, "bob", c.UserName) + }) +} + +func TestClient_AclRole_Resolution(t *testing.T) { + t.Run("EmptyIsNone", func(t *testing.T) { + m := &Client{ClientRole: ""} + assert.Equal(t, acl.RoleNone, m.AclRole()) + }) + + t.Run("ClientIsClient", func(t *testing.T) { + m := &Client{ClientRole: "client"} + assert.Equal(t, acl.RoleClient, m.AclRole()) + }) +} + +func TestClient_SetRole_AliasNoneAndCase(t *testing.T) { + m := &Client{} + m.SetRole("NoNe") + assert.True(t, m.HasRole(acl.RoleNone)) + + m.SetRole("") + assert.True(t, m.HasRole(acl.RoleNone)) +} + +func TestClient_SetFormValues_DoesNotOverrideUID(t *testing.T) { + m := NewClient() + m.ClientName = "KeepUID" + m.ClientUID = "cs5cpu17n6gj9r08" + if err := m.Save(); err != nil { + t.Fatal(err) + } + + // Attempt to override with a different id via form; should be ignored. + c := m.SetFormValues(form.Client{ClientID: "cs5cpu17n6gj9zzz", ClientName: "KeepUID2"}) + assert.Equal(t, "cs5cpu17n6gj9r08", c.ClientUID) + assert.Equal(t, "KeepUID2", c.ClientName) +} + func TestClient_Validate(t *testing.T) { t.Run("Success", func(t *testing.T) { m := Client{