8000 chore: create type for unique role names by Emyrk · Pull Request #13506 · coder/coder · GitHub
[go: up one dir, main page]

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.

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
Prev Previous commit
Next Next commit
Continue to fix all compile issues
  • Loading branch information
Emyrk committed Jun 10, 2024
commit 1bc2cdfa67b5bf9e454dfd6941edae3d38b758e6
3 changes: 2 additions & 1 deletion coderd/audit.go
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
6 changes: 3 additions & 3 deletions coderd/coderdtest/coderdtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -663,11 +663,11 @@ 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.RoleName) (*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.RoleName, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) {
return createAnotherUserRetry(t, client, organizationID, 5, roles, mutators...)
}

Expand All @@ -690,7 +690,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.RoleName, mutators ...func(r *codersdk.CreateUserRequest)) (*codersdk.Client, codersdk.User) {
req := codersdk.CreateUserRequest{
Email: namesgenerator.GetRandomName(10) + "@coder.com",
Username: RandomUsername(t),
Expand Down
30 changes: 16 additions & 14 deletions coderd/database/db2sdk/db2sdk.go
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.RoleName{
Name: roleName,
OrganizationID: uuid.Nil,
})
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.Name.OrganizationID != uuid.Nil {
orgID = role.Name.OrganizationID.String()
}

return codersdk.SlimRole{
DisplayName: role.DisplayName,
Name: roleName,
OrganizationID: orgIDStr,
Name: role.Name.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
77 changes: 43 additions & 34 deletions coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ var (
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "provisionerd",
Name: rbac.RoleName{Name: "provisionerd"},
DisplayName: "Provisioner Daemon",
Site: rbac.Permissions(map[string][]policy.Action{
// TODO: Add ProvisionerJob resource type.
Expand Down Expand Up @@ -191,7 +191,7 @@ var (
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "autostart",
Name: rbac.RoleName{Name: "autostart"},
DisplayName: "Autostart Daemon",
Site: rbac.Permissions(map[string][]policy.Action{
rbac.ResourceSystem.Type: {policy.WildcardSymbol},
Expand All @@ -213,7 +213,7 @@ var (
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "hangdetector",
Name: rbac.RoleName{Name: "hangdetector"},
DisplayName: "Hang Detector Daemon",
Site: rbac.Permissions(map[string][]policy.Action{
rbac.ResourceSystem.Type: {policy.WildcardSymbol},
Expand All @@ -232,7 +232,7 @@ var (
ID: uuid.Nil.String(),
Roles: rbac.Roles([]rbac.Role{
{
Name: "system",
Name: rbac.RoleName{Name: "system"},
DisplayName: "Coder",
Site: rbac.Permissions(map[string][]policy.Action{
rbac.ResourceWildcard.Type: {policy.ActionRead},
Expand Down Expand Up @@ -582,24 +582,33 @@ func (q *querier) authorizeUpdateFileTemplate(ctx context.Context, file database
}
}

// uniqueOrganizationRoles converts a set of scoped role names to their unique
// convertToOrganizationRoles converts a set of scoped role names to their unique
// scoped names.
func (q *querier) uniqueOrganizationRoles(organizationID uuid.UUID, names []string) ([]rbac.RoleName, error) {
func (q *querier) convertToOrganizationRoles(organizationID uuid.UUID, names []string) ([]rbac.RoleName, error) {
uniques := make([]rbac.RoleName, 0, len(names))
for _, name := range names {
// This check is a developer safety check. Old code might try to invoke this code path with
// organization id suffixes. Catch this and return a nice error so it can be fixed.
_, foundOrg, _ := rbac.RoleSplit(rbac.RoleName(name))
if foundOrg != "" {
if strings.Contains(name, ":") {
return nil, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", name)
}

uniques = append(uniques, rbac.RoleName(name, organizationID.String()))
uniques = append(uniques, rbac.RoleName{Name: name, OrganizationID: organizationID})
}

return uniques, nil
}

// convertToDeploymentRoles converts string role names into deployment wide roles.
func (q *querier) convertToDeploymentRoles(names []string) []rbac.RoleName {
uniques := make([]rbac.RoleName, 0, len(names))
for _, name := range names {
uniques = append(uniques, rbac.RoleName{Name: name})
}

return uniques
}

// canAssignRoles handles assigning built in and custom roles.
func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, removed []rbac.RoleName) error {
actor, ok := ActorFromContext(ctx)
Expand All @@ -618,25 +627,21 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
customRoles := make([]rbac.RoleName, 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())
}
}
Expand Down Expand Up @@ -667,7 +672,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.Name.Name, role.Name) && customRole.Name.OrganizationID == role.OrganizationID
}) {
return xerrors.Errorf("%q is not a supported role", role)
}
Expand Down Expand Up @@ -2489,9 +2494,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.RoleName{})
if err != nil {
return database.OrganizationMember{}, err
}
Expand Down Expand Up @@ -2577,8 +2587,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.RoleName{rbac.RoleMember()}, q.conver 10000 tToDeploymentRoles(arg.RBACRoles)...)
err := q.canAssignRoles(ctx, nil, impliedRoles, []rbac.RoleName{})
if err != nil {
return database.User{}, err
}
Expand Down Expand Up @@ -2865,23 +2875,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([]rbac.RoleName, 0, len(arg.GrantedRoles))
for _, grantedRole := range arg.GrantedRoles {
// This check is a developer safety check. Old code might try to invoke this code path with
// organization id suffixes. Catch this and return a nice error so it can be fixed.
_, foundOrg, _ := rbac.RoleSplit(grantedRole)
if foundOrg != "" {
return database.OrganizationMember{}, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", grantedRole)
}

scopedGranted = append(scopedGranted, rbac.RoleName(grantedRole, arg.OrgID.String()))
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
Expand Down Expand Up @@ -3222,9 +3231,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
Expand Down
2 changes: 1 addition & 1 deletion coderd/database/dbmem/dbmem.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
22 changes: 17 additions & 5 deletions coderd/database/modelmethods.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import (
"strconv"
"time"

"github.com/google/uuid"
"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"
Expand Down Expand Up @@ -375,9 +375,21 @@ func (p ProvisionerJob) FinishedAt() time.Time {
return time.Time{}
}

func (r CustomRole) UniqueName() rbac.RoleName {
if r.OrganizationID.UUID == uuid.Nil {
return rbac.RoleName(r.Name, "")
func (r CustomRole) RoleName() rbac.RoleName {
return rbac.RoleName{
Name: r.Name,
OrganizationID: r.OrganizationID.UUID,
}
return rbac.RoleName(r.Name, r.OrganizationID.UUID.String())
}

func (r GetAuthorizationUserRolesRow) RoleNames() ([]rbac.RoleName, error) {
names := make([]rbac.RoleName, 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
}
10 changes: 9 additions & 1 deletion coderd/httpmw/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 10 additions & 1 deletion coderd/httpmw/workspaceagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.RoleNames(roleNames),
Groups: roles.Groups,
Scope: rbac.WorkspaceAgentScope(rbac.WorkspaceAgentScopeParams{
WorkspaceID: row.Workspace.ID,
Expand Down
16 changes: 14 additions & 2 deletions coderd/identityprovider/tokens.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.RoleNames(roleNames),
Groups: roles.Groups,
Scope: rbac.ScopeAll,
}
Expand Down Expand Up @@ -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.RoleNames(roleNames),
Groups: roles.Groups,
Scope: rbac.ScopeAll,
}
Expand Down
Loading
0