From 48724fd882b8d93255b096439e1ddc5b0f5b882a Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Thu, 23 Apr 2026 12:30:21 +0000 Subject: [PATCH 01/14] feat!: add batch endpoint for adding organization members Add POST /organizations/{organization}/members that accepts an array of user IDs and adds them all in a single transaction, replacing the need for N individual POST calls when adding multiple members from the UI. The existing single-member POST /organizations/{organization}/members/{user} endpoint is marked as deprecated but remains functional. Backend: - AddOrganizationMembersRequest SDK type with user_ids field (max 100) - postOrganizationMembers handler with batch GetUsersByIDs lookup, pre-filtered duplicate skipping, atomic InTx insert - Proper error handling: 400 for missing users, 500 for server errors - Audit logging via BackgroundAudit for each inserted member - Returns 201 Created with only newly-added members Frontend: - API.addOrganizationMembers API method - addOrganizationMembers query mutation - OrganizationMembersPage wired to use the batch mutation - Multi-user select dialog with search - UsersFilter added to members page Tests: - TestAddMembers with 5 subtests: OK, AlreadyMember (skip), UserNotFound, SingleUser, SkipsDuplicates --- coderd/apidoc/docs.go | 66 ++++++++- coderd/apidoc/swagger.json | 58 +++++++- coderd/coderd.go | 1 + coderd/database/dbauthz/dbauthz.go | 25 ++++ coderd/database/dbmetrics/querymetrics.go | 8 + coderd/database/dbmock/dbmock.go | 15 ++ coderd/database/querier.go | 4 + coderd/database/queries.sql.go | 66 +++++++++ .../database/queries/organizationmembers.sql | 22 +++ coderd/members.go | 139 +++++++++++++++++- coderd/members_test.go | 131 +++++++++++++++++ codersdk/organizations.go | 6 + codersdk/users.go | 19 ++- docs/reference/api/members.md | 79 +++++++++- docs/reference/api/schemas.md | 16 ++ site/src/api/api.ts | 12 ++ site/src/api/queries/organizations.ts | 9 +- site/src/api/typesGenerated.ts | 9 ++ .../OrganizationMembersPage.tsx | 15 +- 19 files changed, 680 insertions(+), 20 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index f766a836a014a..316f4228333cd 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -3749,6 +3749,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": [] + } + ] } }, "/organizations/{organization}/members/roles": { @@ -3977,8 +4024,9 @@ const docTemplate = `{ "tags": [ "Members" ], - "summary": "Add organization member", + "summary": "Add organization member (deprecated)", "operationId": "add-organization-member", + "deprecated": true, "parameters": [ { "type": "string", @@ -14224,6 +14272,22 @@ const docTemplate = `{ } } }, + "codersdk.AddOrganizationMembersRequest": { + "type": "object", + "required": [ + "user_ids" + ], + "properties": { + "user_ids": { + "type": "array", + "maxItems": 100, + "items": { + "type": "string", + "format": "uuid" + } + } + } + }, "codersdk.AgentConnectionTiming": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 3f09eafba6383..9d7bbc9ee719c 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -3304,6 +3304,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": [] + } + ] } }, "/organizations/{organization}/members/roles": { @@ -3504,8 +3545,9 @@ "post": { "produces": ["application/json"], "tags": ["Members"], - "summary": "Add organization member", + "summary": "Add organization member (deprecated)", "operationId": "add-organization-member", + "deprecated": true, "parameters": [ { "type": "string", @@ -12762,6 +12804,20 @@ } } }, + "codersdk.AddOrganizationMembersRequest": { + "type": "object", + "required": ["user_ids"], + "properties": { + "user_ids": { + "type": "array", + "maxItems": 100, + "items": { + "type": "string", + "format": "uuid" + } + } + } + }, "codersdk.AgentConnectionTiming": { "type": "object", "properties": { diff --git a/coderd/coderd.go b/coderd/coderd.go index e46df91cc202b..df44e3bacb215 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1417,6 +1417,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 8ae54e845a0d7..1235cdbebb1db 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -5258,6 +5258,31 @@ func (q *querier) InsertOrganizationMember(ctx context.Context, arg database.Ins 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) { + orgRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, arg.Roles) + if err != nil { + return nil, xerrors.Errorf("converting to organization roles: %w", err) + } + + // All roles are added roles. Org member is always implied. + //nolint:gocritic + addedRoles := append(orgRoles, rbac.ScopedRoleOrgMember(arg.OrganizationID)) + err = q.canAssignRoles(ctx, arg.OrganizationID, addedRoles, []rbac.RoleIdentifier{}) + if err != nil { + return nil, err + } + + // Authorize creation for each user in the batch. + for _, uid := range arg.UserIds { + obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID).WithID(uid) + if err := q.authorizeContext(ctx, policy.ActionCreate, obj); err != nil { + return nil, err + } + } + + return q.db.InsertOrganizationMembersBatch(ctx, arg) +} + func (q *querier) InsertPreset(ctx context.Context, arg database.InsertPresetParams) (database.TemplateVersionPreset, error) { err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceTemplate) if err != nil { diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index cda2c66bbc595..39835c963f2ce 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -3672,6 +3672,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 e7bedcf10757c..b3840092f7089 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -6881,6 +6881,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 34858374c8a48..6b92de0607270 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -837,6 +837,10 @@ 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. + 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 94f9f58589362..65d91fc0c3961 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -15630,6 +15630,72 @@ 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 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. +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 78e7e3116327f..b7b035c017eb3 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/coderd/database/queries/organizationmembers.sql @@ -50,6 +50,28 @@ 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. +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 DO NOTHING +RETURNING *; + -- name: DeleteOrganizationMember :exec DELETE FROM diff --git a/coderd/members.go b/coderd/members.go index 70b8380f109ed..1bbac3af4f465 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -18,11 +18,12 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/searchquery" + "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) -// @Summary Add organization member +// @Summary Add organization member (deprecated) // @ID add-organization-member // @Security CoderSessionToken // @Produce json @@ -31,6 +32,7 @@ import ( // @Param user path string true "User ID, name, or me" // @Success 200 {object} codersdk.OrganizationMember // @Router /organizations/{organization}/members/{user} [post] +// @Deprecated use POST /organizations/{organization}/members instead func (api *API) postOrganizationMember(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -90,6 +92,137 @@ 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 /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() + ) + + sw, ok := rw.(*tracing.StatusWriter) + if !ok { + httpapi.InternalServerError(rw, xerrors.New("developer error: http.ResponseWriter is not *tracing.StatusWriter")) + return + } + + var req codersdk.AddOrganizationMembersRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + // auditMembers is populated after the transaction succeeds and + // read by the deferred audit closure once the final response + // status is known. usersByID maps each user ID to its database + // row so we can look up usernames for audit entries. On + // early-return error paths the slice stays nil, so no audit + // events are emitted. + var auditMembers []database.OrganizationMember + var usersByID map[uuid.UUID]database.User + defer func() { + for _, member := range auditMembers { + 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: sw.Status, + Old: database.AuditableOrganizationMember{}, + New: member.Auditable(usersByID[member.UserID].Username), + }) + } + }() + + // Resolve all users in a single query. 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 err != nil { + httpapi.InternalServerError(rw, err) + return + } + + usersByID = make(map[uuid.UUID]database.User, len(users)) + for _, u := range users { + usersByID[u.ID] = u + } + + // Check that every requested user was found and none are + // deleted. GetUsersByIDs intentionally includes deleted users + // (see query comment), so we reject them explicitly. + 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: %v", 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: %v", deleted), + }) + return + } + + // Validate OIDC org-sync constraints for all users before + // starting the transaction. + for _, user := range users { + if !api.manualOrganizationMembership(ctx, rw, user) { + 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() + allMembers, err := api.Database.InsertOrganizationMembersBatch(ctx, database.InsertOrganizationMembersBatchParams{ + OrganizationID: organization.ID, + UserIds: req.UserIDs, + CreatedAt: now, + UpdatedAt: now, + Roles: []string{}, + }) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + + // Populate the audit slice so the deferred closure emits + // events with the real response status code. + auditMembers = allMembers + + resp, err := convertOrganizationMembers(ctx, api.Database, allMembers) + 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 @@ -501,8 +634,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) diff --git a/coderd/members_test.go b/coderd/members_test.go index c2bf219c1ebc2..460437bcbb1f7 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -3,6 +3,7 @@ package coderd_test import ( "context" "database/sql" + "net/http" "testing" "github.com/google/uuid" @@ -50,6 +51,136 @@ 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) + }) +} + func TestDeleteMember(t *testing.T) { t.Parallel() diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 8c17b50e56932..f40c106ecc87b 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -100,6 +100,12 @@ 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 []uuid.UUID `json:"user_ids" validate:"required,gt=0,lte=100,dive" 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 90b3147c15ec1..0ae4a367099e2 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -615,7 +615,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 { @@ -629,6 +631,21 @@ 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. +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 96a30b0fa2ef8..2216e0bc3e0cd 100644 --- a/docs/reference/api/members.md +++ b/docs/reference/api/members.md @@ -102,6 +102,83 @@ Status Code **200** To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Batch add organization members + +### Code samples + +```shell +# 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 /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 @@ -627,7 +704,7 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/members To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Add organization member +## Add organization member (deprecated) ### Code samples diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 9d23823eaa9df..59a209df307bd 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1405,6 +1405,22 @@ |-----------|--------|----------|--------------|-------------| | `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 | | | + ## codersdk.AgentConnectionTiming ```json diff --git a/site/src/api/api.ts b/site/src/api/api.ts index f47d4972a35f4..8d60125146588 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -814,6 +814,18 @@ class ApiMethods { return response.data; }; + addOrganizationMembers = async ( + organization: string, + req: TypesGen.AddOrganizationMembersRequest, + ) => { + const response = await this.axios.post( + `/api/v2/organizations/${organization}/members`, + req, + ); + + return response.data; + }; + /** * @param organization Can be the organization's ID or name */ diff --git a/site/src/api/queries/organizations.ts b/site/src/api/queries/organizations.ts index 1dcaac36596f6..fc36a4559e193 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 c0abec1ba3c19..bbd98a63ed589 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -782,6 +782,15 @@ 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 { + readonly user_ids: readonly string[]; +} + // From codersdk/deployment.go export type Addon = "ai_governance"; diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx index 7e148675cddca..f7ae9caa0eef1 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx @@ -5,7 +5,7 @@ import { toast } from "sonner"; import { getErrorMessage } from "#/api/errors"; import { groupsByUserIdInOrganization } from "#/api/queries/groups"; import { - addOrganizationMember, + addOrganizationMembers, paginatedOrganizationMembers, removeOrganizationMember, updateOrganizationMemberRoles, @@ -60,8 +60,8 @@ const OrganizationMembersPage: FC = () => { }, ); - const addMemberMutation = useMutation( - addOrganizationMember(queryClient, organizationName), + const addMembersMutation = useMutation( + addOrganizationMembers(queryClient, organizationName), ); const removeMemberMutation = useMutation( removeOrganizationMember(queryClient, organizationName), @@ -104,7 +104,7 @@ const OrganizationMembersPage: FC = () => { membersQuery.error ?? organizationRolesQuery.error ?? groupsByUserIdQuery.error ?? - addMemberMutation.error ?? + addMembersMutation.error ?? removeMemberMutation.error ?? updateMemberRolesMutation.error } @@ -114,12 +114,7 @@ const OrganizationMembersPage: FC = () => { members={members} membersQuery={membersQuery} 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)), - ); + await addMembersMutation.mutateAsync(users.map((user) => user.id)); void membersQuery.refetch(); }} removeMember={setMemberToDelete} From e8eef0ed54afa065f4c9cf92de0f00616e7895b6 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 22 Jun 2026 17:01:34 +0000 Subject: [PATCH 02/14] =?UTF-8?q?=F0=9F=A4=96=20fix(coderd):=20address=20E?= =?UTF-8?q?myrk=20review=20on=20batch=20org=20member=20endpoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Simplify the dbauthz authorization for InsertOrganizationMembersBatch. The org-member create permission is independent of user ID, so a single org-scoped check replaces the per-user loop. - Return 403 instead of 500 when InsertOrganizationMembersBatch fails the dbauthz check by detecting httpapi.IsUnauthorizedError. - Add dbauthz_test case for InsertOrganizationMembersBatch so the method suite's accounting check passes. - Add AuditLogsOnlyForNewMembers test verifying that users who are already members do not generate audit log entries (ON CONFLICT DO NOTHING path). --- coderd/database/dbauthz/dbauthz.go | 9 ++--- coderd/database/dbauthz/dbauthz_test.go | 19 ++++++++++ coderd/members.go | 4 +++ coderd/members_test.go | 47 +++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 55701bcb6d09c..bd75fe2a9deef 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -6108,12 +6108,9 @@ func (q *querier) InsertOrganizationMembersBatch(ctx context.Context, arg databa return nil, err } - // Authorize creation for each user in the batch. - for _, uid := range arg.UserIds { - obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID).WithID(uid) - if err := q.authorizeContext(ctx, policy.ActionCreate, obj); err != nil { - return nil, err - } + obj := rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID) + if err := q.authorizeContext(ctx, policy.ActionCreate, obj); err != nil { + return nil, err } return q.db.InsertOrganizationMembersBatch(ctx, arg) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index aed9a9580b09c..82fd87be6413d 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2494,6 +2494,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{}}) + 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/members.go b/coderd/members.go index 33bce2b9dc853..0846ec76ff326 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -205,6 +205,10 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) UpdatedAt: now, Roles: []string{}, }) + if httpapi.IsUnauthorizedError(err) { + httpapi.Forbidden(rw) + return + } if err != nil { httpapi.InternalServerError(rw, err) return diff --git a/coderd/members_test.go b/coderd/members_test.go index 460437bcbb1f7..67cf18c083677 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -10,6 +10,7 @@ import ( "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" @@ -179,6 +180,52 @@ func TestAddMembers(t *testing.T) { require.Contains(t, ids, user1.ID) require.Contains(t, ids, user2.ID) }) + + 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) { From 875c34a6811caa2ce71352aea9d79b96a65595c6 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Tue, 23 Jun 2026 04:00:01 +0000 Subject: [PATCH 03/14] =?UTF-8?q?=F0=9F=A4=96=20test(coderd):=20cover=20de?= =?UTF-8?q?leted-user=20rejection=20in=20batch=20add=20members?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds TestAddMembers/DeletedUser, which soft-deletes a user, batch-adds them alongside a live user, and asserts the request is rejected with 400 "Deleted users cannot be added to an organization" and that the live user is not inserted (the rejection runs before the batch insert). --- coderd/members_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/coderd/members_test.go b/coderd/members_test.go index 67cf18c083677..9efa5921e8002 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -181,6 +181,45 @@ func TestAddMembers(t *testing.T) { require.Contains(t, ids, user2.ID) }) + 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() From fd443ef10b3e6463a77401a105f63c2c042e3719 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Tue, 23 Jun 2026 04:19:48 +0000 Subject: [PATCH 04/14] =?UTF-8?q?=F0=9F=A4=96=20fix(coderd):=20align=20dep?= =?UTF-8?q?recated=20org=20member=20@ID=20with=20summary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The swaggerparser test enforces that the Router @ID matches the @Summary slug. Renaming the singular endpoint summary to "Add organization member (deprecated)" produced the slug "add-organization-member-deprecated", but @ID was still "add-organization-member", so TestEndpointsDocumented and TestEnterpriseEndpointsDocumented failed. Update the @ID and regenerate apidoc/swagger. --- coderd/apidoc/docs.go | 2 +- coderd/apidoc/swagger.json | 2 +- coderd/members.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 3ffcd43e417c5..8c4577f37d9c4 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5144,7 +5144,7 @@ const docTemplate = `{ "Members" ], "summary": "Add organization member (deprecated)", - "operationId": "add-organization-member", + "operationId": "add-organization-member-deprecated", "deprecated": true, "parameters": [ { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index a97480932b8e4..14bc30b0e86f3 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4541,7 +4541,7 @@ "produces": ["application/json"], "tags": ["Members"], "summary": "Add organization member (deprecated)", - "operationId": "add-organization-member", + "operationId": "add-organization-member-deprecated", "deprecated": true, "parameters": [ { diff --git a/coderd/members.go b/coderd/members.go index 0846ec76ff326..475b3e13d970c 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -24,7 +24,7 @@ import ( ) // @Summary Add organization member (deprecated) -// @ID add-organization-member +// @ID add-organization-member-deprecated // @Security CoderSessionToken // @Produce json // @Tags Members From b81945a6127bc7c9756b1a854f1a595e5501ca52 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Tue, 23 Jun 2026 04:49:44 +0000 Subject: [PATCH 05/14] =?UTF-8?q?=F0=9F=A4=96=20refactor(coderd):=20tighte?= =?UTF-8?q?n=20batch=20org=20members=20handler=20and=20authz?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - coderd/database/dbauthz: include the org default member roles in the canAssignRoles check for InsertOrganizationMembersBatch, mirroring the pattern that was added to InsertOrganizationMember in PR #25994. This closes a gap where the batch endpoint could bypass authorization on the implicit default roles. - coderd/members: emit audit events synchronously after a successful batch insert (Status hardcoded to 201 Created), instead of relying on a deferred closure that observed the wrapped status writer. Surface GetUsersByIDs authz failures as 403. Hoist the OrganizationSyncEnabled check out of the per-user loop and report all OIDC-blocked users in a single 400 response. - site: drop the redundant members refetch in the add-members handler; the mutation's onSuccess already invalidates the members query key. --- coderd/database/dbauthz/dbauthz.go | 14 ++++ coderd/database/dbauthz/dbauthz_test.go | 2 +- coderd/members.go | 78 +++++++++---------- .../OrganizationMembersPage.tsx | 1 - 4 files changed, 53 insertions(+), 42 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index bd75fe2a9deef..52d7616a9c291 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -6100,9 +6100,23 @@ func (q *querier) InsertOrganizationMembersBatch(ctx context.Context, arg databa return nil, xerrors.Errorf("converting to organization roles: %w", 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 nil, xerrors.Errorf("get organization: %w", err) + } + defaultRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, org.DefaultOrgMemberRoles) + if err != nil { + return nil, xerrors.Errorf("convert default member roles: %w", 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 nil, err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 82fd87be6413d..8328b0de45288 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2495,7 +2495,7 @@ func (s *MethodTestSuite) TestOrganization() { ) })) s.Run("InsertOrganizationMembersBatch", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { - o := testutil.Fake(s.T(), faker, database.Organization{DefaultOrgMemberRoles: []string{}}) + 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{ diff --git a/coderd/members.go b/coderd/members.go index 475b3e13d970c..e2bc7c70fe6f0 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -18,7 +18,6 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/searchquery" - "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -110,52 +109,25 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) auditor = api.Auditor.Load() ) - sw, ok := rw.(*tracing.StatusWriter) - if !ok { - httpapi.InternalServerError(rw, xerrors.New("developer error: http.ResponseWriter is not *tracing.StatusWriter")) - return - } - var req codersdk.AddOrganizationMembersRequest if !httpapi.Read(ctx, rw, r, &req) { return } - // auditMembers is populated after the transaction succeeds and - // read by the deferred audit closure once the final response - // status is known. usersByID maps each user ID to its database - // row so we can look up usernames for audit entries. On - // early-return error paths the slice stays nil, so no audit - // events are emitted. - var auditMembers []database.OrganizationMember - var usersByID map[uuid.UUID]database.User - defer func() { - for _, member := range auditMembers { - 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: sw.Status, - Old: database.AuditableOrganizationMember{}, - New: member.Auditable(usersByID[member.UserID].Username), - }) - } - }() - // Resolve all users in a single query. 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)) + usersByID := make(map[uuid.UUID]database.User, len(users)) for _, u := range users { usersByID[u.ID] = u } @@ -186,10 +158,21 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) return } - // Validate OIDC org-sync constraints for all users before - // starting the transaction. - for _, user := range users { - if !api.manualOrganizationMembership(ctx, rw, user) { + // Validate OIDC org-sync constraints for all users. Resolve the feature flag + // once outside the loop, and report every offending user 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, codersdk.Response{ + Message: "Organization sync is enabled for OIDC users, meaning manual organization assignment is not allowed for these users. Have the users re-login to refresh their organizations.", + Detail: fmt.Sprintf("Users %v are OIDC users and organization sync is enabled. Ask an administrator to resolve the membership in your external IDP.", oidcUsernames), + }) return } } @@ -214,9 +197,24 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) return } - // Populate the audit slice so the deferred closure emits - // events with the real response status code. - auditMembers = allMembers + // Emit audit events synchronously once the insert succeeds. The response + // status is fixed at 201 from this point, so we record it directly instead + // of routing through a deferred closure that would otherwise have to + // observe the final HTTP status. + for _, member := range allMembers { + 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, allMembers) if err != nil { diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx index dc0c14647b53c..3de60a1365ef4 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.tsx @@ -134,7 +134,6 @@ const OrganizationMembersPage: FC = () => { showAISeatColumn={showAISeatColumn} addMembers={async (users: User[]) => { await addMembersMutation.mutateAsync(users.map((user) => user.id)); - void membersQuery.refetch(); }} onEditMemberRoles={setMemberToEditRoles} isUpdatingMemberRoles={updateMemberRolesMutation.isPending} From 20eef3edde87df2bf0d8b8a710aa27820e965b04 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Thu, 25 Jun 2026 03:03:50 +0000 Subject: [PATCH 06/14] =?UTF-8?q?=F0=9F=A4=96=20refactor(codersdk):=20tigh?= =?UTF-8?q?ten=20AddOrganizationMembersRequest=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch the user_ids tag to `required,min=1,max=100`. The previous `gt=0,lte=100,dive` form had a trailing `dive` with no per-element rules, which made the tag look like each UUID was being length-bound instead of the slice. The slice-length constraints are now stated once, the no-op `dive` is gone, and a docstring documents the 1..100 range. The generated swagger now also emits `minItems: 1` for the field. --- coderd/apidoc/docs.go | 2 ++ coderd/apidoc/swagger.json | 2 ++ codersdk/organizations.go | 4 +++- docs/reference/api/schemas.md | 6 +++--- site/src/api/typesGenerated.ts | 4 ++++ 5 files changed, 14 insertions(+), 4 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 8c4577f37d9c4..f5a70cf1ffcbf 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -16014,8 +16014,10 @@ const docTemplate = `{ ], "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.", "type": "array", "maxItems": 100, + "minItems": 1, "items": { "type": "string", "format": "uuid" diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 14bc30b0e86f3..6b28352329e86 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -14344,8 +14344,10 @@ "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.", "type": "array", "maxItems": 100, + "minItems": 1, "items": { "type": "string", "format": "uuid" diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 0fce8cef2a8fc..cd9e0fd4a6afd 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -107,7 +107,9 @@ type PaginatedMembersResponse struct { // AddOrganizationMembersRequest provides a list of user IDs to add // as members to an organization in a single batch request. type AddOrganizationMembersRequest struct { - UserIDs []uuid.UUID `json:"user_ids" validate:"required,gt=0,lte=100,dive" format:"uuid"` + // UserIDs is the list of user IDs to add as organization members. The + // slice must contain between 1 and 100 IDs. + UserIDs []uuid.UUID `json:"user_ids" validate:"required,min=1,max=100" format:"uuid"` } type CreateOrganizationRequest struct { diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 24758c0819003..6cb72896f5a02 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1231,9 +1231,9 @@ None ### Properties -| Name | Type | Required | Restrictions | Description | -|------------|-----------------|----------|--------------|-------------| -| `user_ids` | array of string | true | | | +| 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. | ## codersdk.AgentChatSendShortcut diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index fdffdbc6c9ac1..2883d527ca839 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -979,6 +979,10 @@ export interface AddLicenseRequest { * 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. + */ readonly user_ids: readonly string[]; } From 69e5d6fc068e8ec715f3a40c7b3eae1fa436d59d Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 20 Jul 2026 04:27:24 +0000 Subject: [PATCH 07/14] =?UTF-8?q?=F0=9F=A4=96=20chore(docs):=20regenerate?= =?UTF-8?q?=20members=20API=20reference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reference/api/members.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/api/members.md b/docs/reference/api/members.md index 8c2de4bd4f19b..6e13a1e03199b 100644 --- a/docs/reference/api/members.md +++ b/docs/reference/api/members.md @@ -106,7 +106,7 @@ To perform this operation, you must be authenticated. [Learn more](authenticatio ### Code samples -```shell +```sh # Example request using curl curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/members \ -H 'Content-Type: application/json' \ From 9126b517434be111ce9efc888674e69f5bfce07d Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 20 Jul 2026 04:36:14 +0000 Subject: [PATCH 08/14] =?UTF-8?q?=F0=9F=A4=96=20refactor(coderd):=20dedupe?= =?UTF-8?q?=20OIDC=20org-sync=20block=20message=20and=20cover=20duplicate?= =?UTF-8?q?=20user=20IDs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review nits on the batch member endpoint: - Factor the OIDC manual-membership block message into a shared manualMembershipBlockedResponse helper used by both the single-member and batch handlers so the wording stays in sync (CRF-2). - Add TestAddMembers/DuplicateUserIDs covering a request that repeats the same user ID (CRF-4). --- coderd/members.go | 28 ++++++++++++++++++++-------- coderd/members_test.go | 23 +++++++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/coderd/members.go b/coderd/members.go index e2bc7c70fe6f0..0020003ba8c10 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -169,10 +169,7 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) } } if len(oidcUsernames) > 0 { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Organization sync is enabled for OIDC users, meaning manual organization assignment is not allowed for these users. Have the users re-login to refresh their organizations.", - Detail: fmt.Sprintf("Users %v are OIDC users and organization sync is enabled. Ask an administrator to resolve the membership in your external IDP.", oidcUsernames), - }) + httpapi.Write(ctx, rw, http.StatusBadRequest, manualMembershipBlockedResponse(oidcUsernames)) return } } @@ -741,11 +738,26 @@ 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 %v are OIDC users", 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), + } +} diff --git a/coderd/members_test.go b/coderd/members_test.go index 9efa5921e8002..79827eb869953 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -181,6 +181,29 @@ func TestAddMembers(t *testing.T) { 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. GetUsersByIDs + // dedupes on lookup and the batch insert uses ON CONFLICT DO + // NOTHING, 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("DeletedUser", func(t *testing.T) { t.Parallel() owner, db := coderdtest.NewWithDatabase(t, nil) From 23d38a0a3eb71c3a39cb4fc406a343b7827a81ac Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 20 Jul 2026 04:47:49 +0000 Subject: [PATCH 09/14] =?UTF-8?q?=F0=9F=A4=96=20refactor(site):=20remove?= =?UTF-8?q?=20dead=20addOrganizationMember=20API=20method?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batch addOrganizationMembers endpoint replaced the last caller of the singular addOrganizationMember method, leaving it unused. Remove it (CRF-3). --- site/src/api/api.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 58f0f445cf0f1..144539449aeb1 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -840,17 +840,6 @@ 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}`, - ); - - return response.data; - }; - addOrganizationMembers = async ( organization: string, req: TypesGen.AddOrganizationMembersRequest, From c61b66d388e0311ae6c7b6cd776da2decc4b33a2 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 20 Jul 2026 05:42:42 +0000 Subject: [PATCH 10/14] =?UTF-8?q?=F0=9F=A4=96=20refactor(coderd):=20addres?= =?UTF-8?q?s=20batch=20org=20member=20review=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel review follow-ups for the batch add-organization-members endpoint: - CRF-8/CRF-22: extract shared authorizeOrganizationMemberInsert helper so the single-member and batch dbauthz wrappers cannot drift; use slices.Concat and drop the gocritic suppression. - CRF-11: render UUID/username lists in error messages via strings.Join instead of Go's bracketed %v slice format. - CRF-12/CRF-13: restore the deprecated endpoint's @ID add-organization-member and move the migration hint into @Description so it reaches the docs. - CRF-16: document why the batch cap is 100. - CRF-17: name the ON CONFLICT target (organization_id, user_id). - CRF-19/CRF-20/CRF-21: correct/trim comments to describe actual behaviour. - CRF-5: assert the batch endpoint rejects OIDC-synced users (enterprise). - CRF-9: Storybook play coverage for the batch add-members flow. - CRF-15: cap dialog selection at 100 to avoid a blind 400. CRF-18 (RETURNING order): kept the ElementsMatch test discipline rather than a CTE, which would change the generated row type. --- coderd/apidoc/docs.go | 5 +- coderd/apidoc/swagger.json | 5 +- coderd/database/dbauthz/dbauthz.go | 84 +++++++-------- coderd/database/querier.go | 3 +- coderd/database/queries.sql.go | 5 +- .../database/queries/organizationmembers.sql | 5 +- coderd/members.go | 35 +++--- coderd/members_test.go | 6 +- codersdk/organizations.go | 4 +- docs/reference/api/members.md | 2 + docs/reference/api/schemas.md | 6 +- enterprise/coderd/userauth_test.go | 6 ++ site/src/api/typesGenerated.ts | 4 +- .../OrganizationMembersPage.stories.tsx | 100 ++++++++++++++++++ .../OrganizationMembersPageView.tsx | 24 ++++- 15 files changed, 216 insertions(+), 78 deletions(-) create mode 100644 site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index adac873b96606..6f229bcfa7369 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5166,6 +5166,7 @@ const docTemplate = `{ ] }, "post": { + "description": "Deprecated: use POST /organizations/{organization}/members instead.", "produces": [ "application/json" ], @@ -5173,7 +5174,7 @@ const docTemplate = `{ "Members" ], "summary": "Add organization member (deprecated)", - "operationId": "add-organization-member-deprecated", + "operationId": "add-organization-member", "deprecated": true, "parameters": [ { @@ -16118,7 +16119,7 @@ const docTemplate = `{ ], "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.", + "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, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 723d9aeba3bb3..c8292c23c22d7 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4565,10 +4565,11 @@ ] }, "post": { + "description": "Deprecated: use POST /organizations/{organization}/members instead.", "produces": ["application/json"], "tags": ["Members"], "summary": "Add organization member (deprecated)", - "operationId": "add-organization-member-deprecated", + "operationId": "add-organization-member", "deprecated": true, "parameters": [ { @@ -14439,7 +14440,7 @@ "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.", + "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, diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index fb69aea5a8f44..51e82af74c097 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1697,6 +1697,37 @@ func scopedOrgRoleIdentifiers(names []string, orgID uuid.UUID) []rbac.RoleIdenti return out } +// authorizeOrganizationMemberInsert authorizes granting the given roles when +// adding a member to an organization. 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; they diverge +// only in the object the ActionCreate is authorized against. +func (q *querier) authorizeOrganizationMemberInsert(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,65 +6232,22 @@ 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) - } - - // 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) - } - - // 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 { + if err := q.authorizeOrganizationMemberInsert(ctx, arg.OrganizationID, arg.Roles); err != nil { return database.OrganizationMember{}, 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) { - orgRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, arg.Roles) - if err != nil { - return nil, xerrors.Errorf("converting to organization roles: %w", 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 nil, xerrors.Errorf("get organization: %w", err) - } - defaultRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, org.DefaultOrgMemberRoles) - if err != nil { - return nil, xerrors.Errorf("convert default member roles: %w", 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 { + if err := q.authorizeOrganizationMemberInsert(ctx, arg.OrganizationID, arg.Roles); err != nil { return nil, 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 diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 52658fd991102..a816d64101af6 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1094,7 +1094,8 @@ type sqlcQuerier interface { 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. + // 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) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 6c4a47382113d..b8070e0560cdf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -19625,7 +19625,7 @@ SELECT $4 :: text[] FROM UNNEST($5 :: uuid[]) AS user_id -ON CONFLICT DO NOTHING +ON CONFLICT (organization_id, user_id) DO NOTHING RETURNING user_id, organization_id, created_at, updated_at, roles ` @@ -19639,7 +19639,8 @@ type InsertOrganizationMembersBatchParams struct { // 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. +// 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, diff --git a/coderd/database/queries/organizationmembers.sql b/coderd/database/queries/organizationmembers.sql index b7b035c017eb3..249a975307df9 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/coderd/database/queries/organizationmembers.sql @@ -53,7 +53,8 @@ VALUES -- 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. +-- 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, @@ -69,7 +70,7 @@ SELECT @roles :: text[] FROM UNNEST(@user_ids :: uuid[]) AS user_id -ON CONFLICT DO NOTHING +ON CONFLICT (organization_id, user_id) DO NOTHING RETURNING *; -- name: DeleteOrganizationMember :exec diff --git a/coderd/members.go b/coderd/members.go index 0020003ba8c10..19947f9459789 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" @@ -23,7 +24,7 @@ import ( ) // @Summary Add organization member (deprecated) -// @ID add-organization-member-deprecated +// @ID add-organization-member // @Security CoderSessionToken // @Produce json // @Tags Members @@ -31,7 +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 use POST /organizations/{organization}/members instead +// @Deprecated +// @Description Deprecated: use POST /organizations/{organization}/members instead. func (api *API) postOrganizationMember(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -147,20 +149,19 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) } if len(missing) > 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Users not found: %v", missing), + 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: %v", deleted), + Message: fmt.Sprintf("Deleted users cannot be added to an organization: %s", joinUUIDs(deleted)), }) return } - // Validate OIDC org-sync constraints for all users. Resolve the feature flag - // once outside the loop, and report every offending user together so the - // caller can fix the request in a single round-trip. + // 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 { @@ -194,10 +195,10 @@ func (api *API) postOrganizationMembers(rw http.ResponseWriter, r *http.Request) return } - // Emit audit events synchronously once the insert succeeds. The response - // status is fixed at 201 from this point, so we record it directly instead - // of routing through a deferred closure that would otherwise have to - // observe the final HTTP status. + // Emit audit events inline once the insert succeeds. The response status is + // fixed at 201 from this point, so we record it directly instead of routing + // through a deferred closure that would otherwise have to observe the final + // HTTP status. for _, member := range allMembers { audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.AuditableOrganizationMember]{ Audit: *auditor, @@ -754,10 +755,20 @@ func manualMembershipBlockedResponse(usernames []string) codersdk.Response { 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 %v are OIDC users", usernames) + 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_test.go b/coderd/members_test.go index 79827eb869953..ba0885da8e851 100644 --- a/coderd/members_test.go +++ b/coderd/members_test.go @@ -192,9 +192,9 @@ func TestAddMembers(t *testing.T) { _, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) - // The same user ID appears twice in the request. GetUsersByIDs - // dedupes on lookup and the batch insert uses ON CONFLICT DO - // NOTHING, so the user is added exactly once with no error. + // 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}, diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 394801c64b481..32c84b9c18eff 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -108,7 +108,9 @@ type PaginatedMembersResponse struct { // 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. + // 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"` } diff --git a/docs/reference/api/members.md b/docs/reference/api/members.md index 6e13a1e03199b..bbd0197cec2fe 100644 --- a/docs/reference/api/members.md +++ b/docs/reference/api/members.md @@ -717,6 +717,8 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/member `POST /api/v2/organizations/{organization}/members/{user}` +Deprecated: use POST /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 8a4558e6ab4e8..4e3776f3868e2 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1258,9 +1258,9 @@ None ### 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. | +| 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 diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 5a0986788acea..4ce355ee6d192 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/typesGenerated.ts b/site/src/api/typesGenerated.ts index 4055acd7a4992..9215ebfa5f392 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1049,7 +1049,9 @@ export interface AddLicenseRequest { export interface AddOrganizationMembersRequest { /** * UserIDs is the list of user IDs to add as organization members. The - * slice must contain between 1 and 100 IDs. + * 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[]; } diff --git a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPage.stories.tsx new file mode 100644 index 0000000000000..0f244f992a15c --- /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/OrganizationMembersPageView.tsx b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx index 3a6b2e1654bd2..105ce17a35c54 100644 --- a/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx +++ b/site/src/pages/OrganizationSettingsPage/OrganizationMembersPageView.tsx @@ -77,6 +77,10 @@ export const OrganizationMembersPageView: React.FC< ); }; +// The server caps a single add-members request at 100 user IDs, so keep the +// selection within the same bound to avoid a blind 400 on submit. +const MAX_MEMBERS_PER_ADD = 100; + interface AddUsersDialogProps { onSubmit: (users: User[]) => Promise; } @@ -118,6 +122,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 +132,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. + + )} +