From 300e80f1f8922c6e373858789530048b903da11c Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 12 Mar 2025 14:15:15 +0000 Subject: [PATCH 01/25] add prebuilds system user database changes and associated changes Signed-off-by: Danny Kopping --- coderd/database/dbauthz/dbauthz.go | 28 ++++++++++ coderd/database/dump.sql | 20 ++++++++ .../migrations/000301_system_user.down.sql | 20 ++++++++ .../migrations/000301_system_user.up.sql | 51 +++++++++++++++++++ coderd/database/modelmethods.go | 1 + coderd/database/modelqueries.go | 1 + coderd/database/models.go | 2 + coderd/database/queries.sql.go | 34 +++++++++---- coderd/prebuilds/id.go | 5 ++ docs/admin/security/audit-logs.md | 2 +- enterprise/audit/table.go | 1 + enterprise/coderd/groups_test.go | 17 ++++++- enterprise/coderd/roles_test.go | 8 +++ enterprise/coderd/templates_test.go | 9 +++- 14 files changed, 184 insertions(+), 15 deletions(-) create mode 100644 coderd/database/migrations/000301_system_user.down.sql create mode 100644 coderd/database/migrations/000301_system_user.up.sql create mode 100644 coderd/prebuilds/id.go diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9c88e986cbffc..796127c83259b 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -18,6 +18,7 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/rbac/rolestore" @@ -358,6 +359,27 @@ var ( }), Scope: rbac.ScopeAll, }.WithCachedASTValue() + + subjectPrebuildsOrchestrator = rbac.Subject{ + FriendlyName: "Prebuilds Orchestrator", + ID: prebuilds.OwnerID.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "prebuilds-orchestrator"}, + DisplayName: "Coder", + Site: rbac.Permissions(map[string][]policy.Action{ + // May use template, read template-related info, & insert template-related resources (preset prebuilds). + rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionUse}, + // May CRUD workspaces, and start/stop them. + rbac.ResourceWorkspace.Type: { + policy.ActionCreate, policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, + policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, + }, + }), + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() ) // AsProvisionerd returns a context with an actor that has permissions required @@ -412,6 +434,12 @@ func AsSystemReadProvisionerDaemons(ctx context.Context) context.Context { return context.WithValue(ctx, authContextKey{}, subjectSystemReadProvisionerDaemons) } +// AsPrebuildsOrchestrator returns a context with an actor that has permissions +// to read orchestrator workspace prebuilds. +func AsPrebuildsOrchestrator(ctx context.Context) context.Context { + return context.WithValue(ctx, authContextKey{}, subjectPrebuildsOrchestrator) +} + var AsRemoveActor = rbac.Subject{ ID: "remove-actor", } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 492aaefc12aa5..6961b1386e176 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -445,6 +445,17 @@ BEGIN END; $$; +CREATE FUNCTION prevent_system_user_changes() RETURNS trigger + LANGUAGE plpgsql + AS $$ +BEGIN + IF OLD.is_system = true THEN + RAISE EXCEPTION 'Cannot modify or delete system users'; + END IF; + RETURN OLD; +END; +$$; + CREATE FUNCTION protect_deleting_organizations() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -854,6 +865,7 @@ CREATE TABLE users ( github_com_user_id bigint, hashed_one_time_passcode bytea, one_time_passcode_expires_at timestamp with time zone, + is_system boolean DEFAULT false, CONSTRAINT one_time_passcode_set CHECK ((((hashed_one_time_passcode IS NULL) AND (one_time_passcode_expires_at IS NULL)) OR ((hashed_one_time_passcode IS NOT NULL) AND (one_time_passcode_expires_at IS NOT NULL)))) ); @@ -867,6 +879,8 @@ COMMENT ON COLUMN users.hashed_one_time_passcode IS 'A hash of the one-time-pass COMMENT ON COLUMN users.one_time_passcode_expires_at IS 'The time when the one-time-passcode expires.'; +COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions'; + CREATE VIEW group_members_expanded AS WITH all_members AS ( SELECT group_members.user_id, @@ -2362,6 +2376,8 @@ COMMENT ON INDEX template_usage_stats_start_time_template_id_user_id_idx IS 'Ind CREATE UNIQUE INDEX templates_organization_id_name_idx ON templates USING btree (organization_id, lower((name)::text)) WHERE (deleted = false); +CREATE INDEX user_is_system_idx ON users USING btree (is_system); + CREATE UNIQUE INDEX user_links_linked_id_login_type_idx ON user_links USING btree (linked_id, login_type) WHERE (linked_id <> ''::text); CREATE UNIQUE INDEX users_email_lower_idx ON users USING btree (lower(email)) WHERE (deleted = false); @@ -2450,6 +2466,10 @@ CREATE OR REPLACE VIEW provisioner_job_stats AS CREATE TRIGGER inhibit_enqueue_if_disabled BEFORE INSERT ON notification_messages FOR EACH ROW EXECUTE FUNCTION inhibit_enqueue_if_disabled(); +CREATE TRIGGER prevent_system_user_deletions BEFORE DELETE ON users FOR EACH ROW WHEN ((old.is_system = true)) EXECUTE FUNCTION prevent_system_user_changes(); + +CREATE TRIGGER prevent_system_user_updates BEFORE UPDATE ON users FOR EACH ROW WHEN ((old.is_system = true)) EXECUTE FUNCTION prevent_system_user_changes(); + CREATE TRIGGER protect_deleting_organizations BEFORE UPDATE ON organizations FOR EACH ROW WHEN (((new.deleted = true) AND (old.deleted = false))) EXECUTE FUNCTION protect_deleting_organizations(); CREATE TRIGGER remove_organization_member_custom_role BEFORE DELETE ON custom_roles FOR EACH ROW EXECUTE FUNCTION remove_organization_member_role(); diff --git a/coderd/database/migrations/000301_system_user.down.sql b/coderd/database/migrations/000301_system_user.down.sql new file mode 100644 index 0000000000000..1117ddb2f2513 --- /dev/null +++ b/coderd/database/migrations/000301_system_user.down.sql @@ -0,0 +1,20 @@ +-- Remove system user from organizations +DELETE FROM organization_members +WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; + +-- Drop triggers first +DROP TRIGGER IF EXISTS prevent_system_user_updates ON users; +DROP TRIGGER IF EXISTS prevent_system_user_deletions ON users; + +-- Drop function +DROP FUNCTION IF EXISTS prevent_system_user_changes(); + +-- Delete system user +DELETE FROM users +WHERE id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; + +-- Drop index +DROP INDEX IF EXISTS user_is_system_idx; + +-- Drop column +ALTER TABLE users DROP COLUMN IF EXISTS is_system; diff --git a/coderd/database/migrations/000301_system_user.up.sql b/coderd/database/migrations/000301_system_user.up.sql new file mode 100644 index 0000000000000..0edb25ef076d6 --- /dev/null +++ b/coderd/database/migrations/000301_system_user.up.sql @@ -0,0 +1,51 @@ +ALTER TABLE users + ADD COLUMN is_system bool DEFAULT false; + +CREATE INDEX user_is_system_idx ON users USING btree (is_system); + +COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions'; + +-- TODO: tried using "none" for login type, but the migration produced this error: 'unsafe use of new value "none" of enum type login_type' +-- -> not sure why though? it exists on the login_type enum. +INSERT INTO users (id, email, username, name, created_at, updated_at, status, rbac_roles, hashed_password, is_system, login_type) +VALUES ('c42fdf75-3097-471c-8c33-fb52454d81c0', 'prebuilds@system', 'prebuilds', 'Prebuilds Owner', now(), now(), + 'active', '{}', 'none', true, 'password'::login_type); + +-- Create function to check system user modifications +CREATE OR REPLACE FUNCTION prevent_system_user_changes() + RETURNS TRIGGER AS +$$ +BEGIN + IF OLD.is_system = true THEN + RAISE EXCEPTION 'Cannot modify or delete system users'; + END IF; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +-- Create trigger to prevent updates to system users +CREATE TRIGGER prevent_system_user_updates + BEFORE UPDATE ON users + FOR EACH ROW + WHEN (OLD.is_system = true) +EXECUTE FUNCTION prevent_system_user_changes(); + +-- Create trigger to prevent deletion of system users +CREATE TRIGGER prevent_system_user_deletions + BEFORE DELETE ON users + FOR EACH ROW + WHEN (OLD.is_system = true) +EXECUTE FUNCTION prevent_system_user_changes(); + +-- TODO: do we *want* to use the default org here? how do we handle multi-org? +WITH default_org AS (SELECT id + FROM organizations + WHERE is_default = true + LIMIT 1) +INSERT +INTO organization_members (organization_id, user_id, created_at, updated_at) +SELECT default_org.id, + 'c42fdf75-3097-471c-8c33-fb52454d81c0', -- The system user responsible for prebuilds. + NOW(), + NOW() +FROM default_org; diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index a9dbc3e530994..5b197a0649dcf 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -423,6 +423,7 @@ func ConvertUserRows(rows []GetUsersRow) []User { AvatarURL: r.AvatarURL, Deleted: r.Deleted, LastSeenAt: r.LastSeenAt, + IsSystem: r.IsSystem, } } diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index cc19de5132f37..506cd7a69fe34 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -421,6 +421,7 @@ func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, &i.Count, ); err != nil { return nil, err diff --git a/coderd/database/models.go b/coderd/database/models.go index e0064916b0135..8e98510389118 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3186,6 +3186,8 @@ type User struct { HashedOneTimePasscode []byte `db:"hashed_one_time_passcode" json:"hashed_one_time_passcode"` // The time when the one-time-passcode expires. OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` + // Determines if a user is a system user, and therefore cannot login or perform normal actions + IsSystem sql.NullBool `db:"is_system" json:"is_system"` } type UserConfig struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index b394a0b0121ec..2c5138dbee4af 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -11451,7 +11451,7 @@ func (q *sqlQuerier) GetUserAppearanceSettings(ctx context.Context, userID uuid. const getUserByEmailOrUsername = `-- name: GetUserByEmailOrUsername :one SELECT - id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system FROM users WHERE @@ -11487,13 +11487,14 @@ func (q *sqlQuerier) GetUserByEmailOrUsername(ctx context.Context, arg GetUserBy &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } const getUserByID = `-- name: GetUserByID :one SELECT - id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system FROM users WHERE @@ -11523,6 +11524,7 @@ func (q *sqlQuerier) GetUserByID(ctx context.Context, id uuid.UUID) (User, error &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -11545,7 +11547,7 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { const getUsers = `-- name: GetUsers :many SELECT - id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, COUNT(*) OVER() AS count + id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system, COUNT(*) OVER() AS count FROM users WHERE @@ -11659,6 +11661,7 @@ type GetUsersRow struct { GithubComUserID sql.NullInt64 `db:"github_com_user_id" json:"github_com_user_id"` HashedOneTimePasscode []byte `db:"hashed_one_time_passcode" json:"hashed_one_time_passcode"` OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` + IsSystem sql.NullBool `db:"is_system" json:"is_system"` Count int64 `db:"count" json:"count"` } @@ -11701,6 +11704,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUse &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, &i.Count, ); err != nil { return nil, err @@ -11717,7 +11721,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUse } const getUsersByIDs = `-- name: GetUsersByIDs :many -SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at FROM users WHERE id = ANY($1 :: uuid [ ]) +SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system FROM users WHERE id = ANY($1 :: uuid [ ]) ` // This shouldn't check for deleted, because it's frequently used @@ -11750,6 +11754,7 @@ func (q *sqlQuerier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ); err != nil { return nil, err } @@ -11783,7 +11788,7 @@ VALUES -- if the status passed in is empty, fallback to dormant, which is what -- we were doing before. COALESCE(NULLIF($10::text, '')::user_status, 'dormant'::user_status) - ) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + ) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type InsertUserParams struct { @@ -11831,6 +11836,7 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -11996,7 +12002,7 @@ SET last_seen_at = $2, updated_at = $3 WHERE - id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserLastSeenAtParams struct { @@ -12026,6 +12032,7 @@ func (q *sqlQuerier) UpdateUserLastSeenAt(ctx context.Context, arg UpdateUserLas &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -12043,7 +12050,7 @@ SET '':: bytea END WHERE - id = $2 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + id = $2 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserLoginTypeParams struct { @@ -12072,6 +12079,7 @@ func (q *sqlQuerier) UpdateUserLoginType(ctx context.Context, arg UpdateUserLogi &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -12087,7 +12095,7 @@ SET name = $6 WHERE id = $1 -RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at +RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserProfileParams struct { @@ -12127,6 +12135,7 @@ func (q *sqlQuerier) UpdateUserProfile(ctx context.Context, arg UpdateUserProfil &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -12138,7 +12147,7 @@ SET quiet_hours_schedule = $2 WHERE id = $1 -RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at +RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserQuietHoursScheduleParams struct { @@ -12167,6 +12176,7 @@ func (q *sqlQuerier) UpdateUserQuietHoursSchedule(ctx context.Context, arg Updat &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -12179,7 +12189,7 @@ SET rbac_roles = ARRAY(SELECT DISTINCT UNNEST($1 :: text[])) WHERE id = $2 -RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at +RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserRolesParams struct { @@ -12208,6 +12218,7 @@ func (q *sqlQuerier) UpdateUserRoles(ctx context.Context, arg UpdateUserRolesPar &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } @@ -12219,7 +12230,7 @@ SET status = $2, updated_at = $3 WHERE - id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at + id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserStatusParams struct { @@ -12249,6 +12260,7 @@ func (q *sqlQuerier) UpdateUserStatus(ctx context.Context, arg UpdateUserStatusP &i.GithubComUserID, &i.HashedOneTimePasscode, &i.OneTimePasscodeExpiresAt, + &i.IsSystem, ) return i, err } diff --git a/coderd/prebuilds/id.go b/coderd/prebuilds/id.go new file mode 100644 index 0000000000000..bde76e3f7bf14 --- /dev/null +++ b/coderd/prebuilds/id.go @@ -0,0 +1,5 @@ +package prebuilds + +import "github.com/google/uuid" + +var OwnerID = uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index 778e9f9c2e26e..47f3b8757a7bb 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -28,7 +28,7 @@ We track the following resources: | RoleSyncSettings
| |
FieldTracked
fieldtrue
mappingtrue
| | Template
write, delete | |
FieldTracked
active_version_idtrue
activity_bumptrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
autostart_block_days_of_weektrue
autostop_requirement_days_of_weektrue
autostop_requirement_weekstrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
default_ttltrue
deletedfalse
deprecatedtrue
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
max_port_sharing_leveltrue
nametrue
organization_display_namefalse
organization_iconfalse
organization_idfalse
organization_namefalse
provisionertrue
require_active_versiontrue
time_til_dormanttrue
time_til_dormant_autodeletetrue
updated_atfalse
user_acltrue
| | TemplateVersion
create, write | |
FieldTracked
archivedtrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_usernamefalse
external_auth_providersfalse
idtrue
job_idfalse
messagefalse
nametrue
organization_idfalse
readmetrue
source_example_idfalse
template_idtrue
updated_atfalse
| -| User
create, write, delete | |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
github_com_user_idfalse
hashed_one_time_passcodefalse
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typetrue
nametrue
one_time_passcode_expires_attrue
quiet_hours_scheduletrue
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| User
create, write, delete | |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
github_com_user_idfalse
hashed_one_time_passcodefalse
hashed_passwordtrue
idtrue
is_systemtrue
last_seen_atfalse
login_typetrue
nametrue
one_time_passcode_expires_attrue
quiet_hours_scheduletrue
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| | WorkspaceAgent
connect, disconnect | |
FieldTracked
api_versionfalse
architecturefalse
auth_instance_idfalse
auth_tokenfalse
connection_timeout_secondsfalse
created_atfalse
directoryfalse
disconnected_atfalse
display_appsfalse
display_orderfalse
environment_variablesfalse
expanded_directoryfalse
first_connected_atfalse
idfalse
instance_metadatafalse
last_connected_atfalse
last_connected_replica_idfalse
lifecycle_statefalse
logs_lengthfalse
logs_overflowedfalse
motd_filefalse
namefalse
operating_systemfalse
ready_atfalse
resource_idfalse
resource_metadatafalse
started_atfalse
subsystemsfalse
troubleshooting_urlfalse
updated_atfalse
versionfalse
| | WorkspaceApp
open, close | |
FieldTracked
agent_idfalse
commandfalse
created_atfalse
display_namefalse
display_orderfalse
externalfalse
healthfalse
healthcheck_intervalfalse
healthcheck_thresholdfalse
healthcheck_urlfalse
hiddenfalse
iconfalse
idfalse
open_infalse
sharing_levelfalse
slugfalse
subdomainfalse
urlfalse
| | WorkspaceBuild
start, stop | |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_by_avatar_urlfalse
initiator_by_usernamefalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
template_version_preset_idfalse
transitionfalse
updated_atfalse
workspace_idfalse
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 6fd3f46308975..84cc7d451b4f1 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -151,6 +151,7 @@ var auditableResourcesTypes = map[any]map[string]Action{ "github_com_user_id": ActionIgnore, "hashed_one_time_passcode": ActionIgnore, "one_time_passcode_expires_at": ActionTrack, + "is_system": ActionTrack, // Should never change, but track it anyway. }, &database.WorkspaceTable{}: { "id": ActionTrack, diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 1baf62211dcd9..a6c9212b955f8 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -6,6 +6,9 @@ import ( "testing" "time" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/prebuilds" + "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -819,8 +822,14 @@ func TestGroup(t *testing.T) { }) t.Run("everyoneGroupReturnsEmpty", func(t *testing.T) { + // TODO (sasswart): this test seems to have drifted from its original intention. evaluate and remove/fix t.Parallel() + // TODO: we should not be returning the prebuilds user in Group, and this is not returned in dbmem. + if !dbtestutil.WillUsePostgres() { + t.Skip("This test requires postgres") + } + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, @@ -829,16 +838,20 @@ func TestGroup(t *testing.T) { userAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleUserAdmin()) _, user1 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) _, user2 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) - ctx := testutil.Context(t, testutil.WaitLong) + + // nolint:gocritic // "This client is operating as the owner user" is fine in this case. + prebuildsUser, err := client.User(ctx, prebuilds.OwnerID.String()) + require.NoError(t, err) // The 'Everyone' group always has an ID that matches the organization ID. group, err := userAdminClient.Group(ctx, user.OrganizationID) require.NoError(t, err) - require.Len(t, group.Members, 4) + require.Len(t, group.Members, 5) require.Equal(t, "Everyone", group.Name) require.Equal(t, user.OrganizationID, group.OrganizationID) require.Contains(t, group.Members, user1.ReducedUser) require.Contains(t, group.Members, user2.ReducedUser) + require.Contains(t, group.Members, prebuildsUser.ReducedUser) }) } diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go index 57b66a368248c..b2d7da47a608d 100644 --- a/enterprise/coderd/roles_test.go +++ b/enterprise/coderd/roles_test.go @@ -7,6 +7,8 @@ import ( "slices" "testing" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -333,6 +335,12 @@ func TestCustomOrganizationRole(t *testing.T) { // Verify deleting a custom role cascades to all members t.Run("DeleteRoleCascadeMembers", func(t *testing.T) { t.Parallel() + + // TODO: we should not be returning the prebuilds user in OrganizationMembers, and this is not returned in dbmem. + if !dbtestutil.WillUsePostgres() { + t.Skip("This test requires postgres") + } + owner, first := coderdenttest.New(t, &coderdenttest.Options{ LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index a40ed7b64a6db..e66adebca4680 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -18,6 +18,7 @@ import ( "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/dbtestutil" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" "github.com/coder/coder/v2/coderd/rbac" @@ -922,6 +923,12 @@ func TestTemplateACL(t *testing.T) { t.Run("everyoneGroup", func(t *testing.T) { t.Parallel() + + // TODO: we should not be returning the prebuilds user in TemplateACL, and this is not returned in dbmem. + if !dbtestutil.WillUsePostgres() { + t.Skip("This test requires postgres") + } + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, @@ -940,7 +947,7 @@ func TestTemplateACL(t *testing.T) { require.NoError(t, err) require.Len(t, acl.Groups, 1) - require.Len(t, acl.Groups[0].Members, 2) + require.Len(t, acl.Groups[0].Members, 3) // orgAdmin + TemplateAdmin + prebuilds user require.Len(t, acl.Users, 0) }) From b7882374e12052d769ee595be880925e668cfce3 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 11:44:32 +0000 Subject: [PATCH 02/25] optionally prevent system users from counting to user count Signed-off-by: Danny Kopping --- cli/server.go | 2 +- coderd/database/dbauthz/dbauthz.go | 17 +++++---- coderd/database/dbauthz/dbauthz_test.go | 6 ++-- coderd/database/dbmem/dbmem.go | 11 ++++-- coderd/database/dbmetrics/querymetrics.go | 13 +++---- coderd/database/dbmock/dbmock.go | 24 ++++++------- .../migrations/000301_system_user.down.sql | 4 +++ coderd/database/modelqueries.go | 1 + coderd/database/querier.go | 6 ++-- coderd/database/querier_test.go | 1 + coderd/database/queries.sql.go | 35 ++++++++++++++----- .../database/queries/organizationmembers.sql | 6 ++++ coderd/database/queries/users.sql | 14 ++++++-- coderd/userauth.go | 3 +- coderd/userauth_test.go | 3 +- coderd/users.go | 4 +-- coderd/users_test.go | 3 +- enterprise/coderd/license/license.go | 2 +- enterprise/dbcrypt/cliutil.go | 5 +-- 19 files changed, 105 insertions(+), 55 deletions(-) diff --git a/cli/server.go b/cli/server.go index 745794a236200..7a47b1d4e1135 100644 --- a/cli/server.go +++ b/cli/server.go @@ -1894,7 +1894,7 @@ func getGithubOAuth2ConfigParams(ctx context.Context, db database.Store, vals *c if defaultEligibleNotSet { // nolint:gocritic // User count requires system privileges - userCount, err := db.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + userCount, err := db.GetUserCount(dbauthz.AsSystemRestricted(ctx), false) if err != nil { return nil, xerrors.Errorf("get user count: %w", err) } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 796127c83259b..b3770f07d7362 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1084,13 +1084,13 @@ func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg database.Activi return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg) } -func (q *querier) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) { +func (q *querier) AllUserIDs(ctx context.Context, includeSystem bool) ([]uuid.UUID, error) { // Although this technically only reads users, only system-related functions should be // allowed to call this. if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return nil, err } - return q.db.AllUserIDs(ctx) + return q.db.AllUserIDs(ctx, includeSystem) } func (q *querier) ArchiveUnusedTemplateVersions(ctx context.Context, arg database.ArchiveUnusedTemplateVersionsParams) ([]uuid.UUID, error) { @@ -1343,7 +1343,10 @@ func (q *querier) DeleteOldWorkspaceAgentStats(ctx context.Context) error { func (q *querier) DeleteOrganizationMember(ctx context.Context, arg database.DeleteOrganizationMemberParams) error { return deleteQ[database.OrganizationMember](q.log, q.auth, func(ctx context.Context, arg database.DeleteOrganizationMemberParams) (database.OrganizationMember, error) { - member, err := database.ExpectOne(q.OrganizationMembers(ctx, database.OrganizationMembersParams(arg))) + member, err := database.ExpectOne(q.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: arg.OrganizationID, + UserID: arg.UserID, + })) if err != nil { return database.OrganizationMember{}, err } @@ -1529,11 +1532,11 @@ func (q *querier) GetAPIKeysLastUsedAfter(ctx context.Context, lastUsed time.Tim return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetAPIKeysLastUsedAfter)(ctx, lastUsed) } -func (q *querier) GetActiveUserCount(ctx context.Context) (int64, error) { +func (q *querier) GetActiveUserCount(ctx context.Context, includeSystem bool) (int64, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return 0, err } - return q.db.GetActiveUserCount(ctx) + return q.db.GetActiveUserCount(ctx, includeSystem) } func (q *querier) GetActiveWorkspaceBuildsByTemplateID(ctx context.Context, templateID uuid.UUID) ([]database.WorkspaceBuild, error) { @@ -2557,11 +2560,11 @@ func (q *querier) GetUserByID(ctx context.Context, id uuid.UUID) (database.User, return fetch(q.log, q.auth, q.db.GetUserByID)(ctx, id) } -func (q *querier) GetUserCount(ctx context.Context) (int64, error) { +func (q *querier) GetUserCount(ctx context.Context, includeSystem bool) (int64, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return 0, err } - return q.db.GetUserCount(ctx) + return q.db.GetUserCount(ctx, includeSystem) } func (q *querier) GetUserLatencyInsights(ctx context.Context, arg database.GetUserLatencyInsightsParams) ([]database.GetUserLatencyInsightsRow, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index ec8ced783fa0a..ca1379fdbdc84 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1664,7 +1664,7 @@ func (s *MethodTestSuite) TestUser() { s.Run("AllUserIDs", s.Subtest(func(db database.Store, check *expects) { a := dbgen.User(s.T(), db, database.User{}) b := dbgen.User(s.T(), db, database.User{}) - check.Args().Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(slice.New(a.ID, b.ID)) + check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(slice.New(a.ID, b.ID)) })) s.Run("CustomRoles", s.Subtest(func(db database.Store, check *expects) { check.Args(database.CustomRolesParams{}).Asserts(rbac.ResourceAssignRole, policy.ActionRead).Returns([]database.CustomRole{}) @@ -3649,7 +3649,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { check.Args().Asserts(rbac.ResourceSystem, policy.ActionRead) })) s.Run("GetActiveUserCount", s.Subtest(func(db database.Store, check *expects) { - check.Args().Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(int64(0)) + check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(int64(0)) })) s.Run("GetUnexpiredLicenses", s.Subtest(func(db database.Store, check *expects) { check.Args().Asserts(rbac.ResourceSystem, policy.ActionRead) @@ -3692,7 +3692,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { check.Args(time.Now().Add(time.Hour*-1)).Asserts(rbac.ResourceSystem, policy.ActionRead) })) s.Run("GetUserCount", s.Subtest(func(db database.Store, check *expects) { - check.Args().Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(int64(0)) + check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead).Returns(int64(0)) })) s.Run("GetTemplates", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 1ece2571f4960..5ac92468c3188 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1549,7 +1549,7 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.Ac return sql.ErrNoRows } -func (q *FakeQuerier) AllUserIDs(_ context.Context) ([]uuid.UUID, error) { +func (q *FakeQuerier) AllUserIDs(_ context.Context, includeSystem bool) ([]uuid.UUID, error) { q.mutex.RLock() defer q.mutex.RUnlock() userIDs := make([]uuid.UUID, 0, len(q.users)) @@ -2644,7 +2644,7 @@ func (q *FakeQuerier) GetAPIKeysLastUsedAfter(_ context.Context, after time.Time return apiKeys, nil } -func (q *FakeQuerier) GetActiveUserCount(_ context.Context) (int64, error) { +func (q *FakeQuerier) GetActiveUserCount(_ context.Context, includeSystem bool) (int64, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -6200,7 +6200,8 @@ func (q *FakeQuerier) GetUserByID(_ context.Context, id uuid.UUID) (database.Use return q.getUserByIDNoLock(id) } -func (q *FakeQuerier) GetUserCount(_ context.Context) (int64, error) { +// nolint:revive // Not a control flag; used for filtering. +func (q *FakeQuerier) GetUserCount(_ context.Context, includeSystem bool) (int64, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -6209,6 +6210,10 @@ func (q *FakeQuerier) GetUserCount(_ context.Context) (int64, error) { if !u.Deleted { existing++ } + + if !includeSystem && u.IsSystem.Valid && u.IsSystem.Bool { + continue + } } return existing, nil } diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 407d9e48bfcf8..bae68852bbf2b 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -12,6 +12,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" @@ -115,9 +116,9 @@ func (m queryMetricsStore) ActivityBumpWorkspace(ctx context.Context, arg databa return r0 } -func (m queryMetricsStore) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) { +func (m queryMetricsStore) AllUserIDs(ctx context.Context, includeSystem bool) ([]uuid.UUID, error) { start := time.Now() - r0, r1 := m.s.AllUserIDs(ctx) + r0, r1 := m.s.AllUserIDs(ctx, includeSystem) m.queryLatencies.WithLabelValues("AllUserIDs").Observe(time.Since(start).Seconds()) return r0, r1 } @@ -514,9 +515,9 @@ func (m queryMetricsStore) GetAPIKeysLastUsedAfter(ctx context.Context, lastUsed return apiKeys, err } -func (m queryMetricsStore) GetActiveUserCount(ctx context.Context) (int64, error) { +func (m queryMetricsStore) GetActiveUserCount(ctx context.Context, includeSystem bool) (int64, error) { start := time.Now() - count, err := m.s.GetActiveUserCount(ctx) + count, err := m.s.GetActiveUserCount(ctx, includeSystem) m.queryLatencies.WithLabelValues("GetActiveUserCount").Observe(time.Since(start).Seconds()) return count, err } @@ -1424,9 +1425,9 @@ func (m queryMetricsStore) GetUserByID(ctx context.Context, id uuid.UUID) (datab return user, err } -func (m queryMetricsStore) GetUserCount(ctx context.Context) (int64, error) { +func (m queryMetricsStore) GetUserCount(ctx context.Context, includeSystem bool) (int64, error) { start := time.Now() - count, err := m.s.GetUserCount(ctx) + count, err := m.s.GetUserCount(ctx, includeSystem) m.queryLatencies.WithLabelValues("GetUserCount").Observe(time.Since(start).Seconds()) return count, err } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index fbe4d0745fbb0..b2e8eabe4594e 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -103,18 +103,18 @@ func (mr *MockStoreMockRecorder) ActivityBumpWorkspace(ctx, arg any) *gomock.Cal } // AllUserIDs mocks base method. -func (m *MockStore) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) { +func (m *MockStore) AllUserIDs(ctx context.Context, includeSystem bool) ([]uuid.UUID, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AllUserIDs", ctx) + ret := m.ctrl.Call(m, "AllUserIDs", ctx, includeSystem) ret0, _ := ret[0].([]uuid.UUID) ret1, _ := ret[1].(error) return ret0, ret1 } // AllUserIDs indicates an expected call of AllUserIDs. -func (mr *MockStoreMockRecorder) AllUserIDs(ctx any) *gomock.Call { +func (mr *MockStoreMockRecorder) AllUserIDs(ctx, includeSystem any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AllUserIDs", reflect.TypeOf((*MockStore)(nil).AllUserIDs), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AllUserIDs", reflect.TypeOf((*MockStore)(nil).AllUserIDs), ctx, includeSystem) } // ArchiveUnusedTemplateVersions mocks base method. @@ -923,18 +923,18 @@ func (mr *MockStoreMockRecorder) GetAPIKeysLastUsedAfter(ctx, lastUsed any) *gom } // GetActiveUserCount mocks base method. -func (m *MockStore) GetActiveUserCount(ctx context.Context) (int64, error) { +func (m *MockStore) GetActiveUserCount(ctx context.Context, includeSystem bool) (int64, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetActiveUserCount", ctx) + ret := m.ctrl.Call(m, "GetActiveUserCount", ctx, includeSystem) ret0, _ := ret[0].(int64) ret1, _ := ret[1].(error) return ret0, ret1 } // GetActiveUserCount indicates an expected call of GetActiveUserCount. -func (mr *MockStoreMockRecorder) GetActiveUserCount(ctx any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetActiveUserCount(ctx, includeSystem any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetActiveUserCount", reflect.TypeOf((*MockStore)(nil).GetActiveUserCount), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetActiveUserCount", reflect.TypeOf((*MockStore)(nil).GetActiveUserCount), ctx, includeSystem) } // GetActiveWorkspaceBuildsByTemplateID mocks base method. @@ -2978,18 +2978,18 @@ func (mr *MockStoreMockRecorder) GetUserByID(ctx, id any) *gomock.Call { } // GetUserCount mocks base method. -func (m *MockStore) GetUserCount(ctx context.Context) (int64, error) { +func (m *MockStore) GetUserCount(ctx context.Context, includeSystem bool) (int64, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserCount", ctx) + ret := m.ctrl.Call(m, "GetUserCount", ctx, includeSystem) ret0, _ := ret[0].(int64) ret1, _ := ret[1].(error) return ret0, ret1 } // GetUserCount indicates an expected call of GetUserCount. -func (mr *MockStoreMockRecorder) GetUserCount(ctx any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetUserCount(ctx, includeSystem any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserCount", reflect.TypeOf((*MockStore)(nil).GetUserCount), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserCount", reflect.TypeOf((*MockStore)(nil).GetUserCount), ctx, includeSystem) } // GetUserLatencyInsights mocks base method. diff --git a/coderd/database/migrations/000301_system_user.down.sql b/coderd/database/migrations/000301_system_user.down.sql index 1117ddb2f2513..c286cfa193745 100644 --- a/coderd/database/migrations/000301_system_user.down.sql +++ b/coderd/database/migrations/000301_system_user.down.sql @@ -9,6 +9,10 @@ DROP TRIGGER IF EXISTS prevent_system_user_deletions ON users; -- Drop function DROP FUNCTION IF EXISTS prevent_system_user_changes(); +-- Delete user status changes +DELETE FROM user_status_changes +WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; + -- Delete system user DELETE FROM users WHERE id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index 506cd7a69fe34..a16c26fc6840c 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -393,6 +393,7 @@ func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, arg.LastSeenAfter, arg.CreatedBefore, arg.CreatedAfter, + arg.IncludeSystem, arg.OffsetOpt, arg.LimitOpt, ) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d72469650f0ea..0de928932652b 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -49,7 +49,7 @@ type sqlcQuerier interface { // We only bump when 5% of the deadline has elapsed. ActivityBumpWorkspace(ctx context.Context, arg ActivityBumpWorkspaceParams) error // AllUserIDs returns all UserIDs regardless of user status or deletion. - AllUserIDs(ctx context.Context) ([]uuid.UUID, error) + AllUserIDs(ctx context.Context, includeSystem bool) ([]uuid.UUID, error) // Archiving templates is a soft delete action, so is reversible. // Archiving prevents the version from being used and discovered // by listing. @@ -124,7 +124,7 @@ type sqlcQuerier interface { GetAPIKeysByLoginType(ctx context.Context, loginType LoginType) ([]APIKey, error) GetAPIKeysByUserID(ctx context.Context, arg GetAPIKeysByUserIDParams) ([]APIKey, error) GetAPIKeysLastUsedAfter(ctx context.Context, lastUsed time.Time) ([]APIKey, error) - GetActiveUserCount(ctx context.Context) (int64, error) + GetActiveUserCount(ctx context.Context, includeSystem bool) (int64, error) GetActiveWorkspaceBuildsByTemplateID(ctx context.Context, templateID uuid.UUID) ([]WorkspaceBuild, error) GetAllTailnetAgents(ctx context.Context) ([]TailnetAgent, error) // For PG Coordinator HTMLDebug @@ -309,7 +309,7 @@ type sqlcQuerier interface { GetUserAppearanceSettings(ctx context.Context, userID uuid.UUID) (string, error) GetUserByEmailOrUsername(ctx context.Context, arg GetUserByEmailOrUsernameParams) (User, error) GetUserByID(ctx context.Context, id uuid.UUID) (User, error) - GetUserCount(ctx context.Context) (int64, error) + GetUserCount(ctx context.Context, includeSystem bool) (int64, error) // GetUserLatencyInsights returns the median and 95th percentile connection // latency that users have experienced. The result can be filtered on // template_ids, meaning only user data from workspaces based on those templates diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 837068f1fa03e..7606e90b5107c 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2c5138dbee4af..2a448ca836c35 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5216,11 +5216,18 @@ WHERE user_id = $2 ELSE true END + -- Filter by system type + AND CASE + WHEN $3::bool THEN TRUE + ELSE + is_system = false + END ` type OrganizationMembersParams struct { OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` UserID uuid.UUID `db:"user_id" json:"user_id"` + IncludeSystem bool `db:"include_system" json:"include_system"` } type OrganizationMembersRow struct { @@ -5237,7 +5244,7 @@ type OrganizationMembersRow struct { // - Use just 'user_id' to get all orgs a user is a member of // - Use both to get a specific org member row func (q *sqlQuerier) OrganizationMembers(ctx context.Context, arg OrganizationMembersParams) ([]OrganizationMembersRow, error) { - rows, err := q.db.QueryContext(ctx, organizationMembers, arg.OrganizationID, arg.UserID) + rows, err := q.db.QueryContext(ctx, organizationMembers, arg.OrganizationID, arg.UserID, arg.IncludeSystem) if err != nil { return nil, err } @@ -11325,11 +11332,12 @@ func (q *sqlQuerier) UpdateUserLinkedID(ctx context.Context, arg UpdateUserLinke const allUserIDs = `-- name: AllUserIDs :many SELECT DISTINCT id FROM USERS + WHERE CASE WHEN $1::bool THEN TRUE ELSE is_system = false END ` // AllUserIDs returns all UserIDs regardless of user status or deletion. -func (q *sqlQuerier) AllUserIDs(ctx context.Context) ([]uuid.UUID, error) { - rows, err := q.db.QueryContext(ctx, allUserIDs) +func (q *sqlQuerier) AllUserIDs(ctx context.Context, includeSystem bool) ([]uuid.UUID, error) { + rows, err := q.db.QueryContext(ctx, allUserIDs, includeSystem) if err != nil { return nil, err } @@ -11358,10 +11366,11 @@ FROM users WHERE status = 'active'::user_status AND deleted = false + AND CASE WHEN $1::bool THEN TRUE ELSE is_system = false END ` -func (q *sqlQuerier) GetActiveUserCount(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, getActiveUserCount) +func (q *sqlQuerier) GetActiveUserCount(ctx context.Context, includeSystem bool) (int64, error) { + row := q.db.QueryRowContext(ctx, getActiveUserCount, includeSystem) var count int64 err := row.Scan(&count) return count, err @@ -11536,10 +11545,11 @@ FROM users WHERE deleted = false + AND CASE WHEN $1::bool THEN TRUE ELSE is_system = false END ` -func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, getUserCount) +func (q *sqlQuerier) GetUserCount(ctx context.Context, includeSystem bool) (int64, error) { + row := q.db.QueryRowContext(ctx, getUserCount, includeSystem) var count int64 err := row.Scan(&count) return count, err @@ -11618,16 +11628,21 @@ WHERE created_at >= $8 ELSE true END + AND CASE + WHEN $9::bool THEN TRUE + ELSE + is_system = false + END -- End of filters -- Authorize Filter clause will be injected below in GetAuthorizedUsers -- @authorize_filter ORDER BY -- Deterministic and consistent ordering of all users. This is to ensure consistent pagination. - LOWER(username) ASC OFFSET $9 + LOWER(username) ASC OFFSET $10 LIMIT -- A null limit means "no limit", so 0 means return all - NULLIF($10 :: int, 0) + NULLIF($11 :: int, 0) ` type GetUsersParams struct { @@ -11639,6 +11654,7 @@ type GetUsersParams struct { LastSeenAfter time.Time `db:"last_seen_after" json:"last_seen_after"` CreatedBefore time.Time `db:"created_before" json:"created_before"` CreatedAfter time.Time `db:"created_after" json:"created_after"` + IncludeSystem bool `db:"include_system" json:"include_system"` OffsetOpt int32 `db:"offset_opt" json:"offset_opt"` LimitOpt int32 `db:"limit_opt" json:"limit_opt"` } @@ -11676,6 +11692,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUse arg.LastSeenAfter, arg.CreatedBefore, arg.CreatedAfter, + arg.IncludeSystem, arg.OffsetOpt, arg.LimitOpt, ) diff --git a/coderd/database/queries/organizationmembers.sql b/coderd/database/queries/organizationmembers.sql index a92cd681eabf6..9d570bc1c49ee 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/coderd/database/queries/organizationmembers.sql @@ -22,6 +22,12 @@ WHERE WHEN @user_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = @user_id ELSE true + END + -- Filter by system type + AND CASE + WHEN @include_system::bool THEN TRUE + ELSE + is_system = false END; -- name: InsertOrganizationMember :one diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 79f19c1784155..4900e70262e3c 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -46,7 +46,8 @@ SELECT FROM users WHERE - deleted = false; + deleted = false + AND CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END; -- name: GetActiveUserCount :one SELECT @@ -54,7 +55,8 @@ SELECT FROM users WHERE - status = 'active'::user_status AND deleted = false; + status = 'active'::user_status AND deleted = false + AND CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END; -- name: InsertUser :one INSERT INTO @@ -223,6 +225,11 @@ WHERE created_at >= @created_after ELSE true END + AND CASE + WHEN @include_system::bool THEN TRUE + ELSE + is_system = false + END -- End of filters -- Authorize Filter clause will be injected below in GetAuthorizedUsers @@ -319,7 +326,8 @@ RETURNING id, email, username, last_seen_at; -- AllUserIDs returns all UserIDs regardless of user status or deletion. -- name: AllUserIDs :many -SELECT DISTINCT id FROM USERS; +SELECT DISTINCT id FROM USERS + WHERE CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END; -- name: UpdateUserHashedOneTimePasscode :exec UPDATE diff --git a/coderd/userauth.go b/coderd/userauth.go index 3c1481b1f9039..a6c7c1d319fc9 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -24,6 +24,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/cryptokeys" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/jwtutils" @@ -1665,7 +1666,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } // nolint:gocritic // Getting user count is a system function. - userCount, err := tx.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + userCount, err := tx.GetUserCount(dbauthz.AsSystemRestricted(ctx), false) if err != nil { return xerrors.Errorf("unable to fetch user count: %w", err) } diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ee6ee957ba861..4b67320164fc2 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -28,6 +28,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" @@ -304,7 +305,7 @@ func TestUserOAuth2Github(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) // nolint:gocritic // Unit test - count, err := db.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + count, err := db.GetUserCount(dbauthz.AsSystemRestricted(ctx), false) require.NoError(t, err) require.Equal(t, int64(1), count) diff --git a/coderd/users.go b/coderd/users.go index bbb10c4787a27..f6b961b7c2f8a 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -85,7 +85,7 @@ func (api *API) userDebugOIDC(rw http.ResponseWriter, r *http.Request) { func (api *API) firstUser(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() // nolint:gocritic // Getting user count is a system function. - userCount, err := api.Database.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + userCount, err := api.Database.GetUserCount(dbauthz.AsSystemRestricted(ctx), false) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching user count.", @@ -128,7 +128,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { // This should only function for the first user. // nolint:gocritic // Getting user count is a system function. - userCount, err := api.Database.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + userCount, err := api.Database.GetUserCount(dbauthz.AsSystemRestricted(ctx), false) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching user count.", diff --git a/coderd/users_test.go b/coderd/users_test.go index 2d85a9823a587..378aaab2d3a70 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -9,12 +9,13 @@ import ( "testing" "time" + "github.com/coder/serpent" + "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" "github.com/coder/coder/v2/coderd/rbac/policy" - "github.com/coder/serpent" "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" diff --git a/enterprise/coderd/license/license.go b/enterprise/coderd/license/license.go index 6f0e827eb3320..fbd53dcaac58c 100644 --- a/enterprise/coderd/license/license.go +++ b/enterprise/coderd/license/license.go @@ -33,7 +33,7 @@ func Entitlements( } // nolint:gocritic // Getting active user count is a system function. - activeUserCount, err := db.GetActiveUserCount(dbauthz.AsSystemRestricted(ctx)) + activeUserCount, err := db.GetActiveUserCount(dbauthz.AsSystemRestricted(ctx), false) // Don't include system user in license count. if err != nil { return codersdk.Entitlements{}, xerrors.Errorf("query active user count: %w", err) } diff --git a/enterprise/dbcrypt/cliutil.go b/enterprise/dbcrypt/cliutil.go index 120b41972de05..a94760d3d6e65 100644 --- a/enterprise/dbcrypt/cliutil.go +++ b/enterprise/dbcrypt/cliutil.go @@ -7,6 +7,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" ) @@ -19,7 +20,7 @@ func Rotate(ctx context.Context, log slog.Logger, sqlDB *sql.DB, ciphers []Ciphe return xerrors.Errorf("create cryptdb: %w", err) } - userIDs, err := db.AllUserIDs(ctx) + userIDs, err := db.AllUserIDs(ctx, false) if err != nil { return xerrors.Errorf("get users: %w", err) } @@ -109,7 +110,7 @@ func Decrypt(ctx context.Context, log slog.Logger, sqlDB *sql.DB, ciphers []Ciph } cryptDB.primaryCipherDigest = "" - userIDs, err := db.AllUserIDs(ctx) + userIDs, err := db.AllUserIDs(ctx, false) if err != nil { return xerrors.Errorf("get users: %w", err) } From 812259528b6440140ec0e8cc5febd362a5b33540 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 19:19:40 +0000 Subject: [PATCH 03/25] appease the linter Signed-off-by: Danny Kopping --- coderd/database/dbauthz/dbauthz.go | 2 ++ coderd/database/dbmem/dbmem.go | 12 +++++++++++- coderd/httpmw/organizationparam.go | 1 + coderd/idpsync/role.go | 2 ++ coderd/members.go | 1 + coderd/users.go | 1 + enterprise/coderd/groups.go | 1 + 7 files changed, 19 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index b3770f07d7362..38f6af62a4705 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1346,6 +1346,7 @@ func (q *querier) DeleteOrganizationMember(ctx context.Context, arg database.Del member, err := database.ExpectOne(q.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: arg.OrganizationID, UserID: arg.UserID, + IncludeSystem: false, })) if err != nil { return database.OrganizationMember{}, err @@ -3776,6 +3777,7 @@ func (q *querier) UpdateMemberRoles(ctx context.Context, arg database.UpdateMemb member, err := database.ExpectOne(q.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: arg.OrgID, UserID: arg.UserID, + IncludeSystem: false, })) if err != nil { return database.OrganizationMember{}, err diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 5ac92468c3188..0c39738069cb0 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1549,11 +1549,16 @@ func (q *FakeQuerier) ActivityBumpWorkspace(ctx context.Context, arg database.Ac return sql.ErrNoRows } +// nolint:revive // It's not a control flag, it's a filter. func (q *FakeQuerier) AllUserIDs(_ context.Context, includeSystem bool) ([]uuid.UUID, error) { q.mutex.RLock() defer q.mutex.RUnlock() userIDs := make([]uuid.UUID, 0, len(q.users)) for idx := range q.users { + if !includeSystem && q.users[idx].IsSystem.Valid && q.users[idx].IsSystem.Bool { + continue + } + userIDs = append(userIDs, q.users[idx].ID) } return userIDs, nil @@ -2644,12 +2649,17 @@ func (q *FakeQuerier) GetAPIKeysLastUsedAfter(_ context.Context, after time.Time return apiKeys, nil } +// nolint:revive // It's not a control flag, it's a filter. func (q *FakeQuerier) GetActiveUserCount(_ context.Context, includeSystem bool) (int64, error) { q.mutex.RLock() defer q.mutex.RUnlock() active := int64(0) for _, u := range q.users { + if !includeSystem && u.IsSystem.Valid && u.IsSystem.Bool { + continue + } + if u.Status == database.UserStatusActive && !u.Deleted { active++ } @@ -6200,7 +6210,7 @@ func (q *FakeQuerier) GetUserByID(_ context.Context, id uuid.UUID) (database.Use return q.getUserByIDNoLock(id) } -// nolint:revive // Not a control flag; used for filtering. +// nolint:revive // It's not a control flag, it's a filter. func (q *FakeQuerier) GetUserCount(_ context.Context, includeSystem bool) (int64, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 2eba0dcedf5b8..18938ec1e792d 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -126,6 +126,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: organization.ID, UserID: user.ID, + IncludeSystem: false, })) if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go index 22e0edc3bc662..54ec787661826 100644 --- a/coderd/idpsync/role.go +++ b/coderd/idpsync/role.go @@ -10,6 +10,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/rbac" @@ -91,6 +92,7 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data orgMemberships, err := tx.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: uuid.Nil, UserID: user.ID, + IncludeSystem: false, }) if err != nil { return xerrors.Errorf("get organizations by user id: %w", err) diff --git a/coderd/members.go b/coderd/members.go index 1852e6448408f..d1c4cdf01770c 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -160,6 +160,7 @@ func (api *API) listMembers(rw http.ResponseWriter, r *http.Request) { members, err := api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: organization.ID, UserID: uuid.Nil, + IncludeSystem: false, }) if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) diff --git a/coderd/users.go b/coderd/users.go index f6b961b7c2f8a..240ccb786019c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1191,6 +1191,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { memberships, err := api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ UserID: user.ID, OrganizationID: uuid.Nil, + IncludeSystem: false, }) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 6b94adb2c5b78..fef4a1bbef04b 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -170,6 +170,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { _, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: group.OrganizationID, UserID: uuid.MustParse(id), + IncludeSystem: false, })) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ From bfb7c2817e94a5e6cad3bfeeb38cf4a36681526f Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 19:49:32 +0000 Subject: [PATCH 04/25] add unit test for system user behaviour Signed-off-by: Danny Kopping --- coderd/users_test.go | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/coderd/users_test.go b/coderd/users_test.go index 378aaab2d3a70..0b83d33e4b435 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "context" + "database/sql" "fmt" "net/http" "slices" @@ -13,8 +14,10 @@ import ( "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" + "github.com/coder/coder/v2/coderd/database/migrations" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" + "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/golang-jwt/jwt/v4" @@ -2415,3 +2418,92 @@ func BenchmarkUsersMe(b *testing.B) { require.NoError(b, err) } } + +func TestSystemUserBehaviour(t *testing.T) { + // Setup. + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + sqlDB := testSQLDB(t) + err := migrations.Up(sqlDB) // coderd/database/migrations/00030*_system_user.up.sql will create a system user. + require.NoError(t, err, "migrations") + + db := database.New(sqlDB) + + // ================================================================================================================= + + // When: retrieving users with the include_system flag enabled. + other := dbgen.User(t, db, database.User{}) + users, err := db.GetUsers(ctx, database.GetUsersParams{ + IncludeSystem: true, + }) + + // Then: system users are returned, alongside other users. + require.NoError(t, err) + require.Len(t, users, 2) + + var systemUser, regularUser database.GetUsersRow + for _, u := range users { + if u.IsSystem.Bool { + systemUser = u + } else { + regularUser = u + } + } + require.NotNil(t, systemUser) + require.NotNil(t, regularUser) + + require.True(t, systemUser.IsSystem.Bool) + require.Equal(t, systemUser.ID, prebuilds.OwnerID) + require.False(t, regularUser.IsSystem.Bool) + require.Equal(t, regularUser.ID, other.ID) + + // ================================================================================================================= + + // When: retrieving users with the include_system flag disabled. + users, err = db.GetUsers(ctx, database.GetUsersParams{ + IncludeSystem: false, + }) + + // Then: only regular users are returned. + require.NoError(t, err) + require.Len(t, users, 1) + require.False(t, users[0].IsSystem.Bool) + + // ================================================================================================================= + + // When: attempting to update a system user's name. + _, err = db.UpdateUserProfile(ctx, database.UpdateUserProfileParams{ + ID: systemUser.ID, + Name: "not prebuilds", + }) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") + + // When: attempting to delete a system user. + err = db.UpdateUserDeletedByID(ctx, systemUser.ID) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") + + // When: attempting to update a user's roles. + _, err = db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ + ID: systemUser.ID, + GrantedRoles: []string{rbac.RoleAuditor().String()}, + }) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") +} + +func testSQLDB(t testing.TB) *sql.DB { + t.Helper() + + connection, err := dbtestutil.Open(t) + require.NoError(t, err) + + db, err := sql.Open("postgres", connection) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + return db +} From 6639167999a35b1c25dcae0a90cca3576e34827c Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 19:55:25 +0000 Subject: [PATCH 05/25] reverting RBAC changes; not relevant here appeasing linter Signed-off-by: Danny Kopping --- coderd/database/dbauthz/dbauthz.go | 28 ---------------------------- coderd/users_test.go | 4 ++-- 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 38f6af62a4705..34c6999a16a0b 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -18,7 +18,6 @@ import ( "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/rbac/rolestore" @@ -359,27 +358,6 @@ var ( }), Scope: rbac.ScopeAll, }.WithCachedASTValue() - - subjectPrebuildsOrchestrator = rbac.Subject{ - FriendlyName: "Prebuilds Orchestrator", - ID: prebuilds.OwnerID.String(), - Roles: rbac.Roles([]rbac.Role{ - { - Identifier: rbac.RoleIdentifier{Name: "prebuilds-orchestrator"}, - DisplayName: "Coder", - Site: rbac.Permissions(map[string][]policy.Action{ - // May use template, read template-related info, & insert template-related resources (preset prebuilds). - rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionUse}, - // May CRUD workspaces, and start/stop them. - rbac.ResourceWorkspace.Type: { - policy.ActionCreate, policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, - policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, - }, - }), - }, - }), - Scope: rbac.ScopeAll, - }.WithCachedASTValue() ) // AsProvisionerd returns a context with an actor that has permissions required @@ -434,12 +412,6 @@ func AsSystemReadProvisionerDaemons(ctx context.Context) context.Context { return context.WithValue(ctx, authContextKey{}, subjectSystemReadProvisionerDaemons) } -// AsPrebuildsOrchestrator returns a context with an actor that has permissions -// to read orchestrator workspace prebuilds. -func AsPrebuildsOrchestrator(ctx context.Context) context.Context { - return context.WithValue(ctx, authContextKey{}, subjectPrebuildsOrchestrator) -} - var AsRemoveActor = rbac.Subject{ ID: "remove-actor", } diff --git a/coderd/users_test.go b/coderd/users_test.go index 0b83d33e4b435..99b10cba961dd 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -2428,7 +2428,7 @@ func TestSystemUserBehaviour(t *testing.T) { sqlDB := testSQLDB(t) err := migrations.Up(sqlDB) // coderd/database/migrations/00030*_system_user.up.sql will create a system user. require.NoError(t, err, "migrations") - + db := database.New(sqlDB) // ================================================================================================================= @@ -2488,7 +2488,7 @@ func TestSystemUserBehaviour(t *testing.T) { // When: attempting to update a user's roles. _, err = db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ - ID: systemUser.ID, + ID: systemUser.ID, GrantedRoles: []string{rbac.RoleAuditor().String()}, }) // Then: the attempt is rejected by a postgres trigger. From 769ae1d24c708e4e4ade40195939a4b9acd9f573 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 20:07:29 +0000 Subject: [PATCH 06/25] removing unnecessary changes Signed-off-by: Danny Kopping --- enterprise/coderd/groups_test.go | 1 - enterprise/coderd/roles_test.go | 8 -------- enterprise/coderd/templates_test.go | 1 - 3 files changed, 10 deletions(-) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index a6c9212b955f8..2f6660c9eb280 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -825,7 +825,6 @@ func TestGroup(t *testing.T) { // TODO (sasswart): this test seems to have drifted from its original intention. evaluate and remove/fix t.Parallel() - // TODO: we should not be returning the prebuilds user in Group, and this is not returned in dbmem. if !dbtestutil.WillUsePostgres() { t.Skip("This test requires postgres") } diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go index b2d7da47a608d..57b66a368248c 100644 --- a/enterprise/coderd/roles_test.go +++ b/enterprise/coderd/roles_test.go @@ -7,8 +7,6 @@ import ( "slices" "testing" - "github.com/coder/coder/v2/coderd/database/dbtestutil" - "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -335,12 +333,6 @@ func TestCustomOrganizationRole(t *testing.T) { // Verify deleting a custom role cascades to all members t.Run("DeleteRoleCascadeMembers", func(t *testing.T) { t.Parallel() - - // TODO: we should not be returning the prebuilds user in OrganizationMembers, and this is not returned in dbmem. - if !dbtestutil.WillUsePostgres() { - t.Skip("This test requires postgres") - } - owner, first := coderdenttest.New(t, &coderdenttest.Options{ LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index e66adebca4680..424d4279328cf 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -924,7 +924,6 @@ func TestTemplateACL(t *testing.T) { t.Run("everyoneGroup", func(t *testing.T) { t.Parallel() - // TODO: we should not be returning the prebuilds user in TemplateACL, and this is not returned in dbmem. if !dbtestutil.WillUsePostgres() { t.Skip("This test requires postgres") } From e7e9c2739ab7b8565cbe022d0d3c52b5c460e8f4 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 13 Mar 2025 21:10:30 +0000 Subject: [PATCH 07/25] exclude system user db tests from non-linux OSs Signed-off-by: Danny Kopping --- coderd/users_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/users_test.go b/coderd/users_test.go index 99b10cba961dd..2f3e39cac9552 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "net/http" + "runtime" "slices" "strings" "testing" @@ -2423,6 +2424,10 @@ func TestSystemUserBehaviour(t *testing.T) { // Setup. t.Parallel() + if runtime.GOOS != "linux" { + t.Skip("skipping because non-linux platforms are tricky to run the mock db container on, and there's no platform-dependence on these tests") + } + ctx := testutil.Context(t, testutil.WaitLong) sqlDB := testSQLDB(t) From 39360476439d434dd71e3dd22ffc2b741e8d8fbe Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 17 Mar 2025 05:13:27 +0000 Subject: [PATCH 08/25] Rename prebuild system user reference --- coderd/prebuilds/id.go | 2 +- coderd/users_test.go | 2 +- enterprise/coderd/groups_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/prebuilds/id.go b/coderd/prebuilds/id.go index bde76e3f7bf14..7c2bbe79b7a6f 100644 --- a/coderd/prebuilds/id.go +++ b/coderd/prebuilds/id.go @@ -2,4 +2,4 @@ package prebuilds import "github.com/google/uuid" -var OwnerID = uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") +var SystemUserID = uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") diff --git a/coderd/users_test.go b/coderd/users_test.go index 2f3e39cac9552..0ed98647c820f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -2460,7 +2460,7 @@ func TestSystemUserBehaviour(t *testing.T) { require.NotNil(t, regularUser) require.True(t, systemUser.IsSystem.Bool) - require.Equal(t, systemUser.ID, prebuilds.OwnerID) + require.Equal(t, systemUser.ID, prebuilds.SystemUserID) require.False(t, regularUser.IsSystem.Bool) require.Equal(t, regularUser.ID, other.ID) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 2f6660c9eb280..1f5fb6934ac65 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -840,7 +840,7 @@ func TestGroup(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) // nolint:gocritic // "This client is operating as the owner user" is fine in this case. - prebuildsUser, err := client.User(ctx, prebuilds.OwnerID.String()) + prebuildsUser, err := client.User(ctx, prebuilds.SystemUserID.String()) require.NoError(t, err) // The 'Everyone' group always has an ID that matches the organization ID. group, err := userAdminClient.Group(ctx, user.OrganizationID) From 8bdcafbba60e9ad74429ba867970fb268d8ebae4 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 17 Mar 2025 07:45:16 +0000 Subject: [PATCH 09/25] ensure that users.IsSystem is not nullable --- coderd/database/dbmem/dbmem.go | 6 +++--- coderd/database/dump.sql | 2 +- coderd/database/migrations/000301_system_user.up.sql | 2 +- coderd/database/models.go | 2 +- coderd/database/queries.sql.go | 2 +- coderd/users_test.go | 8 ++++---- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 0c39738069cb0..b899309d3910a 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1555,7 +1555,7 @@ func (q *FakeQuerier) AllUserIDs(_ context.Context, includeSystem bool) ([]uuid. defer q.mutex.RUnlock() userIDs := make([]uuid.UUID, 0, len(q.users)) for idx := range q.users { - if !includeSystem && q.users[idx].IsSystem.Valid && q.users[idx].IsSystem.Bool { + if !includeSystem && q.users[idx].IsSystem { continue } @@ -2656,7 +2656,7 @@ func (q *FakeQuerier) GetActiveUserCount(_ context.Context, includeSystem bool) active := int64(0) for _, u := range q.users { - if !includeSystem && u.IsSystem.Valid && u.IsSystem.Bool { + if !includeSystem && u.IsSystem { continue } @@ -6221,7 +6221,7 @@ func (q *FakeQuerier) GetUserCount(_ context.Context, includeSystem bool) (int64 existing++ } - if !includeSystem && u.IsSystem.Valid && u.IsSystem.Bool { + if !includeSystem && u.IsSystem { continue } } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 6961b1386e176..359bf5472b119 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -865,7 +865,7 @@ CREATE TABLE users ( github_com_user_id bigint, hashed_one_time_passcode bytea, one_time_passcode_expires_at timestamp with time zone, - is_system boolean DEFAULT false, + is_system boolean DEFAULT false NOT NULL, CONSTRAINT one_time_passcode_set CHECK ((((hashed_one_time_passcode IS NULL) AND (one_time_passcode_expires_at IS NULL)) OR ((hashed_one_time_passcode IS NOT NULL) AND (one_time_passcode_expires_at IS NOT NULL)))) ); diff --git a/coderd/database/migrations/000301_system_user.up.sql b/coderd/database/migrations/000301_system_user.up.sql index 0edb25ef076d6..7be8883de567c 100644 --- a/coderd/database/migrations/000301_system_user.up.sql +++ b/coderd/database/migrations/000301_system_user.up.sql @@ -1,5 +1,5 @@ ALTER TABLE users - ADD COLUMN is_system bool DEFAULT false; + ADD COLUMN is_system bool DEFAULT false NOT NULL; CREATE INDEX user_is_system_idx ON users USING btree (is_system); diff --git a/coderd/database/models.go b/coderd/database/models.go index 8e98510389118..d384b988bd1d7 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3187,7 +3187,7 @@ type User struct { // The time when the one-time-passcode expires. OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` // Determines if a user is a system user, and therefore cannot login or perform normal actions - IsSystem sql.NullBool `db:"is_system" json:"is_system"` + IsSystem bool `db:"is_system" json:"is_system"` } type UserConfig struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2a448ca836c35..8c8288f45f7f2 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -11677,7 +11677,7 @@ type GetUsersRow struct { GithubComUserID sql.NullInt64 `db:"github_com_user_id" json:"github_com_user_id"` HashedOneTimePasscode []byte `db:"hashed_one_time_passcode" json:"hashed_one_time_passcode"` OneTimePasscodeExpiresAt sql.NullTime `db:"one_time_passcode_expires_at" json:"one_time_passcode_expires_at"` - IsSystem sql.NullBool `db:"is_system" json:"is_system"` + IsSystem bool `db:"is_system" json:"is_system"` Count int64 `db:"count" json:"count"` } diff --git a/coderd/users_test.go b/coderd/users_test.go index 0ed98647c820f..ca87ce303c003 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -2450,7 +2450,7 @@ func TestSystemUserBehaviour(t *testing.T) { var systemUser, regularUser database.GetUsersRow for _, u := range users { - if u.IsSystem.Bool { + if u.IsSystem { systemUser = u } else { regularUser = u @@ -2459,9 +2459,9 @@ func TestSystemUserBehaviour(t *testing.T) { require.NotNil(t, systemUser) require.NotNil(t, regularUser) - require.True(t, systemUser.IsSystem.Bool) + require.True(t, systemUser.IsSystem) require.Equal(t, systemUser.ID, prebuilds.SystemUserID) - require.False(t, regularUser.IsSystem.Bool) + require.False(t, regularUser.IsSystem) require.Equal(t, regularUser.ID, other.ID) // ================================================================================================================= @@ -2474,7 +2474,7 @@ func TestSystemUserBehaviour(t *testing.T) { // Then: only regular users are returned. require.NoError(t, err) require.Len(t, users, 1) - require.False(t, users[0].IsSystem.Bool) + require.False(t, users[0].IsSystem) // ================================================================================================================= From 324fde215478eeac76ea5e97ab32d47b9f542d50 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 17 Mar 2025 12:53:48 +0000 Subject: [PATCH 10/25] Fixes Signed-off-by: Danny Kopping --- coderd/database/querier_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 7606e90b5107c..837068f1fa03e 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -15,7 +15,6 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" From 896c8815d6122801f79bb96b499ab65f0ae03efc Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 18 Mar 2025 12:54:34 +0000 Subject: [PATCH 11/25] renumber migrations --- .../database/migrations/000195_oauth2_provider_codes.up.sql | 4 ++++ ...00301_system_user.down.sql => 000302_system_user.down.sql} | 0 .../{000301_system_user.up.sql => 000302_system_user.up.sql} | 0 3 files changed, 4 insertions(+) rename coderd/database/migrations/{000301_system_user.down.sql => 000302_system_user.down.sql} (100%) rename coderd/database/migrations/{000301_system_user.up.sql => 000302_system_user.up.sql} (100%) diff --git a/coderd/database/migrations/000195_oauth2_provider_codes.up.sql b/coderd/database/migrations/000195_oauth2_provider_codes.up.sql index 04333c0ed2ad4..225a1107122b6 100644 --- a/coderd/database/migrations/000195_oauth2_provider_codes.up.sql +++ b/coderd/database/migrations/000195_oauth2_provider_codes.up.sql @@ -43,6 +43,10 @@ AFTER DELETE ON oauth2_provider_app_tokens FOR EACH ROW EXECUTE PROCEDURE delete_deleted_oauth2_provider_app_token_api_key(); +-- This migration has been modified after its initial commit. +-- The new implementation makes the same changes as the original, but +-- takes into account the message in create_migration.sh. This is done +-- to allow the insertion of a user with the "none" login type in later migrations. CREATE TYPE new_logintype AS ENUM ( 'password', 'github', diff --git a/coderd/database/migrations/000301_system_user.down.sql b/coderd/database/migrations/000302_system_user.down.sql similarity index 100% rename from coderd/database/migrations/000301_system_user.down.sql rename to coderd/database/migrations/000302_system_user.down.sql diff --git a/coderd/database/migrations/000301_system_user.up.sql b/coderd/database/migrations/000302_system_user.up.sql similarity index 100% rename from coderd/database/migrations/000301_system_user.up.sql rename to coderd/database/migrations/000302_system_user.up.sql From de4fb8ac80e3dc8d0815ecc28ce3dda453daaa7b Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 07:01:57 +0000 Subject: [PATCH 12/25] ensure that system users are filtered and returned consistently --- coderd/database/dbauthz/dbauthz.go | 10 ++--- coderd/database/dbauthz/dbauthz_test.go | 10 ++++- coderd/database/dbauthz/groupsauth_test.go | 5 ++- coderd/database/dbgen/dbgen_test.go | 5 ++- coderd/database/dbmem/dbmem.go | 15 ++++--- coderd/database/dbmetrics/querymetrics.go | 8 ++-- coderd/database/dbmock/dbmock.go | 16 +++---- coderd/database/dump.sql | 1 + .../migrations/000302_system_user.down.sql | 36 +++++++++++++++ .../migrations/000302_system_user.up.sql | 40 +++++++++++++++-- coderd/database/models.go | 1 + coderd/database/querier.go | 4 +- coderd/database/querier_test.go | 5 ++- coderd/database/queries.sql.go | 44 +++++++++++++++---- coderd/database/queries/groupmembers.sql | 20 ++++++++- enterprise/coderd/groups.go | 40 +++++++++++++---- enterprise/coderd/groups_test.go | 15 +------ enterprise/coderd/templates.go | 20 +++++++-- enterprise/coderd/templates_test.go | 7 +-- 19 files changed, 227 insertions(+), 75 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9bb091a8a9133..52360f2b40dd8 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1747,15 +1747,15 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, return q.db.GetGroupMembers(ctx) } -func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) { - return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id) +func (q *querier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) { + return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, arg) } -func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check +func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) { + if _, err := q.GetGroupByID(ctx, arg.GroupID); err != nil { // AuthZ check return 0, err } - memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID) + memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, arg) if err != nil { return 0, err } diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index cb5defb1231ca..8a3908ea9c23d 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -387,12 +387,18 @@ func (s *MethodTestSuite) TestGroup() { g := dbgen.Group(s.T(), db, database.Group{}) u := dbgen.User(s.T(), db, database.User{}) gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) - check.Args(g.ID).Asserts(gm, policy.ActionRead) + check.Args(database.GetGroupMembersByGroupIDParams{ + GroupID: g.ID, + IncludeSystem: false, + }).Asserts(gm, policy.ActionRead) })) s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) g := dbgen.Group(s.T(), db, database.Group{}) - check.Args(g.ID).Asserts(g, policy.ActionRead) + check.Args(database.GetGroupMembersCountByGroupIDParams{ + GroupID: g.ID, + IncludeSystem: false, + }).Asserts(g, policy.ActionRead) })) s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) diff --git a/coderd/database/dbauthz/groupsauth_test.go b/coderd/database/dbauthz/groupsauth_test.go index 04d816629ac65..a9f26e303d644 100644 --- a/coderd/database/dbauthz/groupsauth_test.go +++ b/coderd/database/dbauthz/groupsauth_test.go @@ -147,7 +147,10 @@ func TestGroupsAuth(t *testing.T) { require.Error(t, err, "group read") } - members, err := db.GetGroupMembersByGroupID(actorCtx, group.ID) + members, err := db.GetGroupMembersByGroupID(actorCtx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if tc.ReadMembers { require.NoError(t, err, "member read") require.Len(t, members, tc.MembersExpected, "member count found does not match") diff --git a/coderd/database/dbgen/dbgen_test.go b/coderd/database/dbgen/dbgen_test.go index eec6e90d5904a..de45f90d91f2a 100644 --- a/coderd/database/dbgen/dbgen_test.go +++ b/coderd/database/dbgen/dbgen_test.go @@ -105,7 +105,10 @@ func TestGenerator(t *testing.T) { gm := dbgen.GroupMember(t, db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) exp := []database.GroupMember{gm} - require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), g.ID))) + require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), database.GetGroupMembersByGroupIDParams{ + GroupID: g.ID, + IncludeSystem: false, + }))) }) t.Run("Organization", func(t *testing.T) { diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 163d655a9d6f5..24a291af9b2fe 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3427,17 +3427,17 @@ func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMemb return groupMembers, nil } -func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) { +func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() - if q.isEveryoneGroup(id) { - return q.getEveryoneGroupMembersNoLock(ctx, id), nil + if q.isEveryoneGroup(arg.GroupID) { + return q.getEveryoneGroupMembersNoLock(ctx, arg.GroupID), nil } var groupMembers []database.GroupMember for _, member := range q.groupMembers { - if member.GroupID == id { + if member.GroupID == arg.GroupID { groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID) if errors.Is(err, errUserDeleted) { continue @@ -3452,8 +3452,11 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID return groupMembers, nil } -func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - users, err := q.GetGroupMembersByGroupID(ctx, groupID) +func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) { + users, err := q.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: arg.GroupID, + IncludeSystem: arg.IncludeSystem, + }) if err != nil { return 0, err } diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index eb198baf75c7d..cbf0cae978001 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -767,16 +767,16 @@ func (m queryMetricsStore) GetGroupMembers(ctx context.Context) ([]database.Grou return r0, r1 } -func (m queryMetricsStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]database.GroupMember, error) { +func (m queryMetricsStore) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) { start := time.Now() - users, err := m.s.GetGroupMembersByGroupID(ctx, groupID) + users, err := m.s.GetGroupMembersByGroupID(ctx, arg) m.queryLatencies.WithLabelValues("GetGroupMembersByGroupID").Observe(time.Since(start).Seconds()) return users, err } -func (m queryMetricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { +func (m queryMetricsStore) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) { start := time.Now() - r0, r1 := m.s.GetGroupMembersCountByGroupID(ctx, groupID) + r0, r1 := m.s.GetGroupMembersCountByGroupID(ctx, arg) m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupID").Observe(time.Since(start).Seconds()) return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 5007c6746b76d..6f37930418c33 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1538,33 +1538,33 @@ func (mr *MockStoreMockRecorder) GetGroupMembers(ctx any) *gomock.Call { } // GetGroupMembersByGroupID mocks base method. -func (m *MockStore) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]database.GroupMember, error) { +func (m *MockStore) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetGroupMembersByGroupID", ctx, groupID) + ret := m.ctrl.Call(m, "GetGroupMembersByGroupID", ctx, arg) ret0, _ := ret[0].([]database.GroupMember) ret1, _ := ret[1].(error) return ret0, ret1 } // GetGroupMembersByGroupID indicates an expected call of GetGroupMembersByGroupID. -func (mr *MockStoreMockRecorder) GetGroupMembersByGroupID(ctx, groupID any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetGroupMembersByGroupID(ctx, arg any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersByGroupID), ctx, groupID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersByGroupID), ctx, arg) } // GetGroupMembersCountByGroupID mocks base method. -func (m *MockStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { +func (m *MockStore) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupID", ctx, groupID) + ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupID", ctx, arg) ret0, _ := ret[0].(int64) ret1, _ := ret[1].(error) return ret0, ret1 } // GetGroupMembersCountByGroupID indicates an expected call of GetGroupMembersCountByGroupID. -func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(ctx, groupID any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(ctx, arg any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), ctx, groupID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), ctx, arg) } // GetGroups mocks base method. diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 63b7df0e5010a..772bd8c3c5d9c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -906,6 +906,7 @@ CREATE VIEW group_members_expanded AS users.quiet_hours_schedule AS user_quiet_hours_schedule, users.name AS user_name, users.github_com_user_id AS user_github_com_user_id, + users.is_system AS user_is_system, groups.organization_id, groups.name AS group_name, all_members.group_id diff --git a/coderd/database/migrations/000302_system_user.down.sql b/coderd/database/migrations/000302_system_user.down.sql index c286cfa193745..74707f1cf92a0 100644 --- a/coderd/database/migrations/000302_system_user.down.sql +++ b/coderd/database/migrations/000302_system_user.down.sql @@ -1,3 +1,39 @@ +DROP VIEW IF EXISTS group_members_expanded; +CREATE VIEW group_members_expanded AS + WITH all_members AS ( + SELECT group_members.user_id, + group_members.group_id + FROM group_members + UNION + SELECT organization_members.user_id, + organization_members.organization_id AS group_id + FROM organization_members + ) + SELECT users.id AS user_id, + users.email AS user_email, + users.username AS user_username, + users.hashed_password AS user_hashed_password, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.status AS user_status, + users.rbac_roles AS user_rbac_roles, + users.login_type AS user_login_type, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.last_seen_at AS user_last_seen_at, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + users.name AS user_name, + users.github_com_user_id AS user_github_com_user_id, + groups.organization_id, + groups.name AS group_name, + all_members.group_id + FROM ((all_members + JOIN users ON ((users.id = all_members.user_id))) + JOIN groups ON ((groups.id = all_members.group_id))) + WHERE (users.deleted = false); + +COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).'; + -- Remove system user from organizations DELETE FROM organization_members WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; diff --git a/coderd/database/migrations/000302_system_user.up.sql b/coderd/database/migrations/000302_system_user.up.sql index 7be8883de567c..73c04eaa69f14 100644 --- a/coderd/database/migrations/000302_system_user.up.sql +++ b/coderd/database/migrations/000302_system_user.up.sql @@ -5,11 +5,9 @@ CREATE INDEX user_is_system_idx ON users USING btree (is_system); COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions'; --- TODO: tried using "none" for login type, but the migration produced this error: 'unsafe use of new value "none" of enum type login_type' --- -> not sure why though? it exists on the login_type enum. INSERT INTO users (id, email, username, name, created_at, updated_at, status, rbac_roles, hashed_password, is_system, login_type) VALUES ('c42fdf75-3097-471c-8c33-fb52454d81c0', 'prebuilds@system', 'prebuilds', 'Prebuilds Owner', now(), now(), - 'active', '{}', 'none', true, 'password'::login_type); + 'active', '{}', 'none', true, 'none'::login_type); -- Create function to check system user modifications CREATE OR REPLACE FUNCTION prevent_system_user_changes() @@ -37,6 +35,42 @@ CREATE TRIGGER prevent_system_user_deletions WHEN (OLD.is_system = true) EXECUTE FUNCTION prevent_system_user_changes(); +DROP VIEW IF EXISTS group_members_expanded; +CREATE VIEW group_members_expanded AS + WITH all_members AS ( + SELECT group_members.user_id, + group_members.group_id + FROM group_members + UNION + SELECT organization_members.user_id, + organization_members.organization_id AS group_id + FROM organization_members + ) + SELECT users.id AS user_id, + users.email AS user_email, + users.username AS user_username, + users.hashed_password AS user_hashed_password, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.status AS user_status, + users.rbac_roles AS user_rbac_roles, + users.login_type AS user_login_type, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.last_seen_at AS user_last_seen_at, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + users.name AS user_name, + users.github_com_user_id AS user_github_com_user_id, + users.is_system AS user_is_system, + groups.organization_id, + groups.name AS group_name, + all_members.group_id + FROM ((all_members + JOIN users ON ((users.id = all_members.user_id))) + JOIN groups ON ((groups.id = all_members.group_id))) + WHERE (users.deleted = false); + +COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).'; -- TODO: do we *want* to use the default org here? how do we handle multi-org? WITH default_org AS (SELECT id FROM organizations diff --git a/coderd/database/models.go b/coderd/database/models.go index 0f92fe0fcced8..f4503573c24df 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2610,6 +2610,7 @@ type GroupMember struct { UserQuietHoursSchedule string `db:"user_quiet_hours_schedule" json:"user_quiet_hours_schedule"` UserName string `db:"user_name" json:"user_name"` UserGithubComUserID sql.NullInt64 `db:"user_github_com_user_id" json:"user_github_com_user_id"` + UserIsSystem bool `db:"user_is_system" json:"user_is_system"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` GroupName string `db:"group_name" json:"group_name"` GroupID uuid.UUID `db:"group_id" json:"group_id"` diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 05fcfb5c4b9e8..909bbd9e1298c 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -173,11 +173,11 @@ type sqlcQuerier interface { GetGroupByID(ctx context.Context, id uuid.UUID) (Group, error) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) GetGroupMembers(ctx context.Context) ([]GroupMember, error) - GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) + GetGroupMembersByGroupID(ctx context.Context, arg GetGroupMembersByGroupIDParams) ([]GroupMember, error) // Returns the total count of members in a group. Shows the total // count even if the caller does not have read access to ResourceGroupMember. // They only need ResourceGroup read access. - GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) + GetGroupMembersCountByGroupID(ctx context.Context, arg GetGroupMembersCountByGroupIDParams) (int64, error) GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) GetHealthSettings(ctx context.Context) (string, error) GetHungProvisionerJobs(ctx context.Context, updatedAt time.Time) ([]ProvisionerJob, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 837068f1fa03e..5544a7948ae4f 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1505,7 +1505,10 @@ func TestWorkspaceQuotas(t *testing.T) { }) // Fetch the 'Everyone' group members - everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, org.ID) + everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: everyoneGroup.ID, + IncludeSystem: false, + }) require.NoError(t, err) require.ElementsMatch(t, db2sdk.List(everyoneMembers, groupMemberIDs), diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 72f6ff7a86e77..a2227ed80bd5f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1579,7 +1579,7 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG } const getGroupMembers = `-- name: GetGroupMembers :many -SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded +SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, user_is_system, organization_id, group_name, group_id FROM group_members_expanded ` func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) { @@ -1607,6 +1607,7 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) &i.UserQuietHoursSchedule, &i.UserName, &i.UserGithubComUserID, + &i.UserIsSystem, &i.OrganizationID, &i.GroupName, &i.GroupID, @@ -1625,11 +1626,24 @@ func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) } const getGroupMembersByGroupID = `-- name: GetGroupMembersByGroupID :many -SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded WHERE group_id = $1 +SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, user_is_system, organization_id, group_name, group_id +FROM group_members_expanded +WHERE group_id = $1 + -- Filter by system type + AND CASE + WHEN $2::bool THEN TRUE + ELSE + user_is_system = false + END ` -func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid.UUID) ([]GroupMember, error) { - rows, err := q.db.QueryContext(ctx, getGroupMembersByGroupID, groupID) +type GetGroupMembersByGroupIDParams struct { + GroupID uuid.UUID `db:"group_id" json:"group_id"` + IncludeSystem bool `db:"include_system" json:"include_system"` +} + +func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, arg GetGroupMembersByGroupIDParams) ([]GroupMember, error) { + rows, err := q.db.QueryContext(ctx, getGroupMembersByGroupID, arg.GroupID, arg.IncludeSystem) if err != nil { return nil, err } @@ -1653,6 +1667,7 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. &i.UserQuietHoursSchedule, &i.UserName, &i.UserGithubComUserID, + &i.UserIsSystem, &i.OrganizationID, &i.GroupName, &i.GroupID, @@ -1671,14 +1686,27 @@ func (q *sqlQuerier) GetGroupMembersByGroupID(ctx context.Context, groupID uuid. } const getGroupMembersCountByGroupID = `-- name: GetGroupMembersCountByGroupID :one -SELECT COUNT(*) FROM group_members_expanded WHERE group_id = $1 +SELECT COUNT(*) +FROM group_members_expanded +WHERE group_id = $1 + -- Filter by system type + AND CASE + WHEN $2::bool THEN TRUE + ELSE + user_is_system = false + END ` +type GetGroupMembersCountByGroupIDParams struct { + GroupID uuid.UUID `db:"group_id" json:"group_id"` + IncludeSystem bool `db:"include_system" json:"include_system"` +} + // Returns the total count of members in a group. Shows the total // count even if the caller does not have read access to ResourceGroupMember. // They only need ResourceGroup read access. -func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) { - row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, groupID) +func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg GetGroupMembersCountByGroupIDParams) (int64, error) { + row := q.db.QueryRowContext(ctx, getGroupMembersCountByGroupID, arg.GroupID, arg.IncludeSystem) var count int64 err := row.Scan(&count) return count, err @@ -7857,7 +7885,7 @@ FROM ( -- Select all groups this user is a member of. This will also include -- the "Everyone" group for organizations the user is a member of. - SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded + SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, user_is_system, organization_id, group_name, group_id FROM group_members_expanded WHERE $1 = user_id AND $2 = group_members_expanded.organization_id diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 4efe9bf488590..f5a42e6ce17eb 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -2,13 +2,29 @@ SELECT * FROM group_members_expanded; -- name: GetGroupMembersByGroupID :many -SELECT * FROM group_members_expanded WHERE group_id = @group_id; +SELECT * +FROM group_members_expanded +WHERE group_id = @group_id + -- Filter by system type + AND CASE + WHEN @include_system::bool THEN TRUE + ELSE + user_is_system = false + END; -- name: GetGroupMembersCountByGroupID :one -- Returns the total count of members in a group. Shows the total -- count even if the caller does not have read access to ResourceGroupMember. -- They only need ResourceGroup read access. -SELECT COUNT(*) FROM group_members_expanded WHERE group_id = @group_id; +SELECT COUNT(*) +FROM group_members_expanded +WHERE group_id = @group_id + -- Filter by system type + AND CASE + WHEN @include_system::bool THEN TRUE + ELSE + user_is_system = false + END; -- InsertUserGroupsByName adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByName :exec diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index fef4a1bbef04b..3c5ecf6bfbff5 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -153,7 +153,10 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { return } - currentMembers, err := api.Database.GetGroupMembersByGroupID(ctx, group.ID) + currentMembers, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -283,7 +286,10 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { httpapi.InternalServerError(rw, err) } - patchedMembers, err := api.Database.GetGroupMembersByGroupID(ctx, group.ID) + patchedMembers, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -291,7 +297,10 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { aReq.New = group.Auditable(patchedMembers) - memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -334,7 +343,10 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { return } - groupMembers, getMembersErr := api.Database.GetGroupMembersByGroupID(ctx, group.ID) + groupMembers, getMembersErr := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if getMembersErr != nil { httpapi.InternalServerError(rw, getMembersErr) return @@ -385,13 +397,19 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) { httpapi.InternalServerError(rw, err) } - users, err := api.Database.GetGroupMembersByGroupID(ctx, group.ID) + users, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if err != nil && !errors.Is(err, sql.ErrNoRows) { httpapi.InternalServerError(rw, err) return } - memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.ID) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -484,12 +502,18 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { resp := make([]codersdk.Group, 0, len(groups)) for _, group := range groups { - members, err := api.Database.GetGroupMembersByGroupID(ctx, group.Group.ID) + members, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return } - memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, group.Group.ID) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 1f5fb6934ac65..690a476fcb1ba 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -6,9 +6,6 @@ import ( "testing" "time" - "github.com/coder/coder/v2/coderd/database/dbtestutil" - "github.com/coder/coder/v2/coderd/prebuilds" - "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -822,13 +819,7 @@ func TestGroup(t *testing.T) { }) t.Run("everyoneGroupReturnsEmpty", func(t *testing.T) { - // TODO (sasswart): this test seems to have drifted from its original intention. evaluate and remove/fix t.Parallel() - - if !dbtestutil.WillUsePostgres() { - t.Skip("This test requires postgres") - } - client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, @@ -839,18 +830,14 @@ func TestGroup(t *testing.T) { _, user2 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) ctx := testutil.Context(t, testutil.WaitLong) - // nolint:gocritic // "This client is operating as the owner user" is fine in this case. - prebuildsUser, err := client.User(ctx, prebuilds.SystemUserID.String()) - require.NoError(t, err) // The 'Everyone' group always has an ID that matches the organization ID. group, err := userAdminClient.Group(ctx, user.OrganizationID) require.NoError(t, err) - require.Len(t, group.Members, 5) + require.Len(t, group.Members, 4) require.Equal(t, "Everyone", group.Name) require.Equal(t, user.OrganizationID, group.OrganizationID) require.Contains(t, group.Members, user1.ReducedUser) require.Contains(t, group.Members, user2.ReducedUser) - require.Contains(t, group.Members, prebuildsUser.ReducedUser) }) } diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 37c0151749196..b1f3d2cac3ac5 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -62,14 +62,20 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req sdkGroups := make([]codersdk.Group, 0, len(groups)) for _, group := range groups { // nolint:gocritic - members, err := api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), group.Group.ID) + members, err := api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return } // nolint:gocritic - memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), group.Group.ID) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -138,13 +144,19 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { // them read the group members. // We should probably at least return more truncated user data here. // nolint:gocritic - members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), group.ID) + members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return } // nolint:gocritic - memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), group.ID) + memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDParams{ + GroupID: group.Group.ID, + IncludeSystem: false, + }) if err != nil { httpapi.InternalServerError(rw, err) return diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 424d4279328cf..b6c2048190e9a 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -18,7 +18,6 @@ import ( "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/dbtestutil" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" "github.com/coder/coder/v2/coderd/rbac" @@ -924,10 +923,6 @@ func TestTemplateACL(t *testing.T) { t.Run("everyoneGroup", func(t *testing.T) { t.Parallel() - if !dbtestutil.WillUsePostgres() { - t.Skip("This test requires postgres") - } - client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, @@ -946,7 +941,7 @@ func TestTemplateACL(t *testing.T) { require.NoError(t, err) require.Len(t, acl.Groups, 1) - require.Len(t, acl.Groups[0].Members, 3) // orgAdmin + TemplateAdmin + prebuilds user + require.Len(t, acl.Groups[0].Members, 2) // orgAdmin + TemplateAdmin require.Len(t, acl.Users, 0) }) From 2751d5b46ecb099ee4811a89c29734c7d8b93be7 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 07:55:55 +0000 Subject: [PATCH 13/25] make -B lint --- coderd/database/dbmem/dbmem.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 24a291af9b2fe..4f4451310df05 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3453,10 +3453,7 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, arg database } func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) { - users, err := q.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ - GroupID: arg.GroupID, - IncludeSystem: arg.IncludeSystem, - }) + users, err := q.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams(arg)) if err != nil { return 0, err } From 1042c39b03629b9a56b8a9549d56448d5e41c2f3 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 08:45:46 +0000 Subject: [PATCH 14/25] rewrite prebuilds system user tests in our usual style --- coderd/database/querier_test.go | 115 ++++++++++++++++++++++++++++++++ coderd/users_test.go | 97 --------------------------- 2 files changed, 115 insertions(+), 97 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 5544a7948ae4f..c6ffe3ffe84ba 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -25,6 +25,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/migrations" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/provisionersdk" @@ -1364,6 +1365,120 @@ func TestUserLastSeenFilter(t *testing.T) { }) } +func TestGetUsers_IncludeSystem(t *testing.T) { + t.Parallel() + if !dbtestutil.WillUsePostgres() { + t.Skip("test only supports postgres") + } + + tests := []struct { + name string + includeSystem bool + wantSystemUser bool + }{ + { + name: "include system users", + includeSystem: true, + wantSystemUser: true, + }, + { + name: "exclude system users", + includeSystem: false, + wantSystemUser: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + sqlDB := testSQLDB(t) + // Given: a system user introduced by migration coderd/database/migrations/00030*_system_user.up.sql + err := migrations.Up(sqlDB) + require.NoError(t, err, "migrations") + + db := database.New(sqlDB) + other := dbgen.User(t, db, database.User{}) + users, err := db.GetUsers(ctx, database.GetUsersParams{ + IncludeSystem: tt.includeSystem, + }) + require.NoError(t, err) + + // Should always find the regular user + foundRegularUser := false + foundSystemUser := false + + for _, u := range users { + if u.IsSystem { + foundSystemUser = true + require.Equal(t, prebuilds.SystemUserID, u.ID) + } else { + foundRegularUser = true + require.Equal(t, other.ID, u.ID) + } + } + + require.True(t, foundRegularUser, "regular user should always be found") + require.Equal(t, tt.wantSystemUser, foundSystemUser, "system user presence should match includeSystem setting") + require.Equal(t, tt.wantSystemUser, len(users) == 2, "should have 2 users when including system user, 1 otherwise") + }) + } + +} + +func TestUpdateSystemUser(t *testing.T) { + t.Parallel() + + if !dbtestutil.WillUsePostgres() { + t.Skip("test only supports postgres") + } + + ctx := testutil.Context(t, testutil.WaitLong) + + sqlDB := testSQLDB(t) + // Given: a system user introduced by migration coderd/database/migrations/00030*_system_user.up.sql + err := migrations.Up(sqlDB) + require.NoError(t, err, "migrations") + + db := database.New(sqlDB) + // Given a system user. + users, err := db.GetUsers(ctx, database.GetUsersParams{ + IncludeSystem: true, + }) + require.NoError(t, err) + var systemUser database.GetUsersRow + for _, u := range users { + if u.IsSystem { + systemUser = u + } + } + require.NotNil(t, systemUser) + + // When: attempting to update a system user's name. + _, err = db.UpdateUserProfile(ctx, database.UpdateUserProfileParams{ + ID: systemUser.ID, + Name: "not prebuilds", + }) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") + + // When: attempting to delete a system user. + err = db.UpdateUserDeletedByID(ctx, systemUser.ID) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") + + // When: attempting to update a user's roles. + _, err = db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ + ID: systemUser.ID, + GrantedRoles: []string{rbac.RoleAuditor().String()}, + }) + // Then: the attempt is rejected by a postgres trigger. + require.ErrorContains(t, err, "Cannot modify or delete system users") +} + func TestUserChangeLoginType(t *testing.T) { t.Parallel() if testing.Short() { diff --git a/coderd/users_test.go b/coderd/users_test.go index ca87ce303c003..378aaab2d3a70 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -2,10 +2,8 @@ package coderd_test import ( "context" - "database/sql" "fmt" "net/http" - "runtime" "slices" "strings" "testing" @@ -15,10 +13,8 @@ import ( "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" - "github.com/coder/coder/v2/coderd/database/migrations" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" - "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/golang-jwt/jwt/v4" @@ -2419,96 +2415,3 @@ func BenchmarkUsersMe(b *testing.B) { require.NoError(b, err) } } - -func TestSystemUserBehaviour(t *testing.T) { - // Setup. - t.Parallel() - - if runtime.GOOS != "linux" { - t.Skip("skipping because non-linux platforms are tricky to run the mock db container on, and there's no platform-dependence on these tests") - } - - ctx := testutil.Context(t, testutil.WaitLong) - - sqlDB := testSQLDB(t) - err := migrations.Up(sqlDB) // coderd/database/migrations/00030*_system_user.up.sql will create a system user. - require.NoError(t, err, "migrations") - - db := database.New(sqlDB) - - // ================================================================================================================= - - // When: retrieving users with the include_system flag enabled. - other := dbgen.User(t, db, database.User{}) - users, err := db.GetUsers(ctx, database.GetUsersParams{ - IncludeSystem: true, - }) - - // Then: system users are returned, alongside other users. - require.NoError(t, err) - require.Len(t, users, 2) - - var systemUser, regularUser database.GetUsersRow - for _, u := range users { - if u.IsSystem { - systemUser = u - } else { - regularUser = u - } - } - require.NotNil(t, systemUser) - require.NotNil(t, regularUser) - - require.True(t, systemUser.IsSystem) - require.Equal(t, systemUser.ID, prebuilds.SystemUserID) - require.False(t, regularUser.IsSystem) - require.Equal(t, regularUser.ID, other.ID) - - // ================================================================================================================= - - // When: retrieving users with the include_system flag disabled. - users, err = db.GetUsers(ctx, database.GetUsersParams{ - IncludeSystem: false, - }) - - // Then: only regular users are returned. - require.NoError(t, err) - require.Len(t, users, 1) - require.False(t, users[0].IsSystem) - - // ================================================================================================================= - - // When: attempting to update a system user's name. - _, err = db.UpdateUserProfile(ctx, database.UpdateUserProfileParams{ - ID: systemUser.ID, - Name: "not prebuilds", - }) - // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") - - // When: attempting to delete a system user. - err = db.UpdateUserDeletedByID(ctx, systemUser.ID) - // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") - - // When: attempting to update a user's roles. - _, err = db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ - ID: systemUser.ID, - GrantedRoles: []string{rbac.RoleAuditor().String()}, - }) - // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") -} - -func testSQLDB(t testing.TB) *sql.DB { - t.Helper() - - connection, err := dbtestutil.Open(t) - require.NoError(t, err) - - db, err := sql.Open("postgres", connection) - require.NoError(t, err) - t.Cleanup(func() { _ = db.Close() }) - - return db -} From f9e9d11145d608f95d51bbe03030933c7e30d728 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 09:46:35 +0000 Subject: [PATCH 15/25] add support for prebuilds user to dbmem --- coderd/database/dbmem/dbmem.go | 24 ++++++++++++++++++++++++ coderd/database/querier_test.go | 15 +++++---------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 4f4451310df05..eb312bcd8164f 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -23,6 +23,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/notifications/types" + "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -153,6 +154,21 @@ func New() database.Store { panic(xerrors.Errorf("failed to create psk provisioner key: %w", err)) } + q.mutex.Lock() + q.data.users = append(q.data.users, database.User{ + ID: prebuilds.SystemUserID, + Email: "prebuilds@coder.com", + Username: "prebuilds", + CreatedAt: dbtime.Now(), + UpdatedAt: dbtime.Now(), + Status: "active", + LoginType: "none", + HashedPassword: []byte{}, + IsSystem: true, + Deleted: false, + }) + q.mutex.Unlock() + return q } @@ -439,6 +455,7 @@ func convertUsers(users []database.User, count int64) []database.GetUsersRow { Deleted: u.Deleted, LastSeenAt: u.LastSeenAt, Count: count, + IsSystem: u.IsSystem, } } @@ -6608,6 +6625,12 @@ func (q *FakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users = users[:params.LimitOpt] } + if !params.IncludeSystem { + users = slices.DeleteFunc(users, func(u database.User) bool { + return u.IsSystem + }) + } + return convertUsers(users, int64(beforePageCount)), nil } @@ -8888,6 +8911,7 @@ func (q *FakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam Status: status, RBACRoles: arg.RBACRoles, LoginType: arg.LoginType, + IsSystem: false, } q.users = append(q.users, user) sort.Slice(q.users, func(i, j int) bool { diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index c6ffe3ffe84ba..65aab53bbc8c6 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1367,9 +1367,6 @@ func TestUserLastSeenFilter(t *testing.T) { func TestGetUsers_IncludeSystem(t *testing.T) { t.Parallel() - if !dbtestutil.WillUsePostgres() { - t.Skip("test only supports postgres") - } tests := []struct { name string @@ -1395,12 +1392,10 @@ func TestGetUsers_IncludeSystem(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) - sqlDB := testSQLDB(t) - // Given: a system user introduced by migration coderd/database/migrations/00030*_system_user.up.sql - err := migrations.Up(sqlDB) - require.NoError(t, err, "migrations") - - db := database.New(sqlDB) + // Given: a system user + // postgres: introduced by migration coderd/database/migrations/00030*_system_user.up.sql + // dbmem: created in dbmem/dbmem.go + db, _ := dbtestutil.NewDB(t) other := dbgen.User(t, db, database.User{}) users, err := db.GetUsers(ctx, database.GetUsersParams{ IncludeSystem: tt.includeSystem, @@ -1417,7 +1412,7 @@ func TestGetUsers_IncludeSystem(t *testing.T) { require.Equal(t, prebuilds.SystemUserID, u.ID) } else { foundRegularUser = true - require.Equal(t, other.ID, u.ID) + require.Equalf(t, other.ID.String(), u.ID.String(), "found unexpected regular user") } } From 7492965413f8e82b51b991f2bb9b53a11772ae5f Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 09:47:51 +0000 Subject: [PATCH 16/25] appease the linter --- coderd/database/querier_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 65aab53bbc8c6..4fd85de94679d 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1421,7 +1421,6 @@ func TestGetUsers_IncludeSystem(t *testing.T) { require.Equal(t, tt.wantSystemUser, len(users) == 2, "should have 2 users when including system user, 1 otherwise") }) } - } func TestUpdateSystemUser(t *testing.T) { From 29e2020a319eb49490b1157f662eabf9f2297ffa Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 13:53:02 +0000 Subject: [PATCH 17/25] add support for the prebuilds system user to dbmem --- coderd/database/dbauthz/dbauthz.go | 4 ++-- coderd/database/dbauthz/dbauthz_test.go | 2 +- coderd/database/dbmem/dbmem.go | 14 ++++++++------ coderd/database/dbmetrics/querymetrics.go | 4 ++-- coderd/database/dbmock/dbmock.go | 8 ++++---- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 16 ++++++++++++---- coderd/database/queries/groupmembers.sql | 7 ++++++- coderd/database/queries/users.sql | 7 +++++-- coderd/telemetry/telemetry.go | 2 +- 10 files changed, 42 insertions(+), 24 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 52360f2b40dd8..42e2fca5732b2 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1740,11 +1740,11 @@ func (q *querier) GetGroupByOrgAndName(ctx context.Context, arg database.GetGrou return fetch(q.log, q.auth, q.db.GetGroupByOrgAndName)(ctx, arg) } -func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, error) { +func (q *querier) GetGroupMembers(ctx context.Context, includeSystem bool) ([]database.GroupMember, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return nil, err } - return q.db.GetGroupMembers(ctx) + return q.db.GetGroupMembers(ctx, includeSystem) } func (q *querier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 8a3908ea9c23d..891a221fde766 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -405,7 +405,7 @@ func (s *MethodTestSuite) TestGroup() { g := dbgen.Group(s.T(), db, database.Group{}) u := dbgen.User(s.T(), db, database.User{}) dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID}) - check.Asserts(rbac.ResourceSystem, policy.ActionRead) + check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead) })) s.Run("System/GetGroups", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index eb312bcd8164f..f81e3508f63c7 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3414,7 +3414,7 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr return database.Group{}, sql.ErrNoRows } -func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMember, error) { +func (q *FakeQuerier) GetGroupMembers(ctx context.Context, includeSystem bool) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -3422,6 +3422,9 @@ func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMemb members = append(members, q.groupMembers...) for _, org := range q.organizations { for _, user := range q.users { + if !includeSystem && user.IsSystem { + continue + } members = append(members, database.GroupMemberTable{ UserID: user.ID, GroupID: org.ID, @@ -6254,13 +6257,12 @@ func (q *FakeQuerier) GetUserCount(_ context.Context, includeSystem bool) (int64 existing := int64(0) for _, u := range q.users { - if !u.Deleted { - existing++ - } - if !includeSystem && u.IsSystem { continue } + if !u.Deleted { + existing++ + } } return existing, nil } @@ -10026,7 +10028,7 @@ func (q *FakeQuerier) UpdateInactiveUsersToDormant(_ context.Context, params dat var updated []database.UpdateInactiveUsersToDormantRow for index, user := range q.users { - if user.Status == database.UserStatusActive && user.LastSeenAt.Before(params.LastSeenAfter) { + if user.Status == database.UserStatusActive && user.LastSeenAt.Before(params.LastSeenAfter) && !user.IsSystem { q.users[index].Status = database.UserStatusDormant q.users[index].UpdatedAt = params.UpdatedAt updated = append(updated, database.UpdateInactiveUsersToDormantRow{ diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index cbf0cae978001..77add9eb72039 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -760,9 +760,9 @@ func (m queryMetricsStore) GetGroupByOrgAndName(ctx context.Context, arg databas return group, err } -func (m queryMetricsStore) GetGroupMembers(ctx context.Context) ([]database.GroupMember, error) { +func (m queryMetricsStore) GetGroupMembers(ctx context.Context, includeSystem bool) ([]database.GroupMember, error) { start := time.Now() - r0, r1 := m.s.GetGroupMembers(ctx) + r0, r1 := m.s.GetGroupMembers(ctx, includeSystem) m.queryLatencies.WithLabelValues("GetGroupMembers").Observe(time.Since(start).Seconds()) return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 6f37930418c33..a61d87664ad91 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1523,18 +1523,18 @@ func (mr *MockStoreMockRecorder) GetGroupByOrgAndName(ctx, arg any) *gomock.Call } // GetGroupMembers mocks base method. -func (m *MockStore) GetGroupMembers(ctx context.Context) ([]database.GroupMember, error) { +func (m *MockStore) GetGroupMembers(ctx context.Context, includeSystem bool) ([]database.GroupMember, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetGroupMembers", ctx) + ret := m.ctrl.Call(m, "GetGroupMembers", ctx, includeSystem) ret0, _ := ret[0].([]database.GroupMember) ret1, _ := ret[1].(error) return ret0, ret1 } // GetGroupMembers indicates an expected call of GetGroupMembers. -func (mr *MockStoreMockRecorder) GetGroupMembers(ctx any) *gomock.Call { +func (mr *MockStoreMockRecorder) GetGroupMembers(ctx, includeSystem any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembers", reflect.TypeOf((*MockStore)(nil).GetGroupMembers), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembers", reflect.TypeOf((*MockStore)(nil).GetGroupMembers), ctx, includeSystem) } // GetGroupMembersByGroupID mocks base method. diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 909bbd9e1298c..93b14d7a7d41d 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -172,7 +172,7 @@ type sqlcQuerier interface { GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetGroupByID(ctx context.Context, id uuid.UUID) (Group, error) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) - GetGroupMembers(ctx context.Context) ([]GroupMember, error) + GetGroupMembers(ctx context.Context, includeSystem bool) ([]GroupMember, error) GetGroupMembersByGroupID(ctx context.Context, arg GetGroupMembersByGroupIDParams) ([]GroupMember, error) // Returns the total count of members in a group. Shows the total // count even if the caller does not have read access to ResourceGroupMember. diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a2227ed80bd5f..8913bc4034132 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1580,10 +1580,15 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG const getGroupMembers = `-- name: GetGroupMembers :many SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_name, user_github_com_user_id, user_is_system, organization_id, group_name, group_id FROM group_members_expanded +WHERE CASE + WHEN $1::bool THEN TRUE + ELSE + user_is_system = false + END ` -func (q *sqlQuerier) GetGroupMembers(ctx context.Context) ([]GroupMember, error) { - rows, err := q.db.QueryContext(ctx, getGroupMembers) +func (q *sqlQuerier) GetGroupMembers(ctx context.Context, includeSystem bool) ([]GroupMember, error) { + rows, err := q.db.QueryContext(ctx, getGroupMembers, includeSystem) if err != nil { return nil, err } @@ -11891,10 +11896,11 @@ UPDATE users SET status = 'dormant'::user_status, - updated_at = $1 + updated_at = $1 WHERE last_seen_at < $2 :: timestamp AND status = 'active'::user_status + AND NOT is_system RETURNING id, email, username, last_seen_at ` @@ -12095,7 +12101,9 @@ SET '':: bytea END WHERE - id = $2 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system + id = $2 + AND NOT is_system +RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, quiet_hours_schedule, name, github_com_user_id, hashed_one_time_passcode, one_time_passcode_expires_at, is_system ` type UpdateUserLoginTypeParams struct { diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index f5a42e6ce17eb..7de8dbe4e4523 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -1,5 +1,10 @@ -- name: GetGroupMembers :many -SELECT * FROM group_members_expanded; +SELECT * FROM group_members_expanded +WHERE CASE + WHEN @include_system::bool THEN TRUE + ELSE + user_is_system = false + END; -- name: GetGroupMembersByGroupID :many SELECT * diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 4900e70262e3c..5053cd4a6971b 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -11,7 +11,9 @@ SET '':: bytea END WHERE - id = @user_id RETURNING *; + id = @user_id + AND NOT is_system +RETURNING *; -- name: GetUserByID :one SELECT @@ -318,10 +320,11 @@ UPDATE users SET status = 'dormant'::user_status, - updated_at = @updated_at + updated_at = @updated_at WHERE last_seen_at < @last_seen_after :: timestamp AND status = 'active'::user_status + AND NOT is_system RETURNING id, email, username, last_seen_at; -- AllUserIDs returns all UserIDs regardless of user status or deletion. diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 8956fed23990e..d47de25352a77 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -497,7 +497,7 @@ func (r *remoteReporter) createSnapshot() (*Snapshot, error) { return nil }) eg.Go(func() error { - groupMembers, err := r.options.Database.GetGroupMembers(ctx) + groupMembers, err := r.options.Database.GetGroupMembers(ctx, false) if err != nil { return xerrors.Errorf("get groups: %w", err) } From 8c5158512abd31c98476f773fbf32053df76af67 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 14:46:05 +0000 Subject: [PATCH 18/25] linter --- coderd/database/dbmem/dbmem.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index f81e3508f63c7..d28e3e70c9f3e 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3414,6 +3414,7 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr return database.Group{}, sql.ErrNoRows } +//nolint:revive // It's not a control flag, its a filter func (q *FakeQuerier) GetGroupMembers(ctx context.Context, includeSystem bool) ([]database.GroupMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() From cdc5c718112d5c2cb58e3579a06086184a369fed Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 14:49:38 +0000 Subject: [PATCH 19/25] fix dbmem tests --- coderd/database/dbmem/dbmem.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index d28e3e70c9f3e..7745648f9f78f 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -6612,6 +6612,12 @@ func (q *FakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users = usersFilteredByLastSeen } + if !params.IncludeSystem { + users = slices.DeleteFunc(users, func(u database.User) bool { + return u.IsSystem + }) + } + beforePageCount := len(users) if params.OffsetOpt > 0 { @@ -6628,12 +6634,6 @@ func (q *FakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users = users[:params.LimitOpt] } - if !params.IncludeSystem { - users = slices.DeleteFunc(users, func(u database.User) bool { - return u.IsSystem - }) - } - return convertUsers(users, int64(beforePageCount)), nil } From 0d4813ababbbcffb363f76d44dee97594dac7baa Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Mar 2025 14:59:23 +0000 Subject: [PATCH 20/25] remove restriction on modifying system users for now --- .../migrations/000302_system_user.down.sql | 7 ----- .../migrations/000302_system_user.up.sql | 26 ------------------- 2 files changed, 33 deletions(-) diff --git a/coderd/database/migrations/000302_system_user.down.sql b/coderd/database/migrations/000302_system_user.down.sql index 74707f1cf92a0..100cd444d13aa 100644 --- a/coderd/database/migrations/000302_system_user.down.sql +++ b/coderd/database/migrations/000302_system_user.down.sql @@ -38,13 +38,6 @@ COMMENT ON VIEW group_members_expanded IS 'Joins group members with user informa DELETE FROM organization_members WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; --- Drop triggers first -DROP TRIGGER IF EXISTS prevent_system_user_updates ON users; -DROP TRIGGER IF EXISTS prevent_system_user_deletions ON users; - --- Drop function -DROP FUNCTION IF EXISTS prevent_system_user_changes(); - -- Delete user status changes DELETE FROM user_status_changes WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; diff --git a/coderd/database/migrations/000302_system_user.up.sql b/coderd/database/migrations/000302_system_user.up.sql index 73c04eaa69f14..6395d533bd1b7 100644 --- a/coderd/database/migrations/000302_system_user.up.sql +++ b/coderd/database/migrations/000302_system_user.up.sql @@ -9,32 +9,6 @@ INSERT INTO users (id, email, username, name, created_at, updated_at, status, rb VALUES ('c42fdf75-3097-471c-8c33-fb52454d81c0', 'prebuilds@system', 'prebuilds', 'Prebuilds Owner', now(), now(), 'active', '{}', 'none', true, 'none'::login_type); --- Create function to check system user modifications -CREATE OR REPLACE FUNCTION prevent_system_user_changes() - RETURNS TRIGGER AS -$$ -BEGIN - IF OLD.is_system = true THEN - RAISE EXCEPTION 'Cannot modify or delete system users'; - END IF; - RETURN OLD; -END; -$$ LANGUAGE plpgsql; - --- Create trigger to prevent updates to system users -CREATE TRIGGER prevent_system_user_updates - BEFORE UPDATE ON users - FOR EACH ROW - WHEN (OLD.is_system = true) -EXECUTE FUNCTION prevent_system_user_changes(); - --- Create trigger to prevent deletion of system users -CREATE TRIGGER prevent_system_user_deletions - BEFORE DELETE ON users - FOR EACH ROW - WHEN (OLD.is_system = true) -EXECUTE FUNCTION prevent_system_user_changes(); - DROP VIEW IF EXISTS group_members_expanded; CREATE VIEW group_members_expanded AS WITH all_members AS ( From 95d70a30762d7d902ee13803df39dc907c73dc5c Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 20 Mar 2025 06:39:26 +0000 Subject: [PATCH 21/25] remove system user index --- coderd/database/dbmem/dbmem.go | 1 + coderd/database/migrations/000302_system_user.down.sql | 3 --- coderd/database/migrations/000302_system_user.up.sql | 2 -- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 7745648f9f78f..46d392bdef2be 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -155,6 +155,7 @@ func New() database.Store { } q.mutex.Lock() + // We can't insert this user using the interface, because it's a system user. q.data.users = append(q.data.users, database.User{ ID: prebuilds.SystemUserID, Email: "prebuilds@coder.com", diff --git a/coderd/database/migrations/000302_system_user.down.sql b/coderd/database/migrations/000302_system_user.down.sql index 100cd444d13aa..69903b13d3cc5 100644 --- a/coderd/database/migrations/000302_system_user.down.sql +++ b/coderd/database/migrations/000302_system_user.down.sql @@ -46,8 +46,5 @@ WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; DELETE FROM users WHERE id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'; --- Drop index -DROP INDEX IF EXISTS user_is_system_idx; - -- Drop column ALTER TABLE users DROP COLUMN IF EXISTS is_system; diff --git a/coderd/database/migrations/000302_system_user.up.sql b/coderd/database/migrations/000302_system_user.up.sql index 6395d533bd1b7..c024a9587f774 100644 --- a/coderd/database/migrations/000302_system_user.up.sql +++ b/coderd/database/migrations/000302_system_user.up.sql @@ -1,8 +1,6 @@ ALTER TABLE users ADD COLUMN is_system bool DEFAULT false NOT NULL; -CREATE INDEX user_is_system_idx ON users USING btree (is_system); - COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions'; INSERT INTO users (id, email, username, name, created_at, updated_at, status, rbac_roles, hashed_password, is_system, login_type) From 7e009e5fd847d2b495c067bc02443ef9a32b4fed Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 24 Mar 2025 10:54:58 +0000 Subject: [PATCH 22/25] invert tests that check for system user update protection --- coderd/database/querier_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 4fd85de94679d..18b06bb3d3850 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1426,9 +1426,10 @@ func TestGetUsers_IncludeSystem(t *testing.T) { func TestUpdateSystemUser(t *testing.T) { t.Parallel() - if !dbtestutil.WillUsePostgres() { - t.Skip("test only supports postgres") - } + // TODO (sasswart): We've disabled the protection that prevents updates to system users + // while we reassess the mechanism to do so. Rather than skip the test, we've just inverted + // the assertions to ensure that the behaviour is as desired. + // Once we've re-enabeld the system user protection, we'll revert the assertions. ctx := testutil.Context(t, testutil.WaitLong) @@ -1457,12 +1458,14 @@ func TestUpdateSystemUser(t *testing.T) { Name: "not prebuilds", }) // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") + // require.ErrorContains(t, err, "Cannot modify or delete system users") + require.NoError(t, err) // When: attempting to delete a system user. err = db.UpdateUserDeletedByID(ctx, systemUser.ID) // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") + // require.ErrorContains(t, err, "Cannot modify or delete system users") + require.NoError(t, err) // When: attempting to update a user's roles. _, err = db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ @@ -1470,7 +1473,8 @@ func TestUpdateSystemUser(t *testing.T) { GrantedRoles: []string{rbac.RoleAuditor().String()}, }) // Then: the attempt is rejected by a postgres trigger. - require.ErrorContains(t, err, "Cannot modify or delete system users") + // require.ErrorContains(t, err, "Cannot modify or delete system users") + require.NoError(t, err) } func TestUserChangeLoginType(t *testing.T) { From addd7c66dd71cf62a18c6c3a9411f3ab207fb928 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 24 Mar 2025 11:06:21 +0000 Subject: [PATCH 23/25] lint --- coderd/database/querier_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 18b06bb3d3850..aeba385c692e5 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1428,7 +1428,7 @@ func TestUpdateSystemUser(t *testing.T) { // TODO (sasswart): We've disabled the protection that prevents updates to system users // while we reassess the mechanism to do so. Rather than skip the test, we've just inverted - // the assertions to ensure that the behaviour is as desired. + // the assertions to ensure that the behavior is as desired. // Once we've re-enabeld the system user protection, we'll revert the assertions. ctx := testutil.Context(t, testutil.WaitLong) From 7a4ef24f62270d39c97e816e60577baed40272c8 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 24 Mar 2025 11:09:23 +0000 Subject: [PATCH 24/25] Allow TestUpdateSystemUser to run against dbmem --- coderd/database/querier_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index aeba385c692e5..a2d22f9144fb6 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1433,13 +1433,8 @@ func TestUpdateSystemUser(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) - sqlDB := testSQLDB(t) // Given: a system user introduced by migration coderd/database/migrations/00030*_system_user.up.sql - err := migrations.Up(sqlDB) - require.NoError(t, err, "migrations") - - db := database.New(sqlDB) - // Given a system user. + db, _ := dbtestutil.NewDB(t) users, err := db.GetUsers(ctx, database.GetUsersParams{ IncludeSystem: true, }) From 5f0ae5e03e3ce69df39b0d2b0eec3af6def33c0c Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 25 Mar 2025 11:56:50 +0000 Subject: [PATCH 25/25] Renumber migrations --- .../{000306_system_user.down.sql => 000308_system_user.down.sql} | 0 .../{000306_system_user.up.sql => 000308_system_user.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000306_system_user.down.sql => 000308_system_user.down.sql} (100%) rename coderd/database/migrations/{000306_system_user.up.sql => 000308_system_user.up.sql} (100%) diff --git a/coderd/database/migrations/000306_system_user.down.sql b/coderd/database/migrations/000308_system_user.down.sql similarity index 100% rename from coderd/database/migrations/000306_system_user.down.sql rename to coderd/database/migrations/000308_system_user.down.sql diff --git a/coderd/database/migrations/000306_system_user.up.sql b/coderd/database/migrations/000308_system_user.up.sql similarity index 100% rename from coderd/database/migrations/000306_system_user.up.sql rename to coderd/database/migrations/000308_system_user.up.sql