From 5807f2e7b432c4e2d152278aa411471d30e2ca48 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 11 Jun 2026 21:08:41 +0200 Subject: [PATCH] refactor(user): share user-search logic between v1 and v2 Extract the duplicated user-search business logic into two helpers both API versions call, and refactor v1's handlers onto them: - user.SearchUsers wraps ListUsers + email obfuscation (global search) - models.SearchUsersForProject wraps the project read check + ListUsersFromProject Each handler keeps its own forbidden mapping (v1 echo.ErrForbidden vs v2 huma) so v1 stays byte-identical on the wire. --- pkg/models/user_project.go | 20 ++++++++++++++++++++ pkg/routes/api/v1/user_list.go | 21 +++++---------------- pkg/routes/api/v2/user_search.go | 26 ++++++++------------------ pkg/user/users_project.go | 14 ++++++++++++++ 4 files changed, 47 insertions(+), 34 deletions(-) diff --git a/pkg/models/user_project.go b/pkg/models/user_project.go index 34ea85abe..7fe98c772 100644 --- a/pkg/models/user_project.go +++ b/pkg/models/user_project.go @@ -18,10 +18,30 @@ package models import ( "code.vikunja.io/api/pkg/user" + "code.vikunja.io/api/pkg/web" "xorm.io/builder" "xorm.io/xorm" ) +// SearchUsersForProject performs the per-project user search shared by both API +// versions: it checks the caller can read the project, then lists the users +// with access to it. canRead is false (with no error) when the caller lacks +// read access, so each handler can map that to its own forbidden response. +func SearchUsersForProject(s *xorm.Session, project *Project, a web.Auth, currentUser *user.User, search string) (users []*user.User, canRead bool, err error) { + canRead, _, err = project.CanRead(s, a) + if err != nil { + return nil, false, err + } + if !canRead { + return nil, false, nil + } + users, err = ListUsersFromProject(s, project, currentUser, search) + if err != nil { + return nil, true, err + } + return users, true, nil +} + // ProjectUIDs hold all kinds of user IDs from accounts who have access to a project type ProjectUIDs struct { ProjectOwnerID int64 `xorm:"projectOwner"` diff --git a/pkg/routes/api/v1/user_list.go b/pkg/routes/api/v1/user_list.go index db4a777a0..6bc6e392f 100644 --- a/pkg/routes/api/v1/user_list.go +++ b/pkg/routes/api/v1/user_list.go @@ -52,17 +52,12 @@ func UserList(c *echo.Context) error { return err } - users, err := user.ListUsers(s, search, currentUser, nil) + users, err := user.SearchUsers(s, search, currentUser) if err != nil { _ = s.Rollback() return err } - // Obfuscate the mailadresses - for in := range users { - users[in].Email = "" - } - return c.JSON(http.StatusOK, users) } @@ -98,15 +93,6 @@ func ListUsersForProject(c *echo.Context) error { s := db.NewSession() defer s.Close() - canRead, _, err := project.CanRead(s, auth) - if err != nil { - _ = s.Rollback() - return err - } - if !canRead { - return echo.ErrForbidden - } - currentUser, err := user.GetCurrentUser(c) if err != nil { _ = s.Rollback() @@ -114,11 +100,14 @@ func ListUsersForProject(c *echo.Context) error { } search := c.QueryParam("s") - users, err := models.ListUsersFromProject(s, &project, currentUser, search) + users, canRead, err := models.SearchUsersForProject(s, &project, auth, currentUser, search) if err != nil { _ = s.Rollback() return err } + if !canRead { + return echo.ErrForbidden + } if err := s.Commit(); err != nil { _ = s.Rollback() diff --git a/pkg/routes/api/v2/user_search.go b/pkg/routes/api/v2/user_search.go index 2f7ea66b5..5848ba5c0 100644 --- a/pkg/routes/api/v2/user_search.go +++ b/pkg/routes/api/v2/user_search.go @@ -72,7 +72,7 @@ func usersSearch(ctx context.Context, in *struct { return nil, translateDomainError(err) } - users, err := user.ListUsers(s, in.Q, currentUser, nil) + users, err := user.SearchUsers(s, in.Q, currentUser) if err != nil { _ = s.Rollback() return nil, translateDomainError(err) @@ -81,10 +81,6 @@ func usersSearch(ctx context.Context, in *struct { return nil, translateDomainError(err) } - for i := range users { - users[i].Email = "" - } - return &userListBody{Body: NewPaginated(users, int64(len(users)), 1, len(users))}, nil } @@ -100,8 +96,14 @@ func projectUsersSearch(ctx context.Context, in *struct { s := db.NewSession() defer s.Close() + currentUser, err := models.GetUserOrLinkShareUser(s, a) + if err != nil { + _ = s.Rollback() + return nil, translateDomainError(err) + } + project := &models.Project{ID: in.ProjectID} - canRead, _, err := project.CanRead(s, a) + users, canRead, err := models.SearchUsersForProject(s, project, a, currentUser, in.Q) if err != nil { _ = s.Rollback() return nil, translateDomainError(err) @@ -110,18 +112,6 @@ func projectUsersSearch(ctx context.Context, in *struct { _ = s.Rollback() return nil, huma.Error403Forbidden("forbidden") } - - currentUser, err := models.GetUserOrLinkShareUser(s, a) - if err != nil { - _ = s.Rollback() - return nil, translateDomainError(err) - } - - users, err := models.ListUsersFromProject(s, project, currentUser, in.Q) - if err != nil { - _ = s.Rollback() - return nil, translateDomainError(err) - } if err := s.Commit(); err != nil { return nil, translateDomainError(err) } diff --git a/pkg/user/users_project.go b/pkg/user/users_project.go index ab94a4bdf..c08cc23c2 100644 --- a/pkg/user/users_project.go +++ b/pkg/user/users_project.go @@ -34,6 +34,20 @@ type ProjectUserOpts struct { MatchFuzzily bool } +// SearchUsers performs the global user search shared by both API versions: +// it lists users matching the search string and obfuscates their email +// addresses before returning. +func SearchUsers(s *xorm.Session, search string, currentUser *User) (users []*User, err error) { + users, err = ListUsers(s, search, currentUser, nil) + if err != nil { + return nil, err + } + for i := range users { + users[i].Email = "" + } + return users, nil +} + // ListUsers returns a list with all users, filtered by an optional search string func ListUsers(s *xorm.Session, search string, currentUser *User, opts *ProjectUserOpts) (users []*User, err error) { if opts == nil {