WIP

coder · Emyrk · Jun 11, 2024 · Jun 7, 2024 · Jun 7, 2024 · Jun 7, 2024
commit fc8d4142034cc8a878c217d8f9bd92398147e800
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -582,8 +582,26 @@ func (q *querier) authorizeUpdateFileTemplate(ctx context.Context, file database
 	}
 }
 
+// uniqueOrganizationRoles converts a set of scoped role names to their unique
+// scoped names.
+func (q *querier) uniqueOrganizationRoles(organizationID uuid.UUID, names []string) ([]rbac.UniqueRoleName, error) {
+	uniques := make([]rbac.UniqueRoleName, 0, len(names))
+	for _, name := range names {
+		// This check is a developer safety check. Old code might try to invoke this code path with
+		// organization id suffixes. Catch this and return a nice error so it can be fixed.
+		_, foundOrg, _ := rbac.RoleSplit(rbac.UniqueRoleName(name))
+		if foundOrg != "" {
+			return nil, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", name)
+		}
+
+		uniques = append(uniques, rbac.RoleName(name, organizationID.String()))
+	}
+
+	return uniques, nil
+}
+
 // canAssignRoles handles assigning built in and custom roles.
-func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, removed []string) error {
+func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, removed []rbac.UniqueRoleName) error {
 	actor, ok := ActorFromContext(ctx)
 	if !ok {
 		return NoActorError
@@ -597,7 +615,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
 	}
 
 	grantedRoles := append(added, removed...)
-	customRoles := make([]string, 0)
+	customRoles := make([]rbac.UniqueRoleName, 0)
 	// Validate that the roles being assigned are valid.
 	for _, r := range grantedRoles {
 		roleOrgIDStr, isOrgRole := rbac.IsOrgRole(r)
@@ -629,7 +647,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
 		}
 	}
 
-	customRolesMap := make(map[string]struct{}, len(customRoles))
+	customRolesMap := make(map[rbac.UniqueRoleName]struct{}, len(customRoles))
 	for _, r := range customRoles {
 		customRolesMap[r] = struct{}{}
 	}
@@ -2849,7 +2867,7 @@ func (q *querier) UpdateMemberRoles(ctx context.Context, arg database.UpdateMemb
 
 	// The 'rbac' package expects role names to be scoped.
 	// Convert the argument roles for validation.
-	scopedGranted := make([]string, 0, len(arg.GrantedRoles))
+	scopedGranted := make([]rbac.UniqueRoleName, 0, len(arg.GrantedRoles))
 	for _, grantedRole := range arg.GrantedRoles {
 		// This check is a developer safety check. Old code might try to invoke this code path with
 		// organization id suffixes. Catch this and return a nice error so it can be fixed.

diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go
@@ -5,6 +5,7 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/google/uuid"
 	"golang.org/x/exp/maps"
 	"golang.org/x/oauth2"
 
@@ -373,3 +374,10 @@ func (p ProvisionerJob) FinishedAt() time.Time {
 
 	return time.Time{}
 }
+
+func (r CustomRole) UniqueName() rbac.UniqueRoleName {
+	if r.OrganizationID.UUID == uuid.Nil {
+		return rbac.RoleName(r.Name, "")
+	}
+	return rbac.RoleName(r.Name, r.OrganizationID.UUID.String())
+}
diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -114,9 +114,9 @@ func (s Subject) SafeScopeName() string {
 }
 
 // SafeRoleNames prevent nil pointer dereference.
-func (s Subject) SafeRoleNames() []string {
+func (s Subject) SafeRoleNames() []UniqueRoleName {
 	if s.Roles == nil {
-		return []string{}
+		return []UniqueRoleName{}
 	}
 	return s.Roles.Names()
 }
@@ -707,9 +707,15 @@ func (c *authCache) Prepare(ctx context.Context, subject Subject, action policy.
 // rbacTraceAttributes are the attributes that are added to all spans created by
 // the rbac package. These attributes should help to debug slow spans.
 func rbacTraceAttributes(actor Subject, action policy.Action, objectType string, extra ...attribute.KeyValue) trace.SpanStartOption {
+	uniqueRoleNames := actor.SafeRoleNames()
+	roleStrings := make([]string, 0, len(uniqueRoleNames))
+	for _, roleName := range uniqueRoleNames {
+		roleName := roleName
+		roleStrings = append(roleStrings, string(roleName))
+	}
 	return trace.WithAttributes(
 		append(extra,
-			attribute.StringSlice("subject_roles", actor.SafeRoleNames()),
+			attribute.StringSlice("subject_roles", roleStrings),
 			attribute.Int("num_subject_roles", len(actor.SafeRoleNames())),
 			attribute.Int("num_groups", len(actor.Groups)),
 			attribute.String("scope", actor.SafeScopeName()),

diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go
@@ -37,13 +37,13 @@ func init() {
 // RoleNames is a list of user assignable role names. The role names must be
 // in the builtInRoles map. Any non-user assignable roles will generate an
 // error on Expand.
-type RoleNames []string
+type RoleNames []UniqueRoleName
 
 func (names RoleNames) Expand() ([]Role, error) {
 	return rolesByNames(names)
 }
 
-func (names RoleNames) Names() []string {
+func (names RoleNames) Names() []UniqueRoleName {
 	return names
 }
 

diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go
@@ -27,26 +27,26 @@ func CustomRoleMW(next http.Handler) http.Handler {
 // same request lifecycle. Optimizing this to span requests should be done
 // in the future.
 func CustomRoleCacheContext(ctx context.Context) context.Context {
-	return context.WithValue(ctx, customRoleCtxKey{}, syncmap.New[string, rbac.Role]())
+	return context.WithValue(ctx, customRoleCtxKey{}, syncmap.New[rbac.UniqueRoleName, rbac.Role]())
 }
 
-func roleCache(ctx context.Context) *syncmap.Map[string, rbac.Role] {
-	c, ok := ctx.Value(customRoleCtxKey{}).(*syncmap.Map[string, rbac.Role])
+func roleCache(ctx context.Context) *syncmap.Map[rbac.UniqueRoleName, rbac.Role] {
+	c, ok := ctx.Value(customRoleCtxKey{}).(*syncmap.Map[rbac.UniqueRoleName, rbac.Role])
 	if !ok {
-		return syncmap.New[string, rbac.Role]()
+		return syncmap.New[rbac.UniqueRoleName, rbac.Role]()
 	}
 	return c
 }
 
 // Expand will expand built in roles, and fetch custom roles from the database.
-func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles, error) {
+func Expand(ctx context.Context, db database.Store, names []rbac.UniqueRoleName) (rbac.Roles, error) {
 	if len(names) == 0 {
 		// That was easy
 		return []rbac.Role{}, nil
 	}
 
 	cache := roleCache(ctx)
-	lookup := make([]string, 0)
+	lookup := make([]rbac.UniqueRoleName, 0)
 	roles := make([]rbac.Role, 0, len(names))
 
 	for _, name := range names {
@@ -73,7 +73,7 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles,
 		// In the database, org roles are scoped with an organization column.
 		lookupArgs := make([]database.NameOrganizationPair, 0, len(lookup))
 		for _, name := range lookup {
-			roleName, orgID, err := rbac.RoleSplit(name)
+			roleName, orgID, err := name.Split()
 			if err != nil {
 				continue
 			}
@@ -111,7 +111,7 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles,
 				return nil, xerrors.Errorf("convert db role %q: %w", dbrole.Name, err)
 			}
 			roles = append(roles, converted)
-			cache.Store(dbrole.Name, converted)
+			cache.Store(dbrole.UniqueName(), converted)
 		}
 	}
 
@@ -133,12 +133,8 @@ func convertPermissions(dbPerms []database.CustomRolePermission) []rbac.Permissi
 // ConvertDBRole should not be used by any human facing apis. It is used
 // for authz purposes.
 func ConvertDBRole(dbRole database.CustomRole) (rbac.Role, error) {
-	name := dbRole.Name
-	if dbRole.OrganizationID.Valid {
-		name = rbac.RoleName(dbRole.Name, dbRole.OrganizationID.UUID.String())
-	}
 	role := rbac.Role{
-		Name:        name,
+		Name:        dbRole.UniqueName(),
 		DisplayName: dbRole.DisplayName,
 		Site:        convertPermissions(dbRole.SitePermissions),
 		Org:         nil,

diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go
@@ -58,7 +58,7 @@ var builtinScopes = map[ScopeName]Scope{
 	// authorize checks it is usually not used directly and skips scope checks.
 	ScopeAll: {
 		Role: Role{
-			Name:        fmt.Sprintf("Scope_%s", ScopeAll),
+			Name:        RoleName(fmt.Sprintf("Scope_%s", ScopeAll), ""),
 			DisplayName: "All operations",
 			Site: Permissions(map[string][]policy.Action{
 				ResourceWildcard.Type: {policy.WildcardSymbol},
@@ -71,7 +71,7 @@ var builtinScopes = map[ScopeName]Scope{
 
 	ScopeApplicationConnect: {
 		Role: Role{
-			Name:        fmt.Sprintf("Scope_%s", ScopeApplicationConnect),
+			Name:        RoleName(fmt.Sprintf("Scope_%s", ScopeApplicationConnect), ""),
 			DisplayName: "Ability to connect to applications",
 			Site: Permissions(map[string][]policy.Action{
 				ResourceWorkspace.Type: {policy.ActionApplicationConnect},
@@ -114,7 +114,7 @@ func (s Scope) Expand() (Scope, error) {
 	return s, nil
 }
 
-func (s Scope) Name() string {
+func (s Scope) Name() UniqueRoleName {
 	return s.Role.Name
 }