Auth: Add alias for RoleNone and improve unit tests coverage #98

Signed-off-by: Michael Mayer <michael@photoprism.app>

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"

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"
 	}
 

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
 }
 

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])
+	})
+}

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")
+	})
+}

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")
+	})
+}

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)
+}

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")
+	})
+}

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

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))
+	})
+}

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{