diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 54add36baea..a1cbbb5f20a 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -4863,6 +4863,7 @@ const docTemplate = `{ }, "/api/v2/organizations/{organization}/members": { "get": { + "description": "Deprecated: use GET /api/v2/organizations/{organization}/paginated-members instead.", "produces": [ "application/json" ], @@ -4897,6 +4898,53 @@ const docTemplate = `{ "CoderSessionToken": [] } ] + }, + "post": { + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "Members" + ], + "summary": "Batch add organization members", + "operationId": "batch-add-organization-members", + "parameters": [ + { + "type": "string", + "description": "Organization ID", + "name": "organization", + "in": "path", + "required": true + }, + { + "description": "Add members request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.AddOrganizationMembersRequest" + } + } + ], + "responses": { + "201": { + "description": "Created", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.OrganizationMember" + } + } + } + }, + "security": [ + { + "CoderSessionToken": [] + } + ] } }, "/api/v2/organizations/{organization}/members/roles": { @@ -5119,6 +5167,7 @@ const docTemplate = `{ ] }, "post": { + "description": "Deprecated: use POST /api/v2/organizations/{organization}/members instead.", "produces": [ "application/json" ], @@ -5127,6 +5176,7 @@ const docTemplate = `{ ], "summary": "Add organization member", "operationId": "add-organization-member", + "deprecated": true, "parameters": [ { "type": "string", @@ -16063,6 +16113,24 @@ const docTemplate = `{ } } }, + "codersdk.AddOrganizationMembersRequest": { + "type": "object", + "required": [ + "user_ids" + ], + "properties": { + "user_ids": { + "description": "UserIDs is the list of user IDs to add as organization members. The\nslice must contain between 1 and 100 IDs. The upper bound keeps a single\nbatch insert and its audit fan-out bounded; callers adding more members\nshould page the request.", + "type": "array", + "maxItems": 100, + "minItems": 1, + "items": { + "type": "string", + "format": "uuid" + } + } + } + }, "codersdk.AgentChatSendShortcut": { "type": "string", "enum": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 5687248ce4f..08e1b074cae 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4296,6 +4296,7 @@ }, "/api/v2/organizations/{organization}/members": { "get": { + "description": "Deprecated: use GET /api/v2/organizations/{organization}/paginated-members instead.", "produces": ["application/json"], "tags": ["Members"], "summary": "List organization members", @@ -4326,6 +4327,47 @@ "CoderSessionToken": [] } ] + }, + "post": { + "consumes": ["application/json"], + "produces": ["application/json"], + "tags": ["Members"], + "summary": "Batch add organization members", + "operationId": "batch-add-organization-members", + "parameters": [ + { + "type": "string", + "description": "Organization ID", + "name": "organization", + "in": "path", + "required": true + }, + { + "description": "Add members request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.AddOrganizationMembersRequest" + } + } + ], + "responses": { + "201": { + "description": "Created", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.OrganizationMember" + } + } + } + }, + "security": [ + { + "CoderSessionToken": [] + } + ] } }, "/api/v2/organizations/{organization}/members/roles": { @@ -4524,10 +4566,12 @@ ] }, "post": { + "description": "Deprecated: use POST /api/v2/organizations/{organization}/members instead.", "produces": ["application/json"], "tags": ["Members"], "summary": "Add organization member", "operationId": "add-organization-member", + "deprecated": true, "parameters": [ { "type": "string", @@ -14392,6 +14436,22 @@ } } }, + "codersdk.AddOrganizationMembersRequest": { + "type": "object", + "required": ["user_ids"], + "properties": { + "user_ids": { + "description": "UserIDs is the list of user IDs to add as organization members. The\nslice must contain between 1 and 100 IDs. The upper bound keeps a single\nbatch insert and its audit fan-out bounded; callers adding more members\nshould page the request.", + "type": "array", + "maxItems": 100, + "minItems": 1, + "items": { + "type": "string", + "format": "uuid" + } + } + } + }, "codersdk.AgentChatSendShortcut": { "type": "string", "enum": ["enter", "modifier_enter"], diff --git a/coderd/coderd.go b/coderd/coderd.go index aabd12188c0..b0cea091dcb 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1588,6 +1588,7 @@ func New(options *Options) *API { r.Get("/paginated-members", api.paginatedMembers) r.Route("/members", func(r chi.Router) { r.Get("/", api.listMembers) + r.Post("/", api.postOrganizationMembers) r.Route("/roles", func(r chi.Router) { r.Get("/", api.assignableOrgRoles) }) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 6221d042aa1..3bfc53c027c 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1697,6 +1697,38 @@ func scopedOrgRoleIdentifiers(names []string, orgID uuid.UUID) []rbac.RoleIdenti return out } +// authorizeOrganizationMemberRoleAssignment authorizes granting the given roles +// when adding a member to an organization. It does not authorize the member +// insert itself; callers must separately authorize ActionCreate on the +// organization_member object. The org's default_org_member_roles are implied at +// request time by GetAuthorizationUserRoles, so canAssignRoles must cover the +// full effective set (the explicit roles, organization-member, plus the +// defaults). Both the single-member and batch insert wrappers share this +// preamble so the role-assignment check cannot drift between them. +func (q *querier) authorizeOrganizationMemberRoleAssignment(ctx context.Context, organizationID uuid.UUID, roles []string) error { + orgRoles, err := q.convertToOrganizationRoles(organizationID, roles) + if err != nil { + return xerrors.Errorf("converting to organization roles: %w", err) + } + + org, err := q.db.GetOrganizationByID(ctx, organizationID) + if err != nil { + return xerrors.Errorf("get organization: %w", err) + } + defaultRoles, err := q.convertToOrganizationRoles(organizationID, org.DefaultOrgMemberRoles) + if err != nil { + return xerrors.Errorf("convert default member roles: %w", err) + } + + // All roles are added roles. Org member is always implied. + addedRoles := slices.Concat( + orgRoles, + []rbac.RoleIdentifier{rbac.ScopedRoleOrgMember(organizationID)}, + defaultRoles, + ) + return q.canAssignRoles(ctx, organizationID, addedRoles, []rbac.RoleIdentifier{}) +} + func (q *querier) AcquireLock(ctx context.Context, id int64) error { return q.db.AcquireLock(ctx, id) } @@ -6201,35 +6233,28 @@ func (q *querier) InsertOrganization(ctx context.Context, arg database.InsertOrg } func (q *querier) InsertOrganizationMember(ctx context.Context, arg database.InsertOrganizationMemberParams) (database.OrganizationMember, error) { - orgRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, arg.Roles) - if err != nil { - return database.OrganizationMember{}, xerrors.Errorf("converting to organization roles: %w", err) + if err := q.authorizeOrganizationMemberRoleAssignment(ctx, arg.OrganizationID, arg.Roles); err != nil { + return database.OrganizationMember{}, err } - // The org's default_org_member_roles are implied at request time by - // GetAuthorizationUserRoles. Include them in canAssignRoles so the - // caller is required to be authorized to grant the full effective set - // (the explicit roles, organization-member, plus the defaults). - org, err := q.db.GetOrganizationByID(ctx, arg.OrganizationID) - if err != nil { - return database.OrganizationMember{}, xerrors.Errorf("get organization: %w", err) - } - defaultRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, org.DefaultOrgMemberRoles) - if err != nil { - return database.OrganizationMember{}, xerrors.Errorf("convert default member roles: %w", err) + // Scope the create authorization to the specific user being added. + obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID).WithID(arg.UserID) + return insert(q.log, q.auth, obj, q.db.InsertOrganizationMember)(ctx, arg) +} + +func (q *querier) InsertOrganizationMembersBatch(ctx context.Context, arg database.InsertOrganizationMembersBatchParams) ([]database.OrganizationMember, error) { + if err := q.authorizeOrganizationMemberRoleAssignment(ctx, arg.OrganizationID, arg.Roles); err != nil { + return nil, err } - // All roles are added roles. Org member is always implied. - //nolint:gocritic - addedRoles := append(orgRoles, rbac.ScopedRoleOrgMember(arg.OrganizationID)) - addedRoles = append(addedRoles, defaultRoles...) - err = q.canAssignRoles(ctx, arg.OrganizationID, addedRoles, []rbac.RoleIdentifier{}) - if err != nil { - return database.OrganizationMember{}, err + // The batch inserts multiple users, so the create permission is authorized + // at the organization scope rather than a single user ID. + obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID) + if err := q.authorizeContext(ctx, policy.ActionCreate, obj); err != nil { + return nil, err } - obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID).WithID(arg.UserID) - return insert(q.log, q.auth, obj, q.db.InsertOrganizationMember)(ctx, arg) + return q.db.InsertOrganizationMembersBatch(ctx, arg) } func (q *querier) InsertPreset(ctx context.Context, arg database.InsertPresetParams) (database.TemplateVersionPreset, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 28e2b91ae31..f72fbf272df 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2509,6 +2509,25 @@ func (s *MethodTestSuite) TestOrganization() { rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate, ) })) + s.Run("InsertOrganizationMembersBatch", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + o := testutil.Fake(s.T(), faker, database.Organization{DefaultOrgMemberRoles: []string{codersdk.RoleOrganizationAdmin}}) + u1 := testutil.Fake(s.T(), faker, database.User{}) + u2 := testutil.Fake(s.T(), faker, database.User{}) + arg := database.InsertOrganizationMembersBatchParams{ + OrganizationID: o.ID, + UserIds: []uuid.UUID{u1.ID, u2.ID}, + Roles: []string{codersdk.RoleOrganizationAdmin}, + } + dbm.EXPECT().GetOrganizationByID(gomock.Any(), o.ID).Return(o, nil).AnyTimes() + dbm.EXPECT().InsertOrganizationMembersBatch(gomock.Any(), arg).Return([]database.OrganizationMember{ + {OrganizationID: o.ID, UserID: u1.ID, Roles: arg.Roles}, + {OrganizationID: o.ID, UserID: u2.ID, Roles: arg.Roles}, + }, nil).AnyTimes() + check.Args(arg).Asserts( + rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionAssign, + rbac.ResourceOrganizationMember.InOrg(o.ID), policy.ActionCreate, + ) + })) s.Run("InsertPreset", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { arg := database.InsertPresetParams{TemplateVersionID: uuid.New(), Name: "test"} dbm.EXPECT().InsertPreset(gomock.Any(), arg).Return(database.TemplateVersionPreset{}, nil).AnyTimes() diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 1e8c654e73b..87d4edd57eb 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -4305,6 +4305,14 @@ func (m queryMetricsStore) InsertOrganizationMember(ctx context.Context, arg dat return r0, r1 } +func (m queryMetricsStore) InsertOrganizationMembersBatch(ctx context.Context, arg database.InsertOrganizationMembersBatchParams) ([]database.OrganizationMember, error) { + start := time.Now() + r0, r1 := m.s.InsertOrganizationMembersBatch(ctx, arg) + m.queryLatencies.WithLabelValues("InsertOrganizationMembersBatch").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "InsertOrganizationMembersBatch").Inc() + return r0, r1 +} + func (m queryMetricsStore) InsertPreset(ctx context.Context, arg database.InsertPresetParams) (database.TemplateVersionPreset, error) { start := time.Now() r0, r1 := m.s.InsertPreset(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a4e2eda82dd..8510f2a8847 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -8062,6 +8062,21 @@ func (mr *MockStoreMockRecorder) InsertOrganizationMember(ctx, arg any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertOrganizationMember", reflect.TypeOf((*MockStore)(nil).InsertOrganizationMember), ctx, arg) } +// InsertOrganizationMembersBatch mocks base method. +func (m *MockStore) InsertOrganizationMembersBatch(ctx context.Context, arg database.InsertOrganizationMembersBatchParams) ([]database.OrganizationMember, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "InsertOrganizationMembersBatch", ctx, arg) + ret0, _ := ret[0].([]database.OrganizationMember) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// InsertOrganizationMembersBatch indicates an expected call of InsertOrganizationMembersBatch. +func (mr *MockStoreMockRecorder) InsertOrganizationMembersBatch(ctx, arg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertOrganizationMembersBatch", reflect.TypeOf((*MockStore)(nil).InsertOrganizationMembersBatch), ctx, arg) +} + // InsertPreset mocks base method. func (m *MockStore) InsertPreset(ctx context.Context, arg database.InsertPresetParams) (database.TemplateVersionPreset, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 097e7d2ad4d..a816d64101a 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1092,6 +1092,11 @@ type sqlcQuerier interface { InsertOAuth2ProviderAppToken(ctx context.Context, arg InsertOAuth2ProviderAppTokenParams) (OAuth2ProviderAppToken, error) InsertOrganization(ctx context.Context, arg InsertOrganizationParams) (Organization, error) InsertOrganizationMember(ctx context.Context, arg InsertOrganizationMemberParams) (OrganizationMember, error) + // Batch-inserts new organization members. Users that are already members + // are silently skipped via ON CONFLICT DO NOTHING, so the caller does not + // need to pre-filter. Only newly inserted rows are returned. The result + // order is unspecified, so callers must not rely on input ordering. + InsertOrganizationMembersBatch(ctx context.Context, arg InsertOrganizationMembersBatchParams) ([]OrganizationMember, error) InsertPreset(ctx context.Context, arg InsertPresetParams) (TemplateVersionPreset, error) InsertPresetParameters(ctx context.Context, arg InsertPresetParametersParams) ([]TemplateVersionPresetParameter, error) InsertPresetPrebuildSchedule(ctx context.Context, arg InsertPresetPrebuildScheduleParams) (TemplateVersionPresetPrebuildSchedule, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index dbb5c8c7a6f..b8070e0560c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -19609,6 +19609,73 @@ func (q *sqlQuerier) InsertOrganizationMember(ctx context.Context, arg InsertOrg return i, err } +const insertOrganizationMembersBatch = `-- name: InsertOrganizationMembersBatch :many +INSERT INTO organization_members ( + organization_id, + user_id, + created_at, + updated_at, + roles +) +SELECT + $1 :: uuid, + user_id, + $2 :: timestamptz, + $3 :: timestamptz, + $4 :: text[] +FROM + UNNEST($5 :: uuid[]) AS user_id +ON CONFLICT (organization_id, user_id) DO NOTHING +RETURNING user_id, organization_id, created_at, updated_at, roles +` + +type InsertOrganizationMembersBatchParams struct { + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + Roles []string `db:"roles" json:"roles"` + UserIds []uuid.UUID `db:"user_ids" json:"user_ids"` +} + +// Batch-inserts new organization members. Users that are already members +// are silently skipped via ON CONFLICT DO NOTHING, so the caller does not +// need to pre-filter. Only newly inserted rows are returned. The result +// order is unspecified, so callers must not rely on input ordering. +func (q *sqlQuerier) InsertOrganizationMembersBatch(ctx context.Context, arg InsertOrganizationMembersBatchParams) ([]OrganizationMember, error) { + rows, err := q.db.QueryContext(ctx, insertOrganizationMembersBatch, + arg.OrganizationID, + arg.CreatedAt, + arg.UpdatedAt, + pq.Array(arg.Roles), + pq.Array(arg.UserIds), + ) + if err != nil { + return nil, err + } + defer rows.Close() + var items []OrganizationMember + for rows.Next() { + var i OrganizationMember + if err := rows.Scan( + &i.UserID, + &i.OrganizationID, + &i.CreatedAt, + &i.UpdatedAt, + pq.Array(&i.Roles), + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const organizationMembers = `-- name: OrganizationMembers :many SELECT organization_members.user_id, organization_members.organization_id, organization_members.created_at, organization_members.updated_at, organization_members.roles, diff --git a/coderd/database/queries/organizationmembers.sql b/coderd/database/queries/organizationmembers.sql index 78e7e311632..249a975307d 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/coderd/database/queries/organizationmembers.sql @@ -50,6 +50,29 @@ INSERT INTO VALUES ($1, $2, $3, $4, $5) RETURNING *; +-- name: InsertOrganizationMembersBatch :many +-- Batch-inserts new organization members. Users that are already members +-- are silently skipped via ON CONFLICT DO NOTHING, so the caller does not +-- need to pre-filter. Only newly inserted rows are returned. The result +-- order is unspecified, so callers must not rely on input ordering. +INSERT INTO organization_members ( + organization_id, + user_id, + created_at, + updated_at, + roles +) +SELECT + @organization_id :: uuid, + user_id, + @created_at :: timestamptz, + @updated_at :: timestamptz, + @roles :: text[] +FROM + UNNEST(@user_ids :: uuid[]) AS user_id +ON CONFLICT (organization_id, user_id) DO NOTHING +RETURNING *; + -- name: DeleteOrganizationMember :exec DELETE FROM diff --git a/coderd/members.go b/coderd/members.go index 7f1511bebb9..0b67df7d83a 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "net/http" + "strings" "github.com/google/uuid" "golang.org/x/xerrors" @@ -31,6 +32,8 @@ import ( // @Param user path string true "User ID, name, or me" // @Success 200 {object} codersdk.OrganizationMember // @Router /api/v2/organizations/{organization}/members/{user} [post] +// @Deprecated +// @Description Deprecated: use POST /api/v2/organizations/{organization}/members instead. func (api *API) postOrganizationMember(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -90,6 +93,134 @@ func (api *API) postOrganizationMember(rw http.ResponseWriter, r *http.Request) httpapi.Write(ctx, rw, http.StatusOK, resp[0]) } +// @Summary Batch add organization members +// @ID batch-add-organization-members +// @Security CoderSessionToken +// @Accept json +// @Produce json +// @Tags Members +// @Param organization path string true "Organization ID" +// @Param request body codersdk.AddOrganizationMembersRequest true "Add members request" +// @Success 201 {array} codersdk.OrganizationMember +// @Router /api/v2/organizations/{organization}/members [post] +func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + organization = httpmw.OrganizationParam(r) + apiKey = httpmw.APIKey(r) + auditor = api.Auditor.Load() + ) + + var req codersdk.AddOrganizationMembersRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + // The request context (not AsSystemRestricted) is used so dbauthz enforces + // read permission on each target user. + users, err := api.Database.GetUsersByIDs(ctx, req.UserIDs) + if httpapi.IsUnauthorizedError(err) { + httpapi.Forbidden(rw) + return + } + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + usersByID := make(map[uuid.UUID]database.User, len(users)) + for _, u := range users { + usersByID[u.ID] = u + } + + // GetUsersByIDs intentionally includes soft-deleted users, so reject them + // explicitly along with any IDs that resolved to no user. + var missing []uuid.UUID + var deleted []uuid.UUID + for _, uid := range req.UserIDs { + u, ok := usersByID[uid] + if !ok { + missing = append(missing, uid) + } else if u.Deleted { + deleted = append(deleted, uid) + } + } + if len(missing) > 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Users not found: %s", joinUUIDs(missing)), + }) + return + } + if len(deleted) > 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Deleted users cannot be added to an organization: %s", joinUUIDs(deleted)), + }) + return + } + + // Report all blocked OIDC users together so the caller can fix the request + // in a single round-trip. + if api.IDPSync.OrganizationSyncEnabled(ctx, api.Database) { + var oidcUsernames []string + for _, user := range users { + if user.LoginType == database.LoginTypeOIDC { + oidcUsernames = append(oidcUsernames, user.Username) + } + } + if len(oidcUsernames) > 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, manualMembershipBlockedResponse(oidcUsernames)) + return + } + } + + // Batch-insert new members. ON CONFLICT DO NOTHING silently + // skips users who are already members, eliminating the race + // between the check and the insert. + now := dbtime.Now() + insertedMembers, err := api.Database.InsertOrganizationMembersBatch(ctx, database.InsertOrganizationMembersBatchParams{ + OrganizationID: organization.ID, + UserIds: req.UserIDs, + CreatedAt: now, + UpdatedAt: now, + Roles: []string{}, + }) + if httpapi.IsUnauthorizedError(err) { + httpapi.Forbidden(rw) + return + } + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + // Emit an audit event for each newly-inserted member. The rows are already + // committed, so the audit records StatusCreated to reflect the resource + // state; a later conversion failure returns 500 to the caller without + // changing that. + for _, member := range insertedMembers { + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.AuditableOrganizationMember]{ + Audit: *auditor, + Log: api.Logger, + UserID: apiKey.UserID, + OrganizationID: organization.ID, + RequestID: httpmw.RequestID(r), + Action: database.AuditActionCreate, + IP: r.RemoteAddr, + Status: http.StatusCreated, + Old: database.AuditableOrganizationMember{}, + New: member.Auditable(usersByID[member.UserID].Username), + }) + } + + resp, err := convertOrganizationMembers(ctx, api.Database, insertedMembers) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusCreated, resp) +} + // @Summary Remove organization member // @ID remove-organization-member // @Security CoderSessionToken @@ -204,7 +335,6 @@ func (api *API) organizationMember(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, resp[0]) } -// @Deprecated use /organizations/{organization}/paginated-members [get] // @Summary List organization members // @ID list-organization-members // @Security CoderSessionToken @@ -213,6 +343,8 @@ func (api *API) organizationMember(rw http.ResponseWriter, r *http.Request) { // @Param organization path string true "Organization ID" // @Success 200 {object} []codersdk.OrganizationMemberWithUserData // @Router /api/v2/organizations/{organization}/members [get] +// @Deprecated +// @Description Deprecated: use GET /api/v2/organizations/{organization}/paginated-members instead. func (api *API) listMembers(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -501,8 +633,8 @@ func (api *API) allowChangingMemberRoles(ctx context.Context, rw http.ResponseWr return true } -// convertOrganizationMembers batches the role lookup to make only 1 sql call -// We +// convertOrganizationMembers batches the role lookup to make only +// one SQL call instead of one per member. func convertOrganizationMembers(ctx context.Context, db database.Store, mems []database.OrganizationMember) ([]codersdk.OrganizationMember, error) { converted := make([]codersdk.OrganizationMember, 0, len(mems)) roleLookup := make([]database.NameOrganizationPair, 0) @@ -606,11 +738,36 @@ func convertOrganizationMembersWithUserData(ctx context.Context, db database.Sto // since all organization membership is controlled by the external IDP. func (api *API) manualOrganizationMembership(ctx context.Context, rw http.ResponseWriter, user database.User) bool { if user.LoginType == database.LoginTypeOIDC && api.IDPSync.OrganizationSyncEnabled(ctx, api.Database) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Organization sync is enabled for OIDC users, meaning manual organization assignment is not allowed for this user. Have the user re-login to refresh their organizations.", - Detail: fmt.Sprintf("User %s is an OIDC user and organization sync is enabled. Ask an administrator to resolve the membership in your external IDP.", user.Username), - }) + httpapi.Write(ctx, rw, http.StatusBadRequest, manualMembershipBlockedResponse([]string{user.Username})) return false } return true } + +// manualMembershipBlockedResponse builds the error response returned when +// organization sync is enabled and manual organization membership changes are +// attempted for OIDC users. usernames must contain at least one entry. Both the +// single-member and batch handlers route through here so the wording stays in +// sync; the message is pluralized based on the number of blocked users. +func manualMembershipBlockedResponse(usernames []string) codersdk.Response { + target, who := "this user", "the user" + subject := fmt.Sprintf("User %s is an OIDC user", usernames[0]) + if len(usernames) > 1 { + target, who = "these users", "the users" + subject = fmt.Sprintf("Users %s are OIDC users", strings.Join(usernames, ", ")) + } + return codersdk.Response{ + Message: fmt.Sprintf("Organization sync is enabled for OIDC users, meaning manual organization assignment is not allowed for %s. Have %s re-login to refresh their organizations.", target, who), + Detail: fmt.Sprintf("%s and organization sync is enabled. Ask an administrator to resolve the membership in your external IDP.", subject), + } +} + +// joinUUIDs renders a slice of UUIDs as a comma-separated string so error +// messages surface plain IDs instead of Go's bracketed slice format. +func joinUUIDs(ids []uuid.UUID) string { + strs := make([]string, len(ids)) + for i, id := range ids { + strs[i] = id.String() + } + return strings.Join(strs, ", ") +} diff --git a/coderd/members_internal_test.go b/coderd/members_internal_test.go new file mode 100644 index 00000000000..1dee9c87784 --- /dev/null +++ b/coderd/members_internal_test.go @@ -0,0 +1,30 @@ +package coderd + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestManualMembershipBlockedResponse locks the singular and plural wording of +// the OIDC org-sync block message. The plural branch is what an admin sees when +// batch-adding several OIDC-synced users, so it must not regress. +func TestManualMembershipBlockedResponse(t *testing.T) { + t.Parallel() + + t.Run("Single", func(t *testing.T) { + t.Parallel() + resp := manualMembershipBlockedResponse([]string{"alice"}) + require.Contains(t, resp.Message, "not allowed for this user") + require.Contains(t, resp.Message, "Have the user re-login") + require.Contains(t, resp.Detail, "User alice is an OIDC user") + }) + + t.Run("Multiple", func(t *testing.T) { + t.Parallel() + resp := manualMembershipBlockedResponse([]string{"alice", "bob"}) + require.Contains(t, resp.Message, "not allowed for these users") + require.Contains(t, resp.Message, "Have the users re-login") + require.Contains(t, resp.Detail, "Users alice, bob are OIDC users") + }) +} diff --git a/coderd/members_test.go b/coderd/members_test.go index c2bf219c1eb..e0ca1db302b 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -3,12 +3,14 @@ package coderd_test import ( "context" "database/sql" + "net/http" "testing" "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbgen" @@ -50,6 +52,285 @@ func TestAddMember(t *testing.T) { }) } +func TestAddMembers(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + owner, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // Create a second organization via dbgen to avoid needing + // the enterprise multi-org feature. + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + // Create two new users (they'll be in the default org). + _, user1 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + _, user2 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // Batch-add both to the second org. + // nolint:gocritic // must be an owner to add members + members, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user1.ID, user2.ID}, + }) + require.NoError(t, err) + require.Len(t, members, 2) + + memberIDs := make([]uuid.UUID, len(members)) + for i, m := range members { + memberIDs[i] = m.UserID + } + require.ElementsMatch(t, []uuid.UUID{user1.ID, user2.ID}, memberIDs) + }) + + t.Run("AlreadyMember", func(t *testing.T) { + t.Parallel() + owner := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // user is already a member of first.OrganizationID. + // The endpoint should silently skip duplicates and return + // an empty list (no new members added). + // nolint:gocritic // must be an owner to add members + members, err := owner.PostOrganizationMembers(ctx, first.OrganizationID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user.ID}, + }) + require.NoError(t, err) + require.Empty(t, members, "already-member should be skipped, not inserted") + }) + + t.Run("UserNotFound", func(t *testing.T) { + t.Parallel() + owner := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, first.OrganizationID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{uuid.New()}, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "not found") + }) + + t.Run("SingleUser", func(t *testing.T) { + t.Parallel() + owner, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // nolint:gocritic // must be an owner to add members + members, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user.ID}, + }) + require.NoError(t, err) + require.Len(t, members, 1) + require.Equal(t, user.ID, members[0].UserID) + }) + + t.Run("SkipsDuplicates", func(t *testing.T) { + t.Parallel() + owner, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + _, user1 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + _, user2 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // Pre-add user2 to the second org. + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user2.ID}, + }) + require.NoError(t, err) + + // Batch-add both: user1 (new) + user2 (already member). + // user2 should be silently skipped; only user1 is returned. + members, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user1.ID, user2.ID}, + }) + require.NoError(t, err) + require.Len(t, members, 1) + require.Equal(t, user1.ID, members[0].UserID) + + // Verify both users are now members. + allMembers, err := owner.OrganizationMembers(ctx, secondOrg.ID) + require.NoError(t, err) + ids := make([]uuid.UUID, len(allMembers)) + for i, m := range allMembers { + ids[i] = m.UserID + } + require.Contains(t, ids, user1.ID) + require.Contains(t, ids, user2.ID) + }) + + t.Run("DuplicateUserIDs", func(t *testing.T) { + t.Parallel() + owner, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // The same user ID appears twice in the request. The batch insert uses + // ON CONFLICT DO NOTHING, which absorbs the intra-statement duplicate, + // so the user is added exactly once with no error. + // nolint:gocritic // must be an owner to add members + members, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user.ID, user.ID}, + }) + require.NoError(t, err) + require.Len(t, members, 1) + require.Equal(t, user.ID, members[0].UserID) + }) + + t.Run("RejectsEmptyRequest", func(t *testing.T) { + t.Parallel() + owner := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // An empty user_ids slice violates the min=1 validation bound. + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, first.OrganizationID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{}, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("RejectsTooManyUsers", func(t *testing.T) { + t.Parallel() + owner := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // 101 IDs exceeds the max=100 validation bound; the request is + // rejected before any user lookup, so the IDs need not exist. + userIDs := make([]uuid.UUID, 101) + for i := range userIDs { + userIDs[i] = uuid.New() + } + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, first.OrganizationID, codersdk.AddOrganizationMembersRequest{ + UserIDs: userIDs, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("DeletedUser", func(t *testing.T) { + t.Parallel() + owner, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + _, deletedUser := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + _, liveUser := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // Soft-delete the first user so GetUsersByIDs returns them + // with Deleted=true. + // nolint:gocritic // must be an owner to delete the user + require.NoError(t, owner.DeleteUser(ctx, deletedUser.ID)) + + // Batch-add the deleted user alongside a live user. The + // handler must reject the whole request with 400 because + // deleted users cannot be added to an organization. + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{deletedUser.ID, liveUser.ID}, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Deleted users cannot be added") + + // Confirm the live user was not inserted: the rejection + // happens before the batch insert runs. + allMembers, err := owner.OrganizationMembers(ctx, secondOrg.ID) + require.NoError(t, err) + for _, m := range allMembers { + require.NotEqual(t, liveUser.ID, m.UserID, "live user should not be added when the batch contains a deleted user") + } + }) + + t.Run("AuditLogsOnlyForNewMembers", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{Auditor: auditor}) + first := coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitMedium) + + secondOrg := dbgen.Organization(t, db, database.Organization{}) + + _, user1 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + _, user2 := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + // Pre-add user2 to the second org so the next call is a no-op for it. + // nolint:gocritic // must be an owner to add members + _, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user2.ID}, + }) + require.NoError(t, err) + + // Reset audit logs after the setup call so the assertions + // below only see entries from the batch under test. + auditor.ResetLogs() + + // Batch-add user1 (new) and user2 (already member). Only + // user1 should produce an audit log because ON CONFLICT + // DO NOTHING skips user2 and the handler emits audits only + // for inserted rows. + members, err := owner.PostOrganizationMembers(ctx, secondOrg.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user1.ID, user2.ID}, + }) + require.NoError(t, err) + require.Len(t, members, 1) + + var memberCreateLogs []database.AuditLog + for _, log := range auditor.AuditLogs() { + if log.ResourceType == database.ResourceTypeOrganizationMember && + log.Action == database.AuditActionCreate && + log.OrganizationID == secondOrg.ID { + memberCreateLogs = append(memberCreateLogs, log) + } + } + require.Len(t, memberCreateLogs, 1, "expected exactly one audit log for the newly-added member") + require.Equal(t, user1.ID, memberCreateLogs[0].ResourceID, "audit log should reference the new member, not the existing one") + }) +} + func TestDeleteMember(t *testing.T) { t.Parallel() diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 5d949860db3..32c84b9c18e 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -104,6 +104,16 @@ type PaginatedMembersResponse struct { Count int `json:"count"` } +// AddOrganizationMembersRequest provides a list of user IDs to add +// as members to an organization in a single batch request. +type AddOrganizationMembersRequest struct { + // UserIDs is the list of user IDs to add as organization members. The + // slice must contain between 1 and 100 IDs. The upper bound keeps a single + // batch insert and its audit fan-out bounded; callers adding more members + // should page the request. + UserIDs []uuid.UUID `json:"user_ids" validate:"required,min=1,max=100" format:"uuid"` +} + type CreateOrganizationRequest struct { Name string `json:"name" validate:"required,organization_name"` // DisplayName will default to the same value as `Name` if not provided. diff --git a/codersdk/users.go b/codersdk/users.go index 351f20d2a51..21c3967987e 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -711,7 +711,9 @@ func (c *Client) UpdateUserPassword(ctx context.Context, user string, req Update return nil } -// PostOrganizationMember adds a user to an organization +// PostOrganizationMember adds a user to an organization. +// +// Deprecated: Use PostOrganizationMembers instead. func (c *Client) PostOrganizationMember(ctx context.Context, organizationID uuid.UUID, user string) (OrganizationMember, error) { res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/members/%s", organizationID, user), nil) if err != nil { @@ -725,6 +727,23 @@ func (c *Client) PostOrganizationMember(ctx context.Context, organizationID uuid return member, json.NewDecoder(res.Body).Decode(&member) } +// PostOrganizationMembers adds multiple users to an organization in a single +// batch request. Users that are already members are silently skipped, so the +// returned slice contains only the members that were newly added and may be +// shorter than the request (empty when every user was already a member). +func (c *Client) PostOrganizationMembers(ctx context.Context, organizationID uuid.UUID, req AddOrganizationMembersRequest) ([]OrganizationMember, error) { + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/members", organizationID), req) + if err != nil { + return nil, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusCreated { + return nil, ReadBodyAsError(res) + } + var members []OrganizationMember + return members, json.NewDecoder(res.Body).Decode(&members) +} + // DeleteOrganizationMember removes a user from an organization func (c *Client) DeleteOrganizationMember(ctx context.Context, organizationID uuid.UUID, user string) error { res, err := c.Request(ctx, http.MethodDelete, fmt.Sprintf("/api/v2/organizations/%s/members/%s", organizationID, user), nil) diff --git a/docs/reference/api/members.md b/docs/reference/api/members.md index 6660b4a138d..bb455347b4d 100644 --- a/docs/reference/api/members.md +++ b/docs/reference/api/members.md @@ -13,6 +13,8 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/members `GET /api/v2/organizations/{organization}/members` +Deprecated: use GET /api/v2/organizations/{organization}/paginated-members instead. + ### Parameters | Name | In | Type | Required | Description | @@ -102,6 +104,83 @@ Status Code **200** To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Batch add organization members + +### Code samples + +```sh +# Example request using curl +curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/members \ + -H 'Content-Type: application/json' \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +`POST /api/v2/organizations/{organization}/members` + +> Body parameter + +```json +{ + "user_ids": [ + "497f6eca-6276-4993-bfeb-53cbbbba6f08" + ] +} +``` + +### Parameters + +| Name | In | Type | Required | Description | +|----------------|------|--------------------------------------------------------------------------------------------|----------|---------------------| +| `organization` | path | string | true | Organization ID | +| `body` | body | [codersdk.AddOrganizationMembersRequest](schemas.md#codersdkaddorganizationmembersrequest) | true | Add members request | + +### Example responses + +> 201 Response + +```json +[ + { + "created_at": "2019-08-24T14:15:22Z", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", + "roles": [ + { + "display_name": "string", + "name": "string", + "organization_id": "string" + } + ], + "updated_at": "2019-08-24T14:15:22Z", + "user_id": "a169451c-8525-4352-b8ca-070dd449a1a5" + } +] +``` + +### Responses + +| Status | Meaning | Description | Schema | +|--------|--------------------------------------------------------------|-------------|-------------------------------------------------------------------------------| +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | array of [codersdk.OrganizationMember](schemas.md#codersdkorganizationmember) | + +

Response Schema

+ +Status Code **201** + +| Name | Type | Required | Restrictions | Description | +|----------------------|-------------------|----------|--------------|-------------| +| `[array item]` | array | false | | | +| `» created_at` | string(date-time) | false | | | +| `» organization_id` | string(uuid) | false | | | +| `» roles` | array | false | | | +| `»» display_name` | string | false | | | +| `»» name` | string | false | | | +| `»» organization_id` | string | false | | | +| `» updated_at` | string(date-time) | false | | | +| `» user_id` | string(uuid) | false | | | + +To perform this operation, you must be authenticated. [Learn more](authentication.md). + ## Get member roles by organization ### Code samples @@ -640,6 +719,8 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/member `POST /api/v2/organizations/{organization}/members/{user}` +Deprecated: use POST /api/v2/organizations/{organization}/members instead. + ### Parameters | Name | In | Type | Required | Description | diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index fcbe267ec20..4e3776f3868 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1246,6 +1246,22 @@ None |-----------|--------|----------|--------------|-------------| | `license` | string | true | | | +## codersdk.AddOrganizationMembersRequest + +```json +{ + "user_ids": [ + "497f6eca-6276-4993-bfeb-53cbbbba6f08" + ] +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +|------------|-----------------|----------|--------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `user_ids` | array of string | true | | User ids is the list of user IDs to add as organization members. The slice must contain between 1 and 100 IDs. The upper bound keeps a single batch insert and its audit fan-out bounded; callers adding more members should page the request. | + ## codersdk.AgentChatSendShortcut ```json diff --git a/enterprise/coderd/members_test.go b/enterprise/coderd/members_test.go new file mode 100644 index 00000000000..08aecebad4e --- /dev/null +++ b/enterprise/coderd/members_test.go @@ -0,0 +1,71 @@ +package coderd_test + +import ( + "net/http" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/testutil" +) + +// TestBatchAddMembersReadAuthorization verifies that postOrganizationMembers +// resolves target users in the requester's authorization context, so a caller +// that can create organization members but cannot read site users is rejected +// with 403 at the read gate before any insert. The read gate is tamper-evident: +// swapping the handler to AsSystemRestricted does not make this succeed, it +// makes the request fail later at the role-assignment gate (canAssignRoles +// rejects the custom role's implied organization-member grant, a 500), which +// the require.Equal(403) assertion still catches as a non-403. +func TestBatchAddMembersReadAuthorization(t *testing.T) { + t.Parallel() + + owner, first := coderdenttest.New(t, &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + }, + }, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // A custom org role that can create members and assign org roles, but + // grants no site-wide user:read (custom org roles cannot). This is exactly + // the caller that could add arbitrary accounts by UUID if the handler + // skipped the per-user read check. + //nolint:gocritic // owner is required to create a custom role + role, err := owner.CreateOrganizationRole(ctx, codersdk.Role{ + Name: "member-adder", + DisplayName: "Member Adder", + OrganizationID: first.OrganizationID.String(), + OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ + codersdk.ResourceOrganizationMember: {codersdk.ActionCreate, codersdk.ActionRead}, + codersdk.ResourceAssignOrgRole: {codersdk.ActionAssign}, + codersdk.ResourceOrganization: {codersdk.ActionRead}, + }), + }) + require.NoError(t, err, "create custom role") + + adder, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{ + Name: role.Name, + OrganizationID: first.OrganizationID, + }) + + // A target user the adder cannot read. + _, target := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) + + _, err = adder.PostOrganizationMembers(ctx, first.OrganizationID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{target.ID}, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) +} diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 5a0986788ac..4ce355ee6d1 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -194,6 +194,12 @@ func TestUserOIDC(t *testing.T) { _, err = runner.AdminClient.PostOrganizationMember(ctx, orgThree.ID, "alice") require.ErrorContains(t, err, "Organization sync is enabled") + // The batch endpoint must reject OIDC-synced users the same way. + _, err = runner.AdminClient.PostOrganizationMembers(ctx, orgThree.ID, codersdk.AddOrganizationMembersRequest{ + UserIDs: []uuid.UUID{user.ID}, + }) + require.ErrorContains(t, err, "Organization sync is enabled") + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{orgOne.ID, orgTwo.ID}) // Go around the block to add the user to see if they are removed. dbgen.OrganizationMember(t, runner.API.Database, database.OrganizationMember{ diff --git a/site/src/api/api.ts b/site/src/api/api.ts index dc3f5e56e2b..144539449ae 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -840,12 +840,13 @@ class ApiMethods { ); }; - /** - * @param organization Can be the organization's ID or name - */ - addOrganizationMember = async (organization: string, userId: string) => { - const response = await this.axios.post( - `/api/v2/organizations/${organization}/members/${userId}`, + addOrganizationMembers = async ( + organization: string, + req: TypesGen.AddOrganizationMembersRequest, + ) => { + const response = await this.axios.post( + `/api/v2/organizations/${organization}/members`, + req, ); return response.data; diff --git a/site/src/api/queries/organizations.ts b/site/src/api/queries/organizations.ts index 1dcaac36596..fc36a4559e1 100644 --- a/site/src/api/queries/organizations.ts +++ b/site/src/api/queries/organizations.ts @@ -111,10 +111,13 @@ export const paginatedOrganizationMembers = ( }; }; -export const addOrganizationMember = (queryClient: QueryClient, id: string) => { +export const addOrganizationMembers = ( + queryClient: QueryClient, + id: string, +) => { return { - mutationFn: (userId: string) => { - return API.addOrganizationMember(id, userId); + mutationFn: (userIds: string[]) => { + return API.addOrganizationMembers(id, { user_ids: userIds }); }, onSuccess: async () => { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index b4ee9f1ab0b..9215ebfa5f3 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1041,6 +1041,21 @@ export interface AddLicenseRequest { readonly license: string; } +// From codersdk/organizations.go +/** + * AddOrganizationMembersRequest provides a list of user IDs to add + * as members to an organization in a single batch request. + */ +export interface AddOrganizationMembersRequest { + /** + * UserIDs is the list of user IDs to add as organization members. The + * slice must contain between 1 and 100 IDs. The upper bound keeps a single + * batch insert and its audit fan-out bounded; callers adding more members + * should page the request. + */ + readonly user_ids: readonly string[]; +} + // From codersdk/deployment.go export type Addon = "ai_governance"; diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx new file mode 100644 index 00000000000..0f244f992a1 --- /dev/null +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx @@ -0,0 +1,100 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, spyOn, userEvent, waitFor, within } from "storybook/test"; +import { reactRouterParameters } from "storybook-addon-remix-react-router"; +import { API } from "#/api/api"; +import { + MockDefaultOrganization, + MockOrganizationMember, + MockOrganizationMember2, + MockUserMember, + MockUserOwner, +} from "#/testHelpers/entities"; +import { + withAuthProvider, + withDashboardProvider, + withOrganizationSettingsProvider, +} from "#/testHelpers/storybook"; +import OrganizationMembersPage from "./OrganizationMembersPage"; + +const meta: Meta = { + title: "pages/OrganizationMembersPage", + component: OrganizationMembersPage, + decorators: [ + withAuthProvider, + withDashboardProvider, + withOrganizationSettingsProvider, + ], + parameters: { + user: MockUserOwner, + reactRouter: reactRouterParameters({ + location: { + pathParams: { organization: MockDefaultOrganization.name }, + }, + routing: { path: "/organizations/:organization/members" }, + }), + }, + beforeEach: () => { + spyOn(API, "getOrganizationRoles").mockResolvedValue([]); + spyOn(API, "getGroupsByOrganization").mockResolvedValue([]); + spyOn(API, "getUsers").mockResolvedValue({ + users: [MockUserMember, MockUserOwner], + count: 2, + }); + + // The members list starts with a single member and gains the newly + // added member once addOrganizationMembers succeeds, so the story can + // assert the list refreshes after the mutation invalidates its query. + let added = false; + spyOn(API, "getOrganizationPaginatedMembers").mockImplementation( + async () => { + const members = added + ? [MockOrganizationMember, MockOrganizationMember2] + : [MockOrganizationMember]; + return { members, count: members.length }; + }, + ); + spyOn(API, "addOrganizationMembers").mockImplementation(async () => { + added = true; + return []; + }); + }, +}; + +export default meta; +type Story = StoryObj; + +export const AddMembers: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const body = within(document.body); + + // Open the add-members dialog. + await userEvent.click( + await canvas.findByRole("button", { name: "Add users" }), + ); + + // Select two users from the dialog. + const dialog = within(await body.findByRole("dialog")); + await userEvent.click( + await dialog.findByLabelText(`Select user ${MockUserMember.username}`), + ); + await userEvent.click( + await dialog.findByLabelText(`Select user ${MockUserOwner.username}`), + ); + + // Submit the selection. + await userEvent.click(dialog.getByRole("button", { name: "Add users" })); + + // The handler maps the selected users to a single batch request. + await waitFor(() => { + expect(API.addOrganizationMembers).toHaveBeenCalledTimes(1); + }); + expect(API.addOrganizationMembers).toHaveBeenCalledWith( + MockDefaultOrganization.name, + { user_ids: [MockUserMember.id, MockUserOwner.id] }, + ); + + // The members list refetches and shows the newly added member. + await canvas.findByText(MockOrganizationMember2.email); + }, +}; diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx index 524e1d28f26..3de60a1365e 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx @@ -5,7 +5,7 @@ import { toast } from "sonner"; import { getErrorDetail, getErrorMessage } from "#/api/errors"; import { groupsByUserIdInOrganization } from "#/api/queries/groups"; import { - addOrganizationMember, + addOrganizationMembers, paginatedOrganizationMembers, removeOrganizationMember, updateOrganizationMemberRoles, @@ -62,8 +62,8 @@ const OrganizationMembersPage: FC = () => { }, ); - const addMemberMutation = useMutation( - addOrganizationMember(queryClient, organizationName), + const addMembersMutation = useMutation( + addOrganizationMembers(queryClient, organizationName), ); const [memberToEditRoles, setMemberToEditRoles] = @@ -123,7 +123,7 @@ const OrganizationMembersPage: FC = () => { error={ membersQuery.error ?? organizationRolesQuery.error ?? - addMemberMutation.error ?? + addMembersMutation.error ?? removeMemberMutation.error ?? updateMemberRolesMutation.error } @@ -133,13 +133,7 @@ const OrganizationMembersPage: FC = () => { members={members} showAISeatColumn={showAISeatColumn} addMembers={async (users: User[]) => { - // TODO: Replace with a batch endpoint (POST /organizations/{org}/members) - // to add all users in a single request instead of N individual calls. - // See branch jakehwll/devex-112-organizations-batch-endpoint. - await Promise.all( - users.map((user) => addMemberMutation.mutateAsync(user.id)), - ); - void membersQuery.refetch(); + await addMembersMutation.mutateAsync(users.map((user) => user.id)); }} onEditMemberRoles={setMemberToEditRoles} isUpdatingMemberRoles={updateMemberRolesMutation.isPending} diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.stories.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.stories.tsx index b4dd455ba48..6fb5165a7e9 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.stories.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.stories.tsx @@ -1,4 +1,7 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, spyOn, userEvent, within } from "storybook/test"; +import { API } from "#/api/api"; +import type { User } from "#/api/typesGenerated"; import { mockSuccessResult } from "#/components/PaginationWidget/PaginationContainer.mocks"; import type { UsePaginatedQueryResult } from "#/hooks/usePaginatedQuery"; import { @@ -6,6 +9,7 @@ import { MockOrganizationMember2, MockOwnerRole, MockUserAdminRole, + MockUserMember, MockUserOwner, } from "#/testHelpers/entities"; import { OrganizationMembersPageView } from "./OrganizationMembersPageView"; @@ -83,3 +87,67 @@ export const UpdatingMember: Story = { isUpdatingMemberRoles: true, }, }; + +// The add-members dialog caps a single selection at 100 users, mirroring the +// server-side max=100 constraint on AddOrganizationMembersRequest.UserIDs. This +// story exercises the boundary: it selects exactly 100 users, verifies the +// cap warning and counter render, and confirms a 101st selection is ignored. +const SELECTION_CAP = 100; + +// Clone a real MockUser into a unique roster large enough to exceed the cap. +// The dialog's user query is mocked below, so ids/usernames/emails only need +// to be unique within this list. +const bulkUsers: User[] = Array.from({ length: SELECTION_CAP + 1 }, (_, i) => ({ + ...MockUserMember, + id: `bulk-user-${i}`, + username: `bulk-user-${i}`, + email: `bulk-user-${i}@coder.com`, +})); + +export const AddUsersSelectionCap: Story = { + beforeEach: () => { + spyOn(API, "getUsers").mockResolvedValue({ + users: bulkUsers, + count: bulkUsers.length, + }); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const body = within(document.body); + + // Open the add-members dialog. + await userEvent.click( + await canvas.findByRole("button", { name: "Add users" }), + ); + const dialog = within(await body.findByRole("dialog")); + + // Wait for the mocked roster to render before interacting with it. + await dialog.findByLabelText(`Select user ${bulkUsers[0].username}`); + + // Select exactly the cap of 100 users. + for (let i = 0; i < SELECTION_CAP; i++) { + await userEvent.click( + dialog.getByLabelText(`Select user ${bulkUsers[i].username}`), + ); + } + + // The counter and cap warning both reflect that the limit is reached. + await dialog.findByText(`${SELECTION_CAP} of ${SELECTION_CAP} selected`); + await dialog.findByText( + `You can add up to ${SELECTION_CAP} users at a time.`, + ); + + // At exactly the cap the submit button stays enabled. + const submit = dialog.getByRole("button", { name: "Add users" }); + expect(submit).toBeEnabled(); + + // Attempting to select a 101st user is ignored: the checkbox stays + // unchecked and the selection count remains at the cap. + const overflowCheckbox = dialog.getByLabelText( + `Select user ${bulkUsers[SELECTION_CAP].username}`, + ); + await userEvent.click(overflowCheckbox); + expect(overflowCheckbox).not.toBeChecked(); + await dialog.findByText(`${SELECTION_CAP} of ${SELECTION_CAP} selected`); + }, +}; diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx index 3a6b2e1654b..8310087ff71 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx @@ -77,6 +77,13 @@ export const OrganizationMembersPageView: React.FC< ); }; +// Mirrors the server-side `max=100` validate constraint on +// AddOrganizationMembersRequest.UserIDs in codersdk/organizations.go. There is +// no codegen path linking that constraint to this constant, so the two values +// must be changed together. Keeping the selection within the same bound avoids +// a blind 400 on submit. +const MAX_MEMBERS_PER_ADD = 100; + interface AddUsersDialogProps { onSubmit: (users: User[]) => Promise; } @@ -118,6 +125,9 @@ const AddUsersDialog: React.FC = ({ onSubmit }) => { setFilter={setFilter} onChange={(user, checked) => { if (checked) { + if (selected.length >= MAX_MEMBERS_PER_ADD) { + return; + } setSelected([...selected, user]); } else { setSelected(selected.filter((s) => s.id !== user.id)); @@ -125,6 +135,17 @@ const AddUsersDialog: React.FC = ({ onSubmit }) => { }} selected={selected} /> +
+ + {selected.length} of {MAX_MEMBERS_PER_ADD} selected + + {selected.length >= MAX_MEMBERS_PER_ADD && ( + + + You can add up to {MAX_MEMBERS_PER_ADD} users at a time. + + )} +