Skip to content

Commit 195c545

Browse files
EmyrkEmyrk
authored
fix(coderd/rbac): guard builtInRoles with atomic.Pointer (#26384)
<sub>Coder Agents on behalf of @Emyrk.</sub>
1 parent 450ddff commit 195c545

2 files changed

Lines changed: 41 additions & 19 deletions

File tree

‎coderd/rbac/roles.go‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"sort"
88
"strconv"
99
"strings"
10+
"sync/atomic"
1011

1112
"github.com/google/uuid"
1213
"github.com/open-policy-agent/opa/ast"
@@ -314,7 +315,26 @@ func allPermsExcept(excepts ...Objecter) []Permission {
314315
//
315316
// This map will be replaced by database storage defined by this ticket.
316317
// https://github.com/coder/coder/issues/1194
317-
var builtInRoles map[string]func(orgID uuid.UUID) Role
318+
//
319+
// Stored behind an atomic.Pointer so test setups that call
320+
// ReloadBuiltinRoles do not race with handlers that look up roles via
321+
// RoleByName, ReservedRoleName, OrganizationRoles, or SiteBuiltInRoles.
322+
// Production callers reload once at startup; tests reload per coderd.
323+
type builtInRoleMap = map[string]func(orgID uuid.UUID) Role
324+
325+
var builtInRoles atomic.Pointer[builtInRoleMap]
326+
327+
// loadBuiltinRoles returns the current built-in roles snapshot. The
328+
// returned map is safe to read concurrently because ReloadBuiltinRoles
329+
// publishes a fresh map via atomic.Pointer.Store instead of mutating in
330+
// place.
331+
func loadBuiltinRoles() builtInRoleMap {
332+
if m := builtInRoles.Load(); m != nil {
333+
return *m
334+
}
335+
// Return an empty map to prevent nil pointer dereference
336+
return map[string]func(orgID uuid.UUID) Role{}
337+
}
318338

319339
type RoleOptions struct {
320340
NoOwnerWorkspaceExec bool
@@ -333,7 +353,7 @@ type RoleOptions struct {
333353
// ReservedRoleName exists because the database should only allow unique role
334354
// names, but some roles are built in. So these names are reserved
335355
func ReservedRoleName(name string) bool {
336-
_, ok := builtInRoles[name]
356+
_, ok := loadBuiltinRoles()[name]
337357
return ok
338358
}
339359

@@ -515,7 +535,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
515535
ByOrgID: map[string]OrgPermissions{},
516536
}.withCachedRegoValue()
517537

518-
builtInRoles = map[string]func(orgID uuid.UUID) Role{
538+
roles := builtInRoleMap{
519539
// admin grants all actions to all resources.
520540
owner: func(_ uuid.UUID) Role {
521541
return ownerRole
@@ -733,6 +753,8 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
733753
}
734754
},
735755
}
756+
757+
builtInRoles.Store(&roles)
736758
}
737759

738760
// assignRoles is a map of roles that can be assigned if a user has a given
@@ -954,7 +976,7 @@ func CanAssignRole(subjectHasRoles ExpandableRoles, assignedRole RoleIdentifier)
954976
// api. We should maybe make an exported function that returns just the
955977
// human-readable content of the Role struct (name + display name).
956978
func RoleByName(name RoleIdentifier) (Role, error) {
957-
roleFunc, ok := builtInRoles[name.Name]
979+
roleFunc, ok := loadBuiltinRoles()[name.Name]
958980
if !ok {
959981
// No role found
960982
return Role{}, xerrors.Errorf("role %q not found", name.String())
@@ -997,7 +1019,7 @@ func rolesByNames(roleNames []RoleIdentifier) ([]Role, error) {
9971019
// the list from the builtins.
9981020
func OrganizationRoles(organizationID uuid.UUID) []Role {
9991021
var roles []Role
1000-
for _, roleF := range builtInRoles {
1022+
for _, roleF := range loadBuiltinRoles() {
10011023
role := roleF(organizationID)
10021024
if role.Identifier.OrganizationID == organizationID {
10031025
roles = append(roles, role)
@@ -1013,7 +1035,7 @@ func OrganizationRoles(organizationID uuid.UUID) []Role {
10131035
// the list from the builtins.
10141036
func SiteBuiltInRoles() []Role {
10151037
var roles []Role
1016-
for _, roleF := range builtInRoles {
1038+
for _, roleF := range loadBuiltinRoles() {
10171039
// Must provide some non-nil uuid to filter out org roles.
10181040
role := roleF(uuid.New())
10191041
if !role.Identifier.IsOrgRole() {

‎coderd/rbac/roles_internal_test.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -215,19 +215,19 @@ func TestRoleByName(t *testing.T) {
215215
testCases := []struct {
216216
Role Role
217217
}{
218-
{Role: builtInRoles[owner](uuid.Nil)},
219-
{Role: builtInRoles[member](uuid.Nil)},
220-
{Role: builtInRoles[templateAdmin](uuid.Nil)},
221-
{Role: builtInRoles[userAdmin](uuid.Nil)},
222-
{Role: builtInRoles[auditor](uuid.Nil)},
223-
224-
{Role: builtInRoles[orgAdmin](uuid.New())},
225-
{Role: builtInRoles[orgAdmin](uuid.New())},
226-
{Role: builtInRoles[orgAdmin](uuid.New())},
227-
228-
{Role: builtInRoles[orgAuditor](uuid.New())},
229-
{Role: builtInRoles[orgAuditor](uuid.New())},
230-
{Role: builtInRoles[orgAuditor](uuid.New())},
218+
{Role: loadBuiltinRoles()[owner](uuid.Nil)},
219+
{Role: loadBuiltinRoles()[member](uuid.Nil)},
220+
{Role: loadBuiltinRoles()[templateAdmin](uuid.Nil)},
221+
{Role: loadBuiltinRoles()[userAdmin](uuid.Nil)},
222+
{Role: loadBuiltinRoles()[auditor](uuid.Nil)},
223+
224+
{Role: loadBuiltinRoles()[orgAdmin](uuid.New())},
225+
{Role: loadBuiltinRoles()[orgAdmin](uuid.New())},
226+
{Role: loadBuiltinRoles()[orgAdmin](uuid.New())},
227+
228+
{Role: loadBuiltinRoles()[orgAuditor](uuid.New())},
229+
{Role: loadBuiltinRoles()[orgAuditor](uuid.New())},
230+
{Role: loadBuiltinRoles()[orgAuditor](uuid.New())},
231231
}
232232

233233
for _, c := range testCases {

0 commit comments

Comments
 (0)