From 503df2a670ff1e002817382781badb4eefacf576 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 16 Jun 2022 15:54:11 -0500 Subject: [PATCH 1/7] User search query string --- coderd/database/modelmethods.go | 7 +++ coderd/httpapi/queryparams.go | 13 +++++ coderd/users.go | 94 ++++++++++++++++++++++----------- codersdk/users.go | 17 ++++-- site/src/api/api.ts | 2 +- site/src/api/typesGenerated.ts | 41 +++++++------- 6 files changed, 117 insertions(+), 57 deletions(-) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 4d9534f00ef15..1c813c4f4a2ff 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -30,3 +30,10 @@ func (d ProvisionerDaemon) RBACObject() rbac.Object { func (f File) RBACObject() rbac.Object { return rbac.ResourceFile.WithID(f.Hash).WithOwner(f.CreatedBy.String()) } + +// RBACObject returns the RBAC object for the site wide user resource. +// If you are trying to get the RBAC object for the UserData, use +// rbac.ResourceUserData +func (u User) RBACObject() rbac.Object { + return rbac.ResourceFile.WithID(u.ID.String()) +} diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index ea30480e16b4e..1d266fc6fdf64 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -96,6 +96,19 @@ func (p *QueryParamParser) String(vals url.Values, def string, queryParam string return v } +// ParseCustom has to be a function, not a method on QueryParamParser because generics +// cannot be used on struct methods. +func ParseCustom[T any](parser *QueryParamParser, vals url.Values, def T, queryParam string, parseFunc func(v string) (T, error)) T { + v, err := parseQueryParam(vals, parseFunc, def, queryParam) + if err != nil { + parser.Errors = append(parser.Errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", queryParam, err.Error()), + }) + } + return v +} + func parseQueryParam[T any](vals url.Values, parse func(v string) (T, error), def T, queryParam string) (T, error) { if !vals.Has(queryParam) || vals.Get(queryParam) == "" { return def, nil diff --git a/coderd/users.go b/coderd/users.go index 781cc213d1797..837bca7166f4c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "time" @@ -109,35 +110,13 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { } func (api *API) users(rw http.ResponseWriter, r *http.Request) { - var ( - searchName = r.URL.Query().Get("search") - statusFilters = r.URL.Query().Get("status") - ) - - statuses := make([]database.UserStatus, 0) - - if statusFilters != "" { - // Split on commas if present to account for it being a list - for _, filter := range strings.Split(statusFilters, ",") { - switch database.UserStatus(filter) { - case database.UserStatusSuspended, database.UserStatusActive: - statuses = append(statuses, database.UserStatus(filter)) - default: - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("%q is not a valid user status.", filter), - Validations: []httpapi.Error{ - {Field: "status", Detail: "invalid status"}, - }, - }) - return - } - } - } - - // Reading all users across the site. - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) { - httpapi.Forbidden(rw) - return + query := r.URL.Query().Get("q") + params, errs := userSearchQuery(query) + if len(errs) > 0 { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: "Invalid user search query.", + Validations: errs, + }) } paginationParams, ok := parsePagination(rw, r) @@ -149,8 +128,8 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { AfterID: paginationParams.AfterID, OffsetOpt: int32(paginationParams.Offset), LimitOpt: int32(paginationParams.Limit), - Search: searchName, - Status: statuses, + Search: params.Search, + Status: params.Status, }) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(rw, http.StatusOK, []codersdk.User{}) @@ -164,6 +143,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { return } + users = AuthorizeFilter(api, r, rbac.ActionRead, users) userIDs := make([]uuid.UUID, 0, len(users)) for _, user := range users { userIDs = append(userIDs, user.ID) @@ -941,3 +921,55 @@ func findUser(id uuid.UUID, users []database.User) *database.User { } return nil } + +func userSearchQuery(query string) (database.GetUsersParams, []httpapi.Error) { + searchParams := make(url.Values) + if query == "" { + // No filter + return database.GetUsersParams{}, nil + } + // Because we do this in 2 passes, we want to maintain quotes on the first + // pass.Further splitting occurs on the second pass and quotes will be + // dropped. + elements := splitQueryParameterByDelimiter(query, ' ', true) + for _, element := range elements { + parts := splitQueryParameterByDelimiter(element, ':', false) + switch len(parts) { + case 1: + // No key:value pair. + searchParams.Set("search", parts[0]) + case 2: + searchParams.Set(parts[0], parts[1]) + default: + return database.GetUsersParams{}, []httpapi.Error{ + {Field: "q", Detail: fmt.Sprintf("Query element %q can only contain 1 ':'", element)}, + } + } + } + + parser := httpapi.NewQueryParamParser() + filter := database.GetUsersParams{ + Search: parser.String(searchParams, "", "search"), + Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{database.UserStatusActive}, "status", parseUserStatus), + } + + return filter, parser.Errors +} + +// parseUserStatus ensures proper enums are used for user statuses +func parseUserStatus(v string) ([]database.UserStatus, error) { + var statuses []database.UserStatus + if v == "" { + return statuses, nil + } + parts := strings.Split(v, ",") + for _, part := range parts { + switch database.UserStatus(part) { + case database.UserStatusActive, database.UserStatusSuspended: + statuses = append(statuses, database.UserStatus(part)) + default: + return []database.UserStatus{}, xerrors.Errorf("%q is not a valid user status", part) + } + } + return statuses, nil +} diff --git a/codersdk/users.go b/codersdk/users.go index 8b3ac9ca87016..0d6eaaa747902 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "time" "github.com/google/uuid" @@ -22,9 +23,11 @@ const ( ) type UsersRequest struct { - Search string `json:"search,omitempty"` + Search string `json:"search,omitempty" typescript:"-"` // Filter users by status - Status string `json:"status,omitempty"` + Status string `json:"status,omitempty" typescript:"-"` + + SearchQuery string `json:"q,omitempty"` Pagination } @@ -362,8 +365,14 @@ func (c *Client) Users(ctx context.Context, req UsersRequest) ([]User, error) { req.Pagination.asRequestOption(), func(r *http.Request) { q := r.URL.Query() - q.Set("search", req.Search) - q.Set("status", req.Status) + var params []string + if req.Search != "" { + params = append(params, req.Search) + } + if req.Search != "" { + params = append(params, "status:"+req.Status) + } + q.Set("q", strings.Join(params, " ")) r.URL.RawQuery = q.Encode() }, ) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 3fd40d3c81cb4..8b50ea60df571 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -64,7 +64,7 @@ export const getApiKey = async (): Promise => { } export const getUsers = async (): Promise => { - const response = await axios.get("/api/v2/users?status=active,suspended") + const response = await axios.get("/api/v2/users?q=status:active,suspended") return response.data } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 0f2521ad79d87..5e37fadabfdf1 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -12,7 +12,7 @@ export interface AgentGitSSHKey { readonly private_key: string } -// From codersdk/users.go:151:6 +// From codersdk/users.go:154:6 export interface AuthMethods { readonly password: boolean readonly github: boolean @@ -30,7 +30,7 @@ export interface BuildInfoResponse { readonly version: string } -// From codersdk/users.go:42:6 +// From codersdk/users.go:45:6 export interface CreateFirstUserRequest { readonly email: string readonly username: string @@ -38,13 +38,13 @@ export interface CreateFirstUserRequest { readonly organization: string } -// From codersdk/users.go:50:6 +// From codersdk/users.go:53:6 export interface CreateFirstUserResponse { readonly user_id: string readonly organization_id: string } -// From codersdk/users.go:146:6 +// From codersdk/users.go:149:6 export interface CreateOrganizationRequest { readonly name: string } @@ -82,7 +82,7 @@ export interface CreateTemplateVersionRequest { readonly parameter_values?: CreateParameterRequest[] } -// From codersdk/users.go:55:6 +// From codersdk/users.go:58:6 export interface CreateUserRequest { readonly email: string readonly username: string @@ -107,7 +107,7 @@ export interface CreateWorkspaceRequest { readonly parameter_values?: CreateParameterRequest[] } -// From codersdk/users.go:142:6 +// From codersdk/users.go:145:6 export interface GenerateAPIKeyResponse { readonly key: string } @@ -125,13 +125,13 @@ export interface GoogleInstanceIdentityToken { readonly json_web_token: string } -// From codersdk/users.go:131:6 +// From codersdk/users.go:134:6 export interface LoginWithPasswordRequest { readonly email: string readonly password: string } -// From codersdk/users.go:137:6 +// From codersdk/users.go:140:6 export interface LoginWithPasswordResponse { readonly session_token: string } @@ -289,7 +289,7 @@ export interface UpdateActiveTemplateVersion { readonly id: string } -// From codersdk/users.go:71:6 +// From codersdk/users.go:74:6 export interface UpdateRoles { readonly roles: string[] } @@ -301,13 +301,13 @@ export interface UpdateTemplateMeta { readonly min_autostart_interval_ms?: number } -// From codersdk/users.go:66:6 +// From codersdk/users.go:69:6 export interface UpdateUserPasswordRequest { readonly old_password: string readonly password: string } -// From codersdk/users.go:62:6 +// From codersdk/users.go:65:6 export interface UpdateUserProfileRequest { readonly username: string } @@ -327,7 +327,7 @@ export interface UploadResponse { readonly hash: string } -// From codersdk/users.go:32:6 +// From codersdk/users.go:35:6 export interface User { readonly id: string readonly email: string @@ -338,13 +338,13 @@ export interface User { readonly roles: Role[] } -// From codersdk/users.go:96:6 +// From codersdk/users.go:99:6 export interface UserAuthorization { readonly object: UserAuthorizationObject readonly action: string } -// From codersdk/users.go:112:6 +// From codersdk/users.go:115:6 export interface UserAuthorizationObject { readonly resource_type: string readonly owner_id?: string @@ -352,24 +352,23 @@ export interface UserAuthorizationObject { readonly resource_id?: string } -// From codersdk/users.go:85:6 +// From codersdk/users.go:88:6 export interface UserAuthorizationRequest { readonly checks: Record } -// From codersdk/users.go:80:6 +// From codersdk/users.go:83:6 export type UserAuthorizationResponse = Record -// From codersdk/users.go:75:6 +// From codersdk/users.go:78:6 export interface UserRoles { readonly roles: string[] readonly organization_roles: Record } -// From codersdk/users.go:24:6 +// From codersdk/users.go:25:6 export interface UsersRequest extends Pagination { - readonly search?: string - readonly status?: string + readonly q?: string } // From codersdk/workspaces.go:19:6 @@ -515,7 +514,7 @@ export type ProvisionerStorageMethod = "file" // From codersdk/organizations.go:20:6 export type ProvisionerType = "echo" | "terraform" -// From codersdk/users.go:17:6 +// From codersdk/users.go:18:6 export type UserStatus = "active" | "suspended" // From codersdk/workspaceresources.go:13:6 From 9b3c239b7e68fdc95cf1edbd3f28a6f8a37ab848 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 16 Jun 2022 16:20:13 -0500 Subject: [PATCH 2/7] Add rbac_role filter option --- coderd/database/queries.sql.go | 17 +++++++++++++---- coderd/database/queries/users.sql | 11 +++++++++-- coderd/httpapi/queryparams.go | 18 +++++++++++------- coderd/users.go | 5 +++-- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 8667a60ecfa74..6ebf74406bfff 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2289,27 +2289,35 @@ WHERE AND CASE -- @status needs to be a text because it can be empty, If it was -- user_status enum, it would not. - WHEN cardinality($3 :: user_status[]) > 0 THEN ( + WHEN cardinality($3 :: user_status[]) > 0 THEN status = ANY($3 :: user_status[]) - ) ELSE -- Only show active by default status = 'active' END + -- Filter by rbac_roles + AND CASE + -- @rbac_role allows filtering by rbac roles. If 'member' is included, show everyone, as + -- everyone is a member. + WHEN cardinality($4 :: text[]) > 0 AND 'member' != ANY($4 :: text[]) THEN + rbac_roles && $4 :: text[] + ELSE true + END -- End of filters ORDER BY -- Deterministic and consistent ordering of all users, even if they share -- a timestamp. This is to ensure consistent pagination. - (created_at, id) ASC OFFSET $4 + (created_at, id) ASC OFFSET $5 LIMIT -- A null limit means "no limit", so -1 means return all - NULLIF($5 :: int, -1) + NULLIF($6 :: int, -1) ` type GetUsersParams struct { AfterID uuid.UUID `db:"after_id" json:"after_id"` Search string `db:"search" json:"search"` Status []UserStatus `db:"status" json:"status"` + RbacRole []string `db:"rbac_role" json:"rbac_role"` OffsetOpt int32 `db:"offset_opt" json:"offset_opt"` LimitOpt int32 `db:"limit_opt" json:"limit_opt"` } @@ -2319,6 +2327,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, arg.AfterID, arg.Search, pq.Array(arg.Status), + pq.Array(arg.RbacRole), arg.OffsetOpt, arg.LimitOpt, ) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index be4a42a1538b7..7aae87d4b366f 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -108,13 +108,20 @@ WHERE AND CASE -- @status needs to be a text because it can be empty, If it was -- user_status enum, it would not. - WHEN cardinality(@status :: user_status[]) > 0 THEN ( + WHEN cardinality(@status :: user_status[]) > 0 THEN status = ANY(@status :: user_status[]) - ) ELSE -- Only show active by default status = 'active' END + -- Filter by rbac_roles + AND CASE + -- @rbac_role allows filtering by rbac roles. If 'member' is included, show everyone, as + -- everyone is a member. + WHEN cardinality(@rbac_role :: text[]) > 0 AND 'member' != ANY(@rbac_role :: text[]) THEN + rbac_roles && @rbac_role :: text[] + ELSE true + END -- End of filters ORDER BY -- Deterministic and consistent ordering of all users, even if they share diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 1d266fc6fdf64..7bb7dee1cc930 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -84,15 +84,19 @@ func (p *QueryParamParser) UUIDs(vals url.Values, def []uuid.UUID, queryParam st } func (p *QueryParamParser) String(vals url.Values, def string, queryParam string) string { - v, err := parseQueryParam(vals, func(v string) (string, error) { + v, _ := parseQueryParam(vals, func(v string) (string, error) { return v, nil }, def, queryParam) - if err != nil { - p.Errors = append(p.Errors, Error{ - Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a valid string", queryParam), - }) - } + return v +} + +func (p *QueryParamParser) Strings(vals url.Values, def []string, queryParam string) []string { + v, _ := parseQueryParam(vals, func(v string) ([]string, error) { + if v == "" { + return []string{}, nil + } + return strings.Split(v, ","), nil + }, def, queryParam) return v } diff --git a/coderd/users.go b/coderd/users.go index 837bca7166f4c..0a19bdaffb283 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -949,8 +949,9 @@ func userSearchQuery(query string) (database.GetUsersParams, []httpapi.Error) { parser := httpapi.NewQueryParamParser() filter := database.GetUsersParams{ - Search: parser.String(searchParams, "", "search"), - Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{database.UserStatusActive}, "status", parseUserStatus), + Search: parser.String(searchParams, "", "search"), + Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{database.UserStatusActive}, "status", parseUserStatus), + RbacRole: parser.Strings(searchParams, []string{}, "rbac_role"), } return filter, parser.Errors From 4b1b20189ac3d9c380a7048d3f8801726e571ce8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Jun 2022 10:53:16 -0500 Subject: [PATCH 3/7] Add role filter --- coderd/users.go | 3 ++- coderd/users_test.go | 2 +- codersdk/users.go | 13 +++++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 0a19bdaffb283..7f3e06530994d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -130,6 +130,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { LimitOpt: int32(paginationParams.Limit), Search: params.Search, Status: params.Status, + RbacRole: params.RbacRole, }) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(rw, http.StatusOK, []codersdk.User{}) @@ -951,7 +952,7 @@ func userSearchQuery(query string) (database.GetUsersParams, []httpapi.Error) { filter := database.GetUsersParams{ Search: parser.String(searchParams, "", "search"), Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{database.UserStatusActive}, "status", parseUserStatus), - RbacRole: parser.Strings(searchParams, []string{}, "rbac_role"), + RbacRole: parser.Strings(searchParams, []string{}, "role"), } return filter, parser.Errors diff --git a/coderd/users_test.go b/coderd/users_test.go index c1ec00fd97378..e6ec1f07b6a3f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -752,7 +752,7 @@ func TestGetUsers(t *testing.T) { require.NoError(t, err) users, err := client.Users(context.Background(), codersdk.UsersRequest{ - Status: string(codersdk.UserStatusActive), + Status: codersdk.UserStatusActive, }) require.NoError(t, err) require.ElementsMatch(t, active, users) diff --git a/codersdk/users.go b/codersdk/users.go index 0d6eaaa747902..7d51c9d14b344 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -24,8 +24,10 @@ const ( type UsersRequest struct { Search string `json:"search,omitempty" typescript:"-"` - // Filter users by status - Status string `json:"status,omitempty" typescript:"-"` + // Filter users by status. + Status UserStatus `json:"status,omitempty" typescript:"-"` + // Filter users that have the given role. + Role string `json:"role,omitempty" typescript:"-"` SearchQuery string `json:"q,omitempty"` Pagination @@ -369,8 +371,11 @@ func (c *Client) Users(ctx context.Context, req UsersRequest) ([]User, error) { if req.Search != "" { params = append(params, req.Search) } - if req.Search != "" { - params = append(params, "status:"+req.Status) + if req.Status != "" { + params = append(params, "status:"+string(req.Status)) + } + if req.Role != "" { + params = append(params, "role:"+string(req.Role)) } q.Set("q", strings.Join(params, " ")) r.URL.RawQuery = q.Encode() From 350204d525c19b48a5808836211624bef44a8087 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Jun 2022 11:56:33 -0500 Subject: [PATCH 4/7] Unit test user filter --- coderd/database/databasefake/databasefake.go | 14 ++- coderd/httpapi/queryparams.go | 4 +- coderd/users_test.go | 126 +++++++++++++++++++ coderd/util/slice/slice.go | 12 ++ coderd/util/slice/slice_test.go | 29 +++++ codersdk/users.go | 5 +- 6 files changed, 183 insertions(+), 7 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index bb9f65df3cd9c..d6591f3ae9d67 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -291,14 +291,20 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams usersFilteredByStatus := make([]database.User, 0, len(users)) for i, user := range users { - for _, status := range params.Status { - if user.Status == status { - usersFilteredByStatus = append(usersFilteredByStatus, users[i]) - } + if slice.Contains(params.Status, user.Status) { + usersFilteredByStatus = append(usersFilteredByStatus, users[i]) } } users = usersFilteredByStatus + usersFilteredByRole := make([]database.User, 0, len(users)) + for i, user := range users { + if slice.Overlap(params.RbacRole, user.RBACRoles) { + usersFilteredByRole = append(usersFilteredByRole, users[i]) + } + } + users = usersFilteredByRole + if params.OffsetOpt > 0 { if int(params.OffsetOpt) > len(users)-1 { return nil, sql.ErrNoRows diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 7bb7dee1cc930..d810358183957 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -83,14 +83,14 @@ func (p *QueryParamParser) UUIDs(vals url.Values, def []uuid.UUID, queryParam st return v } -func (p *QueryParamParser) String(vals url.Values, def string, queryParam string) string { +func (*QueryParamParser) String(vals url.Values, def string, queryParam string) string { v, _ := parseQueryParam(vals, func(v string) (string, error) { return v, nil }, def, queryParam) return v } -func (p *QueryParamParser) Strings(vals url.Values, def []string, queryParam string) []string { +func (*QueryParamParser) Strings(vals url.Values, def []string, queryParam string) []string { v, _ := parseQueryParam(vals, func(v string) ([]string, error) { if v == "" { return []string{}, nil diff --git a/coderd/users_test.go b/coderd/users_test.go index e6ec1f07b6a3f..4c4e25b567733 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -702,6 +702,132 @@ func TestGetUser(t *testing.T) { }) } +// TestUsersFilter creates a set of users to run various filters against for testing. +func TestUsersFilter(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + first := coderdtest.CreateFirstUser(t, client) + firstUser, err := client.User(context.Background(), codersdk.Me) + require.NoError(t, err, "fetch me") + + users := make([]codersdk.User, 0) + users = append(users, firstUser) + for i := 0; i < 15; i++ { + roles := []string{} + if i%2 == 0 { + roles = append(roles, rbac.RoleAdmin()) + } + if i%3 == 0 { + roles = append(roles, "auditor") + } + userClient := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, roles...) + user, err := userClient.User(context.Background(), codersdk.Me) + require.NoError(t, err, "fetch me") + + if i%4 == 0 { + user, err = client.UpdateUserStatus(context.Background(), user.ID.String(), codersdk.UserStatusSuspended) + require.NoError(t, err, "suspend user") + } + + users = append(users, user) + } + + // --- Setup done --- + testCases := []struct { + Name string + Filter codersdk.UsersRequest + // If FilterF is true, we include it in the expected results + FilterF func(f codersdk.UsersRequest, user codersdk.User) bool + }{ + { + Name: "All", + Filter: codersdk.UsersRequest{ + Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive, + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + return true + }, + }, + { + Name: "Active", + Filter: codersdk.UsersRequest{ + Status: codersdk.UserStatusActive, + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + return u.Status == codersdk.UserStatusActive + }, + }, + { + Name: "Suspended", + Filter: codersdk.UsersRequest{ + Status: codersdk.UserStatusSuspended, + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + return u.Status == codersdk.UserStatusSuspended + }, + }, + { + Name: "NameContains", + Filter: codersdk.UsersRequest{ + Search: "a", + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + return (strings.Contains(u.Username, "a") || strings.Contains(u.Email, "a")) && + u.Status == codersdk.UserStatusActive + }, + }, + { + Name: "Admins", + Filter: codersdk.UsersRequest{ + Role: rbac.RoleAdmin(), + Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive, + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + for _, r := range u.Roles { + if r.Name == rbac.RoleAdmin() { + return true + } + } + return false + }, + }, + { + Name: "SearchQuery", + Filter: codersdk.UsersRequest{ + SearchQuery: "i role:admin", + }, + FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { + for _, r := range u.Roles { + if r.Name == rbac.RoleAdmin() { + return (strings.Contains(u.Username, "i") || strings.Contains(u.Email, "i")) && + u.Status == codersdk.UserStatusActive + } + } + return false + }, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + matched, err := client.Users(context.Background(), c.Filter) + require.NoError(t, err, "fetch workspaces") + + exp := make([]codersdk.User, 0) + for _, made := range users { + match := c.FilterF(c.Filter, made) + if match { + exp = append(exp, made) + } + } + require.ElementsMatch(t, exp, matched, "expected workspaces returned") + }) + } +} + func TestGetUsers(t *testing.T) { t.Parallel() t.Run("AllUsers", func(t *testing.T) { diff --git a/coderd/util/slice/slice.go b/coderd/util/slice/slice.go index dfea2ed26f6c6..5338eee90712c 100644 --- a/coderd/util/slice/slice.go +++ b/coderd/util/slice/slice.go @@ -8,3 +8,15 @@ func Contains[T comparable](haystack []T, needle T) bool { } return false } + +// Overlap returns if the 2 sets have any overlap (element(s) in common) +func Overlap[T comparable](a []T, b []T) bool { + // For each element in b, if at least 1 is contained in 'a', + // return true. + for _, element := range b { + if Contains(a, element) { + return true + } + } + return false +} diff --git a/coderd/util/slice/slice_test.go b/coderd/util/slice/slice_test.go index 1f485b77e3067..d69b6c9c440ed 100644 --- a/coderd/util/slice/slice_test.go +++ b/coderd/util/slice/slice_test.go @@ -21,6 +21,35 @@ func TestContains(t *testing.T) { ) } +func TestOverlap(t *testing.T) { + t.Parallel() + + assertSetOverlaps(t, true, []int{1, 2, 3, 4, 5}, []int{1, 2, 3, 4, 5}) + assertSetOverlaps(t, true, []int{10}, []int{10}) + + assertSetOverlaps(t, false, []int{1, 2, 3, 4, 5}, []int{6, 7, 8, 9}) + assertSetOverlaps(t, false, []int{1, 2, 3, 4, 5}, []int{}) + assertSetOverlaps(t, false, []int{}, []int{}) + + assertSetOverlaps(t, true, []string{"hello", "world", "foo", "bar", "baz"}, []string{"hello", "world", "baz"}) + assertSetOverlaps(t, true, + []uuid.UUID{uuid.New(), uuid.MustParse("c7c6686d-a93c-4df2-bef9-5f837e9a33d5"), uuid.MustParse("8f3b3e0b-2c3f-46a5-a365-fd5b62bd8818")}, + []uuid.UUID{uuid.MustParse("c7c6686d-a93c-4df2-bef9-5f837e9a33d5")}, + ) +} + +func assertSetOverlaps[T comparable](t *testing.T, overlap bool, a []T, b []T) { + t.Helper() + for _, e := range a { + require.True(t, slice.Overlap(a, []T{e}), "elements in set should overlap with itself") + } + for _, e := range b { + require.True(t, slice.Overlap(b, []T{e}), "elements in set should overlap with itself") + } + + require.Equal(t, overlap, slice.Overlap(a, b)) +} + func assertSetContains[T comparable](t *testing.T, set []T, in []T, out []T) { t.Helper() for _, e := range set { diff --git a/codersdk/users.go b/codersdk/users.go index 7d51c9d14b344..7e8c607661ec1 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -375,7 +375,10 @@ func (c *Client) Users(ctx context.Context, req UsersRequest) ([]User, error) { params = append(params, "status:"+string(req.Status)) } if req.Role != "" { - params = append(params, "role:"+string(req.Role)) + params = append(params, "role:"+req.Role) + } + if req.SearchQuery != "" { + params = append(params, req.SearchQuery) } q.Set("q", strings.Join(params, " ")) r.URL.RawQuery = q.Encode() From 23be7b902eea90d983af5ce957e2b36a4eda5369 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 22 Jun 2022 17:36:13 -0500 Subject: [PATCH 5/7] Fix fake get users --- coderd/database/databasefake/databasefake.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 1bb48eb63f8e5..850ed8a5530a4 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -297,13 +297,15 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams } users = usersFilteredByStatus - usersFilteredByRole := make([]database.User, 0, len(users)) - for i, user := range users { - if slice.Overlap(params.RbacRole, user.RBACRoles) { - usersFilteredByRole = append(usersFilteredByRole, users[i]) + if len(params.RbacRole) > 0 { + usersFilteredByRole := make([]database.User, 0, len(users)) + for i, user := range users { + if slice.Overlap(params.RbacRole, user.RBACRoles) { + usersFilteredByRole = append(usersFilteredByRole, users[i]) + } } + users = usersFilteredByRole } - users = usersFilteredByRole if params.OffsetOpt > 0 { if int(params.OffsetOpt) > len(users)-1 { From 1a6b9df24e8867c860f34e3d40c5d9da28243f8d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 22 Jun 2022 17:55:44 -0500 Subject: [PATCH 6/7] Fix rbac unit test --- coderd/coderd_test.go | 3 ++- coderd/database/modelmethods.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 99d81adca1f1e..5d4d382a18985 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -338,10 +338,11 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertAction: rbac.ActionRead, AssertObject: workspaceRBACObj, }, - "POST:/api/v2/users/{user}/organizations/": { + "POST:/api/v2/users/{user}/organizations": { AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceOrganization, }, + "GET:/api/v2/users": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceUser}, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 1c813c4f4a2ff..dd5238da8793d 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -35,5 +35,5 @@ func (f File) RBACObject() rbac.Object { // If you are trying to get the RBAC object for the UserData, use // rbac.ResourceUserData func (u User) RBACObject() rbac.Object { - return rbac.ResourceFile.WithID(u.ID.String()) + return rbac.ResourceUser.WithID(u.ID.String()) } From 9250f9ad32e42f5d93bd5109a9e14a43c3c44537 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 22 Jun 2022 18:26:24 -0500 Subject: [PATCH 7/7] Default to all users --- coderd/database/databasefake/databasefake.go | 16 +++++++--------- coderd/database/queries.sql.go | 4 +--- coderd/database/queries/users.sql | 4 +--- coderd/users.go | 2 +- coderd/users_test.go | 5 ++--- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 850ed8a5530a4..12103eb771f0d 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -285,17 +285,15 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users = tmp } - if len(params.Status) == 0 { - params.Status = []database.UserStatus{database.UserStatusActive} - } - - usersFilteredByStatus := make([]database.User, 0, len(users)) - for i, user := range users { - if slice.Contains(params.Status, user.Status) { - usersFilteredByStatus = append(usersFilteredByStatus, users[i]) + if len(params.Status) > 0 { + usersFilteredByStatus := make([]database.User, 0, len(users)) + for i, user := range users { + if slice.Contains(params.Status, user.Status) { + usersFilteredByStatus = append(usersFilteredByStatus, users[i]) + } } + users = usersFilteredByStatus } - users = usersFilteredByStatus if len(params.RbacRole) > 0 { usersFilteredByRole := make([]database.User, 0, len(users)) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a78db01b0f519..a417b363677bf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2573,9 +2573,7 @@ WHERE -- user_status enum, it would not. WHEN cardinality($3 :: user_status[]) > 0 THEN status = ANY($3 :: user_status[]) - ELSE - -- Only show active by default - status = 'active' + ELSE true END -- Filter by rbac_roles AND CASE diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 7aae87d4b366f..a8b33fec18c46 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -110,9 +110,7 @@ WHERE -- user_status enum, it would not. WHEN cardinality(@status :: user_status[]) > 0 THEN status = ANY(@status :: user_status[]) - ELSE - -- Only show active by default - status = 'active' + ELSE true END -- Filter by rbac_roles AND CASE diff --git a/coderd/users.go b/coderd/users.go index 9daeb783a5b98..4a3945dca3ec2 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -981,7 +981,7 @@ func userSearchQuery(query string) (database.GetUsersParams, []httpapi.Error) { parser := httpapi.NewQueryParamParser() filter := database.GetUsersParams{ Search: parser.String(searchParams, "", "search"), - Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{database.UserStatusActive}, "status", parseUserStatus), + Status: httpapi.ParseCustom(parser, searchParams, []database.UserStatus{}, "status", parseUserStatus), RbacRole: parser.Strings(searchParams, []string{}, "role"), } diff --git a/coderd/users_test.go b/coderd/users_test.go index 9c3c065b4df3a..f2d272fd17b7a 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -775,8 +775,7 @@ func TestUsersFilter(t *testing.T) { Search: "a", }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { - return (strings.Contains(u.Username, "a") || strings.Contains(u.Email, "a")) && - u.Status == codersdk.UserStatusActive + return (strings.Contains(u.Username, "a") || strings.Contains(u.Email, "a")) }, }, { @@ -797,7 +796,7 @@ func TestUsersFilter(t *testing.T) { { Name: "SearchQuery", Filter: codersdk.UsersRequest{ - SearchQuery: "i role:admin", + SearchQuery: "i role:admin status:active", }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { for _, r := range u.Roles {