Auth: Improve privilege level change detection #3512

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

internal/api/users_avatar.go

@@ -36,11 +36,11 @@ func UploadUserAvatar(router *gin.RouterGroup) {
 		}
 
 		// Check if the session user is has user management privileges.
-		isPrivileged := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
+		isAdmin := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
 		uid := clean.UID(c.Param("uid"))
 
 		// Users may only change their own avatar.
-		if !isPrivileged && s.User().UserUID != uid {
+		if !isAdmin && s.User().UserUID != uid {
 			event.AuditErr([]string{ClientIP(c), "session %s", "upload avatar", "user does not match"}, s.RefID)
 			AbortForbidden(c)
 			return

internal/api/users_password.go

@@ -43,19 +43,19 @@ func UpdateUserPassword(router *gin.RouterGroup) {
 		}
 
 		// Check if the session user is has user management privileges.
-		isPrivileged := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
+		isAdmin := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
-		isSuperAdmin := isPrivileged && s.User().IsSuperAdmin()
+		isSuperAdmin := isAdmin && s.User().IsSuperAdmin()
 		uid := clean.UID(c.Param("uid"))
 
 		var u *entity.User
 
 		// Users may only change their own password.
-		if !isPrivileged && s.User().UserUID != uid {
+		if !isAdmin && s.User().UserUID != uid {
 			AbortForbidden(c)
 			return
 		} else if s.User().UserUID == uid {
 			u = s.User()
-			isPrivileged = false
+			isAdmin = false
 			isSuperAdmin = false
 		} else if u = entity.FindUserByUID(uid); u == nil {
 			Abort(c, http.StatusNotFound, i18n.ErrUserNotFound)

internal/api/users_update.go

@@ -61,7 +61,8 @@ func UpdateUser(router *gin.RouterGroup) {
 		}
 
 		// Check if the session user is has user management privileges.
-		isPrivileged := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
+		isAdmin := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage})
+		privilegeLevelChange := isAdmin && m.PrivilegeLevelChange(f)
 
 		// Prevent super admins from locking themselves out.
 		if u := s.User(); u.IsSuperAdmin() && u.Equal(m) && !f.CanLogin {
@@ -69,7 +70,7 @@ func UpdateUser(router *gin.RouterGroup) {
 		}
 
 		// Save model with values from form.
-		if err = m.SaveForm(f, isPrivileged); err != nil {
+		if err = m.SaveForm(f, isAdmin); err != nil {
 			event.AuditErr([]string{ClientIP(c), "session %s", "users", m.UserName, "update", err.Error()}, s.RefID)
 			AbortSaveFailed(c)
 			return
@@ -78,9 +79,9 @@ func UpdateUser(router *gin.RouterGroup) {
 		// Log event.
 		event.AuditInfo([]string{ClientIP(c), "session %s", "users", m.UserName, "updated"}, s.RefID)
 
-		// Delete user sessions after a permission level change.
+		// Delete user sessions after a privilege level change.
 		// see https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change
-		if isPrivileged {
+		if privilegeLevelChange {
 			// Prevent the current session from being deleted.
 			deleted := m.DeleteSessions([]string{s.ID})
 			event.AuditInfo([]string{ClientIP(c), "session %s", "users", m.UserName, "invalidated %s"}, s.RefID,

internal/entity/auth_user.go

@@ -1016,8 +1016,20 @@ func (m *User) Form() (form.User, error) {
 	return frm, nil
 }
 
+// PrivilegeLevelChange checks if saving the form changes the user privileges.
+func (m *User) PrivilegeLevelChange(f form.User) bool {
+	return m.UserRole != f.Role() ||
+		m.SuperAdmin != f.SuperAdmin ||
+		m.CanLogin != f.CanLogin ||
+		m.WebDAV != f.WebDAV ||
+		m.UserAttr != f.Attr() ||
+		m.AuthProvider != f.Provider().String() ||
+		m.BasePath != f.BasePath ||
+		m.UploadPath != f.UploadPath
+}
+
 // SaveForm updates the entity using form data and stores it in the database.
-func (m *User) SaveForm(f form.User, updateRights bool) error {
+func (m *User) SaveForm(f form.User, changePrivileges bool) error {
 	if m.UserName == "" || m.ID <= 0 {
 		return fmt.Errorf("system users cannot be modified")
 	} else if (m.ID == 1 || f.SuperAdmin) && acl.RoleAdmin.NotEqual(f.Role()) {
@@ -1055,8 +1067,8 @@ func (m *User) SaveForm(f form.User, updateRights bool) error {
 		m.VerifyToken = GenerateToken()
 	}
 
-	// Update user rights only if explicitly requested.
+	// Change user privileges only if allowed.
-	if updateRights {
+	if changePrivileges {
 		m.SetRole(f.Role())
 		m.SuperAdmin = f.SuperAdmin
 

internal/entity/auth_user_test.go

@@ -920,6 +920,41 @@ func TestUser_Form(t *testing.T) {
 	})
 }
 
+func TestUser_PrivilegeLevelChange(t *testing.T) {
+	t.Run("True", func(t *testing.T) {
+		m := FindUserByName("alice")
+
+		if m == nil {
+			t.Fatal("result should not be nil")
+		}
+
+		frm, err := m.Form()
+
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		frm.UserRole = "guest"
+
+		assert.True(t, m.PrivilegeLevelChange(frm))
+	})
+	t.Run("False", func(t *testing.T) {
+		m := FindUserByName("alice")
+
+		if m == nil {
+			t.Fatal("result should not be nil")
+		}
+
+		frm, err := m.Form()
+
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		assert.False(t, m.PrivilegeLevelChange(frm))
+	})
+}
+
 func TestUser_SaveForm(t *testing.T) {
 	t.Run("UnknownUser", func(t *testing.T) {
 		frm, err := UnknownUser.Form()