diff --git a/internal/api/users_avatar.go b/internal/api/users_avatar.go index 72a6d3332..96e8694c3 100644 --- a/internal/api/users_avatar.go +++ b/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 diff --git a/internal/api/users_password.go b/internal/api/users_password.go index 776bfc880..4c2148dd9 100644 --- a/internal/api/users_password.go +++ b/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}) - isSuperAdmin := isPrivileged && s.User().IsSuperAdmin() + isAdmin := acl.Resources.AllowAll(acl.ResourceUsers, s.User().AclRole(), acl.Permissions{acl.AccessAll, acl.ActionManage}) + 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) diff --git a/internal/api/users_update.go b/internal/api/users_update.go index 596945414..cb79987f9 100644 --- a/internal/api/users_update.go +++ b/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, diff --git a/internal/entity/auth_user.go b/internal/entity/auth_user.go index 23de7708c..76c238177 100644 --- a/internal/entity/auth_user.go +++ b/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. - if updateRights { + // Change user privileges only if allowed. + if changePrivileges { m.SetRole(f.Role()) m.SuperAdmin = f.SuperAdmin diff --git a/internal/entity/auth_user_test.go b/internal/entity/auth_user_test.go index 50cdbf39b..a8395ea69 100644 --- a/internal/entity/auth_user_test.go +++ b/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()