kodev-org
/
vikunja
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor(admin): share user-mutation logic between v1 and v2 

The admin set-admin-flag, set-status and delete-user operations were
implemented twice — once in the v1 echo handlers, once in the v2 Huma handlers.
Extract the load/guard/mutate logic into models.SetUserAdminFlag,
models.SetUserStatusAsAdmin and models.DeleteUserAsAdmin so both APIs call the
same code; each handler keeps only its own request binding, validation and
response shape. v1 stays byte-identical on the wire.
This commit is contained in:
kolaente 2026-06-11 21:10:16 +02:00 committed by kolaente
parent 5b3ee89edd
commit 53d1fa0735
4 changed files with 127 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

106
pkg/models/admin_user_actions.go Normal file
View File

@ -0,0 +1,106 @@
// Vikunja is a to-do list application to facilitate your life.
// Copyright 2018-present Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package models
import (
"code.vikunja.io/api/pkg/user"
"xorm.io/xorm"
)
// loadAdminTargetUser fetches a user by ID for the admin actions, returning
// ErrUserDoesNotExist for an invalid ID or a missing row.
func loadAdminTargetUser(s *xorm.Session, id int64) (*user.User, error) {
if id < 1 {
return nil, user.ErrUserDoesNotExist{UserID: id}
}
target := &user.User{ID: id}
has, err := s.Get(target)
if err != nil {
return nil, err
}
if !has {
return nil, user.ErrUserDoesNotExist{UserID: id}
}
return target, nil
}
// SetUserAdminFlag sets a user's instance-admin flag. Demoting the last
// reachable admin is refused via GuardLastAdmin. It does not commit; the caller
// owns the transaction.
func SetUserAdminFlag(s *xorm.Session, id int64, isAdmin bool) (*user.User, error) {
target, err := loadAdminTargetUser(s, id)
if err != nil {
return nil, err
}
if !isAdmin {
if err := user.GuardLastAdmin(s, target); err != nil {
return nil, err
}
}
target.IsAdmin = isAdmin
if _, err := s.ID(target.ID).Cols("is_admin").Update(target); err != nil {
return nil, err
}
return target, nil
}
// SetUserStatusAsAdmin sets a user's account status. Moving the last reachable
// admin out of Active is refused via GuardLastAdmin (any non-Active status
// blocks login, so it is equivalent to demotion). It does not commit; the caller
// owns the transaction.
func SetUserStatusAsAdmin(s *xorm.Session, id int64, status user.Status) (*user.User, error) {
target, err := loadAdminTargetUser(s, id)
if err != nil {
return nil, err
}
if target.IsAdmin && status != user.StatusActive {
if err := user.GuardLastAdmin(s, target); err != nil {
return nil, err
}
}
if err := user.SetUserStatus(s, target, status); err != nil {
return nil, err
}
// Reflect the change on the returned struct; GetUserByID refuses disabled accounts.
target.Status = status
return target, nil
}
// DeleteUserAsAdmin removes a user. mode "now" deletes immediately; any other
// value triggers the email-confirmation self-deletion flow. Deleting the last
// reachable admin is refused via GuardLastAdmin. It does not commit; the caller
// owns the transaction.
func DeleteUserAsAdmin(s *xorm.Session, id int64, mode string) error {
target, err := loadAdminTargetUser(s, id)
if err != nil {
return err
}
if err := user.GuardLastAdmin(s, target); err != nil {
return err
}
if mode == "now" {
return DeleteUser(s, target)
}
return user.RequestDeletion(s, target)
}

18
pkg/routes/api/v1/admin/users_admin.go
View File

@ -65,24 +65,8 @@ func PatchAdmin(c *echo.Context) error {
s := db.NewSession()
defer s.Close()
target := &user.User{ID: id}
has, err := s.Get(target)
target, err := models.SetUserAdminFlag(s, id, *body.IsAdmin)
if err != nil {
return err
}
if !has {
return user.ErrUserDoesNotExist{UserID: id}
}
if !*body.IsAdmin {
if err := user.GuardLastAdmin(s, target); err != nil {
_ = s.Rollback()
return err
}
}
target.IsAdmin = *body.IsAdmin
if _, err := s.ID(target.ID).Cols("is_admin").Update(target); err != nil {
_ = s.Rollback()
return err
}

44
pkg/routes/api/v1/admin/users_mgmt.go
View File

@ -67,24 +67,8 @@ func PatchStatus(c *echo.Context) error {
s := db.NewSession()
defer s.Close()
target := &user.User{ID: id}
has, err := s.Get(target)
target, err := models.SetUserStatusAsAdmin(s, id, newStatus)
if err != nil {
return err
}
if !has {
return user.ErrUserDoesNotExist{UserID: id}
}
// Any non-Active status blocks login, so moving an admin out of Active is equivalent to demotion.
if target.IsAdmin && newStatus != user.StatusActive {
if err := user.GuardLastAdmin(s, target); err != nil {
_ = s.Rollback()
return err
}
}
if err := user.SetUserStatus(s, target, newStatus); err != nil {
_ = s.Rollback()
return err
}
@ -92,8 +76,6 @@ func PatchStatus(c *echo.Context) error {
return err
}
// Refresh locally since GetUserByID refuses disabled accounts.
target.Status = newStatus
providers, err := openid.GetAllProviders()
if err != nil {
return err
@ -130,32 +112,10 @@ func DeleteUser(c *echo.Context) error {
s := db.NewSession()
defer s.Close()
target := &user.User{ID: id}
has, err := s.Get(target)
if err != nil {
return err
}
if !has {
return user.ErrUserDoesNotExist{UserID: id}
}
if err := user.GuardLastAdmin(s, target); err != nil {
if err := models.DeleteUserAsAdmin(s, id, mode); err != nil {
_ = s.Rollback()
return err
}
if mode == "now" {
if err := models.DeleteUser(s, target); err != nil {
_ = s.Rollback()
return err
}
} else {
if err := user.RequestDeletion(s, target); err != nil {
_ = s.Rollback()
return err
}
}
if err := s.Commit(); err != nil {
return err
}

91
pkg/routes/api/v2/admin_users.go
View File

@ -137,22 +137,9 @@ func adminUsersPatchAdmin(_ context.Context, in *struct {
if in.Body.IsAdmin == nil {
return nil, translateDomainError(models.ErrInvalidData{Message: "is_admin is required"})
}
target, err := adminMutateUser(in.ID, func(s *xorm.Session, target *user.User) error {
if !*in.Body.IsAdmin {
if err := user.GuardLastAdmin(s, target); err != nil {
return err
}
}
target.IsAdmin = *in.Body.IsAdmin
_, err := s.ID(target.ID).Cols("is_admin").Update(target)
return err
return adminCommitUser(func(s *xorm.Session) (*user.User, error) { //nolint:contextcheck // see adminCommitUser.
return models.SetUserAdminFlag(s, in.ID, *in.Body.IsAdmin)
})
if err != nil {
return nil, err
}
return adminUserResponse(target) //nolint:contextcheck // see adminUserResponse.
}
func adminUsersPatchStatus(_ context.Context, in *struct {
@ -166,23 +153,9 @@ func adminUsersPatchStatus(_ context.Context, in *struct {
if newStatus < user.StatusActive || newStatus > user.StatusAccountLocked {
return nil, translateDomainError(models.ErrInvalidData{Message: "invalid status"})
}
target, err := adminMutateUser(in.ID, func(s *xorm.Session, target *user.User) error {
// Any non-Active status blocks login, so moving an admin out of Active is equivalent to demotion.
if target.IsAdmin && newStatus != user.StatusActive {
if err := user.GuardLastAdmin(s, target); err != nil {
return err
}
}
return user.SetUserStatus(s, target, newStatus)
return adminCommitUser(func(s *xorm.Session) (*user.User, error) { //nolint:contextcheck // see adminCommitUser.
return models.SetUserStatusAsAdmin(s, in.ID, newStatus)
})
if err != nil {
return nil, err
}
// Refresh locally since GetUserByID refuses disabled accounts.
target.Status = newStatus
return adminUserResponse(target) //nolint:contextcheck // see adminUserResponse.
}
func adminUsersDelete(_ context.Context, in *struct {
@ -197,62 +170,34 @@ func adminUsersDelete(_ context.Context, in *struct {
return nil, translateDomainError(models.ErrInvalidData{Message: "invalid mode, expected 'now' or 'scheduled'"})
}
_, err := adminMutateUser(in.ID, func(s *xorm.Session, target *user.User) error {
if err := user.GuardLastAdmin(s, target); err != nil {
return err
}
if mode == "now" {
return models.DeleteUser(s, target)
}
return user.RequestDeletion(s, target)
})
if err != nil {
return nil, err
}
return &emptyBody{}, nil
}
// adminMutateUser opens a session, loads the user by ID, runs mutate against it,
// then commits — owning the transaction so each handler only supplies its
// distinct guard-and-write step. mutate must not commit or rollback. Errors
// (load, mutate, commit) are translated to RFC 9457 responses.
func adminMutateUser(id int64, mutate func(s *xorm.Session, target *user.User) error) (*user.User, error) {
s := db.NewSession()
defer s.Close()
target, err := adminLoadUser(s, id)
if err != nil {
return nil, translateDomainError(err)
}
if err := mutate(s, target); err != nil {
if err := models.DeleteUserAsAdmin(s, in.ID, mode); err != nil {
_ = s.Rollback()
return nil, translateDomainError(err)
}
if err := s.Commit(); err != nil {
return nil, translateDomainError(err)
}
return target, nil
return &emptyBody{}, nil
}
// adminLoadUser fetches a user by ID, returning ErrUserDoesNotExist for an
// invalid ID or a missing row (matching v1's 404).
func adminLoadUser(s *xorm.Session, id int64) (*user.User, error) {
if id < 1 {
return nil, user.ErrUserDoesNotExist{UserID: id}
}
target := &user.User{ID: id}
has, err := s.Get(target)
// adminCommitUser runs a user-returning admin action in its own transaction and
// renders the admin user view. The action does the load/guard/mutate against the
// session (shared with v1 via the models layer); this owns the commit and response.
func adminCommitUser(action func(s *xorm.Session) (*user.User, error)) (*adminUserBody, error) {
s := db.NewSession()
defer s.Close()
target, err := action(s)
if err != nil {
return nil, err
_ = s.Rollback()
return nil, translateDomainError(err)
}
if !has {
return nil, user.ErrUserDoesNotExist{UserID: id}
if err := s.Commit(); err != nil {
return nil, translateDomainError(err)
}
return target, nil
}
// adminUserResponse builds the admin user view from an already-mutated user.
func adminUserResponse(target *user.User) (*adminUserBody, error) {
providers, err := openid.GetAllProviders() //nolint:contextcheck // GetAllProviders reads a cached map; it takes no context, like the v1 admin handlers.
if err != nil {
return nil, translateDomainError(err)