From 70348e159f15f05349366d86f995afa65b291c68 Mon Sep 17 00:00:00 2001 From: Awiteb Date: Fri, 13 Dec 2024 05:42:01 +0000 Subject: [PATCH] Ensure `source_id` parameter is not skipped when set to 0 and correctly filter users in `/api/v1/admin/users` endpoint (#6240) Signed-off-by: Awiteb Fixes: #6239 ## Checklist ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [X] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [X] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [X] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6240 Reviewed-by: Otto Reviewed-by: Earl Warren Co-authored-by: Awiteb Co-committed-by: Awiteb --- models/user/search.go | 8 +++--- routers/api/v1/admin/user.go | 10 ++++++- tests/integration/admin_user_test.go | 43 ++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/models/user/search.go b/models/user/search.go index 558daf2e6a..cb90ca850e 100644 --- a/models/user/search.go +++ b/models/user/search.go @@ -24,8 +24,8 @@ type SearchUserOptions struct { Keyword string Type UserType UID int64 - LoginName string // this option should be used only for admin user - SourceID int64 // this option should be used only for admin user + LoginName string // this option should be used only for admin user + SourceID optional.Option[int64] // this option should be used only for admin user OrderBy db.SearchOrderBy Visible []structs.VisibleType Actor *User // The user doing the search @@ -98,8 +98,8 @@ func (opts *SearchUserOptions) toSearchQueryBase(ctx context.Context) *xorm.Sess cond = cond.And(builder.Eq{"id": opts.UID}) } - if opts.SourceID > 0 { - cond = cond.And(builder.Eq{"login_source": opts.SourceID}) + if opts.SourceID.Has() { + cond = cond.And(builder.Eq{"login_source": opts.SourceID.Value()}) } if opts.LoginName != "" { cond = cond.And(builder.Eq{"login_name": opts.LoginName}) diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index b17200381a..184024a246 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "strconv" "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" @@ -430,12 +431,19 @@ func SearchUsers(ctx *context.APIContext) { // "$ref": "#/responses/forbidden" listOptions := utils.GetListOptions(ctx) + intSource, err := strconv.ParseInt(ctx.FormString("source_id"), 10, 64) + var sourceID optional.Option[int64] + if ctx.FormString("source_id") == "" || err != nil { + sourceID = optional.None[int64]() + } else { + sourceID = optional.Some(intSource) + } users, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeIndividual, LoginName: ctx.FormTrim("login_name"), - SourceID: ctx.FormInt64("source_id"), + SourceID: sourceID, OrderBy: db.SearchOrderByAlphabetically, ListOptions: listOptions, }) diff --git a/tests/integration/admin_user_test.go b/tests/integration/admin_user_test.go index 8cdaac3c72..e61a46ab07 100644 --- a/tests/integration/admin_user_test.go +++ b/tests/integration/admin_user_test.go @@ -4,14 +4,17 @@ package integration import ( + "context" "fmt" "net/http" "strconv" "testing" + auth_model "code.gitea.io/gitea/models/auth" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -89,3 +92,43 @@ func TestAdminDeleteUser(t *testing.T) { assertUserDeleted(t, userID, true) unittest.CheckConsistencyFor(t, &user_model.User{}) } + +func TestSourceId(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + testUser23 := &user_model.User{ + Name: "ausersourceid23", + LoginName: "ausersourceid23", + Email: "ausersourceid23@example.com", + Passwd: "ausersourceid23password", + Type: user_model.UserTypeIndividual, + LoginType: auth_model.Plain, + LoginSource: 23, + } + defer createUser(context.Background(), t, testUser23)() + + session := loginUser(t, "user1") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadAdmin) + + // Our new user start with 'a' so it should be the first one + req := NewRequest(t, "GET", "/api/v1/admin/users?limit=1").AddTokenAuth(token) + resp := session.MakeRequest(t, req, http.StatusOK) + var users []api.User + DecodeJSON(t, resp, &users) + assert.Len(t, users, 1) + assert.Equal(t, "ausersourceid23", users[0].UserName) + + // Now our new user should not be in the list, because we filter by source_id 0 + req = NewRequest(t, "GET", "/api/v1/admin/users?limit=1&source_id=0").AddTokenAuth(token) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &users) + assert.Len(t, users, 1) + assert.Equal(t, "the_34-user.with.all.allowedChars", users[0].UserName) + + // Now our new user should be in the list, because we filter by source_id 23 + req = NewRequest(t, "GET", "/api/v1/admin/users?limit=1&source_id=23").AddTokenAuth(token) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &users) + assert.Len(t, users, 1) + assert.Equal(t, "ausersourceid23", users[0].UserName) +}