diff --git a/coderd/coderd.go b/coderd/coderd.go index e08a2a3036885..ab65efcb3087e 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -573,6 +573,15 @@ func New(options *Options) *API { // bugs that may only occur when a key isn't precached in tests and the latency cost is minimal. cryptokeys.StartRotator(ctx, options.Logger, options.Database) + // Ensure all system role permissions are current. + //nolint:gocritic // We need to manage system roles + err = rolestore.ReconcileSystemRoles(dbauthz.AsSystemRestricted(ctx), options.Logger, options.Database) + if err != nil { + // Not ideal, but not using Fatal here and just continuing + // after logging the error would be a potential security hole. + options.Logger.Fatal(ctx, "failed to reconcile system role permissions", slog.Error(err)) + } + // AGPL uses a no-op build usage checker as there are no license // entitlements to enforce. This is swapped out in // enterprise/coderd/coderd.go. diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 4962949f7fd49..58b70672a675e 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -4110,6 +4110,8 @@ func (q *querier) InsertCustomRole(ctx context.Context, arg database.InsertCusto return database.CustomRole{}, err } + // TODO(geokat): should we add MemberPermissions validation too + // now that we have them in system roles? if err := q.customRoleCheck(ctx, database.CustomRole{ Name: arg.Name, DisplayName: arg.DisplayName, @@ -4864,6 +4866,8 @@ func (q *querier) UpdateCustomRole(ctx context.Context, arg database.UpdateCusto return database.CustomRole{}, err } + // TODO(geokat): should we add MemberPermissions validation too + // now that we have them in system roles? if err := q.customRoleCheck(ctx, database.CustomRole{ Name: arg.Name, DisplayName: arg.DisplayName, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 790659669f004..67ba1115aa12d 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -746,6 +746,37 @@ BEGIN END; $$; +CREATE FUNCTION insert_org_member_system_role() RETURNS trigger + LANGUAGE plpgsql + AS $$ +BEGIN + INSERT INTO custom_roles ( + name, + display_name, + organization_id, + site_permissions, + org_permissions, + user_permissions, + member_permissions, + is_system, + created_at, + updated_at + ) VALUES ( + 'organization-member', + '', + NEW.id, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + true, + NOW(), + NOW() + ); + RETURN NEW; +END; +$$; + CREATE FUNCTION insert_user_links_fail_if_user_deleted() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -1202,7 +1233,9 @@ CREATE TABLE custom_roles ( created_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, updated_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, organization_id uuid, - id uuid DEFAULT gen_random_uuid() NOT NULL + id uuid DEFAULT gen_random_uuid() NOT NULL, + is_system boolean DEFAULT false NOT NULL, + member_permissions jsonb DEFAULT '[]'::jsonb NOT NULL ); COMMENT ON TABLE custom_roles IS 'Custom roles allow dynamic roles expanded at runtime'; @@ -1211,6 +1244,8 @@ COMMENT ON COLUMN custom_roles.organization_id IS 'Roles can optionally be scope COMMENT ON COLUMN custom_roles.id IS 'Custom roles ID is used purely for auditing purposes. Name is a better unique identifier.'; +COMMENT ON COLUMN custom_roles.is_system IS 'System roles are managed by Coder and cannot be modified or deleted by users.'; + CREATE TABLE dbcrypt_keys ( number integer NOT NULL, active_key_digest text, @@ -1594,7 +1629,8 @@ CREATE TABLE organizations ( is_default boolean DEFAULT false NOT NULL, display_name text NOT NULL, icon text DEFAULT ''::text NOT NULL, - deleted boolean DEFAULT false NOT NULL + deleted boolean DEFAULT false NOT NULL, + workspace_sharing_disabled boolean DEFAULT false NOT NULL ); CREATE TABLE parameter_schemas ( @@ -3549,6 +3585,8 @@ CREATE TRIGGER trigger_delete_oauth2_provider_app_token AFTER DELETE ON oauth2_p CREATE TRIGGER trigger_insert_apikeys BEFORE INSERT ON api_keys FOR EACH ROW EXECUTE FUNCTION insert_apikey_fail_if_user_deleted(); +CREATE TRIGGER trigger_insert_org_member_system_role AFTER INSERT ON organizations FOR EACH ROW EXECUTE FUNCTION insert_org_member_system_role(); + CREATE TRIGGER trigger_nullify_next_start_at_on_workspace_autostart_modificati AFTER UPDATE ON workspaces FOR EACH ROW EXECUTE FUNCTION nullify_next_start_at_on_workspace_autostart_modification(); CREATE TRIGGER trigger_update_users AFTER INSERT OR UPDATE ON users FOR EACH ROW WHEN ((new.deleted = true)) EXECUTE FUNCTION delete_deleted_user_resources(); diff --git a/coderd/database/lock.go b/coderd/database/lock.go index e5091cdfd29cc..16aff016906ad 100644 --- a/coderd/database/lock.go +++ b/coderd/database/lock.go @@ -13,6 +13,7 @@ const ( LockIDNotificationsReportGenerator LockIDCryptoKeyRotation LockIDReconcilePrebuilds + LockIDReconcileSystemRoles ) // GenLockID generates a unique and consistent lock ID from a given string. diff --git a/coderd/database/migrations/000405_add_system_role_support.down.sql b/coderd/database/migrations/000405_add_system_role_support.down.sql new file mode 100644 index 0000000000000..8198c7b1c7431 --- /dev/null +++ b/coderd/database/migrations/000405_add_system_role_support.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE custom_roles DROP COLUMN IF EXISTS member_permissions; + +ALTER TABLE custom_roles DROP COLUMN IF EXISTS is_system; diff --git a/coderd/database/migrations/000405_add_system_role_support.up.sql b/coderd/database/migrations/000405_add_system_role_support.up.sql new file mode 100644 index 0000000000000..9fbc99afb8491 --- /dev/null +++ b/coderd/database/migrations/000405_add_system_role_support.up.sql @@ -0,0 +1,10 @@ +-- Add is_system column to identify system-managed roles. +ALTER TABLE custom_roles + ADD COLUMN is_system boolean NOT NULL DEFAULT false; + +-- Add member_permissions column for member-scoped permissions within an organization. +ALTER TABLE custom_roles + ADD COLUMN member_permissions jsonb NOT NULL DEFAULT '[]'::jsonb; + +COMMENT ON COLUMN custom_roles.is_system IS + 'System roles are managed by Coder and cannot be modified or deleted by users.'; diff --git a/coderd/database/migrations/000406_add_workspace_sharing_disabled.down.sql b/coderd/database/migrations/000406_add_workspace_sharing_disabled.down.sql new file mode 100644 index 0000000000000..cc35c25e868d6 --- /dev/null +++ b/coderd/database/migrations/000406_add_workspace_sharing_disabled.down.sql @@ -0,0 +1 @@ +ALTER TABLE organizations DROP COLUMN IF EXISTS workspace_sharing_disabled; diff --git a/coderd/database/migrations/000406_add_workspace_sharing_disabled.up.sql b/coderd/database/migrations/000406_add_workspace_sharing_disabled.up.sql new file mode 100644 index 0000000000000..a5563107d62e8 --- /dev/null +++ b/coderd/database/migrations/000406_add_workspace_sharing_disabled.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE organizations + ADD COLUMN workspace_sharing_disabled boolean NOT NULL DEFAULT false; diff --git a/coderd/database/migrations/000407_create_org_member_system_roles.down.sql b/coderd/database/migrations/000407_create_org_member_system_roles.down.sql new file mode 100644 index 0000000000000..9c352650b79a9 --- /dev/null +++ b/coderd/database/migrations/000407_create_org_member_system_roles.down.sql @@ -0,0 +1,6 @@ +-- Drop the trigger and function created by the up migration. +DROP TRIGGER IF EXISTS trigger_insert_org_member_system_role ON organizations; +DROP FUNCTION IF EXISTS insert_org_member_system_role; + +-- Remove organization-member system roles created by the up migration. +DELETE FROM custom_roles WHERE name = 'organization-member' AND is_system = true; diff --git a/coderd/database/migrations/000407_create_org_member_system_roles.up.sql b/coderd/database/migrations/000407_create_org_member_system_roles.up.sql new file mode 100644 index 0000000000000..03ce3e2572e72 --- /dev/null +++ b/coderd/database/migrations/000407_create_org_member_system_roles.up.sql @@ -0,0 +1,81 @@ +-- Create placeholder organization-member system roles for existing +-- organizations. Also add a trigger that creates the placeholder role +-- when an organization is created. Permissions will be empty until +-- populated by the reconciliation routine. +-- +-- Note: why do all this in the database (as opposed to coderd)? Less +-- room for race conditions. If the role doesn't exist when coderd +-- expects it, the only correct option is to panic. On the other hand, +-- a placeholder role with empty permissions is harmless and the +-- reconciliation process is idempotent. + +-- Create roles for the existing organizations. +INSERT INTO custom_roles ( + name, + display_name, + organization_id, + site_permissions, + org_permissions, + user_permissions, + member_permissions, + is_system, + created_at, + updated_at +) +SELECT + 'organization-member', -- reserved role name, so it doesn't exist in DB yet + '', + id, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + true, + NOW(), + NOW() +FROM + organizations +WHERE + NOT EXISTS ( + SELECT 1 + FROM custom_roles + WHERE + custom_roles.name = 'organization-member' + AND custom_roles.organization_id = organizations.id + ); + +-- When we insert a new organization, we also want to create a +-- placeholder org-member system role for it. +CREATE OR REPLACE FUNCTION insert_org_member_system_role() RETURNS trigger AS $$ +BEGIN + INSERT INTO custom_roles ( + name, + display_name, + organization_id, + site_permissions, + org_permissions, + user_permissions, + member_permissions, + is_system, + created_at, + updated_at + ) VALUES ( + 'organization-member', + '', + NEW.id, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + '[]'::jsonb, + true, + NOW(), + NOW() + ); + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER trigger_insert_org_member_system_role + AFTER INSERT ON organizations + FOR EACH ROW + EXECUTE FUNCTION insert_org_member_system_role(); diff --git a/coderd/database/models.go b/coderd/database/models.go index 3bb4097f3398c..be9ee776ffb0e 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3741,6 +3741,9 @@ type CustomRole struct { OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` // Custom roles ID is used purely for auditing purposes. Name is a better unique identifier. ID uuid.UUID `db:"id" json:"id"` + // System roles are managed by Coder and cannot be modified or deleted by users. + IsSystem bool `db:"is_system" json:"is_system"` + MemberPermissions CustomRolePermissions `db:"member_permissions" json:"member_permissions"` } // A table used to store the keys used to encrypt the database. @@ -4006,15 +4009,16 @@ type OAuth2ProviderAppToken struct { } type Organization struct { - ID uuid.UUID `db:"id" json:"id"` - Name string `db:"name" json:"name"` - Description string `db:"description" json:"description"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - UpdatedAt time.Time `db:"updated_at" json:"updated_at"` - IsDefault bool `db:"is_default" json:"is_default"` - DisplayName string `db:"display_name" json:"display_name"` - Icon string `db:"icon" json:"icon"` - Deleted bool `db:"deleted" json:"deleted"` + ID uuid.UUID `db:"id" json:"id"` + Name string `db:"name" json:"name"` + Description string `db:"description" json:"description"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + IsDefault bool `db:"is_default" json:"is_default"` + DisplayName string `db:"display_name" json:"display_name"` + Icon string `db:"icon" json:"icon"` + Deleted bool `db:"deleted" json:"deleted"` + WorkspaceSharingDisabled bool `db:"workspace_sharing_disabled" json:"workspace_sharing_disabled"` } type OrganizationMember struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c803d569365ca..4acbcf9acc0f0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7808,7 +7808,7 @@ func (q *sqlQuerier) UpdateMemberRoles(ctx context.Context, arg UpdateMemberRole const getDefaultOrganization = `-- name: GetDefaultOrganization :one SELECT - id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled FROM organizations WHERE @@ -7830,13 +7830,14 @@ func (q *sqlQuerier) GetDefaultOrganization(ctx context.Context) (Organization, &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ) return i, err } const getOrganizationByID = `-- name: GetOrganizationByID :one SELECT - id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled FROM organizations WHERE @@ -7856,13 +7857,14 @@ func (q *sqlQuerier) GetOrganizationByID(ctx context.Context, id uuid.UUID) (Org &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ) return i, err } const getOrganizationByName = `-- name: GetOrganizationByName :one SELECT - id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled FROM organizations WHERE @@ -7891,6 +7893,7 @@ func (q *sqlQuerier) GetOrganizationByName(ctx context.Context, arg GetOrganizat &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ) return i, err } @@ -7961,7 +7964,7 @@ func (q *sqlQuerier) GetOrganizationResourceCountByID(ctx context.Context, organ const getOrganizations = `-- name: GetOrganizations :many SELECT - id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled FROM organizations WHERE @@ -8005,6 +8008,7 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context, arg GetOrganizationsP &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ); err != nil { return nil, err } @@ -8021,7 +8025,7 @@ func (q *sqlQuerier) GetOrganizations(ctx context.Context, arg GetOrganizationsP const getOrganizationsByUserID = `-- name: GetOrganizationsByUserID :many SELECT - id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled FROM organizations WHERE @@ -8066,6 +8070,7 @@ func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, arg GetOrgani &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ); err != nil { return nil, err } @@ -8085,7 +8090,7 @@ INSERT INTO organizations (id, "name", display_name, description, icon, created_at, updated_at, is_default) VALUES -- If no organizations exist, and this is the first, make it the default. - ($1, $2, $3, $4, $5, $6, $7, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING id, name, description, created_at, updated_at, is_default, display_name, icon, deleted + ($1, $2, $3, $4, $5, $6, $7, (SELECT TRUE FROM organizations LIMIT 1) IS NULL) RETURNING id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled ` type InsertOrganizationParams struct { @@ -8119,6 +8124,7 @@ func (q *sqlQuerier) InsertOrganization(ctx context.Context, arg InsertOrganizat &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ) return i, err } @@ -8134,7 +8140,7 @@ SET icon = $5 WHERE id = $6 -RETURNING id, name, description, created_at, updated_at, is_default, display_name, icon, deleted +RETURNING id, name, description, created_at, updated_at, is_default, display_name, icon, deleted, workspace_sharing_disabled ` type UpdateOrganizationParams struct { @@ -8166,6 +8172,7 @@ func (q *sqlQuerier) UpdateOrganization(ctx context.Context, arg UpdateOrganizat &i.DisplayName, &i.Icon, &i.Deleted, + &i.WorkspaceSharingDisabled, ) return i, err } @@ -11927,7 +11934,7 @@ func (q *sqlQuerier) UpdateReplica(ctx context.Context, arg UpdateReplicaParams) const customRoles = `-- name: CustomRoles :many SELECT - name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id + name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id, is_system, member_permissions FROM custom_roles WHERE @@ -11950,16 +11957,30 @@ WHERE organization_id = $3 ELSE true END + -- Filter system roles. By default, system roles are excluded. + -- System roles are managed by Coder and should be hidden from user-facing APIs. + -- The authorization system uses @include_system_roles = true to load them. + AND CASE WHEN $4 :: boolean THEN + true + ELSE + is_system = false + END ` type CustomRolesParams struct { - LookupRoles []NameOrganizationPair `db:"lookup_roles" json:"lookup_roles"` - ExcludeOrgRoles bool `db:"exclude_org_roles" json:"exclude_org_roles"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + LookupRoles []NameOrganizationPair `db:"lookup_roles" json:"lookup_roles"` + ExcludeOrgRoles bool `db:"exclude_org_roles" json:"exclude_org_roles"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + IncludeSystemRoles bool `db:"include_system_roles" json:"include_system_roles"` } func (q *sqlQuerier) CustomRoles(ctx context.Context, arg CustomRolesParams) ([]CustomRole, error) { - rows, err := q.db.QueryContext(ctx, customRoles, pq.Array(arg.LookupRoles), arg.ExcludeOrgRoles, arg.OrganizationID) + rows, err := q.db.QueryContext(ctx, customRoles, + pq.Array(arg.LookupRoles), + arg.ExcludeOrgRoles, + arg.OrganizationID, + arg.IncludeSystemRoles, + ) if err != nil { return nil, err } @@ -11977,6 +11998,8 @@ func (q *sqlQuerier) CustomRoles(ctx context.Context, arg CustomRolesParams) ([] &i.UpdatedAt, &i.OrganizationID, &i.ID, + &i.IsSystem, + &i.MemberPermissions, ); err != nil { return nil, err } @@ -11997,6 +12020,9 @@ DELETE FROM WHERE name = lower($1) AND organization_id = $2 + -- Prevents accidental deletion of system roles even if the API + -- layer check is bypassed due to a bug. + AND is_system = false ` type DeleteCustomRoleParams struct { @@ -12018,6 +12044,8 @@ INSERT INTO site_permissions, org_permissions, user_permissions, + member_permissions, + is_system, created_at, updated_at ) @@ -12029,19 +12057,23 @@ VALUES ( $4, $5, $6, + $7, + $8, now(), now() ) -RETURNING name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id +RETURNING name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id, is_system, member_permissions ` type InsertCustomRoleParams struct { - Name string `db:"name" json:"name"` - DisplayName string `db:"display_name" json:"display_name"` - OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` - SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"` - OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"` - UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"` + Name string `db:"name" json:"name"` + DisplayName string `db:"display_name" json:"display_name"` + OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` + SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"` + OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"` + UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"` + MemberPermissions CustomRolePermissions `db:"member_permissions" json:"member_permissions"` + IsSystem bool `db:"is_system" json:"is_system"` } func (q *sqlQuerier) InsertCustomRole(ctx context.Context, arg InsertCustomRoleParams) (CustomRole, error) { @@ -12052,6 +12084,8 @@ func (q *sqlQuerier) InsertCustomRole(ctx context.Context, arg InsertCustomRoleP arg.SitePermissions, arg.OrgPermissions, arg.UserPermissions, + arg.MemberPermissions, + arg.IsSystem, ) var i CustomRole err := row.Scan( @@ -12064,6 +12098,8 @@ func (q *sqlQuerier) InsertCustomRole(ctx context.Context, arg InsertCustomRoleP &i.UpdatedAt, &i.OrganizationID, &i.ID, + &i.IsSystem, + &i.MemberPermissions, ) return i, err } @@ -12076,20 +12112,22 @@ SET site_permissions = $2, org_permissions = $3, user_permissions = $4, + member_permissions = $5, updated_at = now() WHERE - name = lower($5) - AND organization_id = $6 -RETURNING name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id + name = lower($6) + AND organization_id = $7 +RETURNING name, display_name, site_permissions, org_permissions, user_permissions, created_at, updated_at, organization_id, id, is_system, member_permissions ` type UpdateCustomRoleParams struct { - DisplayName string `db:"display_name" json:"display_name"` - SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"` - OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"` - UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"` - Name string `db:"name" json:"name"` - OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` + DisplayName string `db:"display_name" json:"display_name"` + SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"` + OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"` + UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"` + MemberPermissions CustomRolePermissions `db:"member_permissions" json:"member_permissions"` + Name string `db:"name" json:"name"` + OrganizationID uuid.NullUUID `db:"organization_id" json:"organization_id"` } func (q *sqlQuerier) UpdateCustomRole(ctx context.Context, arg UpdateCustomRoleParams) (CustomRole, error) { @@ -12098,6 +12136,7 @@ func (q *sqlQuerier) UpdateCustomRole(ctx context.Context, arg UpdateCustomRoleP arg.SitePermissions, arg.OrgPermissions, arg.UserPermissions, + arg.MemberPermissions, arg.Name, arg.OrganizationID, ) @@ -12112,6 +12151,8 @@ func (q *sqlQuerier) UpdateCustomRole(ctx context.Context, arg UpdateCustomRoleP &i.UpdatedAt, &i.OrganizationID, &i.ID, + &i.IsSystem, + &i.MemberPermissions, ) return i, err } diff --git a/coderd/database/queries/roles.sql b/coderd/database/queries/roles.sql index ee5d35d91ab65..8a2ed0cccca13 100644 --- a/coderd/database/queries/roles.sql +++ b/coderd/database/queries/roles.sql @@ -23,6 +23,14 @@ WHERE organization_id = @organization_id ELSE true END + -- Filter system roles. By default, system roles are excluded. + -- System roles are managed by Coder and should be hidden from user-facing APIs. + -- The authorization system uses @include_system_roles = true to load them. + AND CASE WHEN @include_system_roles :: boolean THEN + true + ELSE + is_system = false + END ; -- name: DeleteCustomRole :exec @@ -31,6 +39,9 @@ DELETE FROM WHERE name = lower(@name) AND organization_id = @organization_id + -- Prevents accidental deletion of system roles even if the API + -- layer check is bypassed due to a bug. + AND is_system = false ; -- name: InsertCustomRole :one @@ -42,6 +53,8 @@ INSERT INTO site_permissions, org_permissions, user_permissions, + member_permissions, + is_system, created_at, updated_at ) @@ -53,6 +66,8 @@ VALUES ( @site_permissions, @org_permissions, @user_permissions, + @member_permissions, + @is_system, now(), now() ) @@ -66,6 +81,7 @@ SET site_permissions = @site_permissions, org_permissions = @org_permissions, user_permissions = @user_permissions, + member_permissions = @member_permissions, updated_at = now() WHERE name = lower(@name) diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index a7a821b758e4b..d6a22698454d8 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -53,6 +53,9 @@ sql: - column: "custom_roles.user_permissions" go_type: type: "CustomRolePermissions" + - column: "custom_roles.member_permissions" + go_type: + type: "CustomRolePermissions" - column: "provisioner_daemons.tags" go_type: type: "StringMap" diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 91061f1647020..dea4ec83ce5a1 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -230,15 +230,25 @@ func allPermsExcept(excepts ...Objecter) []Permission { // https://github.com/coder/coder/issues/1194 var builtInRoles map[string]func(orgID uuid.UUID) Role +// systemRoles are roles that have migrated from builtInRoles to +// database storage. This migration is partial - permissions are still +// generated at runtime and reconciled to the database, rather than +// the database being the source of truth. +var systemRoles = map[string]struct{}{ + RoleOrgMember(): {}, +} + type RoleOptions struct { NoOwnerWorkspaceExec bool } // ReservedRoleName exists because the database should only allow unique role -// names, but some roles are built in. So these names are reserved +// names, but some roles are built in or generated at runtime. So these names +// are reserved func ReservedRoleName(name string) bool { - _, ok := builtInRoles[name] - return ok + _, isBuiltIn := builtInRoles[name] + _, isSystem := systemRoles[name] + return isBuiltIn || isSystem } // ReloadBuiltinRoles loads the static roles into the builtInRoles map. @@ -432,39 +442,6 @@ func ReloadBuiltinRoles(opts *RoleOptions) { }, } }, - - // orgMember is an implied role to any member in an organization. - orgMember: func(organizationID uuid.UUID) Role { - return Role{ - Identifier: RoleIdentifier{Name: orgMember, OrganizationID: organizationID}, - DisplayName: "", - Site: []Permission{}, - User: []Permission{}, - ByOrgID: map[string]OrgPermissions{ - organizationID.String(): { - Org: Permissions(map[string][]policy.Action{ - // All users can see the provisioner daemons for workspace - // creation. - ResourceProvisionerDaemon.Type: {policy.ActionRead}, - // All org members can read the organization - ResourceOrganization.Type: {policy.ActionRead}, - // Can read available roles. - ResourceAssignOrgRole.Type: {policy.ActionRead}, - }), - Member: append(allPermsExcept(ResourceWorkspaceDormant, ResourcePrebuiltWorkspace, ResourceUser, ResourceOrganizationMember), - Permissions(map[string][]policy.Action{ - // Reduced permission set on dormant workspaces. No build, ssh, or exec - ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionDelete, policy.ActionCreate, policy.ActionUpdate, policy.ActionWorkspaceStop, policy.ActionCreateAgent, policy.ActionDeleteAgent}, - // Can read their own organization member record - ResourceOrganizationMember.Type: {policy.ActionRead}, - // Users can create provisioner daemons scoped to themselves. - ResourceProvisionerDaemon.Type: {policy.ActionRead, policy.ActionCreate, policy.ActionRead, policy.ActionUpdate}, - })..., - ), - }, - }, - } - }, orgAuditor: func(organizationID uuid.UUID) Role { return Role{ Identifier: RoleIdentifier{Name: orgAuditor, OrganizationID: organizationID}, @@ -916,3 +893,108 @@ func DeduplicatePermissions(perms []Permission) []Permission { } return deduped } + +// PermissionsEqual compares two permission slices using set-based +// comparison. Order does not matter; it only checks that both slices +// contain the same permissions. This is used by the organization +// member system role startup hook to detect when permissions need +// updating. +func PermissionsEqual(a, b []Permission) bool { + if len(a) != len(b) { + return false + } + + set := make(map[string]struct{}, len(a)) + for _, p := range a { + key := p.ResourceType + "\x00" + string(p.Action) + "\x00" + strconv.FormatBool(p.Negate) + set[key] = struct{}{} + } + + for _, p := range b { + key := p.ResourceType + "\x00" + string(p.Action) + "\x00" + strconv.FormatBool(p.Negate) + if _, ok := set[key]; !ok { + return false + } + } + + return true +} + +// OrgMemberPermissions returns the permissions for the organization-member +// system role. The results are usually stored in the database and can vary per +// organization based on the workspace_sharing_disabled setting. +// +// This is the source of truth for org-member permissions, used by: +// +// - The startup reconciliation hook, to keep permissions current with +// RBAC resources +// - The workspace sharing toggle endpoint, when updating the setting +// - The org creation endpoint, when creating the system role for new orgs +func OrgMemberPermissions(workspaceSharingDisabled bool) ( + orgPerms, memberPerms []Permission, +) { + // Organization-level permissions that all org members get. + orgPermMap := map[string][]policy.Action{ + // All users can see provisioner daemons for workspace creation. + ResourceProvisionerDaemon.Type: {policy.ActionRead}, + // All org members can read the organization. + ResourceOrganization.Type: {policy.ActionRead}, + // Can read available roles. + ResourceAssignOrgRole.Type: {policy.ActionRead}, + } + + // When workspace sharing is enabled, members need to see other org members + // and groups to share workspaces with them. + if !workspaceSharingDisabled { + orgPermMap[ResourceOrganizationMember.Type] = []policy.Action{policy.ActionRead} + orgPermMap[ResourceGroup.Type] = []policy.Action{policy.ActionRead} + } + + orgPerms = Permissions(orgPermMap) + + // Member-scoped permissions (resources owned by the member). + // Uses allPermsExcept to automatically include permissions for new resources. + memberPerms = append( + allPermsExcept( + ResourceWorkspaceDormant, + ResourcePrebuiltWorkspace, + ResourceUser, + ResourceOrganizationMember, + ), + Permissions(map[string][]policy.Action{ + // Reduced permission set on dormant workspaces. No build, + // ssh, or exec. + ResourceWorkspaceDormant.Type: { + policy.ActionRead, + policy.ActionDelete, + policy.ActionCreate, + policy.ActionUpdate, + policy.ActionWorkspaceStop, + policy.ActionCreateAgent, + policy.ActionDeleteAgent, + }, + // Can read their own organization member record. + ResourceOrganizationMember.Type: { + policy.ActionRead, + }, + // Users can create provisioner daemons scoped to themselves. + ResourceProvisionerDaemon.Type: { + policy.ActionRead, + policy.ActionCreate, + policy.ActionUpdate, + }, + })..., + ) + + if workspaceSharingDisabled { + // This negation overrides the wildcard permission from + // allPermsExcept. + memberPerms = append(memberPerms, Permission{ + Negate: true, + ResourceType: ResourceWorkspace.Type, + Action: policy.ActionShare, + }) + } + + return orgPerms, memberPerms +} diff --git a/coderd/rbac/roles_internal_test.go b/coderd/rbac/roles_internal_test.go index b99791b5a1f5b..8de7059e41073 100644 --- a/coderd/rbac/roles_internal_test.go +++ b/coderd/rbac/roles_internal_test.go @@ -74,7 +74,7 @@ func TestRegoInputValue(t *testing.T) { // Expand all roles and make sure we have a good copy. // This is because these tests modify the roles, and we don't want to // modify the original roles. - roles, err := RoleIdentifiers{ScopedRoleOrgMember(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}.Expand() + roles, err := RoleIdentifiers{ScopedRoleOrgAuditor(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}.Expand() require.NoError(t, err, "failed to expand roles") for i := range roles { // If all cached values are nil, then the role will not use @@ -224,9 +224,9 @@ func TestRoleByName(t *testing.T) { {Role: builtInRoles[orgAdmin](uuid.New())}, {Role: builtInRoles[orgAdmin](uuid.New())}, - {Role: builtInRoles[orgMember](uuid.New())}, - {Role: builtInRoles[orgMember](uuid.New())}, - {Role: builtInRoles[orgMember](uuid.New())}, + {Role: builtInRoles[orgAuditor](uuid.New())}, + {Role: builtInRoles[orgAuditor](uuid.New())}, + {Role: builtInRoles[orgAuditor](uuid.New())}, } for _, c := range testCases { diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go index c2189c13b0c1f..6506ffe326e6b 100644 --- a/coderd/rbac/rolestore/rolestore.go +++ b/coderd/rbac/rolestore/rolestore.go @@ -7,6 +7,7 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/syncmap" @@ -83,9 +84,10 @@ func Expand(ctx context.Context, db database.Store, names []rbac.RoleIdentifier) // the expansion. These roles are no-ops. Should we raise some kind of // warning when this happens? dbroles, err := db.CustomRoles(ctx, database.CustomRolesParams{ - LookupRoles: lookupArgs, - ExcludeOrgRoles: false, - OrganizationID: uuid.Nil, + LookupRoles: lookupArgs, + ExcludeOrgRoles: false, + OrganizationID: uuid.Nil, + IncludeSystemRoles: true, }) if err != nil { return nil, xerrors.Errorf("fetch custom roles: %w", err) @@ -105,7 +107,8 @@ func Expand(ctx context.Context, db database.Store, names []rbac.RoleIdentifier) return roles, nil } -func convertPermissions(dbPerms []database.CustomRolePermission) []rbac.Permission { +// ConvertDBPermissions converts database permissions to RBAC permissions. +func ConvertDBPermissions(dbPerms []database.CustomRolePermission) []rbac.Permission { n := make([]rbac.Permission, 0, len(dbPerms)) for _, dbPerm := range dbPerms { n = append(n, rbac.Permission{ @@ -117,14 +120,28 @@ func convertPermissions(dbPerms []database.CustomRolePermission) []rbac.Permissi return n } +// ConvertPermissionsToDB converts RBAC permissions to the database +// format. +func ConvertPermissionsToDB(perms []rbac.Permission) []database.CustomRolePermission { + dbPerms := make([]database.CustomRolePermission, 0, len(perms)) + for _, perm := range perms { + dbPerms = append(dbPerms, database.CustomRolePermission{ + Negate: perm.Negate, + ResourceType: perm.ResourceType, + Action: perm.Action, + }) + } + return dbPerms +} + // ConvertDBRole should not be used by any human facing apis. It is used // for authz purposes. func ConvertDBRole(dbRole database.CustomRole) (rbac.Role, error) { role := rbac.Role{ Identifier: dbRole.RoleIdentifier(), DisplayName: dbRole.DisplayName, - Site: convertPermissions(dbRole.SitePermissions), - User: convertPermissions(dbRole.UserPermissions), + Site: ConvertDBPermissions(dbRole.SitePermissions), + User: ConvertDBPermissions(dbRole.UserPermissions), } // Org permissions only make sense if an org id is specified. @@ -135,10 +152,150 @@ func ConvertDBRole(dbRole database.CustomRole) (rbac.Role, error) { if dbRole.OrganizationID.UUID != uuid.Nil { role.ByOrgID = map[string]rbac.OrgPermissions{ dbRole.OrganizationID.UUID.String(): { - Org: convertPermissions(dbRole.OrgPermissions), + Org: ConvertDBPermissions(dbRole.OrgPermissions), + Member: ConvertDBPermissions(dbRole.MemberPermissions), }, } } return role, nil } + +// ReconcileSystemRoles ensures that every organization's org-member +// system role in the DB is up-to-date with permissions reflecting +// current RBAC resources and the organization's +// workspace_sharing_disabled setting. Uses PostgreSQL advisory lock +// (LockIDReconcileSystemRoles) to safely handle multi-instance +// deployments. Uses set-based comparison to avoid unnecessary +// database writes when permissions haven't changed. +func ReconcileSystemRoles(ctx context.Context, log slog.Logger, db database.Store) error { + return db.InTx(func(tx database.Store) error { + // Acquire advisory lock to prevent concurrent updates from + // multiple coderd instances. Other instances will block here + // until we release the lock (when this transaction commits). + err := tx.AcquireLock(ctx, database.LockIDReconcileSystemRoles) + if err != nil { + return xerrors.Errorf("acquire reconcile system roles lock: %w", err) + } + + orgs, err := tx.GetOrganizations(ctx, database.GetOrganizationsParams{}) + if err != nil { + return xerrors.Errorf("fetch organizations: %w", err) + } + + customRoles, err := tx.CustomRoles(ctx, database.CustomRolesParams{ + IncludeSystemRoles: true, + }) + if err != nil { + return xerrors.Errorf("fetch custom roles: %w", err) + } + + // Find org-member roles and index by organization ID for quick lookup. + rolesByOrg := make(map[uuid.UUID]database.CustomRole) + for _, role := range customRoles { + if role.IsSystem && role.Name == rbac.RoleOrgMember() && role.OrganizationID.Valid { + rolesByOrg[role.OrganizationID.UUID] = role + } + } + + for _, org := range orgs { + role, exists := rolesByOrg[org.ID] + if !exists { + // Something is very wrong: the role should have been created by the + // database trigger or migration. Log loudly and try creating it as + // a last-ditch effort before giving up. + log.Critical(ctx, "missing organization-member system role; trying to re-create", + slog.F("organization_id", org.ID)) + + if err := CreateOrgMemberRole(ctx, tx, org); err != nil { + return xerrors.Errorf("create missing organization-member role for organization %s: %w", + org.ID, err) + } + + // Nothing more to do; the new role's permissions are up-to-date. + continue + } + + _, err := ReconcileOrgMemberRole(ctx, tx, role, org.WorkspaceSharingDisabled) + if err != nil { + return xerrors.Errorf("reconcile organization-member role for organization %s: %w", + org.ID, err) + } + } + + return nil + }, nil) +} + +// ReconcileOrgMemberRole ensures passed-in org-member role's perms +// are correct (current) and stored in the DB. Uses set-based +// comparison to avoid unnecessary database writes when permissions +// haven't changed. Returns the correct role. +func ReconcileOrgMemberRole( + ctx context.Context, + tx database.Store, + in database.CustomRole, + workspaceSharingDisabled bool, +) ( + database.CustomRole, error, +) { + // All fields except OrgPermissions and MemberPermissions will be the same. + out := in + + // Paranoia check: we don't use these in custom roles yet. + // TODO(geokat): Have these as check constraints in DB for now? + out.SitePermissions = database.CustomRolePermissions{} + out.UserPermissions = database.CustomRolePermissions{} + out.DisplayName = "" + + inOrgPerms := ConvertDBPermissions(in.OrgPermissions) + inMemberPerms := ConvertDBPermissions(in.MemberPermissions) + + outOrgPerms, outMemberPerms := rbac.OrgMemberPermissions(workspaceSharingDisabled) + + // Compare using set-based comparison (order doesn't matter). + match := rbac.PermissionsEqual(inOrgPerms, outOrgPerms) && + rbac.PermissionsEqual(inMemberPerms, outMemberPerms) + + if !match { + out.OrgPermissions = ConvertPermissionsToDB(outOrgPerms) + out.MemberPermissions = ConvertPermissionsToDB(outMemberPerms) + + _, err := tx.UpdateCustomRole(ctx, database.UpdateCustomRoleParams{ + Name: out.Name, + OrganizationID: out.OrganizationID, + DisplayName: out.DisplayName, + SitePermissions: out.SitePermissions, + UserPermissions: out.UserPermissions, + OrgPermissions: out.OrgPermissions, + MemberPermissions: out.MemberPermissions, + }) + if err != nil { + return out, xerrors.Errorf("update organization-member custom role for organization %s: %w", + in.OrganizationID, err) + } + } + + return out, nil +} + +// CreateOrgMemberRole creates an org-member system role for an organization. +func CreateOrgMemberRole(ctx context.Context, tx database.Store, org database.Organization) error { + orgPerms, memberPerms := rbac.OrgMemberPermissions(org.WorkspaceSharingDisabled) + + _, err := tx.InsertCustomRole(ctx, database.InsertCustomRoleParams{ + Name: rbac.RoleOrgMember(), + DisplayName: "", + OrganizationID: uuid.NullUUID{UUID: org.ID, Valid: true}, + SitePermissions: database.CustomRolePermissions{}, + OrgPermissions: ConvertPermissionsToDB(orgPerms), + UserPermissions: database.CustomRolePermissions{}, + MemberPermissions: ConvertPermissionsToDB(memberPerms), + IsSystem: true, + }) + if err != nil { + return xerrors.Errorf("insert org-member role: %w", err) + } + + return nil +} diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index 913611af283df..f72eeafc6e09b 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -19,7 +19,7 @@ We track the following resources: | AuditOAuthConvertState
| |
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
to_login_typetrue
user_idtrue
| | Group
create, write, delete | |
FieldTracked
avatar_urltrue
display_nametrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
sourcefalse
| | AuditableOrganizationMember
| |
FieldTracked
created_attrue
organization_idfalse
rolestrue
updated_attrue
user_idtrue
usernametrue
| -| CustomRole
| |
FieldTracked
created_atfalse
display_nametrue
idfalse
nametrue
org_permissionstrue
organization_idfalse
site_permissionstrue
updated_atfalse
user_permissionstrue
| +| CustomRole
| |
FieldTracked
created_atfalse
display_nametrue
idfalse
is_systemfalse
member_permissionstrue
nametrue
org_permissionstrue
organization_idfalse
site_permissionstrue
updated_atfalse
user_permissionstrue
| | GitSSHKey
create | |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | GroupSyncSettings
| |
FieldTracked
auto_create_missing_groupstrue
fieldtrue
legacy_group_name_mappingfalse
mappingtrue
regex_filtertrue
| | HealthSettings
| |
FieldTracked
dismissed_healthcheckstrue
idfalse
| @@ -28,7 +28,7 @@ We track the following resources: | NotificationsSettings
| |
FieldTracked
idfalse
notifier_pausedtrue
| | OAuth2ProviderApp
| |
FieldTracked
callback_urltrue
client_id_issued_atfalse
client_secret_expires_attrue
client_typetrue
client_uritrue
contactstrue
created_atfalse
dynamically_registeredtrue
grant_typestrue
icontrue
idfalse
jwkstrue
jwks_uritrue
logo_uritrue
nametrue
policy_uritrue
redirect_uristrue
registration_access_tokentrue
registration_client_uritrue
response_typestrue
scopetrue
software_idtrue
software_versiontrue
token_endpoint_auth_methodtrue
tos_uritrue
updated_atfalse
| | OAuth2ProviderAppSecret
| |
FieldTracked
app_idfalse
created_atfalse
display_secretfalse
hashed_secretfalse
idfalse
last_used_atfalse
secret_prefixfalse
| -| Organization
| |
FieldTracked
created_atfalse
deletedtrue
descriptiontrue
display_nametrue
icontrue
idfalse
is_defaulttrue
nametrue
updated_attrue
| +| Organization
| |
FieldTracked
created_atfalse
deletedtrue
descriptiontrue
display_nametrue
icontrue
idfalse
is_defaulttrue
nametrue
updated_attrue
workspace_sharing_disabledtrue
| | OrganizationSyncSettings
| |
FieldTracked
assign_defaulttrue
fieldtrue
mappingtrue
| | PrebuildsSettings
| |
FieldTracked
idfalse
reconciliation_pausedtrue
| | RoleSyncSettings
| |
FieldTracked
fieldtrue
mappingtrue
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 7006c434c609c..8095ff7301247 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -62,12 +62,14 @@ var auditableResourcesTypes = map[any]map[string]Action{ "roles": ActionTrack, }, &database.CustomRole{}: { - "name": ActionTrack, - "display_name": ActionTrack, - "site_permissions": ActionTrack, - "org_permissions": ActionTrack, - "user_permissions": ActionTrack, - "organization_id": ActionIgnore, // Never changes. + "name": ActionTrack, + "display_name": ActionTrack, + "site_permissions": ActionTrack, + "org_permissions": ActionTrack, + "user_permissions": ActionTrack, + "member_permissions": ActionTrack, + "organization_id": ActionIgnore, // Never changes. + "is_system": ActionIgnore, // Never changes. "id": ActionIgnore, "created_at": ActionIgnore, @@ -310,15 +312,16 @@ var auditableResourcesTypes = map[any]map[string]Action{ "secret_prefix": ActionIgnore, }, &database.Organization{}: { - "id": ActionIgnore, - "name": ActionTrack, - "description": ActionTrack, - "deleted": ActionTrack, - "created_at": ActionIgnore, - "updated_at": ActionTrack, - "is_default": ActionTrack, - "display_name": ActionTrack, - "icon": ActionTrack, + "id": ActionIgnore, + "name": ActionTrack, + "description": ActionTrack, + "deleted": ActionTrack, + "created_at": ActionIgnore, + "updated_at": ActionTrack, + "is_default": ActionTrack, + "display_name": ActionTrack, + "icon": ActionTrack, + "workspace_sharing_disabled": ActionTrack, }, &database.NotificationTemplate{}: { "id": ActionIgnore, diff --git a/enterprise/coderd/organizations.go b/enterprise/coderd/organizations.go index 5a7a4eb777f50..b19043e408255 100644 --- a/enterprise/coderd/organizations.go +++ b/enterprise/coderd/organizations.go @@ -15,6 +15,8 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/rolestore" "github.com/coder/coder/v2/codersdk" ) @@ -281,6 +283,21 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) { if err != nil { return xerrors.Errorf("create organization: %w", err) } + + // Populate the placeholder system role(s) that the DB trigger + // created for us. + _, err := rolestore.ReconcileOrgMemberRole(ctx, tx, database.CustomRole{ + Name: rbac.RoleOrgMember(), + OrganizationID: uuid.NullUUID{ + UUID: organizationID, + Valid: true, + }, + }, organization.WorkspaceSharingDisabled) + if err != nil { + return xerrors.Errorf("reconcile organization-member role for organization %s: %w", + organizationID, err) + } + _, err = tx.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{ OrganizationID: organization.ID, UserID: apiKey.UserID, diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go index f103a8d1b06a0..a36c4b8073788 100644 --- a/enterprise/coderd/roles.go +++ b/enterprise/coderd/roles.go @@ -82,9 +82,9 @@ func (api *API) postOrgRoles(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Role(inserted)) } -// patchRole will allow creating a custom organization role +// putOrgRoles will allow updating a custom organization role // -// @Summary Upsert a custom organization role +// @Summary Update a custom organization role // @ID upsert-a-custom-organization-role // @Security CoderSessionToken // @Accept json @@ -197,6 +197,12 @@ func (api *API) deleteOrgRole(rw http.ResponseWriter, r *http.Request) { defer commitAudit() rolename := chi.URLParam(r, "roleName") + + // Catch requests that try to delete system roles. + if !validOrganizationRoleRequest(ctx, codersdk.CustomRoleRequest{Name: rolename}, rw) { + return + } + roles, err := api.Database.CustomRoles(ctx, database.CustomRolesParams{ LookupRoles: []database.NameOrganizationPair{ {