From 2c6d037ceba5495a96ee4615edfec6ea53461589 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 6 Aug 2024 13:12:39 +0000 Subject: [PATCH 01/24] - allow group members to read basic Group info - allow group members to see they are part of the group, but not see that information about other members - add a GetGroupMembersCountByGroupID SQL query, which allows group members to see members count without revealing other information about the members - expose a reducedGroupsByUserAndOrganization API endpoint --- coderd/apidoc/docs.go | 69 ++++++++++++++++++++++++ coderd/apidoc/swagger.json | 65 ++++++++++++++++++++++ coderd/database/db2sdk/db2sdk.go | 11 ++++ coderd/database/dbauthz/dbauthz.go | 49 ++++++++++++++++- coderd/database/dbmem/dbmem.go | 8 +++ coderd/database/dbmetrics/dbmetrics.go | 7 +++ coderd/database/dbmock/dbmock.go | 15 ++++++ coderd/database/modelmethods.go | 40 +++++++++++++- coderd/database/querier.go | 3 ++ coderd/database/queries.sql.go | 33 ++++++++++++ coderd/database/queries/groupmembers.sql | 25 +++++++++ codersdk/groups.go | 9 ++++ docs/api/enterprise.md | 59 ++++++++++++++++++++ docs/api/schemas.md | 24 +++++++++ enterprise/coderd/coderd.go | 9 ++++ enterprise/coderd/groups.go | 38 +++++++++++++ site/src/api/typesGenerated.ts | 10 ++++ 17 files changed, 471 insertions(+), 3 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 8fa9d0893421c..6df52c9c34406 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -3256,6 +3256,50 @@ const docTemplate = `{ } } }, + "/organizations/{organization}/users/{user}/reduced-groups": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": [ + "application/json" + ], + "tags": [ + "Enterprise" + ], + "summary": "Get reduced groups for a user in an organization", + "operationId": "get-reduced-groups-by-user-and-organization", + "parameters": [ + { + "type": "string", + "description": "Organization ID", + "name": "organization", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "User ID, name, or me", + "name": "user", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.ReducedGroup" + } + } + } + } + } + }, "/regions": { "get": { "security": [ @@ -11627,6 +11671,31 @@ const docTemplate = `{ } } }, + "codersdk.ReducedGroup": { + "type": "object", + "properties": { + "avatar_url": { + "type": "string" + }, + "display_name": { + "type": "string" + }, + "id": { + "type": "string", + "format": "uuid" + }, + "member_count": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "organization_id": { + "type": "string", + "format": "uuid" + } + } + }, "codersdk.ReducedUser": { "type": "object", "required": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 4fbb0f86e195a..7b57889cb968d 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2864,6 +2864,46 @@ } } }, + "/organizations/{organization}/users/{user}/reduced-groups": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": ["application/json"], + "tags": ["Enterprise"], + "summary": "Get reduced groups for a user in an organization", + "operationId": "get-reduced-groups-by-user-and-organization", + "parameters": [ + { + "type": "string", + "description": "Organization ID", + "name": "organization", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "User ID, name, or me", + "name": "user", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.ReducedGroup" + } + } + } + } + } + }, "/regions": { "get": { "security": [ @@ -10497,6 +10537,31 @@ } } }, + "codersdk.ReducedGroup": { + "type": "object", + "properties": { + "avatar_url": { + "type": "string" + }, + "display_name": { + "type": "string" + }, + "id": { + "type": "string", + "format": "uuid" + }, + "member_count": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "organization_id": { + "type": "string", + "format": "uuid" + } + } + }, "codersdk.ReducedUser": { "type": "object", "required": ["created_at", "email", "id", "username"], diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index 818793182e468..b0b8e9ed61c4f 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -192,6 +192,17 @@ func Group(group database.Group, members []database.User) codersdk.Group { } } +func ReducedGroup(group database.Group, memberCount int) codersdk.ReducedGroup { + return codersdk.ReducedGroup{ + ID: group.ID, + Name: group.Name, + DisplayName: group.DisplayName, + OrganizationID: group.OrganizationID, + AvatarURL: group.AvatarURL, + MemberCount: memberCount, + } +} + func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) { // Use a stable sort, similarly to how we would sort in the query, note that // we don't sort in the query because order varies depending on the table diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 1ab1183985098..7403ed5a96270 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1397,10 +1397,55 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, } func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.User, error) { - if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check + group, err := q.GetGroupByID(ctx, id) + if err != nil { // AuthZ check return nil, err } - return q.db.GetGroupMembersByGroupID(ctx, id) + // The UserWithGroupAndOrgID type is used to do the authz check. It ensures + // that group members can see themselves. Unless they have Group read permissions, + // they cannot see other members. + fetch := func(ctx context.Context, _ any) ([]database.UserWithGroupAndOrgID, error) { + users, err := q.db.GetGroupMembersByGroupID(ctx, id) + if err != nil { + return nil, err + } + groupMembers := make([]database.UserWithGroupAndOrgID, len(users)) + for i, user := range users { + groupMembers[i] = database.UserWithGroupAndOrgID{ + User: user, + GroupID: group.ID, + OrganizationID: group.OrganizationID, + } + } + return groupMembers, nil + } + groupMembers, err := fetchWithPostFilter(q.auth, policy.ActionRead, fetch)(ctx, nil) + if err != nil { + return nil, err + } + users := make([]database.User, len(groupMembers)) + for i, groupMember := range groupMembers { + users[i] = groupMember.User + } + return users, nil +} + +func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + group, err := q.GetGroupByID(ctx, groupID) + if err != nil { + return 0, err + } + memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID) + if err != nil { + return 0, err + } + if err := q.authorizeContext(ctx, policy.ActionRead, database.GroupMembersCountRBACHelper{ + GroupID: groupID, + OrganizationID: group.OrganizationID, + }); err != nil { + return 0, err + } + return memberCount, nil } func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 2ad54acd21473..c076e3d814bab 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2524,6 +2524,14 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) return users, nil } +func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + users, err := q.GetGroupMembersByGroupID(ctx, groupID) + if err != nil { + return 0, err + } + return int64(len(users)), nil +} + func (q *FakeQuerier) GetGroups(_ context.Context) ([]database.Group, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 207f02d241e99..9030a6dd3f1fa 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -655,6 +655,13 @@ func (m metricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid return users, err } +func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + start := time.Now() + r0, r1 := m.s.GetGroupMembersCountByGroupID(ctx, groupID) + m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupID").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetGroups(ctx context.Context) ([]database.Group, error) { start := time.Now() r0, r1 := m.s.GetGroups(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 11c1dcd86c831..a8005ad935eb5 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1299,6 +1299,21 @@ func (mr *MockStoreMockRecorder) GetGroupMembersByGroupID(arg0, arg1 any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersByGroupID), arg0, arg1) } +// GetGroupMembersCountByGroupID mocks base method. +func (m *MockStore) GetGroupMembersCountByGroupID(arg0 context.Context, arg1 uuid.UUID) (int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupID", arg0, arg1) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetGroupMembersCountByGroupID indicates an expected call of GetGroupMembersCountByGroupID. +func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), arg0, arg1) +} + // GetGroups mocks base method. func (m *MockStore) GetGroups(arg0 context.Context) ([]database.Group, error) { m.ctrl.T.Helper() diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 775000ac6ba05..85cda85d96f6f 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -12,6 +12,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" ) type WorkspaceStatus string @@ -173,7 +174,44 @@ func (v TemplateVersion) RBACObjectNoTemplate() rbac.Object { func (g Group) RBACObject() rbac.Object { return rbac.ResourceGroup.WithID(g.ID). - InOrg(g.OrganizationID) + InOrg(g.OrganizationID). + // Group members can read the group. + WithGroupACL(map[string][]policy.Action{ + g.ID.String(): { + policy.ActionRead, + }, + }) +} + +type UserWithGroupAndOrgID struct { + User User + GroupID uuid.UUID + OrganizationID uuid.UUID +} + +func (gm UserWithGroupAndOrgID) RBACObject() rbac.Object { + return rbac.ResourceGroup.WithID(gm.GroupID).InOrg(gm.OrganizationID). + // Group member can see they are in the group. + WithACLUserList(map[string][]policy.Action{ + gm.User.ID.String(): { + policy.ActionRead, + }, + }) +} + +type GroupMembersCountRBACHelper struct { + GroupID uuid.UUID + OrganizationID uuid.UUID +} + +func (r GroupMembersCountRBACHelper) RBACObject() rbac.Object { + return rbac.ResourceGroup.WithID(r.GroupID).InOrg(r.OrganizationID). + // Group members can read the member count. + WithGroupACL(map[string][]policy.Action{ + r.GroupID.String(): { + policy.ActionRead, + }, + }) } func (w GetWorkspaceByAgentIDRow) RBACObject() rbac.Object { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 9feb5266cc2a2..bfaac551e67c6 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -149,6 +149,9 @@ type sqlcQuerier interface { // If the group is a user made group, then we need to check the group_members table. // If it is the "Everyone" group, then we need to check the organization_members table. GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]User, error) + // If the group is a user made group, then we need to check the group_members table. + // If it is the "Everyone" group, then we need to check the organization_members table. + GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) GetGroups(ctx context.Context) ([]Group, error) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index ba8129584ccda..282d17e8752b5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1414,6 +1414,39 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. return items, nil } +const getGroupMembersCountByGroupID = `-- name: GetGroupMembersCountByGroupID :one +SELECT + COUNT(users.id) +FROM + users +LEFT JOIN + group_members +ON + group_members.user_id = users.id AND + group_members.group_id = $1 +LEFT JOIN + organization_members +ON + organization_members.user_id = users.id AND + organization_members.organization_id = $1 +WHERE + -- In either case, the group_id will only match an org or a group. + (group_members.group_id = $1 + OR + organization_members.organization_id = $1) +AND + users.deleted = 'false' +` + +// If the group is a user made group, then we need to check the group_members table. +// If it is the "Everyone" group, then we need to check the organization_members table. +func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) + var count int64 + err := row.Scan(&count) + return count, err +} + const insertGroupMember = `-- name: InsertGroupMember :exec INSERT INTO group_members (user_id, group_id) diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 8f4770eff112e..be68ffd0d64a5 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -26,6 +26,31 @@ WHERE AND users.deleted = 'false'; +-- name: GetGroupMembersCountByGroupID :one +SELECT + COUNT(users.id) +FROM + users +-- If the group is a user made group, then we need to check the group_members table. +LEFT JOIN + group_members +ON + group_members.user_id = users.id AND + group_members.group_id = @group_id +-- If it is the "Everyone" group, then we need to check the organization_members table. +LEFT JOIN + organization_members +ON + organization_members.user_id = users.id AND + organization_members.organization_id = @group_id +WHERE + -- In either case, the group_id will only match an org or a group. + (group_members.group_id = @group_id + OR + organization_members.organization_id = @group_id) +AND + users.deleted = 'false'; + -- InsertUserGroupsByName adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByName :exec WITH groups AS ( diff --git a/codersdk/groups.go b/codersdk/groups.go index 4b5b8f5a5f4e6..5cf807279fe4a 100644 --- a/codersdk/groups.go +++ b/codersdk/groups.go @@ -35,6 +35,15 @@ type Group struct { Source GroupSource `json:"source"` } +type ReducedGroup struct { + ID uuid.UUID `json:"id" format:"uuid"` + Name string `json:"name"` + DisplayName string `json:"display_name"` + OrganizationID uuid.UUID `json:"organization_id" format:"uuid"` + MemberCount int `json:"member_count"` + AvatarURL string `json:"avatar_url"` +} + func (g Group) IsEveryone() bool { return g.ID == g.OrganizationID } diff --git a/docs/api/enterprise.md b/docs/api/enterprise.md index be30b790d4aef..4f056aa4fae50 100644 --- a/docs/api/enterprise.md +++ b/docs/api/enterprise.md @@ -1511,6 +1511,65 @@ curl -X DELETE http://coder-server:8080/api/v2/organizations/{organization}/prov To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Get reduced groups for a user in an organization + +### Code samples + +```shell +# Example request using curl +curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/users/{user}/reduced-groups \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +`GET /organizations/{organization}/users/{user}/reduced-groups` + +### Parameters + +| Name | In | Type | Required | Description | +| -------------- | ---- | ------ | -------- | -------------------- | +| `organization` | path | string | true | Organization ID | +| `user` | path | string | true | User ID, name, or me | + +### Example responses + +> 200 Response + +```json +[ + { + "avatar_url": "string", + "display_name": "string", + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "member_count": 0, + "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6" + } +] +``` + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ----------------------------------------------------------------- | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | array of [codersdk.ReducedGroup](schemas.md#codersdkreducedgroup) | + +

Response Schema

+ +Status Code **200** + +| Name | Type | Required | Restrictions | Description | +| ------------------- | ------------ | -------- | ------------ | ----------- | +| `[array item]` | array | false | | | +| `» avatar_url` | string | false | | | +| `» display_name` | string | false | | | +| `» id` | string(uuid) | false | | | +| `» member_count` | integer | false | | | +| `» name` | string | false | | | +| `» organization_id` | string(uuid) | false | | | + +To perform this operation, you must be authenticated. [Learn more](authentication.md). + ## Get active replicas ### Code samples diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 1ece64c0c6a40..a7ecd4b218968 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -4302,6 +4302,30 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `api` | integer | false | | | | `disable_all` | boolean | false | | | +## codersdk.ReducedGroup + +```json +{ + "avatar_url": "string", + "display_name": "string", + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "member_count": 0, + "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ----------------- | ------- | -------- | ------------ | ----------- | +| `avatar_url` | string | false | | | +| `display_name` | string | false | | | +| `id` | string | false | | | +| `member_count` | integer | false | | | +| `name` | string | false | | | +| `organization_id` | string | false | | | + ## codersdk.ReducedUser ```json diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 501e6086eaeb3..955fb6d18593f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -288,6 +288,15 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { r.Get("/", api.groupByOrganization) }) }) + r.Route("/organizations/{organization}/users/{user}/reduced-groups", func(r chi.Router) { + r.Use( + apiKeyMiddleware, + api.templateRBACEnabledMW, + httpmw.ExtractOrganizationParam(api.Database), + httpmw.ExtractUserParam(options.Database), + ) + r.Get("/", api.reducedGroupsByUserAndOrganization) + }) r.Route("/organizations/{organization}/provisionerkeys", func(r chi.Router) { r.Use( apiKeyMiddleware, diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 0b027f21ff2e0..06de4a26764e9 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -420,3 +420,41 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp) } + +// @Summary Get reduced groups for a user in an organization +// @ID get-reduced-groups-by-user-and-organization +// @Security CoderSessionToken +// @Produce json +// @Tags Enterprise +// @Param organization path string true "Organization ID" +// @Param user path string true "User ID, name, or me" +// @Success 200 {array} codersdk.ReducedGroup +// @Router /organizations/{organization}/users/{user}/reduced-groups [get] +func (api *API) reducedGroupsByUserAndOrganization(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + org = httpmw.OrganizationParam(r) + ) + + groups, err := api.Database.GetGroupsByOrganizationAndUserID(ctx, database.GetGroupsByOrganizationAndUserIDParams{ + OrganizationID: org.ID, + UserID: user.ID, + }) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + httpapi.InternalServerError(rw, err) + return + } + resp := make([]codersdk.ReducedGroup, 0, len(groups)) + for _, group := range groups { + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount))) + } + + httpapi.Write(ctx, rw, http.StatusOK, resp) +} diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 27d310cb83515..221550327dfcc 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1066,6 +1066,16 @@ export interface RateLimitConfig { readonly api: number; } +// From codersdk/groups.go +export interface ReducedGroup { + readonly id: string; + readonly name: string; + readonly display_name: string; + readonly organization_id: string; + readonly member_count: number; + readonly avatar_url: string; +} + // From codersdk/users.go export interface ReducedUser extends MinimalUser { readonly name: string; From 2409072260ce1f472a9b2279e292e0b668eff1d6 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 6 Aug 2024 13:39:03 +0000 Subject: [PATCH 02/24] rename ReducedGroup.member_count to ReducedGroup.total_member_count --- coderd/apidoc/docs.go | 7 ++++--- coderd/apidoc/swagger.json | 7 ++++--- coderd/database/db2sdk/db2sdk.go | 14 +++++++------- codersdk/groups.go | 7 +++++-- docs/api/enterprise.md | 22 +++++++++++----------- docs/api/schemas.md | 20 ++++++++++---------- site/src/api/typesGenerated.ts | 2 +- 7 files changed, 42 insertions(+), 37 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 6df52c9c34406..b5d642086b63a 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11684,15 +11684,16 @@ const docTemplate = `{ "type": "string", "format": "uuid" }, - "member_count": { - "type": "integer" - }, "name": { "type": "string" }, "organization_id": { "type": "string", "format": "uuid" + }, + "total_member_count": { + "description": "How many users are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than ` + "`" + `len(Group.Members)` + "`" + `.", + "type": "integer" } } }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 7b57889cb968d..02012129345f4 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10550,15 +10550,16 @@ "type": "string", "format": "uuid" }, - "member_count": { - "type": "integer" - }, "name": { "type": "string" }, "organization_id": { "type": "string", "format": "uuid" + }, + "total_member_count": { + "description": "How many users are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than `len(Group.Members)`.", + "type": "integer" } } }, diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index b0b8e9ed61c4f..4dd2bc86d5443 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -192,14 +192,14 @@ func Group(group database.Group, members []database.User) codersdk.Group { } } -func ReducedGroup(group database.Group, memberCount int) codersdk.ReducedGroup { +func ReducedGroup(group database.Group, totalMemberCount int) codersdk.ReducedGroup { return codersdk.ReducedGroup{ - ID: group.ID, - Name: group.Name, - DisplayName: group.DisplayName, - OrganizationID: group.OrganizationID, - AvatarURL: group.AvatarURL, - MemberCount: memberCount, + ID: group.ID, + Name: group.Name, + DisplayName: group.DisplayName, + OrganizationID: group.OrganizationID, + AvatarURL: group.AvatarURL, + TotalMemberCount: totalMemberCount, } } diff --git a/codersdk/groups.go b/codersdk/groups.go index 5cf807279fe4a..e6fed666bdd72 100644 --- a/codersdk/groups.go +++ b/codersdk/groups.go @@ -40,8 +40,11 @@ type ReducedGroup struct { Name string `json:"name"` DisplayName string `json:"display_name"` OrganizationID uuid.UUID `json:"organization_id" format:"uuid"` - MemberCount int `json:"member_count"` - AvatarURL string `json:"avatar_url"` + // How many users are in this group. Shows the total count, + // even if the user is not authorized to read group member details. + // May be greater than `len(Group.Members)`. + TotalMemberCount int `json:"total_member_count"` + AvatarURL string `json:"avatar_url"` } func (g Group) IsEveryone() bool { diff --git a/docs/api/enterprise.md b/docs/api/enterprise.md index 4f056aa4fae50..fd49c3924d665 100644 --- a/docs/api/enterprise.md +++ b/docs/api/enterprise.md @@ -1541,9 +1541,9 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/users/{ "avatar_url": "string", "display_name": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "member_count": 0, "name": "string", - "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6" + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", + "total_member_count": 0 } ] ``` @@ -1558,15 +1558,15 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/users/{ Status Code **200** -| Name | Type | Required | Restrictions | Description | -| ------------------- | ------------ | -------- | ------------ | ----------- | -| `[array item]` | array | false | | | -| `» avatar_url` | string | false | | | -| `» display_name` | string | false | | | -| `» id` | string(uuid) | false | | | -| `» member_count` | integer | false | | | -| `» name` | string | false | | | -| `» organization_id` | string(uuid) | false | | | +| Name | Type | Required | Restrictions | Description | +| ---------------------- | ------------ | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `[array item]` | array | false | | | +| `» avatar_url` | string | false | | | +| `» display_name` | string | false | | | +| `» id` | string(uuid) | false | | | +| `» name` | string | false | | | +| `» organization_id` | string(uuid) | false | | | +| `» total_member_count` | integer | false | | How many users are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/api/schemas.md b/docs/api/schemas.md index a7ecd4b218968..cbf7cbe23c773 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -4309,22 +4309,22 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "avatar_url": "string", "display_name": "string", "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "member_count": 0, "name": "string", - "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6" + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", + "total_member_count": 0 } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -| ----------------- | ------- | -------- | ------------ | ----------- | -| `avatar_url` | string | false | | | -| `display_name` | string | false | | | -| `id` | string | false | | | -| `member_count` | integer | false | | | -| `name` | string | false | | | -| `organization_id` | string | false | | | +| Name | Type | Required | Restrictions | Description | +| -------------------- | ------- | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `avatar_url` | string | false | | | +| `display_name` | string | false | | | +| `id` | string | false | | | +| `name` | string | false | | | +| `organization_id` | string | false | | | +| `total_member_count` | integer | false | | How many users are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | ## codersdk.ReducedUser diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 221550327dfcc..1e96964c9b80f 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1072,7 +1072,7 @@ export interface ReducedGroup { readonly name: string; readonly display_name: string; readonly organization_id: string; - readonly member_count: number; + readonly total_member_count: number; readonly avatar_url: string; } From 5391c1505c3513dff9a4408c9b6c1dbd4003b698 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 6 Aug 2024 13:52:53 +0000 Subject: [PATCH 03/24] use the reduced groups for user endpoint on the AccountPage --- site/src/api/api.ts | 15 +++++++++++ site/src/api/queries/groups.ts | 8 ++++++ .../AccountPage/AccountPage.tsx | 4 +-- .../AccountPage/AccountUserGroups.stories.tsx | 25 ++++++++++++------- .../AccountPage/AccountUserGroups.tsx | 8 +++--- 5 files changed, 45 insertions(+), 15 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 397f5e0378d75..4c357ee83a7de 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1619,6 +1619,21 @@ class ApiMethods { return response.data; }; + /** + * @param organization Can be the organization's ID or name + * @param user Can be the user's ID, username, or "me" + */ + getReducedGroupsForUser = async ( + organization: string, + user: string, + ): Promise => { + const response = await this.axios.get( + `/api/v2/organizations/${organization}/users/${user}/reduced-groups`, + ); + + return response.data; + }; + addMember = async (groupId: string, userId: string) => { return this.patchGroup(groupId, { name: "", diff --git a/site/src/api/queries/groups.ts b/site/src/api/queries/groups.ts index ed670c5d17a9d..3bc375b8f6ce4 100644 --- a/site/src/api/queries/groups.ts +++ b/site/src/api/queries/groups.ts @@ -4,6 +4,7 @@ import type { CreateGroupRequest, Group, PatchGroupRequest, + ReducedGroup, } from "api/typesGenerated"; type GroupSortOrder = "asc" | "desc"; @@ -125,6 +126,13 @@ export const deleteGroup = (queryClient: QueryClient) => { }; }; +export function reducedGroupsForUser(organization: string, userId: string) { + return { + queryKey: ["organization", organization, "user", userId, "reduced-groups"], + queryFn: () => API.getReducedGroupsForUser(organization, userId), + } as const satisfies UseQueryOptions; +} + export const addMember = (queryClient: QueryClient) => { return { mutationFn: ({ groupId, userId }: { groupId: string; userId: string }) => diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 642c4389ff579..6b72c4f5551c4 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -1,6 +1,6 @@ import type { FC } from "react"; import { useQuery } from "react-query"; -import { groupsForUser } from "api/queries/groups"; +import { reducedGroupsForUser } from "api/queries/groups"; import { Stack } from "components/Stack/Stack"; import { useAuthContext } from "contexts/auth/AuthProvider"; import { useAuthenticated } from "contexts/auth/RequireAuth"; @@ -18,7 +18,7 @@ export const AccountPage: FC = () => { const hasGroupsFeature = entitlements.features.user_role_management.enabled; const groupsQuery = useQuery({ // TODO: This should probably list all groups, not just default org groups - ...groupsForUser("default", me.id), + ...reducedGroupsForUser("default", me.id), enabled: hasGroupsFeature, }); diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx index 6bdbbbd3d2b7d..5cb4b6dc631b8 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx @@ -1,17 +1,24 @@ import type { Meta, StoryObj } from "@storybook/react"; -import type { Group } from "api/typesGenerated"; +import type { ReducedGroup } from "api/typesGenerated"; import { MockGroup as MockGroup1, - MockUser, mockApiError, } from "testHelpers/entities"; import { AccountUserGroups } from "./AccountUserGroups"; -const MockGroup2: Group = { - ...MockGroup1, - avatar_url: "", +const MockReducedGroup1: ReducedGroup = { + id: MockGroup1.id, + name: MockGroup1.name, + display_name: MockGroup1.display_name, + organization_id: MockGroup1.organization_id, + avatar_url: MockGroup1.avatar_url, + total_member_count: 10, +}; + +const MockReducedGroup2: ReducedGroup = { + ...MockReducedGroup1, display_name: "Goofy Goobers", - members: [MockUser], + total_member_count: 5, }; const mockError = mockApiError({ @@ -22,7 +29,7 @@ const meta: Meta = { title: "pages/UserSettingsPage/AccountUserGroups", component: AccountUserGroups, args: { - groups: [MockGroup1, MockGroup2], + groups: [MockReducedGroup1, MockReducedGroup2], loading: false, }, }; @@ -40,7 +47,7 @@ export const NoGroups: Story = { export const OneGroup: Story = { args: { - groups: [MockGroup1], + groups: [MockReducedGroup1], }, }; @@ -61,7 +68,7 @@ export const Error: Story = { export const ErrorWithPreviousData: Story = { args: { - groups: [MockGroup1, MockGroup2], + groups: [MockReducedGroup1, MockReducedGroup2], error: mockError, loading: false, }, diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx index bc544fe1fab30..1ab01060205d1 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx @@ -2,14 +2,14 @@ import { useTheme } from "@emotion/react"; import Grid from "@mui/material/Grid"; import type { FC } from "react"; import { isApiError } from "api/errors"; -import type { Group } from "api/typesGenerated"; +import type { ReducedGroup } from "api/typesGenerated"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { AvatarCard } from "components/AvatarCard/AvatarCard"; import { Loader } from "components/Loader/Loader"; import { Section } from "../Section"; type AccountGroupsProps = { - groups: readonly Group[] | undefined; + groups: readonly ReducedGroup[] | undefined; error: unknown; loading: boolean; }; @@ -57,8 +57,8 @@ export const AccountUserGroups: FC = ({ header={group.display_name || group.name} subtitle={ <> - {group.members.length} member - {group.members.length !== 1 && "s"} + {group.total_member_count} member + {group.total_member_count !== 1 && "s"} } /> From 5fcc218413847c0d8d99e108f03a827b89cfee8e Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Wed, 7 Aug 2024 12:34:23 +0000 Subject: [PATCH 04/24] rename UserWithGroupAndOrgID to GroupMemberRBACHelper --- coderd/database/dbauthz/dbauthz.go | 8 ++++---- coderd/database/modelmethods.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 7403ed5a96270..465c291b5edc1 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1401,17 +1401,17 @@ func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([ if err != nil { // AuthZ check return nil, err } - // The UserWithGroupAndOrgID type is used to do the authz check. It ensures + // The GroupMemberRBACHelper type is used to do the authz check. It ensures // that group members can see themselves. Unless they have Group read permissions, // they cannot see other members. - fetch := func(ctx context.Context, _ any) ([]database.UserWithGroupAndOrgID, error) { + fetch := func(ctx context.Context, _ any) ([]database.GroupMemberRBACHelper, error) { users, err := q.db.GetGroupMembersByGroupID(ctx, id) if err != nil { return nil, err } - groupMembers := make([]database.UserWithGroupAndOrgID, len(users)) + groupMembers := make([]database.GroupMemberRBACHelper, len(users)) for i, user := range users { - groupMembers[i] = database.UserWithGroupAndOrgID{ + groupMembers[i] = database.GroupMemberRBACHelper{ User: user, GroupID: group.ID, OrganizationID: group.OrganizationID, diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 85cda85d96f6f..0fd898709268d 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -183,13 +183,13 @@ func (g Group) RBACObject() rbac.Object { }) } -type UserWithGroupAndOrgID struct { +type GroupMemberRBACHelper struct { User User GroupID uuid.UUID OrganizationID uuid.UUID } -func (gm UserWithGroupAndOrgID) RBACObject() rbac.Object { +func (gm GroupMemberRBACHelper) RBACObject() rbac.Object { return rbac.ResourceGroup.WithID(gm.GroupID).InOrg(gm.OrganizationID). // Group member can see they are in the group. WithACLUserList(map[string][]policy.Action{ From 2878c218bdf5f99d53fee032d689298221da7639 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 10:32:24 +0000 Subject: [PATCH 05/24] add the group_members_expanded db view --- .../000241_group_members_view.down.sql | 1 + .../000241_group_members_view.up.sql | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 coderd/database/migrations/000241_group_members_view.down.sql create mode 100644 coderd/database/migrations/000241_group_members_view.up.sql diff --git a/coderd/database/migrations/000241_group_members_view.down.sql b/coderd/database/migrations/000241_group_members_view.down.sql new file mode 100644 index 0000000000000..99d64047d1211 --- /dev/null +++ b/coderd/database/migrations/000241_group_members_view.down.sql @@ -0,0 +1 @@ +DROP VIEW group_members_expanded; diff --git a/coderd/database/migrations/000241_group_members_view.up.sql b/coderd/database/migrations/000241_group_members_view.up.sql new file mode 100644 index 0000000000000..278e1ea4132e1 --- /dev/null +++ b/coderd/database/migrations/000241_group_members_view.up.sql @@ -0,0 +1,25 @@ +CREATE VIEW + group_members_expanded +AS +WITH all_members AS ( + SELECT user_id, group_id FROM group_members + UNION + SELECT user_id, organization_id AS group_id FROM organization_members +) +SELECT + users.id AS user_id, + users.username, + users.avatar_url AS user_avatar_url, + groups.organization_id AS organization_id, + groups.name AS group_name, + all_members.group_id AS group_id +FROM + all_members +JOIN + users ON users.id = all_members.user_id +JOIN + groups ON groups.id = all_members.group_id +WHERE + users.deleted = 'false'; + +COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).'; From 3aa2a237321d35a33bd7d6d7b4d1ca6e890fa58b Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 11:48:18 +0000 Subject: [PATCH 06/24] add comment on migration --- coderd/database/migrations/000241_group_members_view.up.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/database/migrations/000241_group_members_view.up.sql b/coderd/database/migrations/000241_group_members_view.up.sql index 278e1ea4132e1..c96503efdacac 100644 --- a/coderd/database/migrations/000241_group_members_view.up.sql +++ b/coderd/database/migrations/000241_group_members_view.up.sql @@ -1,6 +1,8 @@ CREATE VIEW group_members_expanded AS +-- If the group is a user made group, then we need to check the group_members table. +-- If it is the "Everyone" group, then we need to check the organization_members table. WITH all_members AS ( SELECT user_id, group_id FROM group_members UNION From e428d4066757176d6f77eba2366b94ab55e80ff4 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 12:03:20 +0000 Subject: [PATCH 07/24] rewrite group member queries to use the group_members_expanded view --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/database/dbmem/dbmem.go | 2 +- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 4 +- coderd/database/dump.sql | 93 ++++++++++++------- coderd/database/models.go | 10 ++ coderd/database/querier.go | 6 +- coderd/database/queries.sql.go | 112 ++++++----------------- coderd/database/queries/groupmembers.sql | 50 +--------- coderd/database/queries/groups.sql | 28 ++---- coderd/database/sqlc.yaml | 2 + 11 files changed, 118 insertions(+), 193 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 465c291b5edc1..c813017579464 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1396,7 +1396,7 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, return q.db.GetGroupMembers(ctx) } -func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.User, error) { +func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) { group, err := q.GetGroupByID(ctx, id) if err != nil { // AuthZ check return nil, err diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index c076e3d814bab..60b5ef9725455 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2495,7 +2495,7 @@ func (q *FakeQuerier) GetGroupMembers(_ context.Context) ([]database.GroupMember return out, nil } -func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) ([]database.User, error) { +func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 9030a6dd3f1fa..eb62316be1902 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -648,7 +648,7 @@ func (m metricsStore) GetGroupMembers(ctx context.Context) ([]database.GroupMemb return r0, r1 } -func (m metricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]database.User, error) { +func (m metricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]database.GroupMember, error) { start := time.Now() users, err := m.s.GetGroupMembersByGroupID(ctx, groupID) m.queryLatencies.WithLabelValues("GetGroupMembersByGroupID").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a8005ad935eb5..9920dafada324 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1285,10 +1285,10 @@ func (mr *MockStoreMockRecorder) GetGroupMembers(arg0 any) *gomock.Call { } // GetGroupMembersByGroupID mocks base method. -func (m *MockStore) GetGroupMembersByGroupID(arg0 context.Context, arg1 uuid.UUID) ([]database.User, error) { +func (m *MockStore) GetGroupMembersByGroupID(arg0 context.Context, arg1 uuid.UUID) ([]database.GroupMember, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetGroupMembersByGroupID", arg0, arg1) - ret0, _ := ret[0].([]database.User) + ret0, _ := ret[0].([]database.GroupMember) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index a16d4bc31847b..72e243f599211 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -563,6 +563,64 @@ COMMENT ON COLUMN groups.display_name IS 'Display name is a custom, human-friend COMMENT ON COLUMN groups.source IS 'Source indicates how the group was created. It can be created by a user manually, or through some system process like OIDC group sync.'; +CREATE TABLE organization_members ( + user_id uuid NOT NULL, + organization_id uuid NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + roles text[] DEFAULT '{}'::text[] NOT NULL +); + +CREATE TABLE users ( + id uuid NOT NULL, + email text NOT NULL, + username text DEFAULT ''::text NOT NULL, + hashed_password bytea NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + status user_status DEFAULT 'dormant'::user_status NOT NULL, + rbac_roles text[] DEFAULT '{}'::text[] NOT NULL, + login_type login_type DEFAULT 'password'::login_type NOT NULL, + avatar_url text DEFAULT ''::text NOT NULL, + deleted boolean DEFAULT false NOT NULL, + last_seen_at timestamp without time zone DEFAULT '0001-01-01 00:00:00'::timestamp without time zone NOT NULL, + quiet_hours_schedule text DEFAULT ''::text NOT NULL, + theme_preference text DEFAULT ''::text NOT NULL, + name text DEFAULT ''::text NOT NULL, + github_com_user_id bigint +); + +COMMENT ON COLUMN users.quiet_hours_schedule IS 'Daily (!) cron schedule (with optional CRON_TZ) signifying the start of the user''s quiet hours. If empty, the default quiet hours on the instance is used instead.'; + +COMMENT ON COLUMN users.theme_preference IS '"" can be interpreted as "the user does not care", falling back to the default theme'; + +COMMENT ON COLUMN users.name IS 'Name of the Coder user'; + +COMMENT ON COLUMN users.github_com_user_id IS 'The GitHub.com numerical user ID. At time of implementation, this is used to check if the user has starred the Coder repository.'; + +CREATE VIEW group_members_expanded AS + WITH all_members AS ( + SELECT group_members.user_id, + group_members.group_id + FROM group_members + UNION + SELECT organization_members.user_id, + organization_members.organization_id AS group_id + FROM organization_members + ) + SELECT users.id AS user_id, + users.username, + users.avatar_url AS user_avatar_url, + groups.organization_id, + groups.name AS group_name, + all_members.group_id + FROM ((all_members + JOIN users ON ((users.id = all_members.user_id))) + JOIN groups ON ((groups.id = all_members.group_id))) + WHERE (users.deleted = false); + +COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).'; + CREATE TABLE jfrog_xray_scans ( agent_id uuid NOT NULL, workspace_id uuid NOT NULL, @@ -680,14 +738,6 @@ CREATE TABLE oauth2_provider_apps ( COMMENT ON TABLE oauth2_provider_apps IS 'A table used to configure apps that can use Coder as an OAuth2 provider, the reverse of what we are calling external authentication.'; -CREATE TABLE organization_members ( - user_id uuid NOT NULL, - organization_id uuid NOT NULL, - created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL, - roles text[] DEFAULT '{}'::text[] NOT NULL -); - CREATE TABLE organizations ( id uuid NOT NULL, name text NOT NULL, @@ -1014,33 +1064,6 @@ COMMENT ON COLUMN template_versions.external_auth_providers IS 'IDs of External COMMENT ON COLUMN template_versions.message IS 'Message describing the changes in this version of the template, similar to a Git commit message. Like a commit message, this should be a short, high-level description of the changes in this version of the template. This message is immutable and should not be updated after the fact.'; -CREATE TABLE users ( - id uuid NOT NULL, - email text NOT NULL, - username text DEFAULT ''::text NOT NULL, - hashed_password bytea NOT NULL, - created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL, - status user_status DEFAULT 'dormant'::user_status NOT NULL, - rbac_roles text[] DEFAULT '{}'::text[] NOT NULL, - login_type login_type DEFAULT 'password'::login_type NOT NULL, - avatar_url text DEFAULT ''::text NOT NULL, - deleted boolean DEFAULT false NOT NULL, - last_seen_at timestamp without time zone DEFAULT '0001-01-01 00:00:00'::timestamp without time zone NOT NULL, - quiet_hours_schedule text DEFAULT ''::text NOT NULL, - theme_preference text DEFAULT ''::text NOT NULL, - name text DEFAULT ''::text NOT NULL, - github_com_user_id bigint -); - -COMMENT ON COLUMN users.quiet_hours_schedule IS 'Daily (!) cron schedule (with optional CRON_TZ) signifying the start of the user''s quiet hours. If empty, the default quiet hours on the instance is used instead.'; - -COMMENT ON COLUMN users.theme_preference IS '"" can be interpreted as "the user does not care", falling back to the default theme'; - -COMMENT ON COLUMN users.name IS 'Name of the Coder user'; - -COMMENT ON COLUMN users.github_com_user_id IS 'The GitHub.com numerical user ID. At time of implementation, this is used to check if the user has starred the Coder repository.'; - CREATE VIEW visible_users AS SELECT users.id, users.username, diff --git a/coderd/database/models.go b/coderd/database/models.go index 4bd012e258fbd..8f735745cde29 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2054,7 +2054,17 @@ type Group struct { Source GroupSource `db:"source" json:"source"` } +// Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group). type GroupMember struct { + UserID uuid.UUID `db:"user_id" json:"user_id"` + Username string `db:"username" json:"username"` + UserAvatarUrl string `db:"user_avatar_url" json:"user_avatar_url"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + GroupName string `db:"group_name" json:"group_name"` + GroupID uuid.UUID `db:"group_id" json:"group_id"` +} + +type GroupMemberTable struct { UserID uuid.UUID `db:"user_id" json:"user_id"` GroupID uuid.UUID `db:"group_id" json:"group_id"` } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index bfaac551e67c6..cd3a0d466b7d6 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -146,11 +146,7 @@ type sqlcQuerier interface { GetGroupByID(ctx context.Context, id uuid.UUID) (Group, error) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) GetGroupMembers(ctx context.Context) ([]GroupMember, error) - // If the group is a user made group, then we need to check the group_members table. - // If it is the "Everyone" group, then we need to check the organization_members table. - GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]User, error) - // If the group is a user made group, then we need to check the group_members table. - // If it is the "Everyone" group, then we need to check the organization_members table. + GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) GetGroups(ctx context.Context) ([]Group, error) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 282d17e8752b5..0a16c78ee0032 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1322,7 +1322,7 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG } const getGroupMembers = `-- name: GetGroupMembers :many -SELECT user_id, group_id FROM group_members +SELECT user_id, username, user_avatar_url, organization_id, group_name, group_id FROM group_members_expanded ` func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) { @@ -1334,7 +1334,14 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) var items []GroupMember for rows.Next() { var i GroupMember - if err := rows.Scan(&i.UserID, &i.GroupID); err != nil { + if err := rows.Scan( + &i.UserID, + &i.Username, + &i.UserAvatarUrl, + &i.OrganizationID, + &i.GroupName, + &i.GroupID, + ); err != nil { return nil, err } items = append(items, i) @@ -1349,57 +1356,25 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) } const getGroupMembersByGroupID = `-- name: GetGroupMembersByGroupID :many -SELECT - users.id, users.email, users.username, users.hashed_password, users.created_at, users.updated_at, users.status, users.rbac_roles, users.login_type, users.avatar_url, users.deleted, users.last_seen_at, users.quiet_hours_schedule, users.theme_preference, users.name, users.github_com_user_id -FROM - users -LEFT JOIN - group_members -ON - group_members.user_id = users.id AND - group_members.group_id = $1 -LEFT JOIN - organization_members -ON - organization_members.user_id = users.id AND - organization_members.organization_id = $1 -WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.group_id = $1 - OR - organization_members.organization_id = $1) -AND - users.deleted = 'false' +SELECT user_id, username, user_avatar_url, organization_id, group_name, group_id FROM group_members_expanded WHERE group_id = $1 ` -// If the group is a user made group, then we need to check the group_members table. -// If it is the "Everyone" group, then we need to check the organization_members table. -func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]User, error) { +func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) { rows, err := q.db.QueryContext(ctx, getGroupMembersByGroupID, groupID) if err != nil { return nil, err } defer rows.Close() - var items []User + var items []GroupMember for rows.Next() { - var i User + var i GroupMember if err := rows.Scan( - &i.ID, - &i.Email, + &i.UserID, &i.Username, - &i.HashedPassword, - &i.CreatedAt, - &i.UpdatedAt, - &i.Status, - &i.RBACRoles, - &i.LoginType, - &i.AvatarURL, - &i.Deleted, - &i.LastSeenAt, - &i.QuietHoursSchedule, - &i.ThemePreference, - &i.Name, - &i.GithubComUserID, + &i.UserAvatarUrl, + &i.OrganizationID, + &i.GroupName, + &i.GroupID, ); err != nil { return nil, err } @@ -1415,31 +1390,9 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. } const getGroupMembersCountByGroupID = `-- name: GetGroupMembersCountByGroupID :one -SELECT - COUNT(users.id) -FROM - users -LEFT JOIN - group_members -ON - group_members.user_id = users.id AND - group_members.group_id = $1 -LEFT JOIN - organization_members -ON - organization_members.user_id = users.id AND - organization_members.organization_id = $1 -WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.group_id = $1 - OR - organization_members.organization_id = $1) -AND - users.deleted = 'false' +SELECT COUNT(*) FROM group_members_expanded WHERE group_id = $1 ` -// If the group is a user made group, then we need to check the group_members table. -// If it is the "Everyone" group, then we need to check the organization_members table. func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) var count int64 @@ -1618,24 +1571,17 @@ SELECT groups.id, groups.name, groups.organization_id, groups.avatar_url, groups.quota_allowance, groups.display_name, groups.source FROM groups - -- If the group is a user made group, then we need to check the group_members table. -LEFT JOIN - group_members -ON - group_members.group_id = groups.id AND - group_members.user_id = $1 - -- If it is the "Everyone" group, then we need to check the organization_members table. -LEFT JOIN - organization_members -ON - organization_members.organization_id = groups.id AND - organization_members.user_id = $1 WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.user_id = $1 OR organization_members.user_id = $1) -AND - -- Ensure the group or organization is the specified organization. - groups.organization_id = $2 + groups.id IN ( + SELECT + group_id + FROM + group_members_expanded gme + WHERE + gme.user_id = $1 + AND + gme.organization_id = $2 + ) ` type GetGroupsByOrganizationAndUserIDParams struct { diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index be68ffd0d64a5..1cfe2d573626a 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -1,55 +1,11 @@ -- name: GetGroupMembers :many -SELECT * FROM group_members; +SELECT * FROM group_members_expanded; -- name: GetGroupMembersByGroupID :many -SELECT - users.* -FROM - users --- If the group is a user made group, then we need to check the group_members table. -LEFT JOIN - group_members -ON - group_members.user_id = users.id AND - group_members.group_id = @group_id --- If it is the "Everyone" group, then we need to check the organization_members table. -LEFT JOIN - organization_members -ON - organization_members.user_id = users.id AND - organization_members.organization_id = @group_id -WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.group_id = @group_id - OR - organization_members.organization_id = @group_id) -AND - users.deleted = 'false'; +SELECT * FROM group_members_expanded WHERE group_id = @group_id; -- name: GetGroupMembersCountByGroupID :one -SELECT - COUNT(users.id) -FROM - users --- If the group is a user made group, then we need to check the group_members table. -LEFT JOIN - group_members -ON - group_members.user_id = users.id AND - group_members.group_id = @group_id --- If it is the "Everyone" group, then we need to check the organization_members table. -LEFT JOIN - organization_members -ON - organization_members.user_id = users.id AND - organization_members.organization_id = @group_id -WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.group_id = @group_id - OR - organization_members.organization_id = @group_id) -AND - users.deleted = 'false'; +SELECT COUNT(*) FROM group_members_expanded WHERE group_id = @group_id; -- InsertUserGroupsByName adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByName :exec diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index 9dea20f0fa6e6..edd8448eacb35 100644 --- a/coderd/database/queries/groups.sql +++ b/coderd/database/queries/groups.sql @@ -36,25 +36,17 @@ SELECT groups.* FROM groups - -- If the group is a user made group, then we need to check the group_members table. -LEFT JOIN - group_members -ON - group_members.group_id = groups.id AND - group_members.user_id = @user_id - -- If it is the "Everyone" group, then we need to check the organization_members table. -LEFT JOIN - organization_members -ON - organization_members.organization_id = groups.id AND - organization_members.user_id = @user_id WHERE - -- In either case, the group_id will only match an org or a group. - (group_members.user_id = @user_id OR organization_members.user_id = @user_id) -AND - -- Ensure the group or organization is the specified organization. - groups.organization_id = @organization_id; - + groups.id IN ( + SELECT + group_id + FROM + group_members_expanded gme + WHERE + gme.user_id = @user_id + AND + gme.organization_id = @organization_id + ); -- name: InsertGroup :one INSERT INTO groups ( diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 2896e7035fcfa..9eeecd6b21d00 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -74,6 +74,8 @@ sql: go_type: type: "[]byte" rename: + group_member: GroupMemberTable + group_members_expanded: GroupMember template: TemplateTable template_with_name: Template workspace_build: WorkspaceBuildTable From 13ca3a9290e5b97efeff147a2b110217818a48f9 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 12:15:13 +0000 Subject: [PATCH 08/24] add the RBAC ResourceGroupMember and add it to relevant roles --- coderd/rbac/object_gen.go | 8 ++++++++ coderd/rbac/policy/policy.go | 5 +++++ coderd/rbac/roles.go | 13 +++++++++---- codersdk/rbacresources_gen.go | 2 ++ site/src/api/rbacresources_gen.ts | 3 +++ site/src/api/typesGenerated.ts | 2 ++ 6 files changed, 29 insertions(+), 4 deletions(-) diff --git a/coderd/rbac/object_gen.go b/coderd/rbac/object_gen.go index 7645f65c5c502..e75dd76292edb 100644 --- a/coderd/rbac/object_gen.go +++ b/coderd/rbac/object_gen.go @@ -93,6 +93,13 @@ var ( Type: "group", } + // ResourceGroupMember + // Valid Actions + // - "ActionRead" :: read group members + ResourceGroupMember = Object{ + Type: "group_member", + } + // ResourceLicense // Valid Actions // - "ActionCreate" :: create a license @@ -287,6 +294,7 @@ func AllResources() []Objecter { ResourceDeploymentStats, ResourceFile, ResourceGroup, + ResourceGroupMember, ResourceLicense, ResourceNotificationPreference, ResourceNotificationTemplate, diff --git a/coderd/rbac/policy/policy.go b/coderd/rbac/policy/policy.go index 54dcbe358007b..398cec2c829b0 100644 --- a/coderd/rbac/policy/policy.go +++ b/coderd/rbac/policy/policy.go @@ -149,6 +149,11 @@ var RBACPermissions = map[string]PermissionDefinition{ ActionUpdate: actDef("update a group"), }, }, + "group_member": { + Actions: map[Action]ActionDefinition{ + ActionRead: actDef("read group members"), + }, + }, "file": { Actions: map[Action]ActionDefinition{ ActionCreate: actDef("create a file"), diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 4511111feded6..14f797ca0b4ee 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -301,10 +301,11 @@ func ReloadBuiltinRoles(opts *RoleOptions) { Site: Permissions(map[string][]policy.Action{ // Should be able to read all template details, even in orgs they // are not in. - ResourceTemplate.Type: {policy.ActionRead, policy.ActionViewInsights}, - ResourceAuditLog.Type: {policy.ActionRead}, - ResourceUser.Type: {policy.ActionRead}, - ResourceGroup.Type: {policy.ActionRead}, + ResourceTemplate.Type: {policy.ActionRead, policy.ActionViewInsights}, + ResourceAuditLog.Type: {policy.ActionRead}, + ResourceUser.Type: {policy.ActionRead}, + ResourceGroup.Type: {policy.ActionRead}, + ResourceGroupMember.Type: {policy.ActionRead}, // Allow auditors to query deployment stats and insights. ResourceDeploymentStats.Type: {policy.ActionRead}, ResourceDeploymentConfig.Type: {policy.ActionRead}, @@ -329,6 +330,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { ResourceOrganization.Type: {policy.ActionRead}, ResourceUser.Type: {policy.ActionRead}, ResourceGroup.Type: {policy.ActionRead}, + ResourceGroupMember.Type: {policy.ActionRead}, // Org roles are not really used yet, so grant the perm at the site level. ResourceOrganizationMember.Type: {policy.ActionRead}, }), @@ -351,6 +353,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { // Full perms to manage org members ResourceOrganizationMember.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, ResourceGroup.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, + ResourceGroupMember.Type: {policy.ActionRead}, }), Org: map[string][]Permission{}, User: []Permission{}, @@ -461,6 +464,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { ResourceAssignOrgRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead}, ResourceOrganizationMember.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, ResourceGroup.Type: ResourceGroup.AvailableActions(), + ResourceGroupMember.Type: ResourceGroupMember.AvailableActions(), }), }, User: []Permission{}, @@ -480,6 +484,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { // Assigning template perms requires this permission. ResourceOrganizationMember.Type: {policy.ActionRead}, ResourceGroup.Type: {policy.ActionRead}, + ResourceGroupMember.Type: {policy.ActionRead}, }), }, User: []Permission{}, diff --git a/codersdk/rbacresources_gen.go b/codersdk/rbacresources_gen.go index 788cab912643b..67a24bf73e04a 100644 --- a/codersdk/rbacresources_gen.go +++ b/codersdk/rbacresources_gen.go @@ -14,6 +14,7 @@ const ( ResourceDeploymentStats RBACResource = "deployment_stats" ResourceFile RBACResource = "file" ResourceGroup RBACResource = "group" + ResourceGroupMember RBACResource = "group_member" ResourceLicense RBACResource = "license" ResourceNotificationPreference RBACResource = "notification_preference" ResourceNotificationTemplate RBACResource = "notification_template" @@ -65,6 +66,7 @@ var RBACResourceActions = map[RBACResource][]RBACAction{ ResourceDeploymentStats: {ActionRead}, ResourceFile: {ActionCreate, ActionRead}, ResourceGroup: {ActionCreate, ActionDelete, ActionRead, ActionUpdate}, + ResourceGroupMember: {ActionRead}, ResourceLicense: {ActionCreate, ActionDelete, ActionRead}, ResourceNotificationPreference: {ActionRead, ActionUpdate}, ResourceNotificationTemplate: {ActionRead, ActionUpdate}, diff --git a/site/src/api/rbacresources_gen.ts b/site/src/api/rbacresources_gen.ts index 6cee389dfbc7a..0f9578aa226bf 100644 --- a/site/src/api/rbacresources_gen.ts +++ b/site/src/api/rbacresources_gen.ts @@ -50,6 +50,9 @@ export const RBACResourceActions: Partial< read: "read groups", update: "update a group", }, + group_member: { + read: "read group members", + }, license: { create: "create a license", delete: "delete license", diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 1e96964c9b80f..d239444988328 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2312,6 +2312,7 @@ export type RBACResource = | "deployment_stats" | "file" | "group" + | "group_member" | "license" | "notification_preference" | "notification_template" @@ -2341,6 +2342,7 @@ export const RBACResources: RBACResource[] = [ "deployment_stats", "file", "group", + "group_member", "license", "notification_preference", "notification_template", From 1027d9f58f6bfb3290e2529a2b7cbf2ffcae5caf Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 14:46:16 +0000 Subject: [PATCH 09/24] rewrite GetGroupMembersByGroupID permission checks --- coderd/database/dbauthz/dbauthz.go | 32 +----------------------------- coderd/database/modelmethods.go | 16 ++------------- 2 files changed, 3 insertions(+), 45 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index c813017579464..6fd0c3b5765ca 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1397,37 +1397,7 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, } func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) { - group, err := q.GetGroupByID(ctx, id) - if err != nil { // AuthZ check - return nil, err - } - // The GroupMemberRBACHelper type is used to do the authz check. It ensures - // that group members can see themselves. Unless they have Group read permissions, - // they cannot see other members. - fetch := func(ctx context.Context, _ any) ([]database.GroupMemberRBACHelper, error) { - users, err := q.db.GetGroupMembersByGroupID(ctx, id) - if err != nil { - return nil, err - } - groupMembers := make([]database.GroupMemberRBACHelper, len(users)) - for i, user := range users { - groupMembers[i] = database.GroupMemberRBACHelper{ - User: user, - GroupID: group.ID, - OrganizationID: group.OrganizationID, - } - } - return groupMembers, nil - } - groupMembers, err := fetchWithPostFilter(q.auth, policy.ActionRead, fetch)(ctx, nil) - if err != nil { - return nil, err - } - users := make([]database.User, len(groupMembers)) - for i, groupMember := range groupMembers { - users[i] = groupMember.User - } - return users, nil + return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id) } func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 0fd898709268d..d0548267028c4 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -183,20 +183,8 @@ func (g Group) RBACObject() rbac.Object { }) } -type GroupMemberRBACHelper struct { - User User - GroupID uuid.UUID - OrganizationID uuid.UUID -} - -func (gm GroupMemberRBACHelper) RBACObject() rbac.Object { - return rbac.ResourceGroup.WithID(gm.GroupID).InOrg(gm.OrganizationID). - // Group member can see they are in the group. - WithACLUserList(map[string][]policy.Action{ - gm.User.ID.String(): { - policy.ActionRead, - }, - }) +func (gm GroupMember) RBACObject() rbac.Object { + return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String()) } type GroupMembersCountRBACHelper struct { From 0aea7f8821ff9d432dcc8bae72a1fb9d89c2ea29 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 15:51:06 +0000 Subject: [PATCH 10/24] - make the GroupMember type contain all user fields - fix type issues coming from replacing User with GroupMember in group member queries --- coderd/apidoc/docs.go | 2 + coderd/apidoc/swagger.json | 2 + coderd/database/db2sdk/db2sdk.go | 33 ++++++++++++++- coderd/database/dbmem/dbmem.go | 42 ++++++++++++------- coderd/database/dump.sql | 15 ++++++- .../000241_group_members_view.up.sql | 15 ++++++- coderd/database/modelmethods.go | 18 ++++---- coderd/database/models.go | 25 ++++++++--- coderd/database/queries.sql.go | 34 +++++++++++++-- docs/api/members.md | 3 ++ docs/api/schemas.md | 1 + enterprise/coderd/groups.go | 4 +- enterprise/coderd/templates.go | 2 +- 13 files changed, 155 insertions(+), 41 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index b5d642086b63a..f91f34c3a28c9 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11610,6 +11610,7 @@ const docTemplate = `{ "deployment_stats", "file", "group", + "group_member", "license", "notification_preference", "notification_template", @@ -11640,6 +11641,7 @@ const docTemplate = `{ "ResourceDeploymentStats", "ResourceFile", "ResourceGroup", + "ResourceGroupMember", "ResourceLicense", "ResourceNotificationPreference", "ResourceNotificationTemplate", diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 02012129345f4..64d4f74027914 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10476,6 +10476,7 @@ "deployment_stats", "file", "group", + "group_member", "license", "notification_preference", "notification_template", @@ -10506,6 +10507,7 @@ "ResourceDeploymentStats", "ResourceFile", "ResourceGroup", + "ResourceGroupMember", "ResourceLicense", "ResourceNotificationPreference", "ResourceNotificationTemplate", diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index 4dd2bc86d5443..ed28585fe20e5 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -159,6 +159,35 @@ func ReducedUser(user database.User) codersdk.ReducedUser { } } +func UserFromGroupMember(member database.GroupMember) database.User { + return database.User{ + ID: member.UserID, + Email: member.UserEmail, + Username: member.UserUsername, + HashedPassword: member.UserHashedPassword, + CreatedAt: member.UserCreatedAt, + UpdatedAt: member.UserUpdatedAt, + Status: member.UserStatus, + RBACRoles: member.UserRbacRoles, + LoginType: member.UserLoginType, + AvatarURL: member.UserAvatarUrl, + Deleted: member.UserDeleted, + LastSeenAt: member.UserLastSeenAt, + QuietHoursSchedule: member.UserQuietHoursSchedule, + ThemePreference: member.UserThemePreference, + Name: member.UserName, + GithubComUserID: member.UserGithubComUserID, + } +} + +func ReducedUserFromGroupMember(member database.GroupMember) codersdk.ReducedUser { + return ReducedUser(UserFromGroupMember(member)) +} + +func ReducedUsersFromGroupMembers(members []database.GroupMember) []codersdk.ReducedUser { + return List(members, ReducedUserFromGroupMember) +} + func ReducedUsers(users []database.User) []codersdk.ReducedUser { return List(users, ReducedUser) } @@ -179,14 +208,14 @@ func Users(users []database.User, organizationIDs map[uuid.UUID][]uuid.UUID) []c }) } -func Group(group database.Group, members []database.User) codersdk.Group { +func Group(group database.Group, members []database.GroupMember) codersdk.Group { return codersdk.Group{ ID: group.ID, Name: group.Name, DisplayName: group.DisplayName, OrganizationID: group.OrganizationID, AvatarURL: group.AvatarURL, - Members: ReducedUsers(members), + Members: ReducedUsersFromGroupMembers(members), QuotaAllowance: int(group.QuotaAllowance), Source: codersdk.GroupSource(group.Source), } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 60b5ef9725455..a23ccb5faf1ea 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -724,9 +724,9 @@ func (q *FakeQuerier) getOrganizationMemberNoLock(orgID uuid.UUID) []database.Or } // getEveryoneGroupMembersNoLock fetches all the users in an organization. -func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database.User { +func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database.GroupMember { var ( - everyone []database.User + everyone []database.GroupMember orgMembers = q.getOrganizationMemberNoLock(orgID) ) for _, member := range orgMembers { @@ -734,7 +734,30 @@ func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database. if err != nil { return nil } - everyone = append(everyone, user) + if user.Deleted { + continue + } + everyone = append(everyone, database.GroupMember{ + UserID: user.ID, + UserEmail: user.Email, + UserUsername: user.Username, + UserHashedPassword: user.HashedPassword, + UserCreatedAt: user.CreatedAt, + UserUpdatedAt: user.UpdatedAt, + UserStatus: user.Status, + UserRbacRoles: user.RBACRoles, + UserLoginType: user.LoginType, + UserAvatarUrl: user.AvatarURL, + UserDeleted: user.Deleted, + UserLastSeenAt: user.LastSeenAt, + UserQuietHoursSchedule: user.QuietHoursSchedule, + UserThemePreference: user.ThemePreference, + UserName: user.Name, + UserGithubComUserID: user.GithubComUserID, + OrganizationID: orgID, + GroupName: "Everyone", + GroupID: orgID, + }) } return everyone } @@ -2510,18 +2533,7 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) } } - users := make([]database.User, 0, len(members)) - - for _, member := range members { - for _, user := range q.users { - if user.ID == member.UserID && !user.Deleted { - users = append(users, user) - break - } - } - } - - return users, nil + return members, nil } func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 72e243f599211..c8d360bdf4cae 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -609,8 +609,21 @@ CREATE VIEW group_members_expanded AS FROM organization_members ) SELECT users.id AS user_id, - users.username, + users.email AS user_email, + users.username AS user_username, + users.hashed_password AS user_hashed_password, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.status AS user_status, + users.rbac_roles AS user_rbac_roles, + users.login_type AS user_login_type, users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.last_seen_at AS user_last_seen_at, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + users.theme_preference AS user_theme_preference, + users.name AS user_name, + users.github_com_user_id AS user_github_com_user_id, groups.organization_id, groups.name AS group_name, all_members.group_id diff --git a/coderd/database/migrations/000241_group_members_view.up.sql b/coderd/database/migrations/000241_group_members_view.up.sql index c96503efdacac..bbc664f6dc6cb 100644 --- a/coderd/database/migrations/000241_group_members_view.up.sql +++ b/coderd/database/migrations/000241_group_members_view.up.sql @@ -10,8 +10,21 @@ WITH all_members AS ( ) SELECT users.id AS user_id, - users.username, + users.email AS user_email, + users.username AS user_username, + users.hashed_password AS user_hashed_password, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.status AS user_status, + users.rbac_roles AS user_rbac_roles, + users.login_type AS user_login_type, users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.last_seen_at AS user_last_seen_at, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + users.theme_preference AS user_theme_preference, + users.name AS user_name, + users.github_com_user_id AS user_github_com_user_id, groups.organization_id AS organization_id, groups.name AS group_name, all_members.group_id AS group_id diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index d0548267028c4..fd9774f11eb9b 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -76,18 +76,18 @@ func (m OrganizationMember) Auditable(username string) AuditableOrganizationMemb type AuditableGroup struct { Group - Members []GroupMember `json:"members"` + Members []GroupMemberTable `json:"members"` } // Auditable returns an object that can be used in audit logs. // Covers both group and group member changes. -func (g Group) Auditable(users []User) AuditableGroup { - members := make([]GroupMember, 0, len(users)) - for _, u := range users { - members = append(members, GroupMember{ - UserID: u.ID, - GroupID: g.ID, - }) +func (g Group) Auditable(members []GroupMember) AuditableGroup { + membersTable := make([]GroupMemberTable, len(members)) + for i, member := range members { + membersTable[i] = GroupMemberTable{ + UserID: member.UserID, + GroupID: member.GroupID, + } } // consistent ordering @@ -97,7 +97,7 @@ func (g Group) Auditable(users []User) AuditableGroup { return AuditableGroup{ Group: g, - Members: members, + Members: membersTable, } } diff --git a/coderd/database/models.go b/coderd/database/models.go index 8f735745cde29..a120d16cf9dc8 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2056,12 +2056,25 @@ type Group struct { // Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group). type GroupMember struct { - UserID uuid.UUID `db:"user_id" json:"user_id"` - Username string `db:"username" json:"username"` - UserAvatarUrl string `db:"user_avatar_url" json:"user_avatar_url"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - GroupName string `db:"group_name" json:"group_name"` - GroupID uuid.UUID `db:"group_id" json:"group_id"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + UserEmail string `db:"user_email" json:"user_email"` + UserUsername string `db:"user_username" json:"user_username"` + UserHashedPassword []byte `db:"user_hashed_password" json:"user_hashed_password"` + UserCreatedAt time.Time `db:"user_created_at" json:"user_created_at"` + UserUpdatedAt time.Time `db:"user_updated_at" json:"user_updated_at"` + UserStatus UserStatus `db:"user_status" json:"user_status"` + UserRbacRoles []string `db:"user_rbac_roles" json:"user_rbac_roles"` + UserLoginType LoginType `db:"user_login_type" json:"user_login_type"` + UserAvatarUrl string `db:"user_avatar_url" json:"user_avatar_url"` + UserDeleted bool `db:"user_deleted" json:"user_deleted"` + UserLastSeenAt time.Time `db:"user_last_seen_at" json:"user_last_seen_at"` + UserQuietHoursSchedule string `db:"user_quiet_hours_schedule" json:"user_quiet_hours_schedule"` + UserThemePreference string `db:"user_theme_preference" json:"user_theme_preference"` + UserName string `db:"user_name" json:"user_name"` + UserGithubComUserID sql.NullInt64 `db:"user_github_com_user_id" json:"user_github_com_user_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + GroupName string `db:"group_name" json:"group_name"` + GroupID uuid.UUID `db:"group_id" json:"group_id"` } type GroupMemberTable struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0a16c78ee0032..c32df9d55b27d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1322,7 +1322,7 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG } const getGroupMembers = `-- name: GetGroupMembers :many -SELECT user_id, username, user_avatar_url, organization_id, group_name, group_id FROM group_members_expanded +SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_theme_preference, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded ` func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) { @@ -1336,8 +1336,21 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) var i GroupMember if err := rows.Scan( &i.UserID, - &i.Username, + &i.UserEmail, + &i.UserUsername, + &i.UserHashedPassword, + &i.UserCreatedAt, + &i.UserUpdatedAt, + &i.UserStatus, + pq.Array(&i.UserRbacRoles), + &i.UserLoginType, &i.UserAvatarUrl, + &i.UserDeleted, + &i.UserLastSeenAt, + &i.UserQuietHoursSchedule, + &i.UserThemePreference, + &i.UserName, + &i.UserGithubComUserID, &i.OrganizationID, &i.GroupName, &i.GroupID, @@ -1356,7 +1369,7 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) } const getGroupMembersByGroupID = `-- name: GetGroupMembersByGroupID :many -SELECT user_id, username, user_avatar_url, organization_id, group_name, group_id FROM group_members_expanded WHERE group_id = $1 +SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_theme_preference, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded WHERE group_id = $1 ` func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) { @@ -1370,8 +1383,21 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. var i GroupMember if err := rows.Scan( &i.UserID, - &i.Username, + &i.UserEmail, + &i.UserUsername, + &i.UserHashedPassword, + &i.UserCreatedAt, + &i.UserUpdatedAt, + &i.UserStatus, + pq.Array(&i.UserRbacRoles), + &i.UserLoginType, &i.UserAvatarUrl, + &i.UserDeleted, + &i.UserLastSeenAt, + &i.UserQuietHoursSchedule, + &i.UserThemePreference, + &i.UserName, + &i.UserGithubComUserID, &i.OrganizationID, &i.GroupName, &i.GroupID, diff --git a/docs/api/members.md b/docs/api/members.md index 472d3dcc90d43..6c3a0a89841d1 100644 --- a/docs/api/members.md +++ b/docs/api/members.md @@ -189,6 +189,7 @@ Status Code **200** | `resource_type` | `deployment_stats` | | `resource_type` | `file` | | `resource_type` | `group` | +| `resource_type` | `group_member` | | `resource_type` | `license` | | `resource_type` | `notification_preference` | | `resource_type` | `notification_template` | @@ -346,6 +347,7 @@ Status Code **200** | `resource_type` | `deployment_stats` | | `resource_type` | `file` | | `resource_type` | `group` | +| `resource_type` | `group_member` | | `resource_type` | `license` | | `resource_type` | `notification_preference` | | `resource_type` | `notification_template` | @@ -728,6 +730,7 @@ Status Code **200** | `resource_type` | `deployment_stats` | | `resource_type` | `file` | | `resource_type` | `group` | +| `resource_type` | `group_member` | | `resource_type` | `license` | | `resource_type` | `notification_preference` | | `resource_type` | `notification_template` | diff --git a/docs/api/schemas.md b/docs/api/schemas.md index cbf7cbe23c773..70f14c1a01a70 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -4267,6 +4267,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `deployment_stats` | | `file` | | `group` | +| `group_member` | | `license` | | `notification_preference` | | `notification_template` | diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 06de4a26764e9..043533d50d6c5 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -77,8 +77,8 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) return } - var emptyUsers []database.User - aReq.New = group.Auditable(emptyUsers) + var emptyMembers []database.GroupMember + aReq.New = group.Auditable(emptyMembers) httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.Group(group, nil)) } diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 9531125d7ceb1..a8f8e736db4e2 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -121,7 +121,7 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { groups := make([]codersdk.TemplateGroup, 0, len(dbGroups)) for _, group := range dbGroups { - var members []database.User + var members []database.GroupMember // This is a bit of a hack. The caller might not have permission to do this, // but they can read the acl list if the function got this far. So we let From e5c5f3f8a9e03591bb45cd48a10e7d58ae95d1e1 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 16:20:45 +0000 Subject: [PATCH 11/24] simplify RBAC check on group member count --- coderd/database/dbauthz/dbauthz.go | 18 ++------------- coderd/database/dbmem/dbmem.go | 16 ++++++++++---- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 4 ++-- coderd/database/modelmethods.go | 27 +++++++++-------------- coderd/database/querier.go | 3 ++- coderd/database/queries.sql.go | 28 +++++++++++++++++++----- coderd/database/queries/groupmembers.sql | 12 +++++++++- enterprise/coderd/groups.go | 2 +- 9 files changed, 63 insertions(+), 49 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 6fd0c3b5765ca..f5b3e817b1846 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1400,22 +1400,8 @@ func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([ return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id) } -func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - group, err := q.GetGroupByID(ctx, groupID) - if err != nil { - return 0, err - } - memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID) - if err != nil { - return 0, err - } - if err := q.authorizeContext(ctx, policy.ActionRead, database.GroupMembersCountRBACHelper{ - GroupID: groupID, - OrganizationID: group.OrganizationID, - }); err != nil { - return 0, err - } - return memberCount, nil +func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { + return fetch(q.log, q.auth, q.db.GetGroupMembersCountByGroupID)(ctx, groupID) } func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index a23ccb5faf1ea..1e6c7d458dc3f 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2536,12 +2536,20 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) return members, nil } -func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - users, err := q.GetGroupMembersByGroupID(ctx, groupID) +func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { + members, err := q.GetGroupMembersByGroupID(ctx, groupID) if err != nil { - return 0, err + return database.GetGroupMembersCountByGroupIDRow{}, err + } + group, err := q.GetGroupByID(ctx, groupID) + if err != nil { + return database.GetGroupMembersCountByGroupIDRow{}, err } - return int64(len(users)), nil + return database.GetGroupMembersCountByGroupIDRow{ + OrganizationID: group.OrganizationID, + GroupID: groupID, + MemberCount: int64(len(members)), + }, nil } func (q *FakeQuerier) GetGroups(_ context.Context) ([]database.Group, error) { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index eb62316be1902..5476c09a6178a 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -655,7 +655,7 @@ func (m metricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid return users, err } -func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { +func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { start := time.Now() r0, r1 := m.s.GetGroupMembersCountByGroupID(ctx, groupID) m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupID").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 9920dafada324..aaa1d5b527257 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1300,10 +1300,10 @@ func (mr *MockStoreMockRecorder) GetGroupMembersByGroupID(arg0, arg1 any) *gomoc } // GetGroupMembersCountByGroupID mocks base method. -func (m *MockStore) GetGroupMembersCountByGroupID(arg0 context.Context, arg1 uuid.UUID) (int64, error) { +func (m *MockStore) GetGroupMembersCountByGroupID(arg0 context.Context, arg1 uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupID", arg0, arg1) - ret0, _ := ret[0].(int64) + ret0, _ := ret[0].(database.GetGroupMembersCountByGroupIDRow) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index fd9774f11eb9b..8bb3b96104ed1 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -172,34 +172,27 @@ func (v TemplateVersion) RBACObjectNoTemplate() rbac.Object { return rbac.ResourceTemplate.InOrg(v.OrganizationID) } -func (g Group) RBACObject() rbac.Object { - return rbac.ResourceGroup.WithID(g.ID). - InOrg(g.OrganizationID). +func groupRBACObject(groupID, organizationID uuid.UUID) rbac.Object { + return rbac.ResourceGroup.WithID(groupID). + InOrg(organizationID). // Group members can read the group. WithGroupACL(map[string][]policy.Action{ - g.ID.String(): { + groupID.String(): { policy.ActionRead, }, }) } -func (gm GroupMember) RBACObject() rbac.Object { - return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String()) +func (g Group) RBACObject() rbac.Object { + return groupRBACObject(g.ID, g.OrganizationID) } -type GroupMembersCountRBACHelper struct { - GroupID uuid.UUID - OrganizationID uuid.UUID +func (gm GroupMember) RBACObject() rbac.Object { + return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String()) } -func (r GroupMembersCountRBACHelper) RBACObject() rbac.Object { - return rbac.ResourceGroup.WithID(r.GroupID).InOrg(r.OrganizationID). - // Group members can read the member count. - WithGroupACL(map[string][]policy.Action{ - r.GroupID.String(): { - policy.ActionRead, - }, - }) +func (r GetGroupMembersCountByGroupIDRow) RBACObject() rbac.Object { + return groupRBACObject(r.GroupID, r.OrganizationID) } func (w GetWorkspaceByAgentIDRow) RBACObject() rbac.Object { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index cd3a0d466b7d6..e3dd909d2c0dc 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -147,7 +147,8 @@ type sqlcQuerier interface { GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) GetGroupMembers(ctx context.Context) ([]GroupMember, error) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) - GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) + // This aggregation is guaranteed to return a single row + GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) GetGroups(ctx context.Context) ([]Group, error) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c32df9d55b27d..f41a949be5cd3 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1416,14 +1416,30 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. } const getGroupMembersCountByGroupID = `-- name: GetGroupMembersCountByGroupID :one -SELECT COUNT(*) FROM group_members_expanded WHERE group_id = $1 -` +SELECT + gme.organization_id, + gme.group_id, + COUNT(*) as member_count +FROM + group_members_expanded gme +WHERE + gme.group_id = $1 +GROUP BY + gme.organization_id, gme.group_id +` + +type GetGroupMembersCountByGroupIDRow struct { + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + GroupID uuid.UUID `db:"group_id" json:"group_id"` + MemberCount int64 `db:"member_count" json:"member_count"` +} -func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { +// This aggregation is guaranteed to return a single row +func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) { row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) - var count int64 - err := row.Scan(&count) - return count, err + var i GetGroupMembersCountByGroupIDRow + err := row.Scan(&i.OrganizationID, &i.GroupID, &i.MemberCount) + return i, err } const insertGroupMember = `-- name: InsertGroupMember :exec diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 1cfe2d573626a..4b0613e248a91 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -5,7 +5,17 @@ SELECT * FROM group_members_expanded; SELECT * FROM group_members_expanded WHERE group_id = @group_id; -- name: GetGroupMembersCountByGroupID :one -SELECT COUNT(*) FROM group_members_expanded WHERE group_id = @group_id; +SELECT + gme.organization_id, + gme.group_id, + COUNT(*) as member_count +FROM + group_members_expanded gme +WHERE + gme.group_id = @group_id +-- This aggregation is guaranteed to return a single row +GROUP BY + gme.organization_id, gme.group_id; -- InsertUserGroupsByName adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByName :exec diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 043533d50d6c5..08ed74ae382c5 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -453,7 +453,7 @@ func (api *API) reducedGroupsByUserAndOrganization(rw http.ResponseWriter, r *ht return } - resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount))) + resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount.MemberCount))) } httpapi.Write(ctx, rw, http.StatusOK, resp) From 16e95d014e5160cf92475c8a3db30323a06482f5 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 16:39:44 +0000 Subject: [PATCH 12/24] add the MemberTotalCount field to codersdk.Group --- coderd/apidoc/docs.go | 4 + coderd/apidoc/swagger.json | 4 + coderd/database/db2sdk/db2sdk.go | 19 ++-- coderd/database/querier.go | 4 +- coderd/database/queries.sql.go | 4 +- coderd/database/queries/groupmembers.sql | 4 +- codersdk/groups.go | 10 +- docs/api/enterprise.md | 115 ++++++++++++----------- docs/api/schemas.md | 27 +++--- enterprise/coderd/groups.go | 25 ++++- enterprise/coderd/templates.go | 15 ++- site/src/api/typesGenerated.ts | 1 + 12 files changed, 146 insertions(+), 86 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index f91f34c3a28c9..55466326eea80 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -10219,6 +10219,10 @@ const docTemplate = `{ }, "source": { "$ref": "#/definitions/codersdk.GroupSource" + }, + "total_member_count": { + "description": "How many members are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than ` + "`" + `len(Group.Members)` + "`" + `.", + "type": "integer" } } }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 64d4f74027914..872399847fdde 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -9173,6 +9173,10 @@ }, "source": { "$ref": "#/definitions/codersdk.GroupSource" + }, + "total_member_count": { + "description": "How many members are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than `len(Group.Members)`.", + "type": "integer" } } }, diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index ed28585fe20e5..aacc1c91da311 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -208,16 +208,17 @@ func Users(users []database.User, organizationIDs map[uuid.UUID][]uuid.UUID) []c }) } -func Group(group database.Group, members []database.GroupMember) codersdk.Group { +func Group(group database.Group, members []database.GroupMember, totalMemberCount int) codersdk.Group { return codersdk.Group{ - ID: group.ID, - Name: group.Name, - DisplayName: group.DisplayName, - OrganizationID: group.OrganizationID, - AvatarURL: group.AvatarURL, - Members: ReducedUsersFromGroupMembers(members), - QuotaAllowance: int(group.QuotaAllowance), - Source: codersdk.GroupSource(group.Source), + ID: group.ID, + Name: group.Name, + DisplayName: group.DisplayName, + OrganizationID: group.OrganizationID, + AvatarURL: group.AvatarURL, + Members: ReducedUsersFromGroupMembers(members), + TotalMemberCount: totalMemberCount, + QuotaAllowance: int(group.QuotaAllowance), + Source: codersdk.GroupSource(group.Source), } } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index e3dd909d2c0dc..6fe8f0e74ef07 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -147,7 +147,9 @@ type sqlcQuerier interface { GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) GetGroupMembers(ctx context.Context) ([]GroupMember, error) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) - // This aggregation is guaranteed to return a single row + // Returns the total count of members in a group. Shows the total + // count even if the caller does not have read access to ResourceGroupMember. + // They only need ResourceGroup read access. GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) GetGroups(ctx context.Context) ([]Group, error) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f41a949be5cd3..25f6eec67d657 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1434,7 +1434,9 @@ type GetGroupMembersCountByGroupIDRow struct { MemberCount int64 `db:"member_count" json:"member_count"` } -// This aggregation is guaranteed to return a single row +// Returns the total count of members in a group. Shows the total +// count even if the caller does not have read access to ResourceGroupMember. +// They only need ResourceGroup read access. func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) { row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) var i GetGroupMembersCountByGroupIDRow diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 4b0613e248a91..3f2aa3ade62ef 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -5,6 +5,9 @@ SELECT * FROM group_members_expanded; SELECT * FROM group_members_expanded WHERE group_id = @group_id; -- name: GetGroupMembersCountByGroupID :one +-- Returns the total count of members in a group. Shows the total +-- count even if the caller does not have read access to ResourceGroupMember. +-- They only need ResourceGroup read access. SELECT gme.organization_id, gme.group_id, @@ -13,7 +16,6 @@ FROM group_members_expanded gme WHERE gme.group_id = @group_id --- This aggregation is guaranteed to return a single row GROUP BY gme.organization_id, gme.group_id; diff --git a/codersdk/groups.go b/codersdk/groups.go index e6fed666bdd72..d2cfafa814eb4 100644 --- a/codersdk/groups.go +++ b/codersdk/groups.go @@ -30,9 +30,13 @@ type Group struct { DisplayName string `json:"display_name"` OrganizationID uuid.UUID `json:"organization_id" format:"uuid"` Members []ReducedUser `json:"members"` - AvatarURL string `json:"avatar_url"` - QuotaAllowance int `json:"quota_allowance"` - Source GroupSource `json:"source"` + // How many members are in this group. Shows the total count, + // even if the user is not authorized to read group member details. + // May be greater than `len(Group.Members)`. + TotalMemberCount int `json:"total_member_count"` + AvatarURL string `json:"avatar_url"` + QuotaAllowance int `json:"quota_allowance"` + Source GroupSource `json:"source"` } type ReducedGroup struct { diff --git a/docs/api/enterprise.md b/docs/api/enterprise.md index fd49c3924d665..946f8280d6d97 100644 --- a/docs/api/enterprise.md +++ b/docs/api/enterprise.md @@ -219,7 +219,8 @@ curl -X GET http://coder-server:8080/api/v2/groups/{group} \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` @@ -277,7 +278,8 @@ curl -X DELETE http://coder-server:8080/api/v2/groups/{group} \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` @@ -350,7 +352,8 @@ curl -X PATCH http://coder-server:8080/api/v2/groups/{group} \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` @@ -1108,7 +1111,8 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/groups "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ] ``` @@ -1123,28 +1127,29 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/groups Status Code **200** -| Name | Type | Required | Restrictions | Description | -| --------------------- | ------------------------------------------------------ | -------- | ------------ | ----------- | -| `[array item]` | array | false | | | -| `» avatar_url` | string | false | | | -| `» display_name` | string | false | | | -| `» id` | string(uuid) | false | | | -| `» members` | array | false | | | -| `»» avatar_url` | string(uri) | false | | | -| `»» created_at` | string(date-time) | true | | | -| `»» email` | string(email) | true | | | -| `»» id` | string(uuid) | true | | | -| `»» last_seen_at` | string(date-time) | false | | | -| `»» login_type` | [codersdk.LoginType](schemas.md#codersdklogintype) | false | | | -| `»» name` | string | false | | | -| `»» status` | [codersdk.UserStatus](schemas.md#codersdkuserstatus) | false | | | -| `»» theme_preference` | string | false | | | -| `»» updated_at` | string(date-time) | false | | | -| `»» username` | string | true | | | -| `» name` | string | false | | | -| `» organization_id` | string(uuid) | false | | | -| `» quota_allowance` | integer | false | | | -| `» source` | [codersdk.GroupSource](schemas.md#codersdkgroupsource) | false | | | +| Name | Type | Required | Restrictions | Description | +| ---------------------- | ------------------------------------------------------ | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `[array item]` | array | false | | | +| `» avatar_url` | string | false | | | +| `» display_name` | string | false | | | +| `» id` | string(uuid) | false | | | +| `» members` | array | false | | | +| `»» avatar_url` | string(uri) | false | | | +| `»» created_at` | string(date-time) | true | | | +| `»» email` | string(email) | true | | | +| `»» id` | string(uuid) | true | | | +| `»» last_seen_at` | string(date-time) | false | | | +| `»» login_type` | [codersdk.LoginType](schemas.md#codersdklogintype) | false | | | +| `»» name` | string | false | | | +| `»» status` | [codersdk.UserStatus](schemas.md#codersdkuserstatus) | false | | | +| `»» theme_preference` | string | false | | | +| `»» updated_at` | string(date-time) | false | | | +| `»» username` | string | true | | | +| `» name` | string | false | | | +| `» organization_id` | string(uuid) | false | | | +| `» quota_allowance` | integer | false | | | +| `» source` | [codersdk.GroupSource](schemas.md#codersdkgroupsource) | false | | | +| `» total_member_count` | integer | false | | How many members are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | #### Enumerated Values @@ -1222,7 +1227,8 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/groups "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` @@ -1281,7 +1287,8 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/groups/ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` @@ -2046,7 +2053,8 @@ curl -X GET http://coder-server:8080/api/v2/templates/{template}/acl/available \ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ], "users": [ @@ -2078,30 +2086,31 @@ curl -X GET http://coder-server:8080/api/v2/templates/{template}/acl/available \ Status Code **200** -| Name | Type | Required | Restrictions | Description | -| ---------------------- | ------------------------------------------------------ | -------- | ------------ | ----------- | -| `[array item]` | array | false | | | -| `» groups` | array | false | | | -| `»» avatar_url` | string | false | | | -| `»» display_name` | string | false | | | -| `»» id` | string(uuid) | false | | | -| `»» members` | array | false | | | -| `»»» avatar_url` | string(uri) | false | | | -| `»»» created_at` | string(date-time) | true | | | -| `»»» email` | string(email) | true | | | -| `»»» id` | string(uuid) | true | | | -| `»»» last_seen_at` | string(date-time) | false | | | -| `»»» login_type` | [codersdk.LoginType](schemas.md#codersdklogintype) | false | | | -| `»»» name` | string | false | | | -| `»»» status` | [codersdk.UserStatus](schemas.md#codersdkuserstatus) | false | | | -| `»»» theme_preference` | string | false | | | -| `»»» updated_at` | string(date-time) | false | | | -| `»»» username` | string | true | | | -| `»» name` | string | false | | | -| `»» organization_id` | string(uuid) | false | | | -| `»» quota_allowance` | integer | false | | | -| `»» source` | [codersdk.GroupSource](schemas.md#codersdkgroupsource) | false | | | -| `» users` | array | false | | | +| Name | Type | Required | Restrictions | Description | +| ----------------------- | ------------------------------------------------------ | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `[array item]` | array | false | | | +| `» groups` | array | false | | | +| `»» avatar_url` | string | false | | | +| `»» display_name` | string | false | | | +| `»» id` | string(uuid) | false | | | +| `»» members` | array | false | | | +| `»»» avatar_url` | string(uri) | false | | | +| `»»» created_at` | string(date-time) | true | | | +| `»»» email` | string(email) | true | | | +| `»»» id` | string(uuid) | true | | | +| `»»» last_seen_at` | string(date-time) | false | | | +| `»»» login_type` | [codersdk.LoginType](schemas.md#codersdklogintype) | false | | | +| `»»» name` | string | false | | | +| `»»» status` | [codersdk.UserStatus](schemas.md#codersdkuserstatus) | false | | | +| `»»» theme_preference` | string | false | | | +| `»»» updated_at` | string(date-time) | false | | | +| `»»» username` | string | true | | | +| `»» name` | string | false | | | +| `»» organization_id` | string(uuid) | false | | | +| `»» quota_allowance` | integer | false | | | +| `»» source` | [codersdk.GroupSource](schemas.md#codersdkgroupsource) | false | | | +| `»» total_member_count` | integer | false | | How many members are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | +| `» users` | array | false | | | #### Enumerated Values diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 70f14c1a01a70..2d1e27e5c1778 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -246,7 +246,8 @@ "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ], "users": [ @@ -2806,22 +2807,24 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "name": "string", "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "quota_allowance": 0, - "source": "user" + "source": "user", + "total_member_count": 0 } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -| ----------------- | ----------------------------------------------------- | -------- | ------------ | ----------- | -| `avatar_url` | string | false | | | -| `display_name` | string | false | | | -| `id` | string | false | | | -| `members` | array of [codersdk.ReducedUser](#codersdkreduceduser) | false | | | -| `name` | string | false | | | -| `organization_id` | string | false | | | -| `quota_allowance` | integer | false | | | -| `source` | [codersdk.GroupSource](#codersdkgroupsource) | false | | | +| Name | Type | Required | Restrictions | Description | +| -------------------- | ----------------------------------------------------- | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `avatar_url` | string | false | | | +| `display_name` | string | false | | | +| `id` | string | false | | | +| `members` | array of [codersdk.ReducedUser](#codersdkreduceduser) | false | | | +| `name` | string | false | | | +| `organization_id` | string | false | | | +| `quota_allowance` | integer | false | | | +| `source` | [codersdk.GroupSource](#codersdkgroupsource) | false | | | +| `total_member_count` | integer | false | | How many members are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | ## codersdk.GroupSource diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 08ed74ae382c5..ad01c90537085 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -80,7 +80,7 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) var emptyMembers []database.GroupMember aReq.New = group.Auditable(emptyMembers) - httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.Group(group, nil)) + httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.Group(group, nil, 0)) } // @Summary Update group by name @@ -285,7 +285,13 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { aReq.New = group.Auditable(patchedMembers) - httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, patchedMembers)) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, patchedMembers, int(memberCount.MemberCount))) } // @Summary Delete group by name @@ -370,7 +376,13 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, users)) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, users, int(memberCount.MemberCount))) } // @Summary Get groups by organization @@ -414,8 +426,13 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { httpapi.InternalServerError(rw, err) return } + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } - resp = append(resp, db2sdk.Group(group, members)) + resp = append(resp, db2sdk.Group(group, members, int(memberCount.MemberCount))) } httpapi.Write(ctx, rw, http.StatusOK, resp) diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index a8f8e736db4e2..66f725b85139d 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -64,8 +64,13 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req httpapi.InternalServerError(rw, err) return } + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } - sdkGroups = append(sdkGroups, db2sdk.Group(group, members)) + sdkGroups = append(sdkGroups, db2sdk.Group(group, members, int(memberCount.MemberCount))) } httpapi.Write(ctx, rw, http.StatusOK, codersdk.ACLAvailable{ @@ -133,8 +138,14 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { httpapi.InternalServerError(rw, err) return } + // nolint:gocritic + memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } groups = append(groups, codersdk.TemplateGroup{ - Group: db2sdk.Group(group.Group, members), + Group: db2sdk.Group(group.Group, members, int(memberCount.MemberCount)), Role: convertToTemplateRole(group.Actions), }) } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index d239444988328..89ce7f04c8270 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -613,6 +613,7 @@ export interface Group { readonly display_name: string; readonly organization_id: string; readonly members: readonly ReducedUser[]; + readonly total_member_count: number; readonly avatar_url: string; readonly quota_allowance: number; readonly source: GroupSource; From d3b4d7cfb9b8dd58223597df08dc082a49b05d21 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 16:53:21 +0000 Subject: [PATCH 13/24] remove the reduced groups endpoint, revert frontend to use the groups by org endpoint --- coderd/apidoc/docs.go | 70 ------------------- coderd/apidoc/swagger.json | 66 ----------------- coderd/database/db2sdk/db2sdk.go | 11 --- codersdk/groups.go | 12 ---- docs/api/enterprise.md | 59 ---------------- docs/api/schemas.md | 24 ------- enterprise/coderd/coderd.go | 9 --- enterprise/coderd/groups.go | 38 ---------- site/src/api/api.ts | 15 ---- site/src/api/queries/groups.ts | 8 --- site/src/api/typesGenerated.ts | 10 --- .../AccountPage/AccountPage.tsx | 4 +- .../AccountPage/AccountUserGroups.stories.tsx | 25 +++---- .../AccountPage/AccountUserGroups.tsx | 8 +-- site/src/testHelpers/entities.ts | 2 + 15 files changed, 17 insertions(+), 344 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 55466326eea80..3c51e33e3e5a4 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -3256,50 +3256,6 @@ const docTemplate = `{ } } }, - "/organizations/{organization}/users/{user}/reduced-groups": { - "get": { - "security": [ - { - "CoderSessionToken": [] - } - ], - "produces": [ - "application/json" - ], - "tags": [ - "Enterprise" - ], - "summary": "Get reduced groups for a user in an organization", - "operationId": "get-reduced-groups-by-user-and-organization", - "parameters": [ - { - "type": "string", - "description": "Organization ID", - "name": "organization", - "in": "path", - "required": true - }, - { - "type": "string", - "description": "User ID, name, or me", - "name": "user", - "in": "path", - "required": true - } - ], - "responses": { - "200": { - "description": "OK", - "schema": { - "type": "array", - "items": { - "$ref": "#/definitions/codersdk.ReducedGroup" - } - } - } - } - } - }, "/regions": { "get": { "security": [ @@ -11677,32 +11633,6 @@ const docTemplate = `{ } } }, - "codersdk.ReducedGroup": { - "type": "object", - "properties": { - "avatar_url": { - "type": "string" - }, - "display_name": { - "type": "string" - }, - "id": { - "type": "string", - "format": "uuid" - }, - "name": { - "type": "string" - }, - "organization_id": { - "type": "string", - "format": "uuid" - }, - "total_member_count": { - "description": "How many users are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than ` + "`" + `len(Group.Members)` + "`" + `.", - "type": "integer" - } - } - }, "codersdk.ReducedUser": { "type": "object", "required": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 872399847fdde..bf4dc370aa3be 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2864,46 +2864,6 @@ } } }, - "/organizations/{organization}/users/{user}/reduced-groups": { - "get": { - "security": [ - { - "CoderSessionToken": [] - } - ], - "produces": ["application/json"], - "tags": ["Enterprise"], - "summary": "Get reduced groups for a user in an organization", - "operationId": "get-reduced-groups-by-user-and-organization", - "parameters": [ - { - "type": "string", - "description": "Organization ID", - "name": "organization", - "in": "path", - "required": true - }, - { - "type": "string", - "description": "User ID, name, or me", - "name": "user", - "in": "path", - "required": true - } - ], - "responses": { - "200": { - "description": "OK", - "schema": { - "type": "array", - "items": { - "$ref": "#/definitions/codersdk.ReducedGroup" - } - } - } - } - } - }, "/regions": { "get": { "security": [ @@ -10543,32 +10503,6 @@ } } }, - "codersdk.ReducedGroup": { - "type": "object", - "properties": { - "avatar_url": { - "type": "string" - }, - "display_name": { - "type": "string" - }, - "id": { - "type": "string", - "format": "uuid" - }, - "name": { - "type": "string" - }, - "organization_id": { - "type": "string", - "format": "uuid" - }, - "total_member_count": { - "description": "How many users are in this group. Shows the total count,\neven if the user is not authorized to read group member details.\nMay be greater than `len(Group.Members)`.", - "type": "integer" - } - } - }, "codersdk.ReducedUser": { "type": "object", "required": ["created_at", "email", "id", "username"], diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index aacc1c91da311..1d513e75aff47 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -222,17 +222,6 @@ func Group(group database.Group, members []database.GroupMember, totalMemberCoun } } -func ReducedGroup(group database.Group, totalMemberCount int) codersdk.ReducedGroup { - return codersdk.ReducedGroup{ - ID: group.ID, - Name: group.Name, - DisplayName: group.DisplayName, - OrganizationID: group.OrganizationID, - AvatarURL: group.AvatarURL, - TotalMemberCount: totalMemberCount, - } -} - func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) { // Use a stable sort, similarly to how we would sort in the query, note that // we don't sort in the query because order varies depending on the table diff --git a/codersdk/groups.go b/codersdk/groups.go index d2cfafa814eb4..fdd7217200738 100644 --- a/codersdk/groups.go +++ b/codersdk/groups.go @@ -39,18 +39,6 @@ type Group struct { Source GroupSource `json:"source"` } -type ReducedGroup struct { - ID uuid.UUID `json:"id" format:"uuid"` - Name string `json:"name"` - DisplayName string `json:"display_name"` - OrganizationID uuid.UUID `json:"organization_id" format:"uuid"` - // How many users are in this group. Shows the total count, - // even if the user is not authorized to read group member details. - // May be greater than `len(Group.Members)`. - TotalMemberCount int `json:"total_member_count"` - AvatarURL string `json:"avatar_url"` -} - func (g Group) IsEveryone() bool { return g.ID == g.OrganizationID } diff --git a/docs/api/enterprise.md b/docs/api/enterprise.md index 946f8280d6d97..c0d7ff7a852c7 100644 --- a/docs/api/enterprise.md +++ b/docs/api/enterprise.md @@ -1518,65 +1518,6 @@ curl -X DELETE http://coder-server:8080/api/v2/organizations/{organization}/prov To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Get reduced groups for a user in an organization - -### Code samples - -```shell -# Example request using curl -curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/users/{user}/reduced-groups \ - -H 'Accept: application/json' \ - -H 'Coder-Session-Token: API_KEY' -``` - -`GET /organizations/{organization}/users/{user}/reduced-groups` - -### Parameters - -| Name | In | Type | Required | Description | -| -------------- | ---- | ------ | -------- | -------------------- | -| `organization` | path | string | true | Organization ID | -| `user` | path | string | true | User ID, name, or me | - -### Example responses - -> 200 Response - -```json -[ - { - "avatar_url": "string", - "display_name": "string", - "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string", - "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", - "total_member_count": 0 - } -] -``` - -### Responses - -| Status | Meaning | Description | Schema | -| ------ | ------------------------------------------------------- | ----------- | ----------------------------------------------------------------- | -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | array of [codersdk.ReducedGroup](schemas.md#codersdkreducedgroup) | - -

Response Schema

- -Status Code **200** - -| Name | Type | Required | Restrictions | Description | -| ---------------------- | ------------ | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `[array item]` | array | false | | | -| `» avatar_url` | string | false | | | -| `» display_name` | string | false | | | -| `» id` | string(uuid) | false | | | -| `» name` | string | false | | | -| `» organization_id` | string(uuid) | false | | | -| `» total_member_count` | integer | false | | How many users are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | - -To perform this operation, you must be authenticated. [Learn more](authentication.md). - ## Get active replicas ### Code samples diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 2d1e27e5c1778..a7e5120a1b338 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -4306,30 +4306,6 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `api` | integer | false | | | | `disable_all` | boolean | false | | | -## codersdk.ReducedGroup - -```json -{ - "avatar_url": "string", - "display_name": "string", - "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", - "name": "string", - "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", - "total_member_count": 0 -} -``` - -### Properties - -| Name | Type | Required | Restrictions | Description | -| -------------------- | ------- | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `avatar_url` | string | false | | | -| `display_name` | string | false | | | -| `id` | string | false | | | -| `name` | string | false | | | -| `organization_id` | string | false | | | -| `total_member_count` | integer | false | | How many users are in this group. Shows the total count, even if the user is not authorized to read group member details. May be greater than `len(Group.Members)`. | - ## codersdk.ReducedUser ```json diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 955fb6d18593f..501e6086eaeb3 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -288,15 +288,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { r.Get("/", api.groupByOrganization) }) }) - r.Route("/organizations/{organization}/users/{user}/reduced-groups", func(r chi.Router) { - r.Use( - apiKeyMiddleware, - api.templateRBACEnabledMW, - httpmw.ExtractOrganizationParam(api.Database), - httpmw.ExtractUserParam(options.Database), - ) - r.Get("/", api.reducedGroupsByUserAndOrganization) - }) r.Route("/organizations/{organization}/provisionerkeys", func(r chi.Router) { r.Use( apiKeyMiddleware, diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index ad01c90537085..7735432e80801 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -437,41 +437,3 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp) } - -// @Summary Get reduced groups for a user in an organization -// @ID get-reduced-groups-by-user-and-organization -// @Security CoderSessionToken -// @Produce json -// @Tags Enterprise -// @Param organization path string true "Organization ID" -// @Param user path string true "User ID, name, or me" -// @Success 200 {array} codersdk.ReducedGroup -// @Router /organizations/{organization}/users/{user}/reduced-groups [get] -func (api *API) reducedGroupsByUserAndOrganization(rw http.ResponseWriter, r *http.Request) { - var ( - ctx = r.Context() - user = httpmw.UserParam(r) - org = httpmw.OrganizationParam(r) - ) - - groups, err := api.Database.GetGroupsByOrganizationAndUserID(ctx, database.GetGroupsByOrganizationAndUserIDParams{ - OrganizationID: org.ID, - UserID: user.ID, - }) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - httpapi.InternalServerError(rw, err) - return - } - resp := make([]codersdk.ReducedGroup, 0, len(groups)) - for _, group := range groups { - memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) - if err != nil { - httpapi.InternalServerError(rw, err) - return - } - - resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount.MemberCount))) - } - - httpapi.Write(ctx, rw, http.StatusOK, resp) -} diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 4c357ee83a7de..397f5e0378d75 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1619,21 +1619,6 @@ class ApiMethods { return response.data; }; - /** - * @param organization Can be the organization's ID or name - * @param user Can be the user's ID, username, or "me" - */ - getReducedGroupsForUser = async ( - organization: string, - user: string, - ): Promise => { - const response = await this.axios.get( - `/api/v2/organizations/${organization}/users/${user}/reduced-groups`, - ); - - return response.data; - }; - addMember = async (groupId: string, userId: string) => { return this.patchGroup(groupId, { name: "", diff --git a/site/src/api/queries/groups.ts b/site/src/api/queries/groups.ts index 3bc375b8f6ce4..ed670c5d17a9d 100644 --- a/site/src/api/queries/groups.ts +++ b/site/src/api/queries/groups.ts @@ -4,7 +4,6 @@ import type { CreateGroupRequest, Group, PatchGroupRequest, - ReducedGroup, } from "api/typesGenerated"; type GroupSortOrder = "asc" | "desc"; @@ -126,13 +125,6 @@ export const deleteGroup = (queryClient: QueryClient) => { }; }; -export function reducedGroupsForUser(organization: string, userId: string) { - return { - queryKey: ["organization", organization, "user", userId, "reduced-groups"], - queryFn: () => API.getReducedGroupsForUser(organization, userId), - } as const satisfies UseQueryOptions; -} - export const addMember = (queryClient: QueryClient) => { return { mutationFn: ({ groupId, userId }: { groupId: string; userId: string }) => diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 89ce7f04c8270..370b4f1f90c76 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1067,16 +1067,6 @@ export interface RateLimitConfig { readonly api: number; } -// From codersdk/groups.go -export interface ReducedGroup { - readonly id: string; - readonly name: string; - readonly display_name: string; - readonly organization_id: string; - readonly total_member_count: number; - readonly avatar_url: string; -} - // From codersdk/users.go export interface ReducedUser extends MinimalUser { readonly name: string; diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 6b72c4f5551c4..642c4389ff579 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -1,6 +1,6 @@ import type { FC } from "react"; import { useQuery } from "react-query"; -import { reducedGroupsForUser } from "api/queries/groups"; +import { groupsForUser } from "api/queries/groups"; import { Stack } from "components/Stack/Stack"; import { useAuthContext } from "contexts/auth/AuthProvider"; import { useAuthenticated } from "contexts/auth/RequireAuth"; @@ -18,7 +18,7 @@ export const AccountPage: FC = () => { const hasGroupsFeature = entitlements.features.user_role_management.enabled; const groupsQuery = useQuery({ // TODO: This should probably list all groups, not just default org groups - ...reducedGroupsForUser("default", me.id), + ...groupsForUser("default", me.id), enabled: hasGroupsFeature, }); diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx index 5cb4b6dc631b8..6bdbbbd3d2b7d 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.stories.tsx @@ -1,24 +1,17 @@ import type { Meta, StoryObj } from "@storybook/react"; -import type { ReducedGroup } from "api/typesGenerated"; +import type { Group } from "api/typesGenerated"; import { MockGroup as MockGroup1, + MockUser, mockApiError, } from "testHelpers/entities"; import { AccountUserGroups } from "./AccountUserGroups"; -const MockReducedGroup1: ReducedGroup = { - id: MockGroup1.id, - name: MockGroup1.name, - display_name: MockGroup1.display_name, - organization_id: MockGroup1.organization_id, - avatar_url: MockGroup1.avatar_url, - total_member_count: 10, -}; - -const MockReducedGroup2: ReducedGroup = { - ...MockReducedGroup1, +const MockGroup2: Group = { + ...MockGroup1, + avatar_url: "", display_name: "Goofy Goobers", - total_member_count: 5, + members: [MockUser], }; const mockError = mockApiError({ @@ -29,7 +22,7 @@ const meta: Meta = { title: "pages/UserSettingsPage/AccountUserGroups", component: AccountUserGroups, args: { - groups: [MockReducedGroup1, MockReducedGroup2], + groups: [MockGroup1, MockGroup2], loading: false, }, }; @@ -47,7 +40,7 @@ export const NoGroups: Story = { export const OneGroup: Story = { args: { - groups: [MockReducedGroup1], + groups: [MockGroup1], }, }; @@ -68,7 +61,7 @@ export const Error: Story = { export const ErrorWithPreviousData: Story = { args: { - groups: [MockReducedGroup1, MockReducedGroup2], + groups: [MockGroup1, MockGroup2], error: mockError, loading: false, }, diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx index 1ab01060205d1..bc544fe1fab30 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx @@ -2,14 +2,14 @@ import { useTheme } from "@emotion/react"; import Grid from "@mui/material/Grid"; import type { FC } from "react"; import { isApiError } from "api/errors"; -import type { ReducedGroup } from "api/typesGenerated"; +import type { Group } from "api/typesGenerated"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { AvatarCard } from "components/AvatarCard/AvatarCard"; import { Loader } from "components/Loader/Loader"; import { Section } from "../Section"; type AccountGroupsProps = { - groups: readonly ReducedGroup[] | undefined; + groups: readonly Group[] | undefined; error: unknown; loading: boolean; }; @@ -57,8 +57,8 @@ export const AccountUserGroups: FC = ({ header={group.display_name || group.name} subtitle={ <> - {group.total_member_count} member - {group.total_member_count !== 1 && "s"} + {group.members.length} member + {group.members.length !== 1 && "s"} } /> diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 288bd3d708fb7..75faf858f1cc7 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -2499,6 +2499,7 @@ export const MockGroup: TypesGen.Group = { members: [MockUser, MockUser2], quota_allowance: 5, source: "user", + total_member_count: 0, }; const everyOneGroup = (organizationId: string): TypesGen.Group => ({ @@ -2510,6 +2511,7 @@ const everyOneGroup = (organizationId: string): TypesGen.Group => ({ avatar_url: "", quota_allowance: 0, source: "user", + total_member_count: 0, }); export const MockTemplateACL: TypesGen.TemplateACL = { From b06f943bd8c76a5440f7e613e0421b33fba5808b Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 17:00:29 +0000 Subject: [PATCH 14/24] revert "simplify RBAC check on group member count" - new query didn't work on empty groups --- coderd/database/dbauthz/dbauthz.go | 18 ++++++++++++++-- coderd/database/dbmem/dbmem.go | 16 ++++---------- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 4 ++-- coderd/database/modelmethods.go | 7 +++++- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 27 ++++++------------------ coderd/database/queries/groupmembers.sql | 11 +--------- enterprise/coderd/groups.go | 6 +++--- enterprise/coderd/templates.go | 4 ++-- 10 files changed, 42 insertions(+), 55 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index f5b3e817b1846..6fd0c3b5765ca 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1400,8 +1400,22 @@ func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([ return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id) } -func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { - return fetch(q.log, q.auth, q.db.GetGroupMembersCountByGroupID)(ctx, groupID) +func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + group, err := q.GetGroupByID(ctx, groupID) + if err != nil { + return 0, err + } + memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID) + if err != nil { + return 0, err + } + if err := q.authorizeContext(ctx, policy.ActionRead, database.GroupMembersCountRBACHelper{ + GroupID: groupID, + OrganizationID: group.OrganizationID, + }); err != nil { + return 0, err + } + return memberCount, nil } func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 1e6c7d458dc3f..a23ccb5faf1ea 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2536,20 +2536,12 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) return members, nil } -func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { - members, err := q.GetGroupMembersByGroupID(ctx, groupID) +func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { + users, err := q.GetGroupMembersByGroupID(ctx, groupID) if err != nil { - return database.GetGroupMembersCountByGroupIDRow{}, err - } - group, err := q.GetGroupByID(ctx, groupID) - if err != nil { - return database.GetGroupMembersCountByGroupIDRow{}, err + return 0, err } - return database.GetGroupMembersCountByGroupIDRow{ - OrganizationID: group.OrganizationID, - GroupID: groupID, - MemberCount: int64(len(members)), - }, nil + return int64(len(users)), nil } func (q *FakeQuerier) GetGroups(_ context.Context) ([]database.Group, error) { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 5476c09a6178a..eb62316be1902 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -655,7 +655,7 @@ func (m metricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid return users, err } -func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { +func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { start := time.Now() r0, r1 := m.s.GetGroupMembersCountByGroupID(ctx, groupID) m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupID").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index aaa1d5b527257..9920dafada324 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1300,10 +1300,10 @@ func (mr *MockStoreMockRecorder) GetGroupMembersByGroupID(arg0, arg1 any) *gomoc } // GetGroupMembersCountByGroupID mocks base method. -func (m *MockStore) GetGroupMembersCountByGroupID(arg0 context.Context, arg1 uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) { +func (m *MockStore) GetGroupMembersCountByGroupID(arg0 context.Context, arg1 uuid.UUID) (int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupID", arg0, arg1) - ret0, _ := ret[0].(database.GetGroupMembersCountByGroupIDRow) + ret0, _ := ret[0].(int64) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 8bb3b96104ed1..90d9e009fd926 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -191,7 +191,12 @@ func (gm GroupMember) RBACObject() rbac.Object { return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String()) } -func (r GetGroupMembersCountByGroupIDRow) RBACObject() rbac.Object { +type GroupMembersCountRBACHelper struct { + GroupID uuid.UUID + OrganizationID uuid.UUID +} + +func (r GroupMembersCountRBACHelper) RBACObject() rbac.Object { return groupRBACObject(r.GroupID, r.OrganizationID) } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 6fe8f0e74ef07..9ddbf2a74ddf6 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -150,7 +150,7 @@ type sqlcQuerier interface { // Returns the total count of members in a group. Shows the total // count even if the caller does not have read access to ResourceGroupMember. // They only need ResourceGroup read access. - GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) + GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) GetGroups(ctx context.Context) ([]Group, error) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 25f6eec67d657..2de986f9471f9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1416,32 +1416,17 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. } const getGroupMembersCountByGroupID = `-- name: GetGroupMembersCountByGroupID :one -SELECT - gme.organization_id, - gme.group_id, - COUNT(*) as member_count -FROM - group_members_expanded gme -WHERE - gme.group_id = $1 -GROUP BY - gme.organization_id, gme.group_id -` - -type GetGroupMembersCountByGroupIDRow struct { - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - GroupID uuid.UUID `db:"group_id" json:"group_id"` - MemberCount int64 `db:"member_count" json:"member_count"` -} +SELECT COUNT(*) FROM group_members_expanded WHERE group_id = $1 +` // Returns the total count of members in a group. Shows the total // count even if the caller does not have read access to ResourceGroupMember. // They only need ResourceGroup read access. -func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (GetGroupMembersCountByGroupIDRow, error) { +func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) - var i GetGroupMembersCountByGroupIDRow - err := row.Scan(&i.OrganizationID, &i.GroupID, &i.MemberCount) - return i, err + var count int64 + err := row.Scan(&count) + return count, err } const insertGroupMember = `-- name: InsertGroupMember :exec diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 3f2aa3ade62ef..0ef2c72323cc9 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -8,16 +8,7 @@ SELECT * FROM group_members_expanded WHERE group_id = @group_id; -- Returns the total count of members in a group. Shows the total -- count even if the caller does not have read access to ResourceGroupMember. -- They only need ResourceGroup read access. -SELECT - gme.organization_id, - gme.group_id, - COUNT(*) as member_count -FROM - group_members_expanded gme -WHERE - gme.group_id = @group_id -GROUP BY - gme.organization_id, gme.group_id; +SELECT COUNT(*) FROM group_members_expanded WHERE group_id = @group_id; -- InsertUserGroupsByName adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByName :exec diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 7735432e80801..c6fdd67cd029b 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -291,7 +291,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, patchedMembers, int(memberCount.MemberCount))) + httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, patchedMembers, int(memberCount))) } // @Summary Delete group by name @@ -382,7 +382,7 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, users, int(memberCount.MemberCount))) + httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Group(group, users, int(memberCount))) } // @Summary Get groups by organization @@ -432,7 +432,7 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { return } - resp = append(resp, db2sdk.Group(group, members, int(memberCount.MemberCount))) + resp = append(resp, db2sdk.Group(group, members, int(memberCount))) } httpapi.Write(ctx, rw, http.StatusOK, resp) diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 66f725b85139d..fb61f0b3c494d 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -70,7 +70,7 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req return } - sdkGroups = append(sdkGroups, db2sdk.Group(group, members, int(memberCount.MemberCount))) + sdkGroups = append(sdkGroups, db2sdk.Group(group, members, int(memberCount))) } httpapi.Write(ctx, rw, http.StatusOK, codersdk.ACLAvailable{ @@ -145,7 +145,7 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { return } groups = append(groups, codersdk.TemplateGroup{ - Group: db2sdk.Group(group.Group, members, int(memberCount.MemberCount)), + Group: db2sdk.Group(group.Group, members, int(memberCount)), Role: convertToTemplateRole(group.Actions), }) } From 4a658743615a4b92c8a55a64f260dea3ae4c2793 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 17:02:18 +0000 Subject: [PATCH 15/24] display `group.total_member_count` instead of `group.members.length` on the account page --- .../pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx index bc544fe1fab30..daccaa2dc01b8 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountUserGroups.tsx @@ -57,8 +57,8 @@ export const AccountUserGroups: FC = ({ header={group.display_name || group.name} subtitle={ <> - {group.members.length} member - {group.members.length !== 1 && "s"} + {group.total_member_count} member + {group.total_member_count !== 1 && "s"} } /> From 1f4dcc791a79fa7df504fa5605f7686ca1e4522d Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 17:09:49 +0000 Subject: [PATCH 16/24] adjust `total_member_count` on `MockGroup` --- site/src/testHelpers/entities.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 75faf858f1cc7..ab665709fed45 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -2499,7 +2499,7 @@ export const MockGroup: TypesGen.Group = { members: [MockUser, MockUser2], quota_allowance: 5, source: "user", - total_member_count: 0, + total_member_count: 2, }; const everyOneGroup = (organizationId: string): TypesGen.Group => ({ From f1513e0e4c62a4c958575c0de6a82c4f2fc45f65 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 17:22:42 +0000 Subject: [PATCH 17/24] fixes after rebase --- ..._members_view.down.sql => 000242_group_members_view.down.sql} | 0 ...roup_members_view.up.sql => 000242_group_members_view.up.sql} | 0 docs/api/members.md | 1 + 3 files changed, 1 insertion(+) rename coderd/database/migrations/{000241_group_members_view.down.sql => 000242_group_members_view.down.sql} (100%) rename coderd/database/migrations/{000241_group_members_view.up.sql => 000242_group_members_view.up.sql} (100%) diff --git a/coderd/database/migrations/000241_group_members_view.down.sql b/coderd/database/migrations/000242_group_members_view.down.sql similarity index 100% rename from coderd/database/migrations/000241_group_members_view.down.sql rename to coderd/database/migrations/000242_group_members_view.down.sql diff --git a/coderd/database/migrations/000241_group_members_view.up.sql b/coderd/database/migrations/000242_group_members_view.up.sql similarity index 100% rename from coderd/database/migrations/000241_group_members_view.up.sql rename to coderd/database/migrations/000242_group_members_view.up.sql diff --git a/docs/api/members.md b/docs/api/members.md index 6c3a0a89841d1..cecb22340fe99 100644 --- a/docs/api/members.md +++ b/docs/api/members.md @@ -474,6 +474,7 @@ Status Code **200** | `resource_type` | `deployment_stats` | | `resource_type` | `file` | | `resource_type` | `group` | +| `resource_type` | `group_member` | | `resource_type` | `license` | | `resource_type` | `notification_preference` | | `resource_type` | `notification_template` | From 9ef0e0d5b51a0a2bde91901425c48329ec58e1e3 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 9 Aug 2024 17:37:57 +0000 Subject: [PATCH 18/24] simplify RBAC check on GetGroupMembersCountByGroupID --- coderd/database/dbauthz/dbauthz.go | 9 +-------- coderd/database/modelmethods.go | 21 ++++----------------- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 6fd0c3b5765ca..282de1faccc22 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1401,20 +1401,13 @@ func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([ } func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - group, err := q.GetGroupByID(ctx, groupID) - if err != nil { + if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check return 0, err } memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID) if err != nil { return 0, err } - if err := q.authorizeContext(ctx, policy.ActionRead, database.GroupMembersCountRBACHelper{ - GroupID: groupID, - OrganizationID: group.OrganizationID, - }); err != nil { - return 0, err - } return memberCount, nil } diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 90d9e009fd926..ac6d7e656d4d6 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -172,34 +172,21 @@ func (v TemplateVersion) RBACObjectNoTemplate() rbac.Object { return rbac.ResourceTemplate.InOrg(v.OrganizationID) } -func groupRBACObject(groupID, organizationID uuid.UUID) rbac.Object { - return rbac.ResourceGroup.WithID(groupID). - InOrg(organizationID). +func (g Group) RBACObject() rbac.Object { + return rbac.ResourceGroup.WithID(g.ID). + InOrg(g.OrganizationID). // Group members can read the group. WithGroupACL(map[string][]policy.Action{ - groupID.String(): { + g.ID.String(): { policy.ActionRead, }, }) } -func (g Group) RBACObject() rbac.Object { - return groupRBACObject(g.ID, g.OrganizationID) -} - func (gm GroupMember) RBACObject() rbac.Object { return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String()) } -type GroupMembersCountRBACHelper struct { - GroupID uuid.UUID - OrganizationID uuid.UUID -} - -func (r GroupMembersCountRBACHelper) RBACObject() rbac.Object { - return groupRBACObject(r.GroupID, r.OrganizationID) -} - func (w GetWorkspaceByAgentIDRow) RBACObject() rbac.Object { return w.Workspace.RBACObject() } From 7627933034e292159b4e0df95be48e705c2f265a Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Mon, 12 Aug 2024 15:28:54 +0000 Subject: [PATCH 19/24] fix tests --- coderd/database/dbauthz/dbauthz_test.go | 18 +-- coderd/database/dbgen/dbgen.go | 44 +++++++- coderd/database/dbgen/dbgen_test.go | 4 +- coderd/database/dbmem/dbmem.go | 139 +++++++++++++++++------- coderd/rbac/roles_test.go | 41 +++++-- coderd/telemetry/telemetry_test.go | 9 +- enterprise/coderd/groups_test.go | 13 +++ 7 files changed, 203 insertions(+), 65 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 4eb764fde57b1..2d41d71466d16 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -305,7 +305,7 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("DeleteGroupMemberFromGroup", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) - m := dbgen.GroupMember(s.T(), db, database.GroupMember{ + m := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{ GroupID: g.ID, }) check.Args(database.DeleteGroupMemberFromGroupParams{ @@ -326,11 +326,15 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("GetGroupMembersByGroupID", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) - _ = dbgen.GroupMember(s.T(), db, database.GroupMember{}) + gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID}) + check.Args(g.ID).Asserts(gm, policy.ActionRead) + })) + s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) { + g := dbgen.Group(s.T(), db, database.Group{}) check.Args(g.ID).Asserts(g, policy.ActionRead) })) s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) { - _ = dbgen.GroupMember(s.T(), db, database.GroupMember{}) + dbgen.GroupMember(s.T(), db, database.GroupMemberTable{}) check.Asserts(rbac.ResourceSystem, policy.ActionRead) })) s.Run("GetGroups", s.Subtest(func(db database.Store, check *expects) { @@ -339,7 +343,7 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("GetGroupsByOrganizationAndUserID", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) - gm := dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g.ID}) + gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID}) check.Args(database.GetGroupsByOrganizationAndUserIDParams{ OrganizationID: g.OrganizationID, UserID: gm.UserID, @@ -368,7 +372,7 @@ func (s *MethodTestSuite) TestGroup() { u1 := dbgen.User(s.T(), db, database.User{}) g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) - _ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID}) + _ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g1.ID, UserID: u1.ID}) check.Args(database.InsertUserGroupsByNameParams{ OrganizationID: o.ID, UserID: u1.ID, @@ -380,8 +384,8 @@ func (s *MethodTestSuite) TestGroup() { u1 := dbgen.User(s.T(), db, database.User{}) g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID}) - _ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID}) - _ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g2.ID, UserID: u1.ID}) + _ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g1.ID, UserID: u1.ID}) + _ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g2.ID, UserID: u1.ID}) check.Args(u1.ID).Asserts(rbac.ResourceSystem, policy.ActionUpdate).Returns() })) s.Run("UpdateGroupByID", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index a6ca57662e28d..81bd3a908cd6c 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -6,6 +6,7 @@ import ( "database/sql" "encoding/hex" "encoding/json" + "errors" "fmt" "net" "strings" @@ -374,8 +375,8 @@ func Group(t testing.TB, db database.Store, orig database.Group) database.Group return group } -func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) database.GroupMember { - member := database.GroupMember{ +func GroupMember(t testing.TB, db database.Store, orig database.GroupMemberTable) database.GroupMember { + member := database.GroupMemberTable{ UserID: takeFirst(orig.UserID, uuid.New()), GroupID: takeFirst(orig.GroupID, uuid.New()), } @@ -385,7 +386,44 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat GroupID: member.GroupID, }) require.NoError(t, err, "insert group member") - return member + + user, err := db.GetUserByID(genCtx, member.UserID) + if errors.Is(err, sql.ErrNoRows) { + user = User(t, db, database.User{ID: member.UserID}) + } else { + require.NoError(t, err, "get user by id") + } + + group, err := db.GetGroupByID(genCtx, member.GroupID) + if errors.Is(err, sql.ErrNoRows) { + group = Group(t, db, database.Group{ID: member.GroupID}) + } else { + require.NoError(t, err, "get group by id") + } + + groupMember := database.GroupMember{ + UserID: user.ID, + UserEmail: user.Email, + UserUsername: user.Username, + UserHashedPassword: user.HashedPassword, + UserCreatedAt: user.CreatedAt, + UserUpdatedAt: user.UpdatedAt, + UserStatus: user.Status, + UserRbacRoles: user.RBACRoles, + UserLoginType: user.LoginType, + UserAvatarUrl: user.AvatarURL, + UserDeleted: user.Deleted, + UserLastSeenAt: user.LastSeenAt, + UserQuietHoursSchedule: user.QuietHoursSchedule, + UserThemePreference: user.ThemePreference, + UserName: user.Name, + UserGithubComUserID: user.GithubComUserID, + OrganizationID: group.OrganizationID, + GroupName: group.Name, + GroupID: group.ID, + } + + return groupMember } // ProvisionerJob is a bit more involved to get the values such as "completedAt", "startedAt", "cancelledAt" set. ps diff --git a/coderd/database/dbgen/dbgen_test.go b/coderd/database/dbgen/dbgen_test.go index 5f9c235f312db..04f6d38d70d00 100644 --- a/coderd/database/dbgen/dbgen_test.go +++ b/coderd/database/dbgen/dbgen_test.go @@ -102,8 +102,8 @@ func TestGenerator(t *testing.T) { db := dbmem.New() g := dbgen.Group(t, db, database.Group{}) u := dbgen.User(t, db, database.User{}) - exp := []database.User{u} - dbgen.GroupMember(t, db, database.GroupMember{GroupID: g.ID, UserID: u.ID}) + gm := dbgen.GroupMember(t, db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) + exp := []database.GroupMember{gm} require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), g.ID))) }) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index a23ccb5faf1ea..e56e611ff6a58 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -60,7 +60,7 @@ func New() database.Store { dbcryptKeys: make([]database.DBCryptKey, 0), externalAuthLinks: make([]database.ExternalAuthLink, 0), groups: make([]database.Group, 0), - groupMembers: make([]database.GroupMember, 0), + groupMembers: make([]database.GroupMemberTable, 0), auditLogs: make([]database.AuditLog, 0), files: make([]database.File, 0), gitSSHKey: make([]database.GitSSHKey, 0), @@ -156,7 +156,7 @@ type data struct { files []database.File externalAuthLinks []database.ExternalAuthLink gitSSHKey []database.GitSSHKey - groupMembers []database.GroupMember + groupMembers []database.GroupMemberTable groups []database.Group jfrogXRayScans []database.JfrogXrayScan licenses []database.License @@ -723,41 +723,68 @@ func (q *FakeQuerier) getOrganizationMemberNoLock(orgID uuid.UUID) []database.Or return members } +var ErrUserDeleted = xerrors.New("user deleted") + +// getGroupMemberNoLock fetches a group member by user ID and group ID. +func (q *FakeQuerier) getGroupMemberNoLock(ctx context.Context, userID, groupID uuid.UUID) (database.GroupMember, error) { + groupName := "Everyone" + orgID := groupID + groupIsEveryone := q.isEveryoneGroup(groupID) + if !groupIsEveryone { + group, err := q.getGroupByIDNoLock(ctx, groupID) + if err != nil { + return database.GroupMember{}, err + } + groupName = group.Name + orgID = group.OrganizationID + } + + user, err := q.getUserByIDNoLock(userID) + if err != nil { + return database.GroupMember{}, err + } + if user.Deleted { + return database.GroupMember{}, ErrUserDeleted + } + + return database.GroupMember{ + UserID: user.ID, + UserEmail: user.Email, + UserUsername: user.Username, + UserHashedPassword: user.HashedPassword, + UserCreatedAt: user.CreatedAt, + UserUpdatedAt: user.UpdatedAt, + UserStatus: user.Status, + UserRbacRoles: user.RBACRoles, + UserLoginType: user.LoginType, + UserAvatarUrl: user.AvatarURL, + UserDeleted: user.Deleted, + UserLastSeenAt: user.LastSeenAt, + UserQuietHoursSchedule: user.QuietHoursSchedule, + UserThemePreference: user.ThemePreference, + UserName: user.Name, + UserGithubComUserID: user.GithubComUserID, + OrganizationID: orgID, + GroupName: groupName, + GroupID: groupID, + }, nil +} + // getEveryoneGroupMembersNoLock fetches all the users in an organization. -func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database.GroupMember { +func (q *FakeQuerier) getEveryoneGroupMembersNoLock(ctx context.Context, orgID uuid.UUID) []database.GroupMember { var ( everyone []database.GroupMember orgMembers = q.getOrganizationMemberNoLock(orgID) ) for _, member := range orgMembers { - user, err := q.getUserByIDNoLock(member.UserID) + groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, orgID) + if errors.Is(err, ErrUserDeleted) { + continue + } if err != nil { return nil } - if user.Deleted { - continue - } - everyone = append(everyone, database.GroupMember{ - UserID: user.ID, - UserEmail: user.Email, - UserUsername: user.Username, - UserHashedPassword: user.HashedPassword, - UserCreatedAt: user.CreatedAt, - UserUpdatedAt: user.UpdatedAt, - UserStatus: user.Status, - UserRbacRoles: user.RBACRoles, - UserLoginType: user.LoginType, - UserAvatarUrl: user.AvatarURL, - UserDeleted: user.Deleted, - UserLastSeenAt: user.LastSeenAt, - UserQuietHoursSchedule: user.QuietHoursSchedule, - UserThemePreference: user.ThemePreference, - UserName: user.Name, - UserGithubComUserID: user.GithubComUserID, - OrganizationID: orgID, - GroupName: "Everyone", - GroupID: orgID, - }) + everyone = append(everyone, groupMember) } return everyone } @@ -2509,31 +2536,59 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr return database.Group{}, sql.ErrNoRows } -func (q *FakeQuerier) GetGroupMembers(_ context.Context) ([]database.GroupMember, error) { +func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() - out := make([]database.GroupMember, len(q.groupMembers)) - copy(out, q.groupMembers) - return out, nil + members := make([]database.GroupMemberTable, 0, len(q.groupMembers)) + members = append(members, q.groupMembers...) + for _, org := range q.organizations { + for _, user := range q.users { + members = append(members, database.GroupMemberTable{ + UserID: user.ID, + GroupID: org.ID, + }) + } + } + + var groupMembers []database.GroupMember + for _, member := range members { + groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID) + if errors.Is(err, ErrUserDeleted) { + continue + } + if err != nil { + return nil, err + } + groupMembers = append(groupMembers, groupMember) + } + + return groupMembers, nil } -func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID) ([]database.GroupMember, error) { +func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() if q.isEveryoneGroup(id) { - return q.getEveryoneGroupMembersNoLock(id), nil + return q.getEveryoneGroupMembersNoLock(ctx, id), nil } - var members []database.GroupMember + var groupMembers []database.GroupMember for _, member := range q.groupMembers { + groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID) + if errors.Is(err, ErrUserDeleted) { + continue + } + if err != nil { + return nil, err + } if member.GroupID == id { - members = append(members, member) + groupMembers = append(groupMembers, groupMember) } } - return members, nil + return groupMembers, nil } func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { @@ -2561,15 +2616,15 @@ func (q *FakeQuerier) GetGroupsByOrganizationAndUserID(_ context.Context, arg da q.mutex.RLock() defer q.mutex.RUnlock() - var groupIds []uuid.UUID + var groupIDs []uuid.UUID for _, member := range q.groupMembers { if member.UserID == arg.UserID { - groupIds = append(groupIds, member.GroupID) + groupIDs = append(groupIDs, member.GroupID) } } groups := []database.Group{} for _, group := range q.groups { - if slices.Contains(groupIds, group.ID) && group.OrganizationID == arg.OrganizationID { + if slices.Contains(groupIDs, group.ID) && group.OrganizationID == arg.OrganizationID { groups = append(groups, group) } } @@ -6254,7 +6309,7 @@ func (q *FakeQuerier) InsertGroupMember(_ context.Context, arg database.InsertGr } //nolint:gosimple - q.groupMembers = append(q.groupMembers, database.GroupMember{ + q.groupMembers = append(q.groupMembers, database.GroupMemberTable{ GroupID: arg.GroupID, UserID: arg.UserID, }) @@ -6794,7 +6849,7 @@ func (q *FakeQuerier) InsertUserGroupsByName(_ context.Context, arg database.Ins } for _, groupID := range groupIDs { - q.groupMembers = append(q.groupMembers, database.GroupMember{ + q.groupMembers = append(q.groupMembers, database.GroupMemberTable{ UserID: arg.UserID, GroupID: groupID, }) diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index 9d71629d95d9f..b3c81564ff941 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -112,6 +112,7 @@ func TestRolePermissions(t *testing.T) { // Subjects to user memberMe := authSubject{Name: "member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember()}}} orgMemberMe := authSubject{Name: "org_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}}} + groupMemberMe := authSubject{Name: "group_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}, Groups: []string{groupID.String()}}} owner := authSubject{Name: "owner", Actor: rbac.Subject{ID: adminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleOwner()}}} templateAdmin := authSubject{Name: "template-admin", Actor: rbac.Subject{ID: templateAdminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleTemplateAdmin()}}} @@ -382,21 +383,47 @@ func TestRolePermissions(t *testing.T) { }, }, { - Name: "Groups", - Actions: []policy.Action{policy.ActionCreate, policy.ActionDelete, policy.ActionUpdate}, - Resource: rbac.ResourceGroup.WithID(groupID).InOrg(orgID), + Name: "Groups", + Actions: []policy.Action{policy.ActionCreate, policy.ActionDelete, policy.ActionUpdate}, + Resource: rbac.ResourceGroup.WithID(groupID).InOrg(orgID).WithGroupACL(map[string][]policy.Action{ + groupID.String(): { + policy.ActionRead, + }, + }), AuthorizeMap: map[bool][]hasAuthSubjects{ true: {owner, orgAdmin, userAdmin, orgUserAdmin}, - false: {setOtherOrg, memberMe, orgMemberMe, templateAdmin, orgTemplateAdmin, orgAuditor}, + false: {setOtherOrg, memberMe, orgMemberMe, templateAdmin, orgTemplateAdmin, orgAuditor, groupMemberMe}, + }, + }, + { + Name: "GroupsRead", + Actions: []policy.Action{policy.ActionRead}, + Resource: rbac.ResourceGroup.WithID(groupID).InOrg(orgID).WithGroupACL(map[string][]policy.Action{ + groupID.String(): { + policy.ActionRead, + }, + }), + AuthorizeMap: map[bool][]hasAuthSubjects{ + true: {owner, orgAdmin, userAdmin, templateAdmin, orgTemplateAdmin, orgUserAdmin, groupMemberMe}, + false: {setOtherOrg, memberMe, orgMemberMe, orgAuditor}, }, }, { - Name: "GroupsRead", + Name: "GroupMemberMeRead", Actions: []policy.Action{policy.ActionRead}, - Resource: rbac.ResourceGroup.WithID(groupID).InOrg(orgID), + Resource: rbac.ResourceGroupMember.WithID(currentUser).InOrg(orgID).WithOwner(currentUser.String()), + AuthorizeMap: map[bool][]hasAuthSubjects{ + true: {owner, orgAdmin, userAdmin, templateAdmin, orgTemplateAdmin, orgUserAdmin, orgMemberMe, groupMemberMe}, + false: {setOtherOrg, memberMe, orgAuditor}, + }, + }, + { + Name: "GroupMemberOtherRead", + Actions: []policy.Action{policy.ActionRead}, + Resource: rbac.ResourceGroupMember.WithID(adminID).InOrg(orgID).WithOwner(adminID.String()), AuthorizeMap: map[bool][]hasAuthSubjects{ true: {owner, orgAdmin, userAdmin, templateAdmin, orgTemplateAdmin, orgUserAdmin}, - false: {setOtherOrg, memberMe, orgMemberMe, orgAuditor}, + false: {setOtherOrg, memberMe, orgAuditor, orgMemberMe, groupMemberMe}, }, }, { diff --git a/coderd/telemetry/telemetry_test.go b/coderd/telemetry/telemetry_test.go index 2eff919ddc63d..fd9f4752bff51 100644 --- a/coderd/telemetry/telemetry_test.go +++ b/coderd/telemetry/telemetry_test.go @@ -49,14 +49,14 @@ func TestTelemetry(t *testing.T) { Provisioner: database.ProvisionerTypeTerraform, }) _ = dbgen.TemplateVersion(t, db, database.TemplateVersion{}) - _ = dbgen.User(t, db, database.User{}) + user := dbgen.User(t, db, database.User{}) _ = dbgen.Workspace(t, db, database.Workspace{}) _ = dbgen.WorkspaceApp(t, db, database.WorkspaceApp{ SharingLevel: database.AppSharingLevelOwner, Health: database.WorkspaceAppHealthDisabled, }) - _ = dbgen.Group(t, db, database.Group{}) - _ = dbgen.GroupMember(t, db, database.GroupMember{}) + group := dbgen.Group(t, db, database.Group{}) + _ = dbgen.GroupMember(t, db, database.GroupMemberTable{UserID: user.ID, GroupID: group.ID}) wsagent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{}) // Update the workspace agent to have a valid subsystem. err = db.UpdateWorkspaceAgentStartupByID(ctx, database.UpdateWorkspaceAgentStartupByIDParams{ @@ -94,7 +94,8 @@ func TestTelemetry(t *testing.T) { require.Len(t, snapshot.TemplateVersions, 1) require.Len(t, snapshot.Users, 1) require.Len(t, snapshot.Groups, 2) - require.Len(t, snapshot.GroupMembers, 1) + // 1 member in the everyone group + 1 member in the custom group + require.Len(t, snapshot.GroupMembers, 2) require.Len(t, snapshot.Workspaces, 1) require.Len(t, snapshot.WorkspaceApps, 1) require.Len(t, snapshot.WorkspaceAgents, 1) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 4d84a24601b1a..3b27fc25cdb83 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "net/http" + "sort" "testing" "github.com/google/uuid" @@ -567,6 +568,12 @@ func TestPatchGroup(t *testing.T) { }) } +func sortGroupMembers(group *codersdk.Group) { + sort.Slice(group.Members, func(i, j int) bool { + return group.Members[i].ID.String() < group.Members[j].ID.String() + }) +} + // TODO: test auth. func TestGroup(t *testing.T) { t.Parallel() @@ -638,6 +645,9 @@ func TestGroup(t *testing.T) { ggroup, err := userAdminClient.Group(ctx, group.ID) require.NoError(t, err) + sortGroupMembers(&group) + sortGroupMembers(&ggroup) + require.Equal(t, group, ggroup) }) @@ -820,6 +830,9 @@ func TestGroups(t *testing.T) { groups, err := userAdminClient.GroupsByOrganization(ctx, user.OrganizationID) require.NoError(t, err) + for _, group := range append(groups, group1, group2) { + sortGroupMembers(&group) + } // 'Everyone' group + 2 custom groups. require.Len(t, groups, 3) require.Contains(t, groups, group1) From 1080b29d32b05577e8e4ab0ad1c36bbde3393014 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Mon, 12 Aug 2024 19:31:12 +0000 Subject: [PATCH 20/24] resolve lint error --- enterprise/coderd/groups_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 3b27fc25cdb83..67281bafcef71 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -830,8 +830,8 @@ func TestGroups(t *testing.T) { groups, err := userAdminClient.GroupsByOrganization(ctx, user.OrganizationID) require.NoError(t, err) - for _, group := range append(groups, group1, group2) { - sortGroupMembers(&group) + for i := range append(groups, group1, group2) { + sortGroupMembers(&groups[i]) } // 'Everyone' group + 2 custom groups. require.Len(t, groups, 3) From 19486da8d710e772d4309c55869acfdc3a88444d Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Mon, 12 Aug 2024 20:11:43 +0000 Subject: [PATCH 21/24] update the groupsauth test to work with new group member permissions --- coderd/database/dbauthz/groupsauth_test.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/coderd/database/dbauthz/groupsauth_test.go b/coderd/database/dbauthz/groupsauth_test.go index d742e15148fa9..a72c4db3af38a 100644 --- a/coderd/database/dbauthz/groupsauth_test.go +++ b/coderd/database/dbauthz/groupsauth_test.go @@ -115,18 +115,15 @@ func TestGroupsAuth(t *testing.T) { Name: "GroupMember", Subject: rbac.Subject{ ID: users[0].ID.String(), - Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(org.ID)}.Expand())), + Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID)}.Expand())), Groups: []string{ - group.Name, + group.ID.String(), }, Scope: rbac.ExpandableScope(rbac.ScopeAll), }, - // TODO: currently group members cannot see their own groups. - // If this is fixed, these booleans should be flipped to true. - ReadGroup: false, - ReadMembers: false, - // TODO: If fixed, they should only be able to see themselves - // MembersExpected: 1, + ReadGroup: true, + ReadMembers: true, + MembersExpected: 1, }, { // Org admin in the incorrect organization @@ -160,8 +157,7 @@ func TestGroupsAuth(t *testing.T) { require.NoError(t, err, "member read") require.Len(t, members, tc.MembersExpected, "member count found does not match") } else { - require.Error(t, err, "member read") - require.True(t, dbauthz.IsNotAuthorizedError(err), "not authorized error") + require.Len(t, members, 0, "member count is not 0") } }) } From 5373dd7d69f7d96f4aa18f6487389ea7fe86cd58 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Mon, 12 Aug 2024 20:11:58 +0000 Subject: [PATCH 22/24] fix a mistake in test --- enterprise/coderd/groups_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 67281bafcef71..73e114f6239d3 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -830,9 +830,14 @@ func TestGroups(t *testing.T) { groups, err := userAdminClient.GroupsByOrganization(ctx, user.OrganizationID) require.NoError(t, err) - for i := range append(groups, group1, group2) { - sortGroupMembers(&groups[i]) + + // sort group members so we can compare them + allGroups := append([]codersdk.Group{}, groups...) + allGroups = append(allGroups, group1, group2) + for i := range allGroups { + sortGroupMembers(&allGroups[i]) } + // 'Everyone' group + 2 custom groups. require.Len(t, groups, 3) require.Contains(t, groups, group1) From 08646da3a8fefbb9e4512a2bae90ff43b8bb885a Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 13 Aug 2024 08:39:45 +0000 Subject: [PATCH 23/24] make ErrUserDeleted private --- coderd/database/dbmem/dbmem.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index e56e611ff6a58..9bbc531dfda9e 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -723,7 +723,7 @@ func (q *FakeQuerier) getOrganizationMemberNoLock(orgID uuid.UUID) []database.Or return members } -var ErrUserDeleted = xerrors.New("user deleted") +var errUserDeleted = xerrors.New("user deleted") // getGroupMemberNoLock fetches a group member by user ID and group ID. func (q *FakeQuerier) getGroupMemberNoLock(ctx context.Context, userID, groupID uuid.UUID) (database.GroupMember, error) { @@ -744,7 +744,7 @@ func (q *FakeQuerier) getGroupMemberNoLock(ctx context.Context, userID, groupID return database.GroupMember{}, err } if user.Deleted { - return database.GroupMember{}, ErrUserDeleted + return database.GroupMember{}, errUserDeleted } return database.GroupMember{ @@ -778,7 +778,7 @@ func (q *FakeQuerier) getEveryoneGroupMembersNoLock(ctx context.Context, orgID u ) for _, member := range orgMembers { groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, orgID) - if errors.Is(err, ErrUserDeleted) { + if errors.Is(err, errUserDeleted) { continue } if err != nil { @@ -2554,7 +2554,7 @@ func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMemb var groupMembers []database.GroupMember for _, member := range members { groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID) - if errors.Is(err, ErrUserDeleted) { + if errors.Is(err, errUserDeleted) { continue } if err != nil { @@ -2577,7 +2577,7 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID var groupMembers []database.GroupMember for _, member := range q.groupMembers { groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID) - if errors.Is(err, ErrUserDeleted) { + if errors.Is(err, errUserDeleted) { continue } if err != nil { From 4ec6adb680d9e667fbf3aa1053b79782ba8e7db3 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 13 Aug 2024 08:40:51 +0000 Subject: [PATCH 24/24] `dbgen.GroupMember` now fails when a supplied user or group doesn't exist --- coderd/database/dbauthz/dbauthz_test.go | 12 ++++++++--- coderd/database/dbgen/dbgen.go | 28 +++++++++++++++---------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 2d41d71466d16..d186b789eabf1 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -305,8 +305,10 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("DeleteGroupMemberFromGroup", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) + u := dbgen.User(s.T(), db, database.User{}) m := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{ GroupID: g.ID, + UserID: u.ID, }) check.Args(database.DeleteGroupMemberFromGroupParams{ UserID: m.UserID, @@ -326,7 +328,8 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("GetGroupMembersByGroupID", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) - gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID}) + u := dbgen.User(s.T(), db, database.User{}) + gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) check.Args(g.ID).Asserts(gm, policy.ActionRead) })) s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) { @@ -334,7 +337,9 @@ func (s *MethodTestSuite) TestGroup() { check.Args(g.ID).Asserts(g, policy.ActionRead) })) s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) { - dbgen.GroupMember(s.T(), db, database.GroupMemberTable{}) + g := dbgen.Group(s.T(), db, database.Group{}) + u := dbgen.User(s.T(), db, database.User{}) + dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) check.Asserts(rbac.ResourceSystem, policy.ActionRead) })) s.Run("GetGroups", s.Subtest(func(db database.Store, check *expects) { @@ -343,7 +348,8 @@ func (s *MethodTestSuite) TestGroup() { })) s.Run("GetGroupsByOrganizationAndUserID", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) - gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID}) + u := dbgen.User(s.T(), db, database.User{}) + gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) check.Args(database.GetGroupsByOrganizationAndUserIDParams{ OrganizationID: g.OrganizationID, UserID: gm.UserID, diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 81bd3a908cd6c..ab7cb9bf1b5a3 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -375,11 +375,19 @@ func Group(t testing.TB, db database.Store, orig database.Group) database.Group return group } -func GroupMember(t testing.TB, db database.Store, orig database.GroupMemberTable) database.GroupMember { - member := database.GroupMemberTable{ - UserID: takeFirst(orig.UserID, uuid.New()), - GroupID: takeFirst(orig.GroupID, uuid.New()), - } +// GroupMember requires a user + group to already exist. +// Example for creating a group member for a random group + user. +// +// GroupMember(t, db, database.GroupMemberTable{ +// UserID: User(t, db, database.User{}).ID, +// GroupID: Group(t, db, database.Group{ +// OrganizationID: must(db.GetDefaultOrganization(genCtx)).ID, +// }).ID, +// }) +func GroupMember(t testing.TB, db database.Store, member database.GroupMemberTable) database.GroupMember { + require.NotEqual(t, member.UserID, uuid.Nil, "A user id is required to use 'dbgen.GroupMember', use 'dbgen.User'.") + require.NotEqual(t, member.GroupID, uuid.Nil, "A group id is required to use 'dbgen.GroupMember', use 'dbgen.Group'.") + //nolint:gosimple err := db.InsertGroupMember(genCtx, database.InsertGroupMemberParams{ UserID: member.UserID, @@ -389,17 +397,15 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMemberTable user, err := db.GetUserByID(genCtx, member.UserID) if errors.Is(err, sql.ErrNoRows) { - user = User(t, db, database.User{ID: member.UserID}) - } else { - require.NoError(t, err, "get user by id") + require.NoErrorf(t, err, "'dbgen.GroupMember' failed as the user with id %s does not exist. A user is required to use this function, use 'dbgen.User'.", member.UserID) } + require.NoError(t, err, "get user by id") group, err := db.GetGroupByID(genCtx, member.GroupID) if errors.Is(err, sql.ErrNoRows) { - group = Group(t, db, database.Group{ID: member.GroupID}) - } else { - require.NoError(t, err, "get group by id") + require.NoErrorf(t, err, "'dbgen.GroupMember' failed as the group with id %s does not exist. A group is required to use this function, use 'dbgen.Group'.", member.GroupID) } + require.NoError(t, err, "get group by id") groupMember := database.GroupMember{ UserID: user.ID,