Skip to content

Commit 90815e5

Browse files
AbhineetJainAbhineetJain
authored
feat: improve Users filter API (#2645)
1 parent d1c6986 commit 90815e5

File tree

7 files changed

+193
-10
lines changed

7 files changed

+193
-10
lines changed

‎coderd/database/databasefake/databasefake.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"golang.org/x/exp/slices"
1313

1414
"github.com/coder/coder/coderd/database"
15+
"github.com/coder/coder/coderd/rbac"
1516
"github.com/coder/coder/coderd/util/slice"
1617
)
1718

@@ -276,9 +277,9 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
276277
if params.Search != "" {
277278
tmp := make([]database.User, 0, len(users))
278279
for i, user := range users {
279-
if strings.Contains(user.Email, params.Search) {
280+
if strings.Contains(strings.ToLower(user.Email), strings.ToLower(params.Search)) {
280281
tmp = append(tmp, users[i])
281-
} else if strings.Contains(user.Username, params.Search) {
282+
} else if strings.Contains(strings.ToLower(user.Username), strings.ToLower(params.Search)) {
282283
tmp = append(tmp, users[i])
283284
}
284285
}
@@ -295,7 +296,7 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
295296
users = usersFilteredByStatus
296297
}
297298

298-
if len(params.RbacRole) > 0 {
299+
if len(params.RbacRole) > 0 && !slice.Contains(params.RbacRole, rbac.RoleMember()) {
299300
usersFilteredByRole := make([]database.User, 0, len(users))
300301
for i, user := range users {
301302
if slice.Overlap(params.RbacRole, user.RBACRoles) {

‎coderd/database/queries.sql.go

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/users.sql

+2-2
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,8 @@ WHERE
9999
-- Filter by name, email or username
100100
AND CASE
101101
WHEN @search :: text != '' THEN (
102-
email LIKE concat('%', @search, '%')
103-
OR username LIKE concat('%', @search, '%')
102+
email ILIKE concat('%', @search, '%')
103+
OR username ILIKE concat('%', @search, '%')
104104
)
105105
ELSE true
106106
END

‎coderd/httpapi/queryparams.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func ParseCustom[T any](parser *QueryParamParser, vals url.Values, def T, queryP
107107
if err != nil {
108108
parser.Errors = append(parser.Errors, Error{
109109
Field: queryParam,
110-
Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", queryParam, err.Error()),
110+
Detail: fmt.Sprintf("Query param %q has invalid value: %s", queryParam, err.Error()),
111111
})
112112
}
113113
return v

‎coderd/users.go

+2
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) {
127127
Message: "Invalid user search query.",
128128
Validations: errs,
129129
})
130+
return
130131
}
131132

132133
paginationParams, ok := parsePagination(rw, r)
@@ -959,6 +960,7 @@ func userSearchQuery(query string) (database.GetUsersParams, []httpapi.Error) {
959960
// No filter
960961
return database.GetUsersParams{}, nil
961962
}
963+
query = strings.ToLower(query)
962964
// Because we do this in 2 passes, we want to maintain quotes on the first
963965
// pass.Further splitting occurs on the second pass and quotes will be
964966
// dropped.

‎coderd/users_internal_test.go

+124
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
package coderd
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
"testing"
7+
8+
"github.com/coder/coder/coderd/database"
9+
"github.com/coder/coder/coderd/rbac"
10+
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
func TestSearchUsers(t *testing.T) {
15+
t.Parallel()
16+
testCases := []struct {
17+
Name string
18+
Query string
19+
Expected database.GetUsersParams
20+
ExpectedErrorContains string
21+
}{
22+
{
23+
Name: "Empty",
24+
Query: "",
25+
Expected: database.GetUsersParams{},
26+
},
27+
{
28+
Name: "Username",
29+
Query: "user-name",
30+
Expected: database.GetUsersParams{
31+
Search: "user-name",
32+
Status: []database.UserStatus{},
33+
RbacRole: []string{},
34+
},
35+
},
36+
{
37+
Name: "Username+Param",
38+
Query: "usEr-name stAtus:actiVe",
39+
Expected: database.GetUsersParams{
40+
Search: "user-name",
41+
Status: []database.UserStatus{database.UserStatusActive},
42+
RbacRole: []string{},
43+
},
44+
},
45+
{
46+
Name: "OnlyParams",
47+
Query: "status:acTIve sEArch:User-Name role:Admin",
48+
Expected: database.GetUsersParams{
49+
Search: "user-name",
50+
Status: []database.UserStatus{database.UserStatusActive},
51+
RbacRole: []string{rbac.RoleAdmin()},
52+
},
53+
},
54+
{
55+
Name: "QuotedParam",
56+
Query: `status:SuSpenDeD sEArch:"User Name" role:meMber`,
57+
Expected: database.GetUsersParams{
58+
Search: "user name",
59+
Status: []database.UserStatus{database.UserStatusSuspended},
60+
RbacRole: []string{rbac.RoleMember()},
61+
},
62+
},
63+
{
64+
Name: "QuotedKey",
65+
Query: `"status":acTIve "sEArch":User-Name "role":Admin`,
66+
Expected: database.GetUsersParams{
67+
Search: "user-name",
68+
Status: []database.UserStatus{database.UserStatusActive},
69+
RbacRole: []string{rbac.RoleAdmin()},
70+
},
71+
},
72+
{
73+
// This will not return an error
74+
Name: "ExtraKeys",
75+
Query: `foo:bar`,
76+
Expected: database.GetUsersParams{
77+
Search: "",
78+
Status: []database.UserStatus{},
79+
RbacRole: []string{},
80+
},
81+
},
82+
{
83+
// Quotes keep elements together
84+
Name: "QuotedSpecial",
85+
Query: `search:"user:name"`,
86+
Expected: database.GetUsersParams{
87+
Search: "user:name",
88+
Status: []database.UserStatus{},
89+
RbacRole: []string{},
90+
},
91+
},
92+
93+
// Failures
94+
{
95+
Name: "ExtraColon",
96+
Query: `search:name:extra`,
97+
ExpectedErrorContains: "can only contain 1 ':'",
98+
},
99+
{
100+
Name: "InvalidStatus",
101+
Query: "status:inActive",
102+
ExpectedErrorContains: "status: Query param \"status\" has invalid value: \"inactive\" is not a valid user status\n",
103+
},
104+
}
105+
106+
for _, c := range testCases {
107+
c := c
108+
t.Run(c.Name, func(t *testing.T) {
109+
t.Parallel()
110+
values, errs := userSearchQuery(c.Query)
111+
if c.ExpectedErrorContains != "" {
112+
require.True(t, len(errs) > 0, "expect some errors")
113+
var s strings.Builder
114+
for _, err := range errs {
115+
_, _ = s.WriteString(fmt.Sprintf("%s: %s\n", err.Field, err.Detail))
116+
}
117+
require.Contains(t, s.String(), c.ExpectedErrorContains)
118+
} else {
119+
require.Len(t, errs, 0, "expected no error")
120+
require.Equal(t, c.Expected, values, "expected values")
121+
}
122+
})
123+
}
124+
}

‎coderd/users_test.go

+58-2
Original file line numberDiff line numberDiff line change
@@ -732,6 +732,13 @@ func TestUsersFilter(t *testing.T) {
732732
require.NoError(t, err, "suspend user")
733733
}
734734

735+
if i%5 == 0 {
736+
user, err = client.UpdateUserProfile(context.Background(), user.ID.String(), codersdk.UpdateUserProfileRequest{
737+
Username: strings.ToUpper(user.Username),
738+
})
739+
require.NoError(t, err, "update username to uppercase")
740+
}
741+
735742
users = append(users, user)
736743
}
737744

@@ -760,6 +767,15 @@ func TestUsersFilter(t *testing.T) {
760767
return u.Status == codersdk.UserStatusActive
761768
},
762769
},
770+
{
771+
Name: "ActiveUppercase",
772+
Filter: codersdk.UsersRequest{
773+
Status: "ACTIVE",
774+
},
775+
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
776+
return u.Status == codersdk.UserStatusActive
777+
},
778+
},
763779
{
764780
Name: "Suspended",
765781
Filter: codersdk.UsersRequest{
@@ -775,7 +791,7 @@ func TestUsersFilter(t *testing.T) {
775791
Search: "a",
776792
},
777793
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
778-
return (strings.Contains(u.Username, "a") || strings.Contains(u.Email, "a"))
794+
return (strings.ContainsAny(u.Username, "aA") || strings.ContainsAny(u.Email, "aA"))
779795
},
780796
},
781797
{
@@ -793,6 +809,31 @@ func TestUsersFilter(t *testing.T) {
793809
return false
794810
},
795811
},
812+
{
813+
Name: "AdminsUppercase",
814+
Filter: codersdk.UsersRequest{
815+
Role: "ADMIN",
816+
Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
817+
},
818+
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
819+
for _, r := range u.Roles {
820+
if r.Name == rbac.RoleAdmin() {
821+
return true
822+
}
823+
}
824+
return false
825+
},
826+
},
827+
{
828+
Name: "Members",
829+
Filter: codersdk.UsersRequest{
830+
Role: rbac.RoleMember(),
831+
Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
832+
},
833+
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
834+
return true
835+
},
836+
},
796837
{
797838
Name: "SearchQuery",
798839
Filter: codersdk.UsersRequest{
@@ -801,7 +842,22 @@ func TestUsersFilter(t *testing.T) {
801842
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
802843
for _, r := range u.Roles {
803844
if r.Name == rbac.RoleAdmin() {
804-
return (strings.Contains(u.Username, "i") || strings.Contains(u.Email, "i")) &&
845+
return (strings.ContainsAny(u.Username, "iI") || strings.ContainsAny(u.Email, "iI")) &&
846+
u.Status == codersdk.UserStatusActive
847+
}
848+
}
849+
return false
850+
},
851+
},
852+
{
853+
Name: "SearchQueryInsensitive",
854+
Filter: codersdk.UsersRequest{
855+
SearchQuery: "i Role:Admin STATUS:Active",
856+
},
857+
FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
858+
for _, r := range u.Roles {
859+
if r.Name == rbac.RoleAdmin() {
860+
return (strings.ContainsAny(u.Username, "iI") || strings.ContainsAny(u.Email, "iI")) &&
805861
u.Status == codersdk.UserStatusActive
806862
}
807863
}

0 commit comments

Comments
 (0)