From 22d82e292b9e68b4ce701ac205da88647af6aec7 Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 24 Apr 2026 11:17:32 +0200 Subject: [PATCH] feat(user): always include own bots in user search User search previously filtered bots only when they happened to match the search string. That produced two bad behaviours: 1. Bots owned by other users could surface on an exact-username match, leaking them into assignee pickers and similar UI. 2. A user could not reliably find their own bots by typing a partial name, so bots became awkward to assign to tasks. Change ListUsers to treat bot ownership explicitly: the existing match branch excludes rows owned by someone else, and a second branch always returns bots owned by the calling user. The own-bots branch also respects any AdditionalCond passed in so project-scoped listings don't start leaking bots from outside the project. --- pkg/db/fixtures/users.yml | 35 +++++++++++ pkg/models/user_list_test.go | 116 ++++++++++++++++++++++++++++++++++- pkg/user/users_project.go | 38 +++++++++++- 3 files changed, 186 insertions(+), 3 deletions(-) diff --git a/pkg/db/fixtures/users.yml b/pkg/db/fixtures/users.yml index 9e7811924..c8be6b39b 100644 --- a/pkg/db/fixtures/users.yml +++ b/pkg/db/fixtures/users.yml @@ -164,3 +164,38 @@ avatar_provider: 'openid' updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 +# User 21 and user 22 below are a pair of bot owners used by the user-search +# tests: user 21 has a bot (23), user 22 has a bot (24). Putting the bots on +# dedicated owners keeps the pre-existing search tests (which mostly use user1) +# unaffected. +- id: 21 + username: 'user_bot_owner_a' + password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' + email: 'user_bot_owner_a@example.com' + issuer: local + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 +- id: 22 + username: 'user_bot_owner_b' + password: '$2a$04$X4aRMEt0ytgPwMIgv36cI..7X9.nhY/.tYwxpqSi0ykRHx2CwQ0S6' + email: 'user_bot_owner_b@example.com' + issuer: local + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 +# Bot owned by user 21 — used to assert own bots are always returned +# regardless of the search string. +- id: 23 + username: 'bot-owner-a-assistant' + name: 'Owner A Assistant' + issuer: local + bot_owner_id: 21 + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 +# Bot owned by user 22 — used to assert other users' bots are never leaked. +- id: 24 + username: 'bot-owner-b-assistant' + name: 'Owner B Assistant' + issuer: local + bot_owner_id: 22 + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 diff --git a/pkg/models/user_list_test.go b/pkg/models/user_list_test.go index d55e86291..7113bd113 100644 --- a/pkg/models/user_list_test.go +++ b/pkg/models/user_list_test.go @@ -58,7 +58,7 @@ func TestListUsers(t *testing.T) { all, err := user.ListAllUsers(s) require.NoError(t, err) - assert.Len(t, all, 20) + assert.Len(t, all, 24) }) t.Run("no search term", func(t *testing.T) { db.LoadAndAssertFixtures(t) @@ -171,7 +171,10 @@ func TestListUsers(t *testing.T) { MatchFuzzily: true, }) require.NoError(t, err) - assert.Len(t, all, 20) + // 22 non-bot users have "user" in their username; the two bot + // fixtures are filtered out because they don't belong to user1 + // and their usernames/names don't contain "user". + assert.Len(t, all, 22) }) // External team discoverability bypass tests @@ -239,4 +242,113 @@ func TestListUsers(t *testing.T) { // Email should be masked because the search was by name, not email assert.Empty(t, all[0].Email) }) + + // Bot visibility in user search: + // - A user's own bots are filtered by the search string (matched against + // username and name), but bypass the discoverable_by_name flag that + // hides regular users unless they opt in. + // - When no search string is provided and ReturnAllIfNoSearchProvided is + // set, own bots are returned alongside regular users. + // - Other users' bots must never leak into the results, even on an exact + // username match. + t.Run("own bot NOT returned when query does not match it", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerA := &user.User{ID: 21} + // Query string deliberately does not match the bot's username or name. + all, err := user.ListUsers(s, "user7", botOwnerA, nil) + require.NoError(t, err) + + for _, u := range all { + assert.NotEqual(t, int64(23), u.ID, "owner A's bot must not appear when the query does not match it") + } + }) + t.Run("other user's bot not returned by exact username match", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerA := &user.User{ID: 21} + // Searching for owner B's bot by its exact username must not return it. + all, err := user.ListUsers(s, "bot-owner-b-assistant", botOwnerA, nil) + require.NoError(t, err) + + for _, u := range all { + assert.NotEqual(t, int64(24), u.ID, "owner B's bot must not leak into owner A's results") + } + }) + t.Run("neither own nor other bot returned when query matches only other owner's bot", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerB := &user.User{ID: 22} + all, err := user.ListUsers(s, "bot-owner-a-assistant", botOwnerB, nil) + require.NoError(t, err) + + // Owner A's bot must not leak. Owner B's bot must not appear either since + // the query does not match its username or name. + for _, u := range all { + assert.NotEqual(t, int64(23), u.ID, "owner A's bot must not leak to owner B") + assert.NotEqual(t, int64(24), u.ID, "owner B's bot must not appear when the query does not match it") + } + }) + t.Run("own bot returned by username match", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerA := &user.User{ID: 21} + all, err := user.ListUsers(s, "bot-owner-a-assistant", botOwnerA, nil) + require.NoError(t, err) + + var foundBot bool + for _, u := range all { + if u.ID == 23 { + foundBot = true + assert.Equal(t, int64(21), u.BotOwnerID) + } + } + assert.True(t, foundBot, "owner A's bot (id=23) should appear when searching by exact username") + }) + t.Run("own bot returned by name match without discoverable_by_name", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerA := &user.User{ID: 21} + // The bot fixture does not set discoverable_by_name=true, but own bots + // should still match by name for their owner. + all, err := user.ListUsers(s, "Owner A Assistant", botOwnerA, nil) + require.NoError(t, err) + + var foundBot bool + for _, u := range all { + if u.ID == 23 { + foundBot = true + assert.Equal(t, int64(21), u.BotOwnerID) + } + } + assert.True(t, foundBot, "owner A's bot (id=23) should appear when searching by name even without discoverable_by_name") + }) + t.Run("own bot returned when no search but ReturnAllIfNoSearchProvided", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + botOwnerA := &user.User{ID: 21} + all, err := user.ListUsers(s, "", botOwnerA, &user.ProjectUserOpts{ReturnAllIfNoSearchProvided: true}) + require.NoError(t, err) + + var foundBot bool + for _, u := range all { + if u.ID == 23 { + foundBot = true + assert.Equal(t, int64(21), u.BotOwnerID) + } + } + assert.True(t, foundBot, "owner A's bot (id=23) should appear in results when no search is provided and ReturnAllIfNoSearchProvided is true") + }) } diff --git a/pkg/user/users_project.go b/pkg/user/users_project.go index 4ce88ed49..ab94a4bdf 100644 --- a/pkg/user/users_project.go +++ b/pkg/user/users_project.go @@ -147,8 +147,44 @@ func ListUsers(s *xorm.Session, search string, currentUser *User, opts *ProjectU } } + notSomeoneElsesBot := builder.Or( + builder.IsNull{"bot_owner_id"}, + builder.Eq{"bot_owner_id": 0}, + builder.Eq{"bot_owner_id": currentUser.ID}, + ) + + // Own bots: filtered by the search string when one is provided (matched + // against username and name), but bypass the discoverable_by_name flag + // regular users have. Without a search, ListUsers only returns rows when + // ReturnAllIfNoSearchProvided is set, so we surface own bots there too. + var ownBotCond builder.Cond = builder.Eq{"bot_owner_id": currentUser.ID} + if search != "" { + botSearchConds := []builder.Cond{} + for _, queryPart := range queryParts { + if opts.MatchFuzzily { + botSearchConds = append(botSearchConds, + db.ILIKE("name", queryPart), + db.ILIKE("username", queryPart), + ) + continue + } + + botSearchConds = append(botSearchConds, + db.ILIKE("username", queryPart), + db.ILIKE("name", queryPart), + ) + } + ownBotCond = builder.And( + builder.Eq{"bot_owner_id": currentUser.ID}, + builder.Or(botSearchConds...), + ) + } + err = s. - Where(cond). + Where(builder.Or( + builder.And(cond, notSomeoneElsesBot), + ownBotCond, + )). OrderBy("id"). Find(&users)