Skip to content

Commit 5ccf508

Browse files
EmyrkEmyrk
authored
chore: create type for unique role names (coder#13506)
* chore: create type for unique role names Using `string` was confusing when something should be combined with org context, and when not to. Naming this new name, "RoleIdentifier"
1 parent c9cca9d commit 5ccf508

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

50 files changed

+551
-456
lines changed

‎cli/server_createadminuser.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command {
192192
HashedPassword: []byte(hashedPassword),
193193
CreatedAt: dbtime.Now(),
194194
UpdatedAt: dbtime.Now(),
195-
RBACRoles: []string{rbac.RoleOwner()},
195+
RBACRoles: []string{rbac.RoleOwner().String()},
196196
LoginType: database.LoginTypePassword,
197197
})
198198
if err != nil {
@@ -222,7 +222,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command {
222222
UserID: newUser.ID,
223223
CreatedAt: dbtime.Now(),
224224
UpdatedAt: dbtime.Now(),
225-
Roles: []string{rbac.ScopedRoleOrgAdmin(org.ID)},
225+
Roles: []string{rbac.RoleOrgAdmin()},
226226
})
227227
if err != nil {
228228
return xerrors.Errorf("insert organization member: %w", err)

‎cli/server_createadminuser_test.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/coder/coder/v2/coderd/database/dbtime"
1818
"github.com/coder/coder/v2/coderd/rbac"
1919
"github.com/coder/coder/v2/coderd/userpassword"
20+
"github.com/coder/coder/v2/codersdk"
2021
"github.com/coder/coder/v2/pty/ptytest"
2122
"github.com/coder/coder/v2/testutil"
2223
)
@@ -56,7 +57,7 @@ func TestServerCreateAdminUser(t *testing.T) {
5657
require.NoError(t, err)
5758
require.True(t, ok, "password does not match")
5859

59-
require.EqualValues(t, []string{rbac.RoleOwner()}, user.RBACRoles, "user does not have owner role")
60+
require.EqualValues(t, []string{codersdk.RoleOwner}, user.RBACRoles, "user does not have owner role")
6061

6162
// Check that user is admin in every org.
6263
orgs, err := db.GetOrganizations(ctx)
@@ -71,7 +72,7 @@ func TestServerCreateAdminUser(t *testing.T) {
7172
orgIDs2 := make(map[uuid.UUID]struct{}, len(orgMemberships))
7273
for _, membership := range orgMemberships {
7374
orgIDs2[membership.OrganizationID] = struct{}{}
74-
assert.Equal(t, []string{rbac.ScopedRoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin")
75+
assert.Equal(t, []string{rbac.RoleOrgAdmin()}, membership.Roles, "user is not org admin")
7576
}
7677

7778
require.Equal(t, orgIDs, orgIDs2, "user is not in all orgs")

‎coderd/audit.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,8 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs
199199
Roles: []codersdk.SlimRole{},
200200
}
201201

202-
for _, roleName := range dblog.UserRoles {
202+
for _, input := range dblog.UserRoles {
203+
roleName, _ := rbac.RoleNameFromString(input)
203204
rbacRole, _ := rbac.RoleByName(roleName)
204205
user.Roles = append(user.Roles, db2sdk.SlimRole(rbacRole))
205206
}

‎coderd/coderdtest/authorize.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,13 @@ func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsse
6060
roles, err := api.Database.GetAuthorizationUserRoles(ctx, key.UserID)
6161
require.NoError(t, err, "fetch user roles")
6262

63+
roleNames, err := roles.RoleNames()
64+
require.NoError(t, err)
65+
6366
return RBACAsserter{
6467
Subject: rbac.Subject{
6568
ID: key.UserID.String(),
66-
Roles: rbac.RoleNames(roles.Roles),
69+
Roles: rbac.RoleIdentifiers(roleNames),
6770
Groups: roles.Groups,
6871
Scope: rbac.ScopeName(key.Scope),
6972
},
@@ -435,7 +438,7 @@ func randomRBACType() string {
435438
func RandomRBACSubject() rbac.Subject {
436439
return rbac.Subject{
437440
ID: uuid.NewString(),
438-
Roles: rbac.RoleNames{rbac.RoleMember()},
441+
Roles: rbac.RoleIdentifiers{rbac.RoleMember()},
439442
Groups: []string{namesgenerator.GetRandomName(1)},
440443
Scope: rbac.ScopeAll,
441444
}

‎coderd/coderdtest/coderdtest.go

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import (
5555
"github.com/coder/coder/v2/coderd/autobuild"
5656
"github.com/coder/coder/v2/coderd/awsidentity"
5757
"github.com/coder/coder/v2/coderd/database"
58+
"github.com/coder/coder/v2/coderd/database/db2sdk"
5859
"github.com/coder/coder/v2/coderd/database/dbauthz"
5960
"github.com/coder/coder/v2/coderd/database/dbrollup"
6061
"github.com/coder/coder/v2/coderd/database/dbtestutil"
@@ -663,21 +664,25 @@ func CreateFirstUser(t testing.TB, client *codersdk.Client) codersdk.CreateFirst
663664

664665
// CreateAnotherUser creates and authenticates a new user.
665666
// Roles can include org scoped roles with 'roleName:<organization_id>'
666-
func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) {
667+
func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...rbac.RoleIdentifier) (*codersdk.Client, codersdk.User) {
667668
return createAnotherUserRetry(t, client, organizationID, 5, roles)
668669
}
669670

670-
func CreateAnotherUserMutators(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles []string, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) {
671+
func CreateAnotherUserMutators(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles []rbac.RoleIdentifier, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) {
671672
return createAnotherUserRetry(t, client, organizationID, 5, roles, mutators...)
672673
}
673674

674675
// AuthzUserSubject does not include the user's groups.
675676
func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject {
676-
roles := make(rbac.RoleNames, 0, len(user.Roles))
677+
roles := make(rbac.RoleIdentifiers, 0, len(user.Roles))
677678
// Member role is always implied
678679
roles = append(roles, rbac.RoleMember())
679680
for _, r := range user.Roles {
680-
roles = append(roles, r.Name)
681+
orgID, _ := uuid.Parse(r.OrganizationID) // defaults to nil
682+
roles = append(roles, rbac.RoleIdentifier{
683+
Name: r.Name,
684+
OrganizationID: orgID,
685+
})
681686
}
682687
// We assume only 1 org exists
683688
roles = append(roles, rbac.ScopedRoleOrgMember(orgID))
@@ -690,7 +695,7 @@ func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject {
690695
}
691696
}
692697

693-
func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, retries int, roles []string, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) {
698+
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) {
694699
req := codersdk.CreateUserRequest{
695700
Email: namesgenerator.GetRandomName(10) + "@coder.com",
696701
Username: RandomUsername(t),
@@ -748,36 +753,37 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI
748753

749754
if len(roles) > 0 {
750755
// Find the roles for the org vs the site wide roles
751-
orgRoles := make(map[string][]string)
752-
var siteRoles []string
756+
orgRoles := make(map[uuid.UUID][]rbac.RoleIdentifier)
757+
var siteRoles []rbac.RoleIdentifier
753758

754759
for _, roleName := range roles {
755-
roleName := roleName
756-
orgID, ok := rbac.IsOrgRole(roleName)
757-
roleName, _, err = rbac.RoleSplit(roleName)
758-
require.NoError(t, err, "split org role name")
760+
ok := roleName.IsOrgRole()
759761
if ok {
760-
roleName, _, err = rbac.RoleSplit(roleName)
761-
require.NoError(t, err, "split rolename")
762-
orgRoles[orgID] = append(orgRoles[orgID], roleName)
762+
orgRoles[roleName.OrganizationID] = append(orgRoles[roleName.OrganizationID], roleName)
763763
} else {
764764
siteRoles = append(siteRoles, roleName)
765765
}
766766
}
767767
// Update the roles
768768
for _, r := range user.Roles {
769-
siteRoles = append(siteRoles, r.Name)
769+
orgID, _ := uuid.Parse(r.OrganizationID)
770+
siteRoles = append(siteRoles, rbac.RoleIdentifier{
771+
Name: r.Name,
772+
OrganizationID: orgID,
773+
})
774+
}
775+
776+
onlyName := func(role rbac.RoleIdentifier) string {
777+
return role.Name
770778
}
771779

772-
user, err = client.UpdateUserRoles(context.Background(), user.ID.String(), codersdk.UpdateRoles{Roles: siteRoles})
780+
user, err = client.UpdateUserRoles(context.Background(), user.ID.String(), codersdk.UpdateRoles{Roles: db2sdk.List(siteRoles, onlyName)})
773781
require.NoError(t, err, "update site roles")
774782

775783
// Update org roles
776784
for orgID, roles := range orgRoles {
777-
organizationID, err := uuid.Parse(orgID)
778-
require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID))
779-
_, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
780-
codersdk.UpdateRoles{Roles: roles})
785+
_, err = client.UpdateOrganizationMemberRoles(context.Background(), orgID, user.ID.String(),
786+
codersdk.UpdateRoles{Roles: db2sdk.List(roles, onlyName)})
781787
require.NoError(t, err, "update org membership roles")
782788
}
783789
}

‎coderd/database/db2sdk/db2sdk.go

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,12 @@ func User(user database.User, organizationIDs []uuid.UUID) codersdk.User {
170170
}
171171

172172
for _, roleName := range user.RBACRoles {
173-
rbacRole, err := rbac.RoleByName(roleName)
173+
// TODO: Currently the api only returns site wide roles.
174+
// Should it return organization roles?
175+
rbacRole, err := rbac.RoleByName(rbac.RoleIdentifier{
176+
Name: roleName,
177+
OrganizationID: uuid.Nil,
178+
})
174179
if err == nil {
175180
convertedUser.Roles = append(convertedUser.Roles, SlimRole(rbacRole))
176181
} else {
@@ -519,29 +524,26 @@ func ProvisionerDaemon(dbDaemon database.ProvisionerDaemon) codersdk.Provisioner
519524
}
520525

521526
func SlimRole(role rbac.Role) codersdk.SlimRole {
522-
roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
523-
if err != nil {
524-
roleName = role.Name
527+
orgID := ""
528+
if role.Identifier.OrganizationID != uuid.Nil {
529+
orgID = role.Identifier.OrganizationID.String()
525530
}
526531

527532
return codersdk.SlimRole{
528533
DisplayName: role.DisplayName,
529-
Name: roleName,
530-
OrganizationID: orgIDStr,
534+
Name: role.Identifier.Name,
535+
OrganizationID: orgID,
531536
}
532537
}
533538

534539
func RBACRole(role rbac.Role) codersdk.Role {
535-
roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
536-
if err != nil {
537-
roleName = role.Name
538-
}
539-
orgPerms := role.Org[orgIDStr]
540+
slim := SlimRole(role)
540541

542+
orgPerms := role.Org[slim.OrganizationID]
541543
return codersdk.Role{
542-
Name: roleName,
543-
OrganizationID: orgIDStr,
544-
DisplayName: role.DisplayName,
544+
Name: slim.Name,
545+
OrganizationID: slim.OrganizationID,
546+
DisplayName: slim.DisplayName,
545547
SitePermissions: List(role.Site, RBACPermission),
546548
OrganizationPermissions: List(orgPerms, RBACPermission),
547549
UserPermissions: List(role.User, RBACPermission),

‎coderd/database/dbauthz/customroles_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func TestUpsertCustomRoles(t *testing.T) {
3535
}
3636

3737
canAssignRole := rbac.Role{
38-
Name: "can-assign",
38+
Identifier: rbac.RoleIdentifier{Name: "can-assign"},
3939
DisplayName: "",
4040
Site: rbac.Permissions(map[string][]policy.Action{
4141
rbac.ResourceAssignRole.Type: {policy.ActionRead, policy.ActionCreate},
@@ -51,7 +51,7 @@ func TestUpsertCustomRoles(t *testing.T) {
5151
all = append(all, t)
5252
case rbac.ExpandableRoles:
5353
all = append(all, must(t.Expand())...)
54-
case string:
54+
case rbac.RoleIdentifier:
5555
all = append(all, must(rbac.RoleByName(t)))
5656
default:
5757
panic("unknown type")
@@ -80,7 +80,7 @@ func TestUpsertCustomRoles(t *testing.T) {
8080
{
8181
// No roles, so no assign role
8282
name: "no-roles",
83-
subject: rbac.RoleNames([]string{}),
83+
subject: rbac.RoleIdentifiers{},
8484
errorContains: "forbidden",
8585
},
8686
{

0 commit comments

Comments
 (0)