Skip to content

chore: Authz should support non-named roles #5855

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 2 commits into from
Jan 25, 2023
Merged

chore: Authz should support non-named roles #5855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
425cec0
chore: Authz should support non-named roles
Emyrk Jan 25, 2023
17ea3c3
Update comments
Emyrk Jan 25, 2023
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
10 changes: 5 additions & 5 deletions coderd/coderdtest/authorize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck

type authCall struct {
SubjectID string
Roles []string
Roles rbac.ExpandableRoles
Groups []string
Scope rbac.ScopeName
Action rbac.Action
Expand All @@ -545,11 +545,11 @@ var _ rbac.Authorizer = (*RecordingAuthorizer)(nil)

// ByRoleNameSQL does not record the call. This matches the postgres behavior
// of not calling Authorize()
func (r *RecordingAuthorizer) ByRoleNameSQL(_ context.Context, _ string, _ []string, _ rbac.ScopeName, _ []string, _ rbac.Action, _ rbac.Object) error {
func (r *RecordingAuthorizer) ByRoleNameSQL(_ context.Context, _ string, _ rbac.ExpandableRoles, _ rbac.ScopeName, _ []string, _ rbac.Action, _ rbac.Object) error {
return r.AlwaysReturn
}

func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, scope rbac.ScopeName, groups []string, action rbac.Action, object rbac.Object) error {
func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames rbac.ExpandableRoles, scope rbac.ScopeName, groups []string, action rbac.Action, object rbac.Object) error {
r.Called = &authCall{
SubjectID: subjectID,
Roles: roleNames,
Expand All @@ -561,7 +561,7 @@ func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, ro
return r.AlwaysReturn
}

func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, scope rbac.ScopeName, groups []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles rbac.ExpandableRoles, scope rbac.ScopeName, groups []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
return &fakePreparedAuthorizer{
Original: r,
SubjectID: subjectID,
Expand All @@ -580,7 +580,7 @@ func (r *RecordingAuthorizer) reset() {
type fakePreparedAuthorizer struct {
Original *RecordingAuthorizer
SubjectID string
Roles []string
Roles rbac.ExpandableRoles
Scope rbac.ScopeName
Action rbac.Action
Groups []string
Expand Down
3 changes: 2 additions & 1 deletion coderd/httpmw/apikey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/rbac"
"github.com/coder/coder/codersdk"
)

Expand Down Expand Up @@ -53,7 +54,7 @@ type userAuthKey struct{}
type Authorization struct {
ID uuid.UUID
Username string
Roles []string
Roles rbac.RoleNames
Groups []string
Scope database.APIKeyScope
}
Expand Down
35 changes: 25 additions & 10 deletions coderd/rbac/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,24 @@ import (
"github.com/coder/coder/coderd/tracing"
)

// 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
// 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.
//
// Note: We may also want to do the same thing with scopes to allow custom scope
// support unavailable to the user. Eg: Scope to a single resource.
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
}

type Authorizer interface {
ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, object Object) error
PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error)
ByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, object Object) error
PrepareByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error)
}

type PreparedAuthorized interface {
Expand All @@ -33,7 +48,7 @@ type PreparedAuthorized interface {
//
// Ideally the 'CompileToSQL' is used instead for large sets. This cost scales
// linearly with the number of objects passed in.
func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, scope ScopeName, groups []string, action Action, objects []O) ([]O, error) {
func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles ExpandableRoles, scope ScopeName, groups []string, action Action, objects []O) ([]O, error) {
if len(objects) == 0 {
// Nothing to filter
return objects, nil
Expand All @@ -45,7 +60,7 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub
// objects, then the span is not interesting. It would just add excessive
// 0 time spans that provide no insight.
ctx, span := tracing.StartSpan(ctx,
rbacTraceAttributes(subjRoles, len(groups), scope, action, objectType,
rbacTraceAttributes(subjRoles.Names(), len(groups), scope, action, objectType,
// For filtering, we are only measuring the total time for the entire
// set of objects. This and the 'PrepareByRoleName' span time
// is all that is required to measure the performance of this
Expand Down Expand Up @@ -179,11 +194,11 @@ type authSubject struct {
// ByRoleName will expand all roleNames into roles before calling Authorize().
// This is the function intended to be used outside this package.
// The role is fetched from the builtin map located in memory.
func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, object Object) error {
func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, object Object) error {
start := time.Now()
ctx, span := tracing.StartSpan(ctx,
trace.WithTimestamp(start), // Reuse the time.Now for metric and trace
rbacTraceAttributes(roleNames, len(groups), scope, action, object.Type,
rbacTraceAttributes(roleNames.Names(), len(groups), scope, action, object.Type,
// For authorizing a single object, this data is useful to know how
// complex our objects are getting.
attribute.Int("object_num_groups", len(object.ACLGroupList)),
Expand All @@ -192,7 +207,7 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa
)
defer span.End()

roles, err := RolesByNames(roleNames)
roles, err := roleNames.Expand()
if err != nil {
return err
}
Expand Down Expand Up @@ -239,15 +254,15 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [
return nil
}

func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) {
func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) {
start := time.Now()
ctx, span := tracing.StartSpan(ctx,
trace.WithTimestamp(start),
rbacTraceAttributes(roleNames, len(groups), scope, action, objectType),
rbacTraceAttributes(roleNames.Names(), len(groups), scope, action, objectType),
)
defer span.End()

roles, err := RolesByNames(roleNames)
roles, err := roleNames.Expand()
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions coderd/rbac/authz_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestFilterError(t *testing.T) {
t.Parallel()
auth := NewAuthorizer(prometheus.NewRegistry())

_, err := Filter(context.Background(), auth, uuid.NewString(), []string{}, ScopeAll, []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace})
_, err := Filter(context.Background(), auth, uuid.NewString(), RoleNames{}, ScopeAll, []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace})
require.ErrorContains(t, err, "object types must be uniform")
}

Expand Down Expand Up @@ -75,7 +75,7 @@ func TestFilter(t *testing.T) {
testCases := []struct {
Name string
SubjectID string
Roles []string
Roles RoleNames
Action Action
Scope ScopeName
ObjectType string
Expand Down
2 changes: 1 addition & 1 deletion coderd/rbac/authz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (

type benchmarkCase struct {
Name string
Roles []string
Roles rbac.RoleNames
Groups []string
UserID uuid.UUID
Scope rbac.ScopeName
Expand Down
19 changes: 18 additions & 1 deletion coderd/rbac/builtin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ const (
orgMember string = "organization-member"
)

// RoleNames 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

func (names RoleNames) Expand() ([]Role, error) {
return rolesByNames(names)
}

func (names RoleNames) Names() []string {
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
Expand Down Expand Up @@ -244,6 +257,10 @@ func CanAssignRole(roles []string, assignedRole string) bool {

// RoleByName returns the permissions associated with a given role name.
// This allows just the role names to be stored and expanded when required.
//
// 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 {
Expand All @@ -266,7 +283,7 @@ func RoleByName(name string) (Role, error) {
return role, nil
}

func RolesByNames(roleNames []string) ([]Role, error) {
func rolesByNames(roleNames []string) ([]Role, error) {
roles := make([]Role, 0, len(roleNames))
for _, n := range roleNames {
r, err := RoleByName(n)
Expand Down
2 changes: 1 addition & 1 deletion coderd/rbac/builtin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ type authSubject struct {
// Name is helpful for test assertions
Name string
UserID string
Roles []string
Roles rbac.RoleNames
Groups []string
}

Expand Down