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

@@ -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(
+		cond = builder.In("id", uids)
-			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 = ""
 	}
 
+	users, err = user.ListUsers(s, search, cond)
 	return
 }

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)

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)
 	})
 }
 

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
+// ListUsers returns a list with all users, filtered by an optional search string
-func ListUsers(s *xorm.Session, search string) (users []*User, err error) {
+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(
+		Where(cond).
-			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},
-			),
-		)).
 		Find(&users)
 	return
 }