Skip to content

chore: create type for unique role names #13506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jun 11, 2024
Merged

chore: create type for unique role names #13506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4f18d66
chore: create type for unique role names
Emyrk Jun 7, 2024
fc8d414
WIP
Emyrk Jun 7, 2024
7be7755
Work on switching the string to a struct
Emyrk Jun 7, 2024
1bc2cdf
Continue to fix all compile issues
Emyrk Jun 7, 2024
d88c836
Compiling
Emyrk Jun 7, 2024
175a03f
fix assign
Emyrk Jun 7, 2024
3f7e2cb
fix unit test
Emyrk Jun 7, 2024
056fc17
Add codersdk roles
Emyrk Jun 7, 2024
7651a18
refactor name to RoleIdentifier
Emyrk Jun 10, 2024
dadd284
more fixups
Emyrk Jun 10, 2024
8988224
more test fixes
Emyrk Jun 10, 2024
521ffcd
fixup! more test fixes
Emyrk Jun 10, 2024
dce7f09
linting
Emyrk Jun 10, 2024
cc68903
fixup test
Emyrk Jun 10, 2024
fa2f704
linting
Emyrk Jun 10, 2024
9bf3298
add comment
Emyrk Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/server_createadminuser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: string does not implement fmt.Stringer :(

LoginType: database.LoginTypePassword,
})
if err != nil {
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions cli/server_createadminuser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand All @@ -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")
Expand Down
3 changes: 2 additions & 1 deletion coderd/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we at least drop an error log if we were unable to covert any of the roles rom the db?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in the convert audit log, I think that could get quite noisy. I agree this is not ideal

rbacRole, _ := rbac.RoleByName(roleName)
user.Roles = append(user.Roles, db2sdk.SlimRole(rbacRole))
}
Expand Down
7 changes: 5 additions & 2 deletions coderd/coderdtest/authorize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
Expand Down Expand Up @@ -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,
}
Expand Down
46 changes: 26 additions & 20 deletions coderd/coderdtest/coderdtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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:<organization_id>'
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))
Expand All @@ -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),
Expand Down Expand Up @@ -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")
}
}
Expand Down
30 changes: 16 additions & 14 deletions coderd/database/db2sdk/db2sdk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Comment on lines +173 to +178
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow-up for TODO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to do this until we get farther in the organization work.

Right now this api is used for the /users page on the UI. If I were to add it, then organization roles would pop up there, which is probably incorrect behavior.

So this is a genuine question if we should expand the roles returned, and on the UI filter out the organization ones. But at present, showing org roles at all is incorrect as orgs do not really exist.

if err == nil {
convertedUser.Roles = append(convertedUser.Roles, SlimRole(rbacRole))
} else {
Expand Down Expand Up @@ -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),
Expand Down
6 changes: 3 additions & 3 deletions coderd/database/dbauthz/customroles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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")
Expand Down Expand Up @@ -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",
},
{
Expand Down
Loading
Loading