coder · Emyrk · Jun 5, 2024 · Jun 4, 2024 · Jun 4, 2024 · Jun 4, 2024
diff --git a/cli/server_createadminuser.go b/cli/server_createadminuser.go
@@ -222,7 +222,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command {
 						UserID:         newUser.ID,
 						CreatedAt:      dbtime.Now(),
 						UpdatedAt:      dbtime.Now(),
-						Roles:          []string{rbac.RoleOrgAdmin(org.ID)},
+						Roles:          []string{rbac.ScopedRoleOrgAdmin(org.ID)},
 					})
 					if err != nil {
 						return xerrors.Errorf("insert organization member: %w", err)

diff --git a/cli/server_createadminuser_test.go b/cli/server_createadminuser_test.go
@@ -71,7 +71,7 @@ func TestServerCreateAdminUser(t *testing.T) {
 		orgIDs2 := make(map[uuid.UUID]struct{}, len(orgMemberships))
 		for _, membership := range orgMemberships {
 			orgIDs2[membership.OrganizationID] = struct{}{}
-			assert.Equal(t, []string{rbac.RoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin")
+			assert.Equal(t, []string{rbac.ScopedRoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin")
 		}
 
 		require.Equal(t, orgIDs, orgIDs2, "user is not in all orgs")

diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/authorize_test.go b/coderd/authorize_test.go
@@ -27,7 +27,7 @@ func TestCheckPermissions(t *testing.T) {
 	memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID)
 	memberUser, err := memberClient.User(ctx, codersdk.Me)
 	require.NoError(t, err)
-	orgAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID, rbac.RoleOrgAdmin(adminUser.OrganizationID))
+	orgAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID, rbac.ScopedRoleOrgAdmin(adminUser.OrganizationID))
 	orgAdminUser, err := orgAdminClient.User(ctx, codersdk.Me)
 	require.NoError(t, err)
 

diff --git a/coderd/batchstats/batcher_internal_test.go b/coderd/batchstats/batcher_internal_test.go
@@ -177,7 +177,7 @@ func setupDeps(t *testing.T, store database.Store, ps pubsub.Pubsub) deps {
 	_, err := store.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
 		OrganizationID: org.ID,
 		UserID:         user.ID,
-		Roles:          []string{rbac.RoleOrgMember(org.ID)},
+		Roles:          []string{rbac.ScopedRoleOrgMember(org.ID)},
 	})
 	require.NoError(t, err)
 	tv := dbgen.TemplateVersion(t, store, database.TemplateVersion{

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -663,6 +663,7 @@ func CreateFirstUser(t testing.TB, client *codersdk.Client) codersdk.CreateFirst
 }
 
 // CreateAnotherUser creates and authenticates a new user.
+// Roles can include org scoped roles with 'roleName:<organization_id>'
 func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) {
 	return createAnotherUserRetry(t, client, organizationID, 5, roles)
 }
@@ -680,7 +681,7 @@ func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject {
 		roles = append(roles, r.Name)
 	}
 	// We assume only 1 org exists
-	roles = append(roles, rbac.RoleOrgMember(orgID))
+	roles = append(roles, rbac.ScopedRoleOrgMember(orgID))
 
 	return rbac.Subject{
 		ID:     user.ID.String(),
@@ -754,6 +755,8 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI
 		for _, roleName := range roles {
 			roleName := roleName
 			orgID, ok := rbac.IsOrgRole(roleName)
+			roleName, _, err = rbac.RoleSplit(roleName)
+			require.NoError(t, err, "split org role name")
 			if ok {
 				orgRoles[orgID] = append(orgRoles[orgID], roleName)
 			} else {

diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go
@@ -204,13 +204,6 @@ func Group(group database.Group, members []database.User) codersdk.Group {
 	}
 }
 
-func SlimRole(role rbac.Role) codersdk.SlimRole {
-	return codersdk.SlimRole{
-		DisplayName: role.DisplayName,
-		Name:        role.Name,
-	}
-}
-
 func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) {
 	// Use a stable sort, similarly to how we would sort in the query, note that
 	// we don't sort in the query because order varies depending on the table
@@ -525,6 +518,19 @@ func ProvisionerDaemon(dbDaemon database.ProvisionerDaemon) codersdk.Provisioner
 	return result
 }
 
+func SlimRole(role rbac.Role) codersdk.SlimRole {
+	roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
+	if err != nil {
+		roleName = role.Name
+	}
+
+	return codersdk.SlimRole{
+		DisplayName:    role.DisplayName,
+		Name:           roleName,
+		OrganizationID: orgIDStr,
+	}
+}
+
 func RBACRole(role rbac.Role) codersdk.Role {
 	roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
 	if err != nil {

diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go
@@ -153,7 +153,7 @@ func TestUpsertCustomRoles(t *testing.T) {
 				UUID:  uuid.New(),
 				Valid: true,
 			},
-			subject: merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
+			subject: merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
 			org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
 				codersdk.ResourceWorkspace: {codersdk.ActionRead},
 			}),
@@ -162,7 +162,7 @@ func TestUpsertCustomRoles(t *testing.T) {
 		{
 			name: "user-escalation",
 			// These roles do not grant user perms
-			subject: merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
+			subject: merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
 			user: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
 				codersdk.ResourceWorkspace: {codersdk.ActionRead},
 			}),
@@ -190,7 +190,7 @@ func TestUpsertCustomRoles(t *testing.T) {
 		},
 		{
 			name:           "read-workspace-in-org",
-			subject:        merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
+			subject:        merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
 			organizationID: orgID,
 			org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
 				codersdk.ResourceWorkspace: {codersdk.ActionRead},

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -2472,7 +2472,7 @@ func (q *querier) InsertOrganization(ctx context.Context, arg database.InsertOrg
 
 func (q *querier) InsertOrganizationMember(ctx context.Context, arg database.InsertOrganizationMemberParams) (database.OrganizationMember, error) {
 	// All roles are added roles. Org member is always implied.
-	addedRoles := append(arg.Roles, rbac.RoleOrgMember(arg.OrganizationID))
+	addedRoles := append(arg.Roles, rbac.ScopedRoleOrgMember(arg.OrganizationID))
 	err := q.canAssignRoles(ctx, &arg.OrganizationID, addedRoles, []string{})
 	if err != nil {
 		return database.OrganizationMember{}, err
@@ -2847,8 +2847,22 @@ func (q *querier) UpdateMemberRoles(ctx context.Context, arg database.UpdateMemb
 		return database.OrganizationMember{}, err
 	}
 
+	// The 'rbac' package expects role names to be scoped.
+	// Convert the argument roles for validation.
+	scopedGranted := make([]string, 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.
+		_, foundOrg, _ := rbac.RoleSplit(grantedRole)
+		if foundOrg != "" {
+			return database.OrganizationMember{}, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", grantedRole)
+		}
+
+		scopedGranted = append(scopedGranted, rbac.RoleName(grantedRole, arg.OrgID.String()))
+	}
+
 	// The org member role is always implied.
-	impliedTypes := append(arg.GrantedRoles, rbac.RoleOrgMember(arg.OrgID))
+	impliedTypes := append(scopedGranted, rbac.ScopedRoleOrgMember(arg.OrgID))
 	added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
 	err = q.canAssignRoles(ctx, &arg.OrgID, added, removed)
 	if err != nil {

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -636,7 +636,7 @@ func (s *MethodTestSuite) TestOrganization() {
 		check.Args(database.InsertOrganizationMemberParams{
 			OrganizationID: o.ID,
 			UserID:         u.ID,
-			Roles:          []string{rbac.RoleOrgAdmin(o.ID)},
+			Roles:          []string{rbac.ScopedRoleOrgAdmin(o.ID)},
 		}).Asserts(
 			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign,
 			rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate)
@@ -664,7 +664,7 @@ func (s *MethodTestSuite) TestOrganization() {
 		mem := dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{
 			OrganizationID: o.ID,
 			UserID:         u.ID,
-			Roles:          []string{rbac.RoleOrgAdmin(o.ID)},
+			Roles:          []string{rbac.ScopedRoleOrgAdmin(o.ID)},
 		})
 		out := mem
 		out.Roles = []string{}

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1997,7 +1997,9 @@ func (q *FakeQuerier) GetAuthorizationUserRoles(_ context.Context, userID uuid.U
 
 	for _, mem := range q.organizationMembers {
 		if mem.UserID == userID {
-			roles = append(roles, mem.Roles...)
+			for _, orgRole := range mem.Roles {
+				roles = append(roles, orgRole+":"+mem.OrganizationID.String())
+			}
 			roles = append(roles, "organization-member:"+mem.OrganizationID.String())
 		}
 	}

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000215_scoped_org_db_roles.down.sql b/coderd/database/migrations/000215_scoped_org_db_roles.down.sql
@@ -0,0 +1 @@
+ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{organization-member}';
diff --git a/coderd/database/migrations/000215_scoped_org_db_roles.up.sql b/coderd/database/migrations/000215_scoped_org_db_roles.up.sql
@@ -0,0 +1,7 @@
+-- The default was 'organization-member', but we imply that in the
+-- 'GetAuthorizationUserRoles' query.
+ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{}';
+
+-- No one should be using organization roles yet. If they are, the names in the
+-- database are now incorrect. Just remove them all.
+UPDATE organization_members SET roles = '{}';
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -227,12 +227,14 @@ SELECT
 		array_append(users.rbac_roles, 'member'),
 		(
 			SELECT
-				array_agg(org_roles)
+				-- The roles are returned as a flat array, org scoped and site side.
+				-- Concatenating the organization id scopes the organization roles.
+				array_agg(org_roles || ':' || organization_members.organization_id::text)
 			FROM
 				organization_members,
-				-- All org_members get the org-member role for their orgs
+				-- All org_members get the organization-member role for their orgs
 				unnest(
-					array_append(roles, 'organization-member:' || organization_members.organization_id::text)
+					array_append(roles, 'organization-member')
 				) AS org_roles
 			WHERE
 				user_id = users.id

diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go
@@ -68,7 +68,7 @@ func TestExtractUserRoles(t *testing.T) {
 					Roles:          orgRoles,
 				})
 				require.NoError(t, err)
-				return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.RoleOrgMember(org.ID))...), token
+				return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID))...), token
 			},
 		},
 		{
@@ -89,7 +89,8 @@ func TestExtractUserRoles(t *testing.T) {
 
 					orgRoles := []string{}
 					if i%2 == 0 {
-						orgRoles = append(orgRoles, rbac.RoleOrgAdmin(organization.ID))
+						orgRoles = append(orgRoles, rbac.RoleOrgAdmin())
+						roles = append(roles, rbac.ScopedRoleOrgAdmin(organization.ID))
 					}
 					_, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
 						OrganizationID: organization.ID,
@@ -99,8 +100,7 @@ func TestExtractUserRoles(t *testing.T) {
 						Roles:          orgRoles,
 					})
 					require.NoError(t, err)
-					roles = append(roles, orgRoles...)
-					roles = append(roles, rbac.RoleOrgMember(organization.ID))
+					roles = append(roles, rbac.ScopedRoleOrgMember(organization.ID))
 				}
 				return user, roles, token
 			},

diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go
@@ -152,7 +152,7 @@ func TestOrganizationParam(t *testing.T) {
 		_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
 			OrganizationID: organization.ID,
 			UserID:         user.ID,
-			Roles:          []string{rbac.RoleOrgMember(organization.ID)},
+			Roles:          []string{rbac.ScopedRoleOrgMember(organization.ID)},
 		})
 		_, err := db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{
 			ID:           user.ID,

diff --git a/coderd/members.go b/coderd/members.go
@@ -68,7 +68,7 @@ func convertOrganizationMember(mem database.OrganizationMember) codersdk.Organiz
 	}
 
 	for _, roleName := range mem.Roles {
-		rbacRole, _ := rbac.RoleByName(roleName)
+		rbacRole, _ := rbac.RoleByName(rbac.RoleName(roleName, mem.OrganizationID.String()))
 		convertedMember.Roles = append(convertedMember.Roles, db2sdk.SlimRole(rbacRole))
 	}
 	return convertedMember

diff --git a/coderd/organizations.go b/coderd/organizations.go
@@ -94,7 +94,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
 				// come back to determining the default role of the person who
 				// creates the org. Until that happens, all users in an organization
 				// should be just regular members.
-				rbac.RoleOrgMember(organization.ID),
+				rbac.ScopedRoleOrgMember(organization.ID),
 			},
 		})
 		if err != nil {
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{organization-member}';