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.
This commit is contained in:
kolaente
kolaente
parent
999e28435e
commit
22d82e292b
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue