diff --git a/cli/server_createadminuser.go b/cli/server_createadminuser.go index 9f8d5ffe3ccf9..e43a9c401b8a0 100644 --- a/cli/server_createadminuser.go +++ b/cli/server_createadminuser.go @@ -192,7 +192,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command { HashedPassword: []byte(hashedPassword), CreatedAt: dbtime.Now(), UpdatedAt: dbtime.Now(), - RBACRoles: []string{rbac.RoleOwner()}, + RBACRoles: []string{rbac.RoleOwner().String()}, LoginType: database.LoginTypePassword, }) if err != nil { @@ -222,7 +222,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command { UserID: newUser.ID, CreatedAt: dbtime.Now(), UpdatedAt: dbtime.Now(), - Roles: []string{rbac.ScopedRoleOrgAdmin(org.ID)}, + Roles: []string{rbac.RoleOrgAdmin()}, }) 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 index 6510e1332f120..9bc6add2ecbd2 100644 --- a/cli/server_createadminuser_test.go +++ b/cli/server_createadminuser_test.go @@ -17,6 +17,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/userpassword" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/pty/ptytest" "github.com/coder/coder/v2/testutil" ) @@ -56,7 +57,7 @@ func TestServerCreateAdminUser(t *testing.T) { require.NoError(t, err) require.True(t, ok, "password does not match") - require.EqualValues(t, []string{rbac.RoleOwner()}, user.RBACRoles, "user does not have owner role") + require.EqualValues(t, []string{codersdk.RoleOwner}, user.RBACRoles, "user does not have owner role") // Check that user is admin in every org. orgs, err := db.GetOrganizations(ctx) @@ -71,7 +72,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.ScopedRoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin") + assert.Equal(t, []string{rbac.RoleOrgAdmin()}, membership.Roles, "user is not org admin") } require.Equal(t, orgIDs, orgIDs2, "user is not in all orgs") diff --git a/coderd/audit.go b/coderd/audit.go index 315913dff49c2..ab82e91698d8c 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -199,7 +199,8 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs Roles: []codersdk.SlimRole{}, } - for _, roleName := range dblog.UserRoles { + for _, input := range dblog.UserRoles { + roleName, _ := rbac.RoleNameFromString(input) rbacRole, _ := rbac.RoleByName(roleName) user.Roles = append(user.Roles, db2sdk.SlimRole(rbacRole)) } diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index e753e66f2d2f6..9586289d60025 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -60,10 +60,13 @@ func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsse roles, err := api.Database.GetAuthorizationUserRoles(ctx, key.UserID) require.NoError(t, err, "fetch user roles") + roleNames, err := roles.RoleNames() + require.NoError(t, err) + return RBACAsserter{ Subject: rbac.Subject{ ID: key.UserID.String(), - Roles: rbac.RoleNames(roles.Roles), + Roles: rbac.RoleIdentifiers(roleNames), Groups: roles.Groups, Scope: rbac.ScopeName(key.Scope), }, @@ -435,7 +438,7 @@ func randomRBACType() string { func RandomRBACSubject() rbac.Subject { return rbac.Subject{ ID: uuid.NewString(), - Roles: rbac.RoleNames{rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, Groups: []string{namesgenerator.GetRandomName(1)}, Scope: rbac.ScopeAll, } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 9ca2f551978f1..49388aa3537a5 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -55,6 +55,7 @@ import ( "github.com/coder/coder/v2/coderd/autobuild" "github.com/coder/coder/v2/coderd/awsidentity" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbrollup" "github.com/coder/coder/v2/coderd/database/dbtestutil" @@ -663,21 +664,25 @@ 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:' -func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) { +func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...rbac.RoleIdentifier) (*codersdk.Client, codersdk.User) { return createAnotherUserRetry(t, client, organizationID, 5, roles) } -func CreateAnotherUserMutators(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles []string, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) { +func CreateAnotherUserMutators(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles []rbac.RoleIdentifier, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) { return createAnotherUserRetry(t, client, organizationID, 5, roles, mutators...) } // AuthzUserSubject does not include the user's groups. func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject { - roles := make(rbac.RoleNames, 0, len(user.Roles)) + roles := make(rbac.RoleIdentifiers, 0, len(user.Roles)) // Member role is always implied roles = append(roles, rbac.RoleMember()) for _, r := range user.Roles { - roles = append(roles, r.Name) + orgID, _ := uuid.Parse(r.OrganizationID) // defaults to nil + roles = append(roles, rbac.RoleIdentifier{ + Name: r.Name, + OrganizationID: orgID, + }) } // We assume only 1 org exists roles = append(roles, rbac.ScopedRoleOrgMember(orgID)) @@ -690,7 +695,7 @@ func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject { } } -func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, retries int, roles []string, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) { +func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, retries int, roles []rbac.RoleIdentifier, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) { req := codersdk.CreateUserRequest{ Email: namesgenerator.GetRandomName(10) + "@coder.com", Username: RandomUsername(t), @@ -748,36 +753,37 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI if len(roles) > 0 { // Find the roles for the org vs the site wide roles - orgRoles := make(map[string][]string) - var siteRoles []string + orgRoles := make(map[uuid.UUID][]rbac.RoleIdentifier) + var siteRoles []rbac.RoleIdentifier for _, roleName := range roles { - roleName := roleName - orgID, ok := rbac.IsOrgRole(roleName) - roleName, _, err = rbac.RoleSplit(roleName) - require.NoError(t, err, "split org role name") + ok := roleName.IsOrgRole() if ok { - roleName, _, err = rbac.RoleSplit(roleName) - require.NoError(t, err, "split rolename") - orgRoles[orgID] = append(orgRoles[orgID], roleName) + orgRoles[roleName.OrganizationID] = append(orgRoles[roleName.OrganizationID], roleName) } else { siteRoles = append(siteRoles, roleName) } } // Update the roles for _, r := range user.Roles { - siteRoles = append(siteRoles, r.Name) + orgID, _ := uuid.Parse(r.OrganizationID) + siteRoles = append(siteRoles, rbac.RoleIdentifier{ + Name: r.Name, + OrganizationID: orgID, + }) + } + + onlyName := func(role rbac.RoleIdentifier) string { + return role.Name } - user, err = client.UpdateUserRoles(context.Background(), user.ID.String(), codersdk.UpdateRoles{Roles: siteRoles}) + user, err = client.UpdateUserRoles(context.Background(), user.ID.String(), codersdk.UpdateRoles{Roles: db2sdk.List(siteRoles, onlyName)}) require.NoError(t, err, "update site roles") // Update org roles for orgID, roles := range orgRoles { - organizationID, err := uuid.Parse(orgID) - require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID)) - _, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(), - codersdk.UpdateRoles{Roles: roles}) + _, err = client.UpdateOrganizationMemberRoles(context.Background(), orgID, user.ID.String(), + codersdk.UpdateRoles{Roles: db2sdk.List(roles, onlyName)}) require.NoError(t, err, "update org membership roles") } } diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index 1dbd62b4d6a96..6734dac38d8c3 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -170,7 +170,12 @@ func User(user database.User, organizationIDs []uuid.UUID) codersdk.User { } for _, roleName := range user.RBACRoles { - rbacRole, err := rbac.RoleByName(roleName) + // TODO: Currently the api only returns site wide roles. + // Should it return organization roles? + rbacRole, err := rbac.RoleByName(rbac.RoleIdentifier{ + Name: roleName, + OrganizationID: uuid.Nil, + }) if err == nil { convertedUser.Roles = append(convertedUser.Roles, SlimRole(rbacRole)) } else { @@ -519,29 +524,26 @@ func ProvisionerDaemon(dbDaemon database.ProvisionerDaemon) codersdk.Provisioner } func SlimRole(role rbac.Role) codersdk.SlimRole { - roleName, orgIDStr, err := rbac.RoleSplit(role.Name) - if err != nil { - roleName = role.Name + orgID := "" + if role.Identifier.OrganizationID != uuid.Nil { + orgID = role.Identifier.OrganizationID.String() } return codersdk.SlimRole{ DisplayName: role.DisplayName, - Name: roleName, - OrganizationID: orgIDStr, + Name: role.Identifier.Name, + OrganizationID: orgID, } } func RBACRole(role rbac.Role) codersdk.Role { - roleName, orgIDStr, err := rbac.RoleSplit(role.Name) - if err != nil { - roleName = role.Name - } - orgPerms := role.Org[orgIDStr] + slim := SlimRole(role) + orgPerms := role.Org[slim.OrganizationID] return codersdk.Role{ - Name: roleName, - OrganizationID: orgIDStr, - DisplayName: role.DisplayName, + Name: slim.Name, + OrganizationID: slim.OrganizationID, + DisplayName: slim.DisplayName, SitePermissions: List(role.Site, RBACPermission), OrganizationPermissions: List(orgPerms, RBACPermission), UserPermissions: List(role.User, RBACPermission), diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go index 814ba88a1b18c..1a9049044e0ce 100644 --- a/coderd/database/dbauthz/customroles_test.go +++ b/coderd/database/dbauthz/customroles_test.go @@ -35,7 +35,7 @@ func TestUpsertCustomRoles(t *testing.T) { } canAssignRole := rbac.Role{ - Name: "can-assign", + Identifier: rbac.RoleIdentifier{Name: "can-assign"}, DisplayName: "", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceAssignRole.Type: {policy.ActionRead, policy.ActionCreate}, @@ -51,7 +51,7 @@ func TestUpsertCustomRoles(t *testing.T) { all = append(all, t) case rbac.ExpandableRoles: all = append(all, must(t.Expand())...) - case string: + case rbac.RoleIdentifier: all = append(all, must(rbac.RoleByName(t))) default: panic("unknown type") @@ -80,7 +80,7 @@ func TestUpsertCustomRoles(t *testing.T) { { // No roles, so no assign role name: "no-roles", - subject: rbac.RoleNames([]string{}), + subject: rbac.RoleIdentifiers{}, errorContains: "forbidden", }, { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 73c73176b5953..bc8bf19763c73 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -162,7 +162,7 @@ var ( ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "provisionerd", + Identifier: rbac.RoleIdentifier{Name: "provisionerd"}, DisplayName: "Provisioner Daemon", Site: rbac.Permissions(map[string][]policy.Action{ // TODO: Add ProvisionerJob resource type. @@ -191,7 +191,7 @@ var ( ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "autostart", + Identifier: rbac.RoleIdentifier{Name: "autostart"}, DisplayName: "Autostart Daemon", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceSystem.Type: {policy.WildcardSymbol}, @@ -213,7 +213,7 @@ var ( ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "hangdetector", + Identifier: rbac.RoleIdentifier{Name: "hangdetector"}, DisplayName: "Hang Detector Daemon", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceSystem.Type: {policy.WildcardSymbol}, @@ -232,7 +232,7 @@ var ( ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "system", + Identifier: rbac.RoleIdentifier{Name: "system"}, DisplayName: "Coder", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceWildcard.Type: {policy.ActionRead}, @@ -582,8 +582,38 @@ func (q *querier) authorizeUpdateFileTemplate(ctx context.Context, file database } } +// convertToOrganizationRoles converts a set of scoped role names to their unique +// scoped names. The database stores roles as an array of strings, and needs to be +// converted. +// TODO: Maybe make `[]rbac.RoleIdentifier` a custom type that implements a sql scanner +// to remove the need for these converters? +func (*querier) convertToOrganizationRoles(organizationID uuid.UUID, names []string) ([]rbac.RoleIdentifier, error) { + uniques := make([]rbac.RoleIdentifier, 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. + if strings.Contains(name, ":") { + return nil, xerrors.Errorf("attempt to assign a role %q, remove the ': suffix", name) + } + + uniques = append(uniques, rbac.RoleIdentifier{Name: name, OrganizationID: organizationID}) + } + + return uniques, nil +} + +// convertToDeploymentRoles converts string role names into deployment wide roles. +func (*querier) convertToDeploymentRoles(names []string) []rbac.RoleIdentifier { + uniques := make([]rbac.RoleIdentifier, 0, len(names)) + for _, name := range names { + uniques = append(uniques, rbac.RoleIdentifier{Name: name}) + } + + return uniques +} + // 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.RoleIdentifier) error { actor, ok := ActorFromContext(ctx) if !ok { return NoActorError @@ -597,28 +627,24 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r } grantedRoles := append(added, removed...) - customRoles := make([]string, 0) + customRoles := make([]rbac.RoleIdentifier, 0) // Validate that the roles being assigned are valid. for _, r := range grantedRoles { - roleOrgIDStr, isOrgRole := rbac.IsOrgRole(r) + isOrgRole := r.OrganizationID != uuid.Nil if shouldBeOrgRoles && !isOrgRole { return xerrors.Errorf("Must only update org roles") } + if !shouldBeOrgRoles && isOrgRole { return xerrors.Errorf("Must only update site wide roles") } if shouldBeOrgRoles { - roleOrgID, err := uuid.Parse(roleOrgIDStr) - if err != nil { - return xerrors.Errorf("role %q has invalid uuid for org: %w", r, err) - } - if orgID == nil { return xerrors.Errorf("should never happen, orgID is nil, but trying to assign an organization role") } - if roleOrgID != *orgID { + if r.OrganizationID != *orgID { return xerrors.Errorf("attempted to assign role from a different org, role %q to %q", r, orgID.String()) } } @@ -629,7 +655,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r } } - customRolesMap := make(map[string]struct{}, len(customRoles)) + customRolesMap := make(map[rbac.RoleIdentifier]struct{}, len(customRoles)) for _, r := range customRoles { customRolesMap[r] = struct{}{} } @@ -649,7 +675,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r // returns them all, but then someone could pass in a large list to make us do // a lot of loop iterations. if !slices.ContainsFunc(expandedCustomRoles, func(customRole rbac.Role) bool { - return strings.EqualFold(customRole.Name, role) + return strings.EqualFold(customRole.Identifier.Name, role.Name) && customRole.Identifier.OrganizationID == role.OrganizationID }) { return xerrors.Errorf("%q is not a supported role", role) } @@ -2471,9 +2497,14 @@ func (q *querier) InsertOrganization(ctx context.Context, arg database.InsertOrg } func (q *querier) InsertOrganizationMember(ctx context.Context, arg database.InsertOrganizationMemberParams) (database.OrganizationMember, error) { + orgRoles, err := q.convertToOrganizationRoles(arg.OrganizationID, arg.Roles) + if err != nil { + return database.OrganizationMember{}, xerrors.Errorf("converting to organization roles: %w", err) + } + // All roles are added roles. Org member is always implied. - addedRoles := append(arg.Roles, rbac.ScopedRoleOrgMember(arg.OrganizationID)) - err := q.canAssignRoles(ctx, &arg.OrganizationID, addedRoles, []string{}) + addedRoles := append(orgRoles, rbac.ScopedRoleOrgMember(arg.OrganizationID)) + err = q.canAssignRoles(ctx, &arg.OrganizationID, addedRoles, []rbac.RoleIdentifier{}) if err != nil { return database.OrganizationMember{}, err } @@ -2559,8 +2590,8 @@ func (q *querier) InsertTemplateVersionWorkspaceTag(ctx context.Context, arg dat func (q *querier) InsertUser(ctx context.Context, arg database.InsertUserParams) (database.User, error) { // Always check if the assigned roles can actually be assigned by this actor. - impliedRoles := append([]string{rbac.RoleMember()}, arg.RBACRoles...) - err := q.canAssignRoles(ctx, nil, impliedRoles, []string{}) + impliedRoles := append([]rbac.RoleIdentifier{rbac.RoleMember()}, q.convertToDeploymentRoles(arg.RBACRoles)...) + err := q.canAssignRoles(ctx, nil, impliedRoles, []rbac.RoleIdentifier{}) if err != nil { return database.User{}, err } @@ -2847,23 +2878,22 @@ func (q *querier) UpdateMemberRoles(ctx context.Context, arg database.UpdateMemb return database.OrganizationMember{}, err } + originalRoles, err := q.convertToOrganizationRoles(member.OrganizationID, member.Roles) + if err != nil { + return database.OrganizationMember{}, xerrors.Errorf("convert original roles: %w", 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 ': suffix", grantedRole) - } - - scopedGranted = append(scopedGranted, rbac.RoleName(grantedRole, arg.OrgID.String())) + scopedGranted, err := q.convertToOrganizationRoles(arg.OrgID, arg.GrantedRoles) + if err != nil { + return database.OrganizationMember{}, err } // The org member role is always implied. impliedTypes := append(scopedGranted, rbac.ScopedRoleOrgMember(arg.OrgID)) - added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes) + + added, removed := rbac.ChangeRoleSet(originalRoles, impliedTypes) err = q.canAssignRoles(ctx, &arg.OrgID, added, removed) if err != nil { return database.OrganizationMember{}, err @@ -3204,9 +3234,9 @@ func (q *querier) UpdateUserRoles(ctx context.Context, arg database.UpdateUserRo } // The member role is always implied. - impliedTypes := append(arg.GrantedRoles, rbac.RoleMember()) + impliedTypes := append(q.convertToDeploymentRoles(arg.GrantedRoles), rbac.RoleMember()) // If the changeset is nothing, less rbac checks need to be done. - added, removed := rbac.ChangeRoleSet(user.RBACRoles, impliedTypes) + added, removed := rbac.ChangeRoleSet(q.convertToDeploymentRoles(user.RBACRoles), impliedTypes) err = q.canAssignRoles(ctx, nil, added, removed) if err != nil { return database.User{}, err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index dbfb4e15e0de0..9d90a4d44114a 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -82,7 +82,7 @@ func TestInTX(t *testing.T) { }, slog.Make(), coderdtest.AccessControlStorePointer()) actor := rbac.Subject{ ID: uuid.NewString(), - Roles: rbac.RoleNames{rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleOwner()}, Groups: []string{}, Scope: rbac.ScopeAll, } @@ -136,7 +136,7 @@ func TestDBAuthzRecursive(t *testing.T) { }, slog.Make(), coderdtest.AccessControlStorePointer()) actor := rbac.Subject{ ID: uuid.NewString(), - Roles: rbac.RoleNames{rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleOwner()}, Groups: []string{}, Scope: rbac.ScopeAll, } @@ -636,7 +636,7 @@ func (s *MethodTestSuite) TestOrganization() { check.Args(database.InsertOrganizationMemberParams{ OrganizationID: o.ID, UserID: u.ID, - Roles: []string{rbac.ScopedRoleOrgAdmin(o.ID)}, + Roles: []string{codersdk.RoleOrganizationAdmin}, }).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.ScopedRoleOrgAdmin(o.ID)}, + Roles: []string{codersdk.RoleOrganizationAdmin}, }) out := mem out.Roles = []string{} @@ -1179,11 +1179,11 @@ func (s *MethodTestSuite) TestUser() { }).Asserts(rbac.ResourceUserObject(link.UserID), policy.ActionUpdatePersonal).Returns(link) })) s.Run("UpdateUserRoles", s.Subtest(func(db database.Store, check *expects) { - u := dbgen.User(s.T(), db, database.User{RBACRoles: []string{rbac.RoleTemplateAdmin()}}) + u := dbgen.User(s.T(), db, database.User{RBACRoles: []string{codersdk.RoleTemplateAdmin}}) o := u - o.RBACRoles = []string{rbac.RoleUserAdmin()} + o.RBACRoles = []string{codersdk.RoleUserAdmin} check.Args(database.UpdateUserRolesParams{ - GrantedRoles: []string{rbac.RoleUserAdmin()}, + GrantedRoles: []string{codersdk.RoleUserAdmin}, ID: u.ID, }).Asserts( u, policy.ActionRead, diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index 95d8b70a42b40..e391b9e2ef3c6 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -123,7 +123,7 @@ func (s *MethodTestSuite) Subtest(testCaseF func(db database.Store, check *expec az := dbauthz.New(db, rec, slog.Make(), coderdtest.AccessControlStorePointer()) actor := rbac.Subject{ ID: testActorID.String(), - Roles: rbac.RoleNames{rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleOwner()}, Groups: []string{}, Scope: rbac.ScopeAll, } diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 6cb2d94429eb1..4f9d6ddc5b28c 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -26,7 +26,7 @@ import ( var ownerCtx = dbauthz.As(context.Background(), rbac.Subject{ ID: "owner", - Roles: rbac.Roles(must(rbac.RoleNames{rbac.RoleOwner()}.Expand())), + Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.RoleOwner()}.Expand())), Groups: []string{}, Scope: rbac.ExpandableScope(rbac.ScopeAll), }) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 61c44d0779307..bc18da548d683 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -33,7 +33,7 @@ import ( // genCtx is to give all generator functions permission if the db is a dbauthz db. var genCtx = dbauthz.As(context.Background(), rbac.Subject{ ID: "owner", - Roles: rbac.Roles(must(rbac.RoleNames{rbac.RoleOwner()}.Expand())), + Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.RoleOwner()}.Expand())), Groups: []string{}, Scope: rbac.ExpandableScope(rbac.ScopeAll), }) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 147eb8eca6a05..55251f71227ca 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -4808,7 +4808,7 @@ func (q *FakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users = usersFilteredByStatus } - if len(params.RbacRole) > 0 && !slice.Contains(params.RbacRole, rbac.RoleMember()) { + if len(params.RbacRole) > 0 && !slice.Contains(params.RbacRole, rbac.RoleMember().String()) { usersFilteredByRole := make([]database.User, 0, len(users)) for i, user := range users { if slice.OverlapCompare(params.RbacRole, user.RBACRoles, strings.EqualFold) { diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index d71c63b089556..e5fd1db60337f 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -7,6 +7,7 @@ import ( "golang.org/x/exp/maps" "golang.org/x/oauth2" + "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/rbac" @@ -373,3 +374,22 @@ func (p ProvisionerJob) FinishedAt() time.Time { return time.Time{} } + +func (r CustomRole) RoleIdentifier() rbac.RoleIdentifier { + return rbac.RoleIdentifier{ + Name: r.Name, + OrganizationID: r.OrganizationID.UUID, + } +} + +func (r GetAuthorizationUserRolesRow) RoleNames() ([]rbac.RoleIdentifier, error) { + names := make([]rbac.RoleIdentifier, 0, len(r.Roles)) + for _, role := range r.Roles { + value, err := rbac.RoleNameFromString(role) + if err != nil { + return nil, xerrors.Errorf("convert role %q: %w", role, err) + } + names = append(names, value) + } + return names, nil +} diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 5bb45424b57f9..bce375337b5c9 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -438,8 +438,16 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon }) } + roleNames, err := roles.RoleNames() + if err != nil { + return write(http.StatusInternalServerError, codersdk.Response{ + Message: "Internal Server Error", + Detail: err.Error(), + }) + } + //nolint:gocritic // Permission to lookup custom roles the user has assigned. - rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roles.Roles) + rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roleNames) if err != nil { return write(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to expand authenticated user roles", diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go index 131040b89b1f4..5d04c5afacdb3 100644 --- a/coderd/httpmw/authorize_test.go +++ b/coderd/httpmw/authorize_test.go @@ -27,27 +27,26 @@ func TestExtractUserRoles(t *testing.T) { t.Parallel() testCases := []struct { Name string - AddUser func(db database.Store) (database.User, []string, string) + AddUser func(db database.Store) (database.User, []rbac.RoleIdentifier, string) }{ { Name: "Member", - AddUser: func(db database.Store) (database.User, []string, string) { - roles := []string{} - user, token := addUser(t, db, roles...) - return user, append(roles, rbac.RoleMember()), token + AddUser: func(db database.Store) (database.User, []rbac.RoleIdentifier, string) { + user, token := addUser(t, db) + return user, []rbac.RoleIdentifier{rbac.RoleMember()}, token }, }, { - Name: "Admin", - AddUser: func(db database.Store) (database.User, []string, string) { - roles := []string{rbac.RoleOwner()} + Name: "Owner", + AddUser: func(db database.Store) (database.User, []rbac.RoleIdentifier, string) { + roles := []string{codersdk.RoleOwner} user, token := addUser(t, db, roles...) - return user, append(roles, rbac.RoleMember()), token + return user, []rbac.RoleIdentifier{rbac.RoleOwner(), rbac.RoleMember()}, token }, }, { Name: "OrgMember", - AddUser: func(db database.Store) (database.User, []string, string) { + AddUser: func(db database.Store) (database.User, []rbac.RoleIdentifier, string) { roles := []string{} user, token := addUser(t, db, roles...) org, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{ @@ -68,15 +67,15 @@ func TestExtractUserRoles(t *testing.T) { Roles: orgRoles, }) require.NoError(t, err) - return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID))...), token + return user, []rbac.RoleIdentifier{rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID)}, token }, }, { Name: "MultipleOrgMember", - AddUser: func(db database.Store) (database.User, []string, string) { - roles := []string{} - user, token := addUser(t, db, roles...) - roles = append(roles, rbac.RoleMember()) + AddUser: func(db database.Store) (database.User, []rbac.RoleIdentifier, string) { + expected := []rbac.RoleIdentifier{} + user, token := addUser(t, db) + expected = append(expected, rbac.RoleMember()) for i := 0; i < 3; i++ { organization, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{ ID: uuid.New(), @@ -89,8 +88,8 @@ func TestExtractUserRoles(t *testing.T) { orgRoles := []string{} if i%2 == 0 { - orgRoles = append(orgRoles, rbac.RoleOrgAdmin()) - roles = append(roles, rbac.ScopedRoleOrgAdmin(organization.ID)) + orgRoles = append(orgRoles, codersdk.RoleOrganizationAdmin) + expected = append(expected, rbac.ScopedRoleOrgAdmin(organization.ID)) } _, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{ OrganizationID: organization.ID, @@ -100,9 +99,9 @@ func TestExtractUserRoles(t *testing.T) { Roles: orgRoles, }) require.NoError(t, err) - roles = append(roles, rbac.ScopedRoleOrgMember(organization.ID)) + expected = append(expected, rbac.ScopedRoleOrgMember(organization.ID)) } - return user, roles, token + return user, expected, token }, }, } @@ -147,6 +146,9 @@ func addUser(t *testing.T, db database.Store, roles ...string) (database.User, s id, secret = randomAPIKeyParts() hashed = sha256.Sum256([]byte(secret)) ) + if roles == nil { + roles = []string{} + } user, err := db.InsertUser(context.Background(), database.InsertUserParams{ ID: uuid.New(), diff --git a/coderd/httpmw/authz_test.go b/coderd/httpmw/authz_test.go index b469a8f23a5ed..706590e210c1f 100644 --- a/coderd/httpmw/authz_test.go +++ b/coderd/httpmw/authz_test.go @@ -11,6 +11,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" ) func TestAsAuthzSystem(t *testing.T) { @@ -34,7 +35,7 @@ func TestAsAuthzSystem(t *testing.T) { actor, ok := dbauthz.ActorFromContext(req.Context()) assert.True(t, ok, "actor should exist") assert.False(t, userActor.Equal(actor), "systemActor should not be the user actor") - assert.Contains(t, actor.Roles.Names(), "system", "should have system role") + assert.Contains(t, actor.Roles.Names(), rbac.RoleIdentifier{Name: "system"}, "should have system role") }) mwAssertUser := mwAssert(func(req *http.Request) { diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index 44d63cd664460..ca3adcabbae01 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -16,7 +16,6 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpmw" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" ) @@ -152,11 +151,11 @@ func TestOrganizationParam(t *testing.T) { _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ OrganizationID: organization.ID, UserID: user.ID, - Roles: []string{rbac.ScopedRoleOrgMember(organization.ID)}, + Roles: []string{codersdk.RoleOrganizationMember}, }) _, err := db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{ ID: user.ID, - GrantedRoles: []string{rbac.RoleTemplateAdmin()}, + GrantedRoles: []string{codersdk.RoleTemplateAdmin}, }) require.NoError(t, err) diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index a320e05af7ffe..1dd12da89df1a 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -16,7 +16,6 @@ import ( "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/httpmw" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" ) @@ -117,7 +116,7 @@ func TestRateLimit(t *testing.T) { db := dbmem.New() u := dbgen.User(t, db, database.User{ - RBACRoles: []string{rbac.RoleOwner()}, + RBACRoles: []string{codersdk.RoleOwner}, }) _, key := dbgen.APIKey(t, db, database.APIKey{UserID: u.ID}) diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index a72d05caecbb2..99889c0bae5fc 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -119,9 +119,18 @@ func ExtractWorkspaceAgentAndLatestBuild(opts ExtractWorkspaceAgentAndLatestBuil return } + roleNames, err := roles.RoleNames() + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal server error", + Detail: err.Error(), + }) + return + } + subject := rbac.Subject{ ID: row.Workspace.OwnerID.String(), - Roles: rbac.RoleNames(roles.Roles), + Roles: rbac.RoleIdentifiers(roleNames), Groups: roles.Groups, Scope: rbac.WorkspaceAgentScope(rbac.WorkspaceAgentScopeParams{ WorkspaceID: row.Workspace.ID, diff --git a/coderd/identityprovider/tokens.go b/coderd/identityprovider/tokens.go index e9c9e743e7225..324ff2819400d 100644 --- a/coderd/identityprovider/tokens.go +++ b/coderd/identityprovider/tokens.go @@ -214,9 +214,15 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database if err != nil { return oauth2.Token{}, err } + + roleNames, err := roles.RoleNames() + if err != nil { + return oauth2.Token{}, xerrors.Errorf("role names: %w", err) + } + userSubj := rbac.Subject{ ID: dbCode.UserID.String(), - Roles: rbac.RoleNames(roles.Roles), + Roles: rbac.RoleIdentifiers(roleNames), Groups: roles.Groups, Scope: rbac.ScopeAll, } @@ -310,9 +316,15 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut if err != nil { return oauth2.Token{}, err } + + roleNames, err := roles.RoleNames() + if err != nil { + return oauth2.Token{}, xerrors.Errorf("role names: %w", err) + } + userSubj := rbac.Subject{ ID: prevKey.UserID.String(), - Roles: rbac.RoleNames(roles.Roles), + Roles: rbac.RoleIdentifiers(roleNames), Groups: roles.Groups, Scope: rbac.ScopeAll, } diff --git a/coderd/members.go b/coderd/members.go index 36660e5cb968e..3110cc51dbcf2 100644 --- 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(rbac.RoleName(roleName, mem.OrganizationID.String())) + rbacRole, _ := rbac.RoleByName(rbac.RoleIdentifier{Name: roleName, OrganizationID: mem.OrganizationID}) convertedMember.Roles = append(convertedMember.Roles, db2sdk.SlimRole(rbacRole)) } return convertedMember diff --git a/coderd/organizations.go b/coderd/organizations.go index 6ae3358e9a2f2..259fb6486dfd8 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -99,7 +99,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.ScopedRoleOrgMember(organization.ID), + rbac.RoleOrgMember(), }, }) if err != nil { diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 859782d0286b1..614150bb1522c 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -110,13 +110,13 @@ func (s Subject) SafeScopeName() string { if s.Scope == nil { return "no-scope" } - return s.Scope.Name() + return s.Scope.Name().String() } // SafeRoleNames prevent nil pointer dereference. -func (s Subject) SafeRoleNames() []string { +func (s Subject) SafeRoleNames() []RoleIdentifier { if s.Roles == nil { - return []string{} + return []RoleIdentifier{} } 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, roleName.String()) + } 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/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index d3d1ae8d9f765..79fe9af67a607 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -56,7 +56,7 @@ func TestFilterError(t *testing.T) { auth := NewAuthorizer(prometheus.NewRegistry()) subject := Subject{ ID: uuid.NewString(), - Roles: RoleNames{}, + Roles: RoleIdentifiers{}, Groups: []string{}, Scope: ScopeAll, } @@ -77,7 +77,7 @@ func TestFilterError(t *testing.T) { subject := Subject{ ID: uuid.NewString(), - Roles: RoleNames{ + Roles: RoleIdentifiers{ RoleOwner(), }, Groups: []string{}, @@ -159,7 +159,7 @@ func TestFilter(t *testing.T) { Name: "NoRoles", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{}, + Roles: RoleIdentifiers{}, }, ObjectType: ResourceWorkspace.Type, Action: policy.ActionRead, @@ -168,7 +168,7 @@ func TestFilter(t *testing.T) { Name: "Admin", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ScopedRoleOrgMember(orgIDs[0]), "auditor", RoleOwner(), RoleMember()}, + Roles: RoleIdentifiers{ScopedRoleOrgMember(orgIDs[0]), RoleAuditor(), RoleOwner(), RoleMember()}, }, ObjectType: ResourceWorkspace.Type, Action: policy.ActionRead, @@ -177,7 +177,7 @@ func TestFilter(t *testing.T) { Name: "OrgAdmin", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgAdmin(orgIDs[0]), RoleMember()}, + Roles: RoleIdentifiers{ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgAdmin(orgIDs[0]), RoleMember()}, }, ObjectType: ResourceWorkspace.Type, Action: policy.ActionRead, @@ -186,7 +186,7 @@ func TestFilter(t *testing.T) { Name: "OrgMember", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgMember(orgIDs[1]), RoleMember()}, + Roles: RoleIdentifiers{ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgMember(orgIDs[1]), RoleMember()}, }, ObjectType: ResourceWorkspace.Type, Action: policy.ActionRead, @@ -195,7 +195,7 @@ func TestFilter(t *testing.T) { Name: "ManyRoles", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ + Roles: RoleIdentifiers{ ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgAdmin(orgIDs[0]), ScopedRoleOrgMember(orgIDs[1]), ScopedRoleOrgAdmin(orgIDs[1]), ScopedRoleOrgMember(orgIDs[2]), ScopedRoleOrgAdmin(orgIDs[2]), @@ -211,7 +211,7 @@ func TestFilter(t *testing.T) { Name: "SiteMember", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{RoleMember()}, + Roles: RoleIdentifiers{RoleMember()}, }, ObjectType: ResourceUser.Type, Action: policy.ActionRead, @@ -220,7 +220,7 @@ func TestFilter(t *testing.T) { Name: "ReadOrgs", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ + Roles: RoleIdentifiers{ ScopedRoleOrgMember(orgIDs[0]), ScopedRoleOrgMember(orgIDs[1]), ScopedRoleOrgMember(orgIDs[2]), @@ -235,7 +235,7 @@ func TestFilter(t *testing.T) { Name: "ScopeApplicationConnect", Actor: Subject{ ID: userIDs[0].String(), - Roles: RoleNames{ScopedRoleOrgMember(orgIDs[0]), "auditor", RoleOwner(), RoleMember()}, + Roles: RoleIdentifiers{ScopedRoleOrgMember(orgIDs[0]), RoleAuditor(), RoleOwner(), RoleMember()}, }, ObjectType: ResourceWorkspace.Type, Action: policy.ActionRead, @@ -394,7 +394,7 @@ func TestAuthorizeDomain(t *testing.T) { ID: "me", Scope: must(ExpandScope(ScopeAll)), Roles: Roles{{ - Name: "deny-all", + Identifier: RoleIdentifier{Name: "deny-all"}, // List out deny permissions explicitly Site: []Permission{ { @@ -607,8 +607,8 @@ func TestAuthorizeDomain(t *testing.T) { Scope: must(ExpandScope(ScopeAll)), Roles: Roles{ { - Name: "ReadOnlyOrgAndUser", - Site: []Permission{}, + Identifier: RoleIdentifier{Name: "ReadOnlyOrgAndUser"}, + Site: []Permission{}, Org: map[string][]Permission{ defOrg.String(): {{ Negate: false, @@ -701,7 +701,7 @@ func TestAuthorizeLevels(t *testing.T) { Roles: Roles{ must(RoleByName(RoleOwner())), { - Name: "org-deny:" + defOrg.String(), + Identifier: RoleIdentifier{Name: "org-deny:", OrganizationID: defOrg}, Org: map[string][]Permission{ defOrg.String(): { { @@ -713,7 +713,7 @@ func TestAuthorizeLevels(t *testing.T) { }, }, { - Name: "user-deny-all", + Identifier: RoleIdentifier{Name: "user-deny-all"}, // List out deny permissions explicitly User: []Permission{ { @@ -761,7 +761,7 @@ func TestAuthorizeLevels(t *testing.T) { Scope: must(ExpandScope(ScopeAll)), Roles: Roles{ { - Name: "site-noise", + Identifier: RoleIdentifier{Name: "site-noise"}, Site: []Permission{ { Negate: true, @@ -772,7 +772,7 @@ func TestAuthorizeLevels(t *testing.T) { }, must(RoleByName(ScopedRoleOrgAdmin(defOrg))), { - Name: "user-deny-all", + Identifier: RoleIdentifier{Name: "user-deny-all"}, // List out deny permissions explicitly User: []Permission{ { @@ -896,7 +896,7 @@ func TestAuthorizeScope(t *testing.T) { }, Scope: Scope{ Role: Role{ - Name: "workspace_agent", + Identifier: RoleIdentifier{Name: "workspace_agent"}, DisplayName: "Workspace Agent", Site: Permissions(map[string][]policy.Action{ // Only read access for workspaces. @@ -985,7 +985,7 @@ func TestAuthorizeScope(t *testing.T) { }, Scope: Scope{ Role: Role{ - Name: "create_workspace", + Identifier: RoleIdentifier{Name: "create_workspace"}, DisplayName: "Create Workspace", Site: Permissions(map[string][]policy.Action{ // Only read access for workspaces. diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 344d85562a094..0c46096c74e6f 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -41,7 +41,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "NoRoles", Actor: rbac.Subject{ ID: user.String(), - Roles: rbac.RoleNames{}, + Roles: rbac.RoleIdentifiers{}, Scope: rbac.ScopeAll, }, }, @@ -49,7 +49,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "Admin", Actor: rbac.Subject{ // Give some extra roles that an admin might have - Roles: rbac.RoleNames{rbac.ScopedRoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(orgs[0]), rbac.RoleAuditor(), rbac.RoleOwner(), rbac.RoleMember()}, ID: user.String(), Scope: rbac.ScopeAll, Groups: noiseGroups, @@ -58,7 +58,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U { Name: "OrgAdmin", Actor: rbac.Subject{ - Roles: rbac.RoleNames{rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgAdmin(orgs[0]), rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgAdmin(orgs[0]), rbac.RoleMember()}, ID: user.String(), Scope: rbac.ScopeAll, Groups: noiseGroups, @@ -68,7 +68,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "OrgMember", Actor: rbac.Subject{ // Member of 2 orgs - Roles: rbac.RoleNames{rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgMember(orgs[1]), rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgMember(orgs[1]), rbac.RoleMember()}, ID: user.String(), Scope: rbac.ScopeAll, Groups: noiseGroups, @@ -78,7 +78,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "ManyRoles", Actor: rbac.Subject{ // Admin of many orgs - Roles: rbac.RoleNames{ + Roles: rbac.RoleIdentifiers{ rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgAdmin(orgs[0]), rbac.ScopedRoleOrgMember(orgs[1]), rbac.ScopedRoleOrgAdmin(orgs[1]), rbac.ScopedRoleOrgMember(orgs[2]), rbac.ScopedRoleOrgAdmin(orgs[2]), @@ -93,7 +93,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "ManyRolesCachedSubject", Actor: rbac.Subject{ // Admin of many orgs - Roles: rbac.RoleNames{ + Roles: rbac.RoleIdentifiers{ rbac.ScopedRoleOrgMember(orgs[0]), rbac.ScopedRoleOrgAdmin(orgs[0]), rbac.ScopedRoleOrgMember(orgs[1]), rbac.ScopedRoleOrgAdmin(orgs[1]), rbac.ScopedRoleOrgMember(orgs[2]), rbac.ScopedRoleOrgAdmin(orgs[2]), @@ -108,7 +108,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "AdminWithScope", Actor: rbac.Subject{ // Give some extra roles that an admin might have - Roles: rbac.RoleNames{rbac.ScopedRoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(orgs[0]), rbac.RoleAuditor(), rbac.RoleOwner(), rbac.RoleMember()}, ID: user.String(), Scope: rbac.ScopeApplicationConnect, Groups: noiseGroups, @@ -119,8 +119,8 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "StaticRoles", Actor: rbac.Subject{ // Give some extra roles that an admin might have - Roles: rbac.RoleNames{ - "auditor", rbac.RoleOwner(), rbac.RoleMember(), + Roles: rbac.RoleIdentifiers{ + rbac.RoleAuditor(), rbac.RoleOwner(), rbac.RoleMember(), rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), }, ID: user.String(), @@ -133,8 +133,8 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Name: "StaticRolesWithCache", Actor: rbac.Subject{ // Give some extra roles that an admin might have - Roles: rbac.RoleNames{ - "auditor", rbac.RoleOwner(), rbac.RoleMember(), + Roles: rbac.RoleIdentifiers{ + rbac.RoleAuditor(), rbac.RoleOwner(), rbac.RoleMember(), rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), }, ID: user.String(), diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index fae31150e2053..41411a2a968a2 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -1,6 +1,7 @@ package rbac import ( + "encoding/json" "errors" "sort" "strings" @@ -34,42 +35,99 @@ func init() { ReloadBuiltinRoles(nil) } -// RoleNames is a list of user assignable role names. The role names must be +// RoleIdentifiers 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 RoleIdentifiers []RoleIdentifier -func (names RoleNames) Expand() ([]Role, error) { +func (names RoleIdentifiers) Expand() ([]Role, error) { return rolesByNames(names) } -func (names RoleNames) Names() []string { +func (names RoleIdentifiers) Names() []RoleIdentifier { return names } -// The functions below ONLY need to exist for roles that are "defaulted" in some way. -// Any other roles (like auditor), can be listed and let the user select/assigned. -// Once we have a database implementation, the "default" roles can be defined on the -// site and orgs, and these functions can be removed. +// RoleIdentifier contains both the name of the role, and any organizational scope. +// Both fields are required to be globally unique and identifiable. +type RoleIdentifier struct { + Name string + // OrganizationID is uuid.Nil for unscoped roles (aka deployment wide) + OrganizationID uuid.UUID +} -func RoleOwner() string { - return RoleName(owner, "") +func (r RoleIdentifier) IsOrgRole() bool { + return r.OrganizationID != uuid.Nil } -func CustomSiteRole() string { return RoleName(customSiteRole, "") } +// RoleNameFromString takes a formatted string '[:org_id]'. +func RoleNameFromString(input string) (RoleIdentifier, error) { + var role RoleIdentifier + + arr := strings.Split(input, ":") + if len(arr) > 2 { + return role, xerrors.Errorf("too many colons in role name") + } + + if len(arr) == 0 { + return role, xerrors.Errorf("empty string not a valid role") + } + + if arr[0] == "" { + return role, xerrors.Errorf("role cannot be the empty string") + } + + role.Name = arr[0] -func RoleTemplateAdmin() string { - return RoleName(templateAdmin, "") + if len(arr) == 2 { + orgID, err := uuid.Parse(arr[1]) + if err != nil { + return role, xerrors.Errorf("%q not a valid uuid: %w", arr[1], err) + } + role.OrganizationID = orgID + } + return role, nil } -func RoleUserAdmin() string { - return RoleName(userAdmin, "") +func (r RoleIdentifier) String() string { + if r.OrganizationID != uuid.Nil { + return r.Name + ":" + r.OrganizationID.String() + } + return r.Name } -func RoleMember() string { - return RoleName(member, "") +func (r *RoleIdentifier) MarshalJSON() ([]byte, error) { + return json.Marshal(r.String()) } +func (r *RoleIdentifier) UnmarshalJSON(data []byte) error { + var str string + err := json.Unmarshal(data, &str) + if err != nil { + return err + } + + v, err := RoleNameFromString(str) + if err != nil { + return err + } + + *r = v + return nil +} + +// The functions below ONLY need to exist for roles that are "defaulted" in some way. +// Any other roles (like auditor), can be listed and let the user select/assigned. +// Once we have a database implementation, the "default" roles can be defined on the +// site and orgs, and these functions can be removed. + +func RoleOwner() RoleIdentifier { return RoleIdentifier{Name: owner} } +func CustomSiteRole() RoleIdentifier { return RoleIdentifier{Name: customSiteRole} } +func RoleTemplateAdmin() RoleIdentifier { return RoleIdentifier{Name: templateAdmin} } +func RoleUserAdmin() RoleIdentifier { return RoleIdentifier{Name: userAdmin} } +func RoleMember() RoleIdentifier { return RoleIdentifier{Name: member} } +func RoleAuditor() RoleIdentifier { return RoleIdentifier{Name: auditor} } + func RoleOrgAdmin() string { return orgAdmin } @@ -81,15 +139,15 @@ func RoleOrgMember() string { // ScopedRoleOrgAdmin is the org role with the organization ID // Deprecated This was used before organization scope was included as a // field in all user facing APIs. Usage of 'ScopedRoleOrgAdmin()' is preferred. -func ScopedRoleOrgAdmin(organizationID uuid.UUID) string { - return RoleName(orgAdmin, organizationID.String()) +func ScopedRoleOrgAdmin(organizationID uuid.UUID) RoleIdentifier { + return RoleIdentifier{Name: orgAdmin, OrganizationID: organizationID} } // ScopedRoleOrgMember is the org role with the organization ID // Deprecated This was used before organization scope was included as a // field in all user facing APIs. Usage of 'ScopedRoleOrgMember()' is preferred. -func ScopedRoleOrgMember(organizationID uuid.UUID) string { - return RoleName(orgMember, organizationID.String()) +func ScopedRoleOrgMember(organizationID uuid.UUID) RoleIdentifier { + return RoleIdentifier{Name: orgMember, OrganizationID: organizationID} } func allPermsExcept(excepts ...Objecter) []Permission { @@ -127,7 +185,7 @@ func allPermsExcept(excepts ...Objecter) []Permission { // // This map will be replaced by database storage defined by this ticket. // https://github.com/coder/coder/issues/1194 -var builtInRoles map[string]func(orgID string) Role +var builtInRoles map[string]func(orgID uuid.UUID) Role type RoleOptions struct { NoOwnerWorkspaceExec bool @@ -158,7 +216,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { // on every authorize call. 'withCachedRegoValue' can be used as well to // preallocate the rego value that is used by the rego eval engine. ownerRole := Role{ - Name: owner, + Identifier: RoleOwner(), DisplayName: "Owner", Site: append( // Workspace dormancy and workspace are omitted. @@ -174,7 +232,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { }.withCachedRegoValue() memberRole := Role{ - Name: member, + Identifier: RoleMember(), DisplayName: "Member", Site: Permissions(map[string][]policy.Action{ ResourceAssignRole.Type: {policy.ActionRead}, @@ -200,7 +258,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { }.withCachedRegoValue() auditorRole := Role{ - Name: auditor, + Identifier: RoleAuditor(), DisplayName: "Auditor", Site: Permissions(map[string][]policy.Action{ // Should be able to read all template details, even in orgs they @@ -220,7 +278,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { }.withCachedRegoValue() templateAdminRole := Role{ - Name: templateAdmin, + Identifier: RoleTemplateAdmin(), DisplayName: "Template Admin", Site: Permissions(map[string][]policy.Action{ ResourceTemplate.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionViewInsights}, @@ -241,7 +299,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { }.withCachedRegoValue() userAdminRole := Role{ - Name: userAdmin, + Identifier: RoleUserAdmin(), DisplayName: "User Admin", Site: Permissions(map[string][]policy.Action{ ResourceAssignRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead}, @@ -257,42 +315,42 @@ func ReloadBuiltinRoles(opts *RoleOptions) { User: []Permission{}, }.withCachedRegoValue() - builtInRoles = map[string]func(orgID string) Role{ + builtInRoles = map[string]func(orgID uuid.UUID) Role{ // admin grants all actions to all resources. - owner: func(_ string) Role { + owner: func(_ uuid.UUID) Role { return ownerRole }, // member grants all actions to all resources owned by the user - member: func(_ string) Role { + member: func(_ uuid.UUID) Role { return memberRole }, // auditor provides all permissions required to effectively read and understand // audit log events. // TODO: Finish the auditor as we add resources. - auditor: func(_ string) Role { + auditor: func(_ uuid.UUID) Role { return auditorRole }, - templateAdmin: func(_ string) Role { + templateAdmin: func(_ uuid.UUID) Role { return templateAdminRole }, - userAdmin: func(_ string) Role { + userAdmin: func(_ uuid.UUID) Role { return userAdminRole }, // orgAdmin returns a role with all actions allows in a given // organization scope. - orgAdmin: func(organizationID string) Role { + orgAdmin: func(organizationID uuid.UUID) Role { return Role{ - Name: RoleName(orgAdmin, organizationID), + Identifier: RoleIdentifier{Name: orgAdmin, OrganizationID: organizationID}, DisplayName: "Organization Admin", Site: []Permission{}, Org: map[string][]Permission{ // Org admins should not have workspace exec perms. - organizationID: append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant), Permissions(map[string][]policy.Action{ + organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant), Permissions(map[string][]policy.Action{ ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionDelete, policy.ActionCreate, policy.ActionUpdate, policy.ActionWorkspaceStop}, ResourceWorkspace.Type: slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH), })...), @@ -303,13 +361,13 @@ func ReloadBuiltinRoles(opts *RoleOptions) { // orgMember has an empty set of permissions, this just implies their membership // in an organization. - orgMember: func(organizationID string) Role { + orgMember: func(organizationID uuid.UUID) Role { return Role{ - Name: RoleName(orgMember, organizationID), + Identifier: RoleIdentifier{Name: orgMember, OrganizationID: organizationID}, DisplayName: "", Site: []Permission{}, Org: map[string][]Permission{ - organizationID: { + organizationID.String(): { { // All org members can read the organization ResourceType: ResourceOrganization.Type, @@ -370,7 +428,7 @@ var assignRoles = map[string]map[string]bool{ } // ExpandableRoles is any type that can be expanded into a []Role. This is implemented -// as an interface so we can have RoleNames for user defined roles, and implement +// as an interface so we can have RoleIdentifiers for user defined roles, and implement // custom ExpandableRoles for system type users (eg autostart/autostop system role). // We want a clear divide between the two types of roles so users have no codepath // to interact or assign system roles. @@ -381,7 +439,7 @@ type ExpandableRoles interface { Expand() ([]Role, error) // Names is for logging and tracing purposes, we want to know the human // names of the expanded roles. - Names() []string + Names() []RoleIdentifier } // Permission is the format passed into the rego. @@ -424,7 +482,7 @@ func (perm Permission) Valid() error { // Users of this package should instead **only** use the role names, and // this package will expand the role names into their json payloads. type Role struct { - Name string `json:"name"` + Identifier RoleIdentifier `json:"name"` // DisplayName is used for UI purposes. If the role has no display name, // that means the UI should never display it. DisplayName string `json:"display_name"` @@ -474,10 +532,10 @@ func (roles Roles) Expand() ([]Role, error) { return roles, nil } -func (roles Roles) Names() []string { - names := make([]string, 0, len(roles)) +func (roles Roles) Names() []RoleIdentifier { + names := make([]RoleIdentifier, 0, len(roles)) for _, r := range roles { - names = append(names, r.Name) + names = append(names, r.Identifier) } return names } @@ -485,32 +543,22 @@ func (roles Roles) Names() []string { // CanAssignRole is a helper function that returns true if the user can assign // the specified role. This also can be used for removing a role. // This is a simple implementation for now. -func CanAssignRole(expandable ExpandableRoles, assignedRole string) bool { +func CanAssignRole(subjectHasRoles ExpandableRoles, assignedRole RoleIdentifier) bool { // For CanAssignRole, we only care about the names of the roles. - roles := expandable.Names() + roles := subjectHasRoles.Names() - assigned, assignedOrg, err := RoleSplit(assignedRole) - if err != nil { - return false - } - - for _, longRole := range roles { - role, orgID, err := RoleSplit(longRole) - if err != nil { - continue - } - - if orgID != "" && orgID != assignedOrg { + for _, myRole := range roles { + if myRole.OrganizationID != uuid.Nil && myRole.OrganizationID != assignedRole.OrganizationID { // Org roles only apply to the org they are assigned to. continue } - allowed, ok := assignRoles[role] + allowedAssignList, ok := assignRoles[myRole.Name] if !ok { continue } - if allowed[assigned] { + if allowedAssignList[assignedRole.Name] { return true } } @@ -523,29 +571,24 @@ func CanAssignRole(expandable ExpandableRoles, assignedRole string) bool { // This function is exported so that the Display name can be returned to the // api. We should maybe make an exported function that returns just the // human-readable content of the Role struct (name + display name). -func RoleByName(name string) (Role, error) { - roleName, orgID, err := RoleSplit(name) - if err != nil { - return Role{}, xerrors.Errorf("parse role name: %w", err) - } - - roleFunc, ok := builtInRoles[roleName] +func RoleByName(name RoleIdentifier) (Role, error) { + roleFunc, ok := builtInRoles[name.Name] if !ok { // No role found - return Role{}, xerrors.Errorf("role %q not found", roleName) + return Role{}, xerrors.Errorf("role %q not found", name.String()) } // Ensure all org roles are properly scoped a non-empty organization id. // This is just some defensive programming. - role := roleFunc(orgID) - if len(role.Org) > 0 && orgID == "" { - return Role{}, xerrors.Errorf("expect a org id for role %q", roleName) + role := roleFunc(name.OrganizationID) + if len(role.Org) > 0 && name.OrganizationID == uuid.Nil { + return Role{}, xerrors.Errorf("expect a org id for role %q", name.String()) } return role, nil } -func rolesByNames(roleNames []string) ([]Role, error) { +func rolesByNames(roleNames []RoleIdentifier) ([]Role, error) { roles := make([]Role, 0, len(roleNames)) for _, n := range roleNames { r, err := RoleByName(n) @@ -557,14 +600,6 @@ func rolesByNames(roleNames []string) ([]Role, error) { return roles, nil } -func IsOrgRole(roleName string) (string, bool) { - _, orgID, err := RoleSplit(roleName) - if err == nil && orgID != "" { - return orgID, true - } - return "", false -} - // OrganizationRoles lists all roles that can be applied to an organization user // in the given organization. This is the list of available roles, // and specific to an organization. @@ -574,13 +609,8 @@ func IsOrgRole(roleName string) (string, bool) { func OrganizationRoles(organizationID uuid.UUID) []Role { var roles []Role for _, roleF := range builtInRoles { - role := roleF(organizationID.String()) - _, scope, err := RoleSplit(role.Name) - if err != nil { - // This should never happen - continue - } - if scope == organizationID.String() { + role := roleF(organizationID) + if role.Identifier.OrganizationID == organizationID { roles = append(roles, role) } } @@ -595,13 +625,9 @@ func OrganizationRoles(organizationID uuid.UUID) []Role { func SiteRoles() []Role { var roles []Role for _, roleF := range builtInRoles { - role := roleF("random") - _, scope, err := RoleSplit(role.Name) - if err != nil { - // This should never happen - continue - } - if scope == "" { + // Must provide some non-nil uuid to filter out org roles. + role := roleF(uuid.New()) + if !role.Identifier.IsOrgRole() { roles = append(roles, role) } } @@ -613,8 +639,8 @@ func SiteRoles() []Role { // removing roles. This set determines the changes, so that the appropriate // RBAC checks can be applied using "ActionCreate" and "ActionDelete" for // "added" and "removed" roles respectively. -func ChangeRoleSet(from []string, to []string) (added []string, removed []string) { - has := make(map[string]struct{}) +func ChangeRoleSet(from []RoleIdentifier, to []RoleIdentifier) (added []RoleIdentifier, removed []RoleIdentifier) { + has := make(map[RoleIdentifier]struct{}) for _, exists := range from { has[exists] = struct{}{} } @@ -639,34 +665,6 @@ func ChangeRoleSet(from []string, to []string) (added []string, removed []string return added, removed } -// RoleName is a quick helper function to return -// -// role_name:scopeID -// -// If no scopeID is required, only 'role_name' is returned -func RoleName(name string, orgID string) string { - if orgID == "" { - return name - } - return name + ":" + orgID -} - -func RoleSplit(role string) (name string, orgID string, err error) { - arr := strings.Split(role, ":") - if len(arr) > 2 { - return "", "", xerrors.Errorf("too many colons in role name") - } - - if arr[0] == "" { - return "", "", xerrors.Errorf("role cannot be the empty string") - } - - if len(arr) == 2 { - return arr[0], arr[1], nil - } - return arr[0], "", nil -} - // Permissions is just a helper function to make building roles that list out resources // and actions a bit easier. func Permissions(perms map[string][]policy.Action) []Permission { diff --git a/coderd/rbac/roles_internal_test.go b/coderd/rbac/roles_internal_test.go index 70296f7519b97..3f2d0d89fe455 100644 --- a/coderd/rbac/roles_internal_test.go +++ b/coderd/rbac/roles_internal_test.go @@ -20,7 +20,7 @@ import ( // A possible large improvement would be to implement the ast.Value interface directly. func BenchmarkRBACValueAllocation(b *testing.B) { actor := Subject{ - Roles: RoleNames{ScopedRoleOrgMember(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}, + Roles: RoleIdentifiers{ScopedRoleOrgMember(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}, ID: uuid.NewString(), Scope: ScopeAll, Groups: []string{uuid.NewString(), uuid.NewString(), uuid.NewString()}, @@ -73,7 +73,7 @@ func TestRegoInputValue(t *testing.T) { // Expand all roles and make sure we have a good copy. // This is because these tests modify the roles, and we don't want to // modify the original roles. - roles, err := RoleNames{ScopedRoleOrgMember(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}.Expand() + roles, err := RoleIdentifiers{ScopedRoleOrgMember(uuid.New()), ScopedRoleOrgAdmin(uuid.New()), RoleMember()}.Expand() require.NoError(t, err, "failed to expand roles") for i := range roles { // If all cached values are nil, then the role will not use @@ -213,25 +213,25 @@ func TestRoleByName(t *testing.T) { testCases := []struct { Role Role }{ - {Role: builtInRoles[owner]("")}, - {Role: builtInRoles[member]("")}, - {Role: builtInRoles[templateAdmin]("")}, - {Role: builtInRoles[userAdmin]("")}, - {Role: builtInRoles[auditor]("")}, - - {Role: builtInRoles[orgAdmin]("4592dac5-0945-42fd-828d-a903957d3dbb")}, - {Role: builtInRoles[orgAdmin]("24c100c5-1920-49c0-8c38-1b640ac4b38c")}, - {Role: builtInRoles[orgAdmin]("4a00f697-0040-4079-b3ce-d24470281a62")}, - - {Role: builtInRoles[orgMember]("3293c50e-fa5d-414f-a461-01112a4dfb6f")}, - {Role: builtInRoles[orgMember]("f88dd23d-bdbd-469d-b82e-36ee06c3d1e1")}, - {Role: builtInRoles[orgMember]("02cfd2a5-016c-4d8d-8290-301f5f18023d")}, + {Role: builtInRoles[owner](uuid.Nil)}, + {Role: builtInRoles[member](uuid.Nil)}, + {Role: builtInRoles[templateAdmin](uuid.Nil)}, + {Role: builtInRoles[userAdmin](uuid.Nil)}, + {Role: builtInRoles[auditor](uuid.Nil)}, + + {Role: builtInRoles[orgAdmin](uuid.New())}, + {Role: builtInRoles[orgAdmin](uuid.New())}, + {Role: builtInRoles[orgAdmin](uuid.New())}, + + {Role: builtInRoles[orgMember](uuid.New())}, + {Role: builtInRoles[orgMember](uuid.New())}, + {Role: builtInRoles[orgMember](uuid.New())}, } for _, c := range testCases { c := c - t.Run(c.Role.Name, func(t *testing.T) { - role, err := RoleByName(c.Role.Name) + t.Run(c.Role.Identifier.String(), func(t *testing.T) { + role, err := RoleByName(c.Role.Identifier) require.NoError(t, err, "role exists") equalRoles(t, c.Role, role) }) @@ -242,20 +242,17 @@ func TestRoleByName(t *testing.T) { t.Run("Errors", func(t *testing.T) { var err error - _, err = RoleByName("") + _, err = RoleByName(RoleIdentifier{}) require.Error(t, err, "empty role") - _, err = RoleByName("too:many:colons") - require.Error(t, err, "too many colons") - - _, err = RoleByName(orgMember) + _, err = RoleByName(RoleIdentifier{Name: orgMember}) require.Error(t, err, "expect orgID") }) } // SameAs compares 2 roles for equality. func equalRoles(t *testing.T, a, b Role) { - require.Equal(t, a.Name, b.Name, "role names") + require.Equal(t, a.Identifier, b.Identifier, "role names") require.Equal(t, a.DisplayName, b.DisplayName, "role display names") require.ElementsMatch(t, a.Site, b.Site, "site permissions") require.ElementsMatch(t, a.User, b.User, "user permissions") diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index f2f0d1d3399e2..a1f607ac756c8 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -26,7 +26,7 @@ func TestBuiltInRoles(t *testing.T) { t.Parallel() for _, r := range rbac.SiteRoles() { r := r - t.Run(r.Name, func(t *testing.T) { + t.Run(r.Identifier.String(), func(t *testing.T) { t.Parallel() require.NoError(t, r.Valid(), "invalid role") }) @@ -34,7 +34,7 @@ func TestBuiltInRoles(t *testing.T) { for _, r := range rbac.OrganizationRoles(uuid.New()) { r := r - t.Run(r.Name, func(t *testing.T) { + t.Run(r.Identifier.String(), func(t *testing.T) { t.Parallel() require.NoError(t, r.Valid(), "invalid role") }) @@ -45,7 +45,7 @@ func TestBuiltInRoles(t *testing.T) { func TestOwnerExec(t *testing.T) { owner := rbac.Subject{ ID: uuid.NewString(), - Roles: rbac.RoleNames{rbac.RoleMember(), rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleOwner()}, Scope: rbac.ScopeAll, } @@ -98,17 +98,17 @@ func TestRolePermissions(t *testing.T) { apiKeyID := uuid.New() // Subjects to user - memberMe := authSubject{Name: "member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleNames{rbac.RoleMember()}}} - orgMemberMe := authSubject{Name: "org_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}}} + memberMe := authSubject{Name: "member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember()}}} + orgMemberMe := authSubject{Name: "org_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}}} - owner := authSubject{Name: "owner", Actor: rbac.Subject{ID: adminID.String(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.RoleOwner()}}} - orgAdmin := authSubject{Name: "org_admin", Actor: rbac.Subject{ID: adminID.String(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID), rbac.ScopedRoleOrgAdmin(orgID)}}} + owner := authSubject{Name: "owner", Actor: rbac.Subject{ID: adminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleOwner()}}} + orgAdmin := authSubject{Name: "org_admin", Actor: rbac.Subject{ID: adminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID), rbac.ScopedRoleOrgAdmin(orgID)}}} - otherOrgMember := authSubject{Name: "org_member_other", Actor: rbac.Subject{ID: uuid.NewString(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.ScopedRoleOrgMember(otherOrg)}}} - otherOrgAdmin := authSubject{Name: "org_admin_other", Actor: rbac.Subject{ID: uuid.NewString(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.ScopedRoleOrgMember(otherOrg), rbac.ScopedRoleOrgAdmin(otherOrg)}}} + otherOrgMember := authSubject{Name: "org_member_other", Actor: rbac.Subject{ID: uuid.NewString(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(otherOrg)}}} + otherOrgAdmin := authSubject{Name: "org_admin_other", Actor: rbac.Subject{ID: uuid.NewString(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(otherOrg), rbac.ScopedRoleOrgAdmin(otherOrg)}}} - templateAdmin := authSubject{Name: "template-admin", Actor: rbac.Subject{ID: templateAdminID.String(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.RoleTemplateAdmin()}}} - userAdmin := authSubject{Name: "user-admin", Actor: rbac.Subject{ID: templateAdminID.String(), Roles: rbac.RoleNames{rbac.RoleMember(), rbac.RoleUserAdmin()}}} + templateAdmin := authSubject{Name: "template-admin", Actor: rbac.Subject{ID: templateAdminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleTemplateAdmin()}}} + userAdmin := authSubject{Name: "user-admin", Actor: rbac.Subject{ID: templateAdminID.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleUserAdmin()}}} // requiredSubjects are required to be asserted in each test case. This is // to make sure one is not forgotten. @@ -616,50 +616,40 @@ func TestIsOrgRole(t *testing.T) { require.NoError(t, err) testCases := []struct { - RoleName string - OrgRole bool - OrgID string + Identifier rbac.RoleIdentifier + OrgRole bool + OrgID uuid.UUID }{ // Not org roles - {RoleName: rbac.RoleOwner()}, - {RoleName: rbac.RoleMember()}, - {RoleName: "auditor"}, - - { - RoleName: "a:bad:role", - OrgRole: false, - }, + {Identifier: rbac.RoleOwner()}, + {Identifier: rbac.RoleMember()}, + {Identifier: rbac.RoleAuditor()}, { - RoleName: "", - OrgRole: false, + Identifier: rbac.RoleIdentifier{}, + OrgRole: false, }, // Org roles { - RoleName: rbac.ScopedRoleOrgAdmin(randomUUID), - OrgRole: true, - OrgID: randomUUID.String(), - }, - { - RoleName: rbac.ScopedRoleOrgMember(randomUUID), - OrgRole: true, - OrgID: randomUUID.String(), + Identifier: rbac.ScopedRoleOrgAdmin(randomUUID), + OrgRole: true, + OrgID: randomUUID, }, { - RoleName: "test:example", - OrgRole: true, - OrgID: "example", + Identifier: rbac.ScopedRoleOrgMember(randomUUID), + OrgRole: true, + OrgID: randomUUID, }, } // nolint:paralleltest for _, c := range testCases { c := c - t.Run(c.RoleName, func(t *testing.T) { + t.Run(c.Identifier.String(), func(t *testing.T) { t.Parallel() - orgID, ok := rbac.IsOrgRole(c.RoleName) + ok := c.Identifier.IsOrgRole() require.Equal(t, c.OrgRole, ok, "match expected org role") - require.Equal(t, c.OrgID, orgID, "match expected org id") + require.Equal(t, c.OrgID, c.Identifier.OrganizationID, "match expected org id") }) } } @@ -670,7 +660,7 @@ func TestListRoles(t *testing.T) { siteRoles := rbac.SiteRoles() siteRoleNames := make([]string, 0, len(siteRoles)) for _, role := range siteRoles { - siteRoleNames = append(siteRoleNames, role.Name) + siteRoleNames = append(siteRoleNames, role.Identifier.Name) } // If this test is ever failing, just update the list to the roles @@ -690,7 +680,7 @@ func TestListRoles(t *testing.T) { orgRoles := rbac.OrganizationRoles(orgID) orgRoleNames := make([]string, 0, len(orgRoles)) for _, role := range orgRoles { - orgRoleNames = append(orgRoleNames, role.Name) + orgRoleNames = append(orgRoleNames, role.Identifier.String()) } require.ElementsMatch(t, []string{ @@ -738,13 +728,22 @@ func TestChangeSet(t *testing.T) { }, } + convert := func(s []string) rbac.RoleIdentifiers { + tmp := make([]rbac.RoleIdentifier, 0, len(s)) + for _, e := range s { + tmp = append(tmp, rbac.RoleIdentifier{Name: e}) + } + return tmp + } + for _, c := range testCases { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - add, remove := rbac.ChangeRoleSet(c.From, c.To) - require.ElementsMatch(t, c.ExpAdd, add, "expect added") - require.ElementsMatch(t, c.ExpRemove, remove, "expect removed") + + add, remove := rbac.ChangeRoleSet(convert(c.From), convert(c.To)) + require.ElementsMatch(t, convert(c.ExpAdd), add, "expect added") + require.ElementsMatch(t, convert(c.ExpRemove), remove, "expect removed") }) } } diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go index 80cbd1165073b..26d354084c7b5 100644 --- a/coderd/rbac/rolestore/rolestore.go +++ b/coderd/rbac/rolestore/rolestore.go @@ -39,14 +39,14 @@ func roleCache(ctx context.Context) *syncmap.Map[string, rbac.Role] { } // 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.RoleIdentifier) (rbac.Roles, error) { if len(names) == 0 { // That was easy return []rbac.Role{}, nil } cache := roleCache(ctx) - lookup := make([]string, 0) + lookup := make([]rbac.RoleIdentifier, 0) roles := make([]rbac.Role, 0, len(names)) for _, name := range names { @@ -58,7 +58,7 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles, } // Check custom role cache - customRole, ok := cache.Load(name) + customRole, ok := cache.Load(name.String()) if ok { roles = append(roles, customRole) continue @@ -69,26 +69,11 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles, } if len(lookup) > 0 { - // The set of roles coming in are formatted as 'rolename[:]'. - // 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) - if err != nil { - continue - } - - parsedOrgID := uuid.Nil // Default to no org ID - if orgID != "" { - parsedOrgID, err = uuid.Parse(orgID) - if err != nil { - continue - } - } - lookupArgs = append(lookupArgs, database.NameOrganizationPair{ - Name: roleName, - OrganizationID: parsedOrgID, + Name: name.Name, + OrganizationID: name.OrganizationID, }) } @@ -111,7 +96,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.RoleIdentifier().String(), converted) } } @@ -133,12 +118,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, + Identifier: dbRole.RoleIdentifier(), DisplayName: dbRole.DisplayName, Site: convertPermissions(dbRole.SitePermissions), Org: nil, diff --git a/coderd/rbac/rolestore/rolestore_test.go b/coderd/rbac/rolestore/rolestore_test.go index 318f2f579b340..b7712357d0721 100644 --- a/coderd/rbac/rolestore/rolestore_test.go +++ b/coderd/rbac/rolestore/rolestore_test.go @@ -35,7 +35,7 @@ func TestExpandCustomRoleRoles(t *testing.T) { }) ctx := testutil.Context(t, testutil.WaitShort) - roles, err := rolestore.Expand(ctx, db, []string{rbac.RoleName(roleName, org.ID.String())}) + roles, err := rolestore.Expand(ctx, db, []rbac.RoleIdentifier{{Name: roleName, OrganizationID: org.ID}}) require.NoError(t, err) require.Len(t, roles, 1, "role found") } diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go index 3eccd8194f31a..d6a95ccec1b35 100644 --- 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), + Identifier: RoleIdentifier{Name: 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), + Identifier: RoleIdentifier{Name: fmt.Sprintf("Scope_%s", ScopeApplicationConnect)}, DisplayName: "Ability to connect to applications", Site: Permissions(map[string][]policy.Action{ ResourceWorkspace.Type: {policy.ActionApplicationConnect}, @@ -87,7 +87,7 @@ type ExpandableScope interface { Expand() (Scope, error) // Name is for logging and tracing purposes, we want to know the human // name of the scope. - Name() string + Name() RoleIdentifier } type ScopeName string @@ -96,8 +96,8 @@ func (name ScopeName) Expand() (Scope, error) { return ExpandScope(name) } -func (name ScopeName) Name() string { - return string(name) +func (name ScopeName) Name() RoleIdentifier { + return RoleIdentifier{Name: string(name)} } // Scope acts the exact same as a Role with the addition that is can also @@ -114,8 +114,8 @@ func (s Scope) Expand() (Scope, error) { return s, nil } -func (s Scope) Name() string { - return s.Role.Name +func (s Scope) Name() RoleIdentifier { + return s.Role.Identifier } func ExpandScope(scope ScopeName) (Scope, error) { diff --git a/coderd/rbac/subject_test.go b/coderd/rbac/subject_test.go index 330ad7403797b..e2a2f24932c36 100644 --- a/coderd/rbac/subject_test.go +++ b/coderd/rbac/subject_test.go @@ -24,13 +24,13 @@ func TestSubjectEqual(t *testing.T) { Name: "Same", A: rbac.Subject{ ID: "id", - Roles: rbac.RoleNames{rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, Groups: []string{"group"}, Scope: rbac.ScopeAll, }, B: rbac.Subject{ ID: "id", - Roles: rbac.RoleNames{rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, Groups: []string{"group"}, Scope: rbac.ScopeAll, }, @@ -49,7 +49,7 @@ func TestSubjectEqual(t *testing.T) { { Name: "RolesNilVs0", A: rbac.Subject{ - Roles: rbac.RoleNames{}, + Roles: rbac.RoleIdentifiers{}, }, B: rbac.Subject{ Roles: nil, @@ -69,20 +69,20 @@ func TestSubjectEqual(t *testing.T) { { Name: "DifferentRoles", A: rbac.Subject{ - Roles: rbac.RoleNames{rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, }, B: rbac.Subject{ - Roles: rbac.RoleNames{rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleOwner()}, }, Expected: false, }, { Name: "Different#Roles", A: rbac.Subject{ - Roles: rbac.RoleNames{rbac.RoleMember()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, }, B: rbac.Subject{ - Roles: rbac.RoleNames{rbac.RoleMember(), rbac.RoleOwner()}, + Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.RoleOwner()}, }, Expected: false, }, diff --git a/coderd/roles.go b/coderd/roles.go index 1e7f1b1473b9a..f4e66b7a56a50 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -133,12 +133,12 @@ func assignableRoles(actorRoles rbac.ExpandableRoles, roles []rbac.Role, customR // The member role is implied, and not assignable. // If there is no display name, then the role is also unassigned. // This is not the ideal logic, but works for now. - if role.Name == rbac.RoleMember() || (role.DisplayName == "") { + if role.Identifier == rbac.RoleMember() || (role.DisplayName == "") { continue } assignable = append(assignable, codersdk.AssignableRoles{ Role: db2sdk.RBACRole(role), - Assignable: rbac.CanAssignRole(actorRoles, role.Name), + Assignable: rbac.CanAssignRole(actorRoles, role.Identifier), BuiltIn: true, }) } @@ -146,7 +146,7 @@ func assignableRoles(actorRoles rbac.ExpandableRoles, roles []rbac.Role, customR for _, role := range customRoles { assignable = append(assignable, codersdk.AssignableRoles{ Role: db2sdk.Role(role), - Assignable: rbac.CanAssignRole(actorRoles, role.Name), + Assignable: rbac.CanAssignRole(actorRoles, role.RoleIdentifier()), BuiltIn: false, }) } diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 24845fea3fa3d..de9724b4bcb4b 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -51,11 +51,11 @@ func TestListRoles(t *testing.T) { x, err := member.ListSiteRoles(ctx) return x, err }, - ExpectedRoles: convertRoles(map[string]bool{ - "owner": false, - "auditor": false, - "template-admin": false, - "user-admin": false, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOwner}: false, + {Name: codersdk.RoleAuditor}: false, + {Name: codersdk.RoleTemplateAdmin}: false, + {Name: codersdk.RoleUserAdmin}: false, }), }, { @@ -63,8 +63,8 @@ func TestListRoles(t *testing.T) { APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) { return member.ListOrganizationRoles(ctx, owner.OrganizationID) }, - ExpectedRoles: convertRoles(map[string]bool{ - rbac.ScopedRoleOrgAdmin(owner.OrganizationID): false, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOrganizationAdmin, OrganizationID: owner.OrganizationID}: false, }), }, { @@ -80,11 +80,11 @@ func TestListRoles(t *testing.T) { APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) { return orgAdmin.ListSiteRoles(ctx) }, - ExpectedRoles: convertRoles(map[string]bool{ - "owner": false, - "auditor": false, - "template-admin": false, - "user-admin": false, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOwner}: false, + {Name: codersdk.RoleAuditor}: false, + {Name: codersdk.RoleTemplateAdmin}: false, + {Name: codersdk.RoleUserAdmin}: false, }), }, { @@ -92,8 +92,8 @@ func TestListRoles(t *testing.T) { APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) { return orgAdmin.ListOrganizationRoles(ctx, owner.OrganizationID) }, - ExpectedRoles: convertRoles(map[string]bool{ - rbac.ScopedRoleOrgAdmin(owner.OrganizationID): true, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOrganizationAdmin, OrganizationID: owner.OrganizationID}: true, }), }, { @@ -109,11 +109,11 @@ func TestListRoles(t *testing.T) { APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) { return client.ListSiteRoles(ctx) }, - ExpectedRoles: convertRoles(map[string]bool{ - "owner": true, - "auditor": true, - "template-admin": true, - "user-admin": true, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOwner}: true, + {Name: codersdk.RoleAuditor}: true, + {Name: codersdk.RoleTemplateAdmin}: true, + {Name: codersdk.RoleUserAdmin}: true, }), }, { @@ -121,8 +121,8 @@ func TestListRoles(t *testing.T) { APICall: func(ctx context.Context) ([]codersdk.AssignableRoles, error) { return client.ListOrganizationRoles(ctx, owner.OrganizationID) }, - ExpectedRoles: convertRoles(map[string]bool{ - rbac.ScopedRoleOrgAdmin(owner.OrganizationID): true, + ExpectedRoles: convertRoles(map[rbac.RoleIdentifier]bool{ + {Name: codersdk.RoleOrganizationAdmin, OrganizationID: owner.OrganizationID}: true, }), }, } @@ -200,12 +200,12 @@ func TestListCustomRoles(t *testing.T) { }) } -func convertRole(roleName string) codersdk.Role { +func convertRole(roleName rbac.RoleIdentifier) codersdk.Role { role, _ := rbac.RoleByName(roleName) return db2sdk.RBACRole(role) } -func convertRoles(assignableRoles map[string]bool) []codersdk.AssignableRoles { +func convertRoles(assignableRoles map[rbac.RoleIdentifier]bool) []codersdk.AssignableRoles { converted := make([]codersdk.AssignableRoles, 0, len(assignableRoles)) for roleName, assignable := range assignableRoles { role := convertRole(roleName) diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index 45f6de2d8bf8a..a0799991d8f9d 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/codersdk" ) @@ -381,7 +380,7 @@ func TestSearchUsers(t *testing.T) { Expected: database.GetUsersParams{ Search: "user-name", Status: []database.UserStatus{database.UserStatusActive}, - RbacRole: []string{rbac.RoleOwner()}, + RbacRole: []string{codersdk.RoleOwner}, }, }, { @@ -390,7 +389,7 @@ func TestSearchUsers(t *testing.T) { Expected: database.GetUsersParams{ Search: "user name", Status: []database.UserStatus{database.UserStatusSuspended}, - RbacRole: []string{rbac.RoleMember()}, + RbacRole: []string{codersdk.RoleMember}, }, }, { @@ -399,7 +398,7 @@ func TestSearchUsers(t *testing.T) { Expected: database.GetUsersParams{ Search: "user-name", Status: []database.UserStatus{database.UserStatusActive}, - RbacRole: []string{rbac.RoleOwner()}, + RbacRole: []string{codersdk.RoleOwner}, }, }, { diff --git a/coderd/userauth.go b/coderd/userauth.go index b41f496814306..1b9164eedbc13 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -240,9 +240,15 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } + roleNames, err := roles.RoleNames() + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + userSubj := rbac.Subject{ ID: user.ID.String(), - Roles: rbac.RoleNames(roles.Roles), + Roles: rbac.RoleIdentifiers(roleNames), Groups: roles.Groups, Scope: rbac.ScopeAll, } @@ -1531,7 +1537,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C ignored := make([]string, 0) filtered := make([]string, 0, len(params.Roles)) for _, role := range params.Roles { - if _, err := rbac.RoleByName(role); err == nil { + // TODO: This only supports mapping deployment wide roles. Organization scoped roles + // are unsupported. + if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil { filtered = append(filtered, role) } else { ignored = append(ignored, role) diff --git a/coderd/users.go b/coderd/users.go index 8db74cadadc9b..1e375232b48e7 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -223,7 +223,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { // Add the admin role to this first user. //nolint:gocritic // needed to create first user _, err = api.Database.UpdateUserRoles(dbauthz.AsSystemRestricted(ctx), database.UpdateUserRolesParams{ - GrantedRoles: []string{rbac.RoleOwner()}, + GrantedRoles: []string{rbac.RoleOwner().String()}, ID: user.ID, }) if err != nil { @@ -805,7 +805,7 @@ func (api *API) putUserStatus(status database.UserStatus) func(rw http.ResponseW Message: "You cannot suspend yourself.", }) return - case slice.Contains(user.RBACRoles, rbac.RoleOwner()): + case slice.Contains(user.RBACRoles, rbac.RoleOwner().String()): // You may not suspend an owner httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("You cannot suspend a user with the %q role. You must remove the role first.", rbac.RoleOwner()), diff --git a/coderd/users_test.go b/coderd/users_test.go index 0fa42c4578c6d..65a16cef2dedd 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -994,7 +994,7 @@ func TestGrantSiteRoles(t *testing.T) { Name: "UserNotExists", Client: admin, AssignToUser: uuid.NewString(), - Roles: []string{rbac.RoleOwner()}, + Roles: []string{codersdk.RoleOwner}, Error: true, StatusCode: http.StatusBadRequest, }, @@ -1020,7 +1020,7 @@ func TestGrantSiteRoles(t *testing.T) { Client: admin, OrgID: first.OrganizationID, AssignToUser: codersdk.Me, - Roles: []string{rbac.RoleOwner()}, + Roles: []string{codersdk.RoleOwner}, Error: true, StatusCode: http.StatusBadRequest, }, @@ -1057,9 +1057,9 @@ func TestGrantSiteRoles(t *testing.T) { Name: "UserAdminMakeMember", Client: userAdmin, AssignToUser: newUser, - Roles: []string{rbac.RoleMember()}, + Roles: []string{codersdk.RoleMember}, ExpectedRoles: []string{ - rbac.RoleMember(), + codersdk.RoleMember, }, Error: false, }, @@ -1124,7 +1124,7 @@ func TestInitialRoles(t *testing.T) { roles, err := client.UserRoles(ctx, codersdk.Me) require.NoError(t, err) require.ElementsMatch(t, roles.Roles, []string{ - rbac.RoleOwner(), + codersdk.RoleOwner, }, "should be a member and admin") require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{}, "should be a member") @@ -1289,12 +1289,12 @@ func TestUsersFilter(t *testing.T) { users := make([]codersdk.User, 0) users = append(users, firstUser) for i := 0; i < 15; i++ { - roles := []string{} + roles := []rbac.RoleIdentifier{} if i%2 == 0 { roles = append(roles, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()) } if i%3 == 0 { - roles = append(roles, "auditor") + roles = append(roles, rbac.RoleAuditor()) } userClient, userData := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, roles...) // Set the last seen for each user to a unique day @@ -1379,12 +1379,12 @@ func TestUsersFilter(t *testing.T) { { Name: "Admins", Filter: codersdk.UsersRequest{ - Role: rbac.RoleOwner(), + Role: codersdk.RoleOwner, Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive, }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { for _, r := range u.Roles { - if r.Name == rbac.RoleOwner() { + if r.Name == codersdk.RoleOwner { return true } } @@ -1399,7 +1399,7 @@ func TestUsersFilter(t *testing.T) { }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { for _, r := range u.Roles { - if r.Name == rbac.RoleOwner() { + if r.Name == codersdk.RoleOwner { return true } } @@ -1409,7 +1409,7 @@ func TestUsersFilter(t *testing.T) { { Name: "Members", Filter: codersdk.UsersRequest{ - Role: rbac.RoleMember(), + Role: codersdk.RoleMember, Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive, }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { @@ -1423,7 +1423,7 @@ func TestUsersFilter(t *testing.T) { }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { for _, r := range u.Roles { - if r.Name == rbac.RoleOwner() { + if r.Name == codersdk.RoleOwner { return (strings.ContainsAny(u.Username, "iI") || strings.ContainsAny(u.Email, "iI")) && u.Status == codersdk.UserStatusActive } @@ -1438,7 +1438,7 @@ func TestUsersFilter(t *testing.T) { }, FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool { for _, r := range u.Roles { - if r.Name == rbac.RoleOwner() { + if r.Name == codersdk.RoleOwner { return (strings.ContainsAny(u.Username, "iI") || strings.ContainsAny(u.Email, "iI")) && u.Status == codersdk.UserStatusActive } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index ef5b63a1e5b19..e04e585d4aa53 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -555,7 +555,7 @@ func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID u if err != nil { return false, xerrors.New("user does not exist") } - return slices.Contains(user.RBACRoles, rbac.RoleOwner()), nil // only user with "owner" role can cancel workspace builds + return slices.Contains(user.RBACRoles, rbac.RoleOwner().String()), nil // only user with "owner" role can cancel workspace builds } // @Summary Get build parameters for workspace build diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 5d99e56820aa1..389e0563f46f8 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -224,7 +224,7 @@ func TestWorkspaceBuilds(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) first := coderdtest.CreateFirstUser(t, client) - second, secondUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, "owner") + second, secondUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleOwner()) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index d91de4a5e26a1..a20a26d2ab161 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -484,7 +484,7 @@ func TestWorkspacesSortOrder(t *testing.T) { client, db := coderdtest.NewWithDatabase(t, nil) firstUser := coderdtest.CreateFirstUser(t, client) - secondUserClient, secondUser := coderdtest.CreateAnotherUserMutators(t, client, firstUser.OrganizationID, []string{"owner"}, func(r *codersdk.CreateUserRequest) { + secondUserClient, secondUser := coderdtest.CreateAnotherUserMutators(t, client, firstUser.OrganizationID, []rbac.RoleIdentifier{rbac.RoleOwner()}, func(r *codersdk.CreateUserRequest) { r.Username = "zzz" }) diff --git a/coderd/workspacestats/batcher_internal_test.go b/coderd/workspacestats/batcher_internal_test.go index 0e797986555e5..97fdaf9f2aec5 100644 --- a/coderd/workspacestats/batcher_internal_test.go +++ b/coderd/workspacestats/batcher_internal_test.go @@ -9,6 +9,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/codersdk" agentproto "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/coderd/database" @@ -16,7 +17,6 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/cryptorand" ) @@ -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.ScopedRoleOrgMember(org.ID)}, + Roles: []string{codersdk.RoleOrganizationMember}, }) require.NoError(t, err) tv := dbgen.TemplateVersion(t, store, database.TemplateVersion{ diff --git a/codersdk/rbacroles.go b/codersdk/rbacroles.go new file mode 100644 index 0000000000000..fe90d98f77384 --- /dev/null +++ b/codersdk/rbacroles.go @@ -0,0 +1,13 @@ +package codersdk + +// Ideally this roles would be generated from the rbac/roles.go package. +const ( + RoleOwner string = "owner" + RoleMember string = "member" + RoleTemplateAdmin string = "template-admin" + RoleUserAdmin string = "user-admin" + RoleAuditor string = "auditor" + + RoleOrganizationAdmin string = "organization-admin" + RoleOrganizationMember string = "organization-member" +) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 26fdab6ec1bfb..743bd628d8630 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -496,7 +496,7 @@ func (api *API) writeEntitlementWarningsHeader(a rbac.Subject, header http.Heade // The member role is implied, and not assignable. // If there is no display name, then the role is also unassigned. // This is not the ideal logic, but works for now. - if role.Name == rbac.RoleMember() || (role.DisplayName == "") { + if role.Identifier == rbac.RoleMember() || (role.DisplayName == "") { continue } nonMemberRoles++ diff --git a/enterprise/coderd/coderd_test.go b/enterprise/coderd/coderd_test.go index d881a21e49423..5183a1d4f6a21 100644 --- a/enterprise/coderd/coderd_test.go +++ b/enterprise/coderd/coderd_test.go @@ -497,7 +497,7 @@ func testDBAuthzRole(ctx context.Context) context.Context { ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "testing", + Identifier: rbac.RoleIdentifier{Name: "testing"}, DisplayName: "Unit Tests", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceWildcard.Type: {policy.WildcardSymbol}, diff --git a/enterprise/coderd/insights_test.go b/enterprise/coderd/insights_test.go index c2d97fea913e3..044c5988eb036 100644 --- a/enterprise/coderd/insights_test.go +++ b/enterprise/coderd/insights_test.go @@ -78,15 +78,15 @@ func TestTemplateInsightsWithRole(t *testing.T) { type test struct { interval codersdk.InsightsReportInterval - role string + role rbac.RoleIdentifier allowed bool } tests := []test{ {codersdk.InsightsReportIntervalDay, rbac.RoleTemplateAdmin(), true}, {"", rbac.RoleTemplateAdmin(), true}, - {codersdk.InsightsReportIntervalDay, "auditor", true}, - {"", "auditor", true}, + {codersdk.InsightsReportIntervalDay, rbac.RoleAuditor(), true}, + {"", rbac.RoleAuditor(), true}, {codersdk.InsightsReportIntervalDay, rbac.RoleUserAdmin(), false}, {"", rbac.RoleUserAdmin(), false}, {codersdk.InsightsReportIntervalDay, rbac.RoleMember(), false}, diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go index e1d6855aff002..239a055540075 100644 --- a/enterprise/coderd/roles_test.go +++ b/enterprise/coderd/roles_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" @@ -57,7 +58,7 @@ func TestCustomOrganizationRole(t *testing.T) { require.NoError(t, err, "upsert role") // Assign the custom template admin role - tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, role.FullName()) + tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{Name: role.Name, OrganizationID: first.OrganizationID}) // Assert the role exists // TODO: At present user roles are not returned by the user endpoints. @@ -124,7 +125,7 @@ func TestCustomOrganizationRole(t *testing.T) { require.ErrorContains(t, err, "roles are not enabled") // Assign the custom template admin role - tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, role.FullName()) + tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{Name: role.Name, OrganizationID: first.OrganizationID}) // Try to create a template version, eg using the custom role coderdtest.CreateTemplateVersion(t, tmplAdmin, first.OrganizationID, nil) @@ -152,7 +153,7 @@ func TestCustomOrganizationRole(t *testing.T) { require.NoError(t, err, "upsert role") // Assign the custom template admin role - tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, role.FullName()) + tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{Name: role.Name, OrganizationID: first.OrganizationID}) // Try to create a template version, eg using the custom role coderdtest.CreateTemplateVersion(t, tmplAdmin, first.OrganizationID, nil) diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 36e0d69cbd622..f30474d607319 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -66,7 +66,7 @@ func TestUserOIDC(t *testing.T) { cfg.AllowSignups = true cfg.UserRoleField = "roles" cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin()}, + oidcRoleName: {rbac.RoleTemplateAdmin().String()}, } }, }) @@ -79,7 +79,7 @@ func TestUserOIDC(t *testing.T) { "roles": oidcRoleName, }) require.Equal(t, http.StatusOK, resp.StatusCode) - runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin()}) + runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String()}) }) // A user has some roles, then on an oauth refresh will lose said @@ -92,12 +92,12 @@ func TestUserOIDC(t *testing.T) { const oidcRoleName = "TemplateAuthor" runner := setupOIDCTest(t, oidcTestConfig{ - Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}}, + Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}}, Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true cfg.UserRoleField = "roles" cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}, + oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, } }, }) @@ -105,10 +105,10 @@ func TestUserOIDC(t *testing.T) { // User starts with the owner role client, resp := runner.Login(t, jwt.MapClaims{ "email": "alice@coder.com", - "roles": []string{"random", oidcRoleName, rbac.RoleOwner()}, + "roles": []string{"random", oidcRoleName, rbac.RoleOwner().String()}, }) require.Equal(t, http.StatusOK, resp.StatusCode) - runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), rbac.RoleOwner()}) + runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String(), rbac.RoleOwner().String()}) // Now refresh the oauth, and check the roles are removed. // Force a refresh, and assert nothing has changes @@ -126,12 +126,12 @@ func TestUserOIDC(t *testing.T) { const oidcRoleName = "TemplateAuthor" runner := setupOIDCTest(t, oidcTestConfig{ - Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}}, + Userinfo: jwt.MapClaims{oidcRoleName: []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}}, Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true cfg.UserRoleField = "roles" cfg.UserRoleMapping = map[string][]string{ - oidcRoleName: {rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}, + oidcRoleName: {rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String()}, } }, }) @@ -139,10 +139,10 @@ func TestUserOIDC(t *testing.T) { // User starts with the owner role _, resp := runner.Login(t, jwt.MapClaims{ "email": "alice@coder.com", - "roles": []string{"random", oidcRoleName, rbac.RoleOwner()}, + "roles": []string{"random", oidcRoleName, rbac.RoleOwner().String()}, }) require.Equal(t, http.StatusOK, resp.StatusCode) - runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), rbac.RoleOwner()}) + runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String(), rbac.RoleUserAdmin().String(), rbac.RoleOwner().String()}) // Now login with oauth again, and check the roles are removed. _, resp = runner.Login(t, jwt.MapClaims{ @@ -175,7 +175,7 @@ func TestUserOIDC(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) _, err := runner.AdminClient.UpdateUserRoles(ctx, "alice", codersdk.UpdateRoles{ Roles: []string{ - rbac.RoleTemplateAdmin(), + rbac.RoleTemplateAdmin().String(), }, }) require.Error(t, err) diff --git a/enterprise/tailnet/pgcoord.go b/enterprise/tailnet/pgcoord.go index 104a649d87839..6bb21d2931689 100644 --- a/enterprise/tailnet/pgcoord.go +++ b/enterprise/tailnet/pgcoord.go @@ -101,7 +101,7 @@ var pgCoordSubject = rbac.Subject{ ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ { - Name: "tailnetcoordinator", + Identifier: rbac.RoleIdentifier{Name: "tailnetcoordinator"}, DisplayName: "Tailnet Coordinator", Site: rbac.Permissions(map[string][]policy.Action{ rbac.ResourceTailnetCoordinator.Type: {policy.WildcardSymbol},