From 382a7884be1f37da5c8f657c4b17316d8691dd59 Mon Sep 17 00:00:00 2001 From: kolaente Date: Tue, 2 Aug 2022 13:26:42 +0200 Subject: [PATCH] fix: make sure to use user discoverability settings when searching list users Resolves https://kolaente.dev/vikunja/frontend/issues/2196 --- pkg/models/user_list.go | 22 +++--------------- pkg/routes/api/v1/user_list.go | 2 +- pkg/user/user_test.go | 41 +++++++++++++++++++++++++++------- pkg/user/users_list.go | 35 ++++++++++++++++++----------- 4 files changed, 59 insertions(+), 41 deletions(-) diff --git a/pkg/models/user_list.go b/pkg/models/user_list.go index bbb579809..abd652afe 100644 --- a/pkg/models/user_list.go +++ b/pkg/models/user_list.go @@ -96,28 +96,12 @@ func ListUsersFromList(s *xorm.Session, l *List, search string) (users []*user.U uids = append(uids, id) } - var cond builder.Cond = builder.Like{"username", "%" + search + "%"} + var cond builder.Cond if len(uids) > 0 { - cond = builder.And( - builder.In("id", uids), - cond, - ) - } - - // Get all users - err = s. - Table("users"). - Select("*"). - Where(cond). - GroupBy("id"). - OrderBy("id"). - Find(&users) - - // Obfuscate all user emails - for _, u := range users { - u.Email = "" + cond = builder.In("id", uids) } + users, err = user.ListUsers(s, search, cond) return } diff --git a/pkg/routes/api/v1/user_list.go b/pkg/routes/api/v1/user_list.go index 28bae9ef1..19f321ef0 100644 --- a/pkg/routes/api/v1/user_list.go +++ b/pkg/routes/api/v1/user_list.go @@ -47,7 +47,7 @@ func UserList(c echo.Context) error { s := db.NewSession() defer s.Close() - users, err := user.ListUsers(s, search) + users, err := user.ListUsers(s, search, nil) if err != nil { _ = s.Rollback() return handler.HandleHTTPError(err, c) diff --git a/pkg/user/user_test.go b/pkg/user/user_test.go index f36f276ef..92c8b4e39 100644 --- a/pkg/user/user_test.go +++ b/pkg/user/user_test.go @@ -18,6 +18,7 @@ package user import ( "testing" + "xorm.io/builder" "code.vikunja.io/api/pkg/db" "github.com/stretchr/testify/assert" @@ -362,7 +363,7 @@ func TestListUsers(t *testing.T) { s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "user1") + all, err := ListUsers(s, "user1", nil) assert.NoError(t, err) assert.True(t, len(all) > 0) assert.Equal(t, all[0].Username, "user1") @@ -381,7 +382,7 @@ func TestListUsers(t *testing.T) { s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "") + all, err := ListUsers(s, "", nil) assert.NoError(t, err) assert.Len(t, all, 0) }) @@ -390,11 +391,12 @@ func TestListUsers(t *testing.T) { s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "user1@example.com") + all, err := ListUsers(s, "user1@example.com", nil) assert.NoError(t, err) assert.Len(t, all, 0) db.AssertExists(t, "users", map[string]interface{}{ - "email": "user1@example.com", + "email": "user1@example.com", + "discoverable_by_email": false, }, false) }) t.Run("not discoverable by name", func(t *testing.T) { @@ -402,11 +404,12 @@ func TestListUsers(t *testing.T) { s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "one else") + all, err := ListUsers(s, "one else", nil) assert.NoError(t, err) assert.Len(t, all, 0) db.AssertExists(t, "users", map[string]interface{}{ - "name": "Some one else", + "name": "Some one else", + "discoverable_by_name": false, }, false) }) t.Run("discoverable by email", func(t *testing.T) { @@ -414,20 +417,42 @@ func TestListUsers(t *testing.T) { s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "user7@example.com") + all, err := ListUsers(s, "user7@example.com", nil) assert.NoError(t, err) assert.Len(t, all, 1) assert.Equal(t, int64(7), all[0].ID) + db.AssertExists(t, "users", map[string]interface{}{ + "email": "user7@example.com", + "discoverable_by_email": true, + }, false) }) t.Run("discoverable by partial name", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() - all, err := ListUsers(s, "with space") + all, err := ListUsers(s, "with space", nil) assert.NoError(t, err) assert.Len(t, all, 1) assert.Equal(t, int64(12), all[0].ID) + db.AssertExists(t, "users", map[string]interface{}{ + "name": "Name with spaces", + "discoverable_by_name": true, + }, false) + }) + t.Run("discoverable by email with extra condition", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + all, err := ListUsers(s, "user7@example.com", builder.In("id", 7)) + assert.NoError(t, err) + assert.Len(t, all, 1) + assert.Equal(t, int64(7), all[0].ID) + db.AssertExists(t, "users", map[string]interface{}{ + "email": "user7@example.com", + "discoverable_by_email": true, + }, false) }) } diff --git a/pkg/user/users_list.go b/pkg/user/users_list.go index 2d72e6a37..7bea66c07 100644 --- a/pkg/user/users_list.go +++ b/pkg/user/users_list.go @@ -23,8 +23,8 @@ import ( "xorm.io/xorm" ) -// ListUsers returns a list with all users, filtered by an optional searchstring -func ListUsers(s *xorm.Session, search string) (users []*User, err error) { +// ListUsers returns a list with all users, filtered by an optional search string +func ListUsers(s *xorm.Session, search string, additionalCond builder.Cond) (users []*User, err error) { // Prevent searching for placeholders search = strings.ReplaceAll(search, "%", "") @@ -33,18 +33,27 @@ func ListUsers(s *xorm.Session, search string) (users []*User, err error) { return } + cond := builder.Or( + builder.Like{"username", "%" + search + "%"}, + builder.And( + builder.Eq{"email": search}, + builder.Eq{"discoverable_by_email": true}, + ), + builder.And( + builder.Like{"name", "%" + search + "%"}, + builder.Eq{"discoverable_by_name": true}, + ), + ) + + if additionalCond != nil { + cond = builder.And( + cond, + additionalCond, + ) + } + err = s. - Where(builder.Or( - builder.Like{"username", "%" + search + "%"}, - builder.And( - builder.Eq{"email": search}, - builder.Eq{"discoverable_by_email": true}, - ), - builder.And( - builder.Like{"name", "%" + search + "%"}, - builder.Eq{"discoverable_by_name": true}, - ), - )). + Where(cond). Find(&users) return }