diff --git a/coderd/authorize.go b/coderd/authorize.go index 7437e436b8ead..855348aaf6184 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -10,9 +10,21 @@ import ( "github.com/coder/coder/coderd/rbac" ) -func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) []O { +func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) ([]O, error) { roles := httpmw.AuthorizationUserRoles(r) - return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) + objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) + if err != nil { + // Log the error as Filter should not be erroring. + api.Logger.Error(r.Context(), "filter failed", + slog.Error(err), + slog.F("user_id", roles.ID), + slog.F("username", roles.Username), + slog.F("route", r.URL.Path), + slog.F("action", action), + ) + return nil, err + } + return objects, nil } // Authorize will return false if the user is not authorized to do the action. diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 29f3a0a605fbb..f3c4baaa227ab 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -63,7 +63,7 @@ func TestBuildInfo(t *testing.T) { // TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered. func TestAuthorizeAllEndpoints(t *testing.T) { t.Parallel() - authorizer := &fakeAuthorizer{} + authorizer := &recordingAuthorizer{} ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -563,21 +563,41 @@ type authCall struct { Object rbac.Object } -type fakeAuthorizer struct { +type recordingAuthorizer struct { Called *authCall AlwaysReturn error } -func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { - f.Called = &authCall{ +func (r *recordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { + r.Called = &authCall{ SubjectID: subjectID, Roles: roleNames, Action: action, Object: object, } - return f.AlwaysReturn + return r.AlwaysReturn } -func (f *fakeAuthorizer) reset() { - f.Called = nil +func (r *recordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) { + return &fakePreparedAuthorizer{ + Original: r, + SubjectID: subjectID, + Roles: roles, + Action: action, + }, nil +} + +func (r *recordingAuthorizer) reset() { + r.Called = nil +} + +type fakePreparedAuthorizer struct { + Original *recordingAuthorizer + SubjectID string + Roles []string + Action rbac.Action +} + +func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error { + return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Action, object) } diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index ac0ef2e2a3710..56af2eb68c0b1 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -50,7 +50,14 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { if daemons == nil { daemons = []database.ProvisionerDaemon{} } - daemons = AuthorizeFilter(api, r, rbac.ActionRead, daemons) + daemons, err = AuthorizeFilter(api, r, rbac.ActionRead, daemons) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching provisioner daemons.", + Detail: err.Error(), + }) + return + } httpapi.Write(rw, http.StatusOK, daemons) } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index b9f9e1f825e9e..4b99c29438bb4 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -11,24 +11,41 @@ import ( type Authorizer interface { ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error + PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) +} + +type PreparedAuthorized interface { + Authorize(ctx context.Context, object Object) error } // Filter takes in a list of objects, and will filter the list removing all -// the elements the subject does not have permission for. -// Filter does not allocate a new slice, and will use the existing one -// passed in. This can cause memory leaks if the slice is held for a prolonged -// period of time. -func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O { +// the elements the subject does not have permission for. All objects must be +// of the same type. +func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) ([]O, error) { + if len(objects) == 0 { + // Nothing to filter + return objects, nil + } + objectType := objects[0].RBACObject().Type + filtered := make([]O, 0) + prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType) + if err != nil { + return nil, xerrors.Errorf("prepare: %w", err) + } for i := range objects { object := objects[i] - err := auth.ByRoleName(ctx, subjID, subjRoles, action, object.RBACObject()) + rbacObj := object.RBACObject() + if rbacObj.Type != objectType { + return nil, xerrors.Errorf("object types must be uniform across the set (%s), found %s", objectType, object.RBACObject().Type) + } + err := prepared.Authorize(ctx, rbacObj) if err == nil { filtered = append(filtered, object) } } - return filtered + return filtered, nil } // RegoAuthorizer will use a prepared rego query for performing authorize() @@ -45,7 +62,7 @@ func NewAuthorizer() (*RegoAuthorizer, error) { query, err := rego.New( // allowed is the `allow` field from the prepared query. This is the field to check if authorization is // granted. - rego.Query("allowed = data.authz.allow"), + rego.Query("data.authz.allow"), rego.Module("policy.rego", policy), ).PrepareForEval(ctx) @@ -64,14 +81,11 @@ type authSubject struct { // 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, action Action, object Object) error { - roles := make([]Role, 0, len(roleNames)) - for _, n := range roleNames { - r, err := RoleByName(n) - if err != nil { - return xerrors.Errorf("get role permissions: %w", err) - } - roles = append(roles, r) + roles, err := RolesByNames(roleNames) + if err != nil { + return err } + return a.Authorize(ctx, subjectID, roles, action, object) } @@ -92,18 +106,29 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return ForbiddenWithInternal(xerrors.Errorf("eval rego: %w", err), input, results) } - if len(results) != 1 { - return ForbiddenWithInternal(xerrors.Errorf("expect only 1 result, got %d", len(results)), input, results) + if !results.Allowed() { + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results) } - allowedResult, ok := (results[0].Bindings["allowed"]).(bool) - if !ok { - return ForbiddenWithInternal(xerrors.Errorf("expected allowed to be a bool but got %T", allowedResult), input, results) + return nil +} + +// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type). +// This will vastly speed up performance if batch authorization on the same type of objects is needed. +func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { + auth, err := newPartialAuthorizer(ctx, subjectID, roles, action, objectType) + if err != nil { + return nil, xerrors.Errorf("new partial authorizer: %w", err) } - if !allowedResult { - return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), input, results) + return auth, nil +} + +func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, objectType string) (PreparedAuthorized, error) { + roles, err := RolesByNames(roleNames) + if err != nil { + return nil, err } - return nil + return a.Prepare(ctx, subjectID, roles, action, objectType) } diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go new file mode 100644 index 0000000000000..3b66031f3cd39 --- /dev/null +++ b/coderd/rbac/authz_internal_test.go @@ -0,0 +1,708 @@ +package rbac + +import ( + "context" + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/testutil" +) + +// subject is required because rego needs +type subject struct { + UserID string `json:"id"` + // For the unit test we want to pass in the roles directly, instead of just + // by name. This allows us to test custom roles that do not exist in the product, + // but test edge cases of the implementation. + Roles []Role `json:"roles"` +} + +type fakeObject struct { + Owner uuid.UUID + OrgOwner uuid.UUID + Type string + Allowed bool +} + +func (w fakeObject) RBACObject() Object { + return Object{ + Owner: w.Owner.String(), + OrgID: w.OrgOwner.String(), + Type: w.Type, + } +} + +func TestFilterError(t *testing.T) { + t.Parallel() + auth, err := NewAuthorizer() + require.NoError(t, err) + + _, err = Filter(context.Background(), auth, uuid.NewString(), []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace}) + require.ErrorContains(t, err, "object types must be uniform") +} + +// TestFilter ensures the filter acts the same as an individual authorize. +// It generates a random set of objects, then runs the Filter batch function +// against the singular ByRoleName function. +func TestFilter(t *testing.T) { + t.Parallel() + + orgIDs := make([]uuid.UUID, 10) + userIDs := make([]uuid.UUID, len(orgIDs)) + for i := range orgIDs { + orgIDs[i] = uuid.New() + userIDs[i] = uuid.New() + } + objects := make([]fakeObject, 0, len(userIDs)*len(orgIDs)) + for i := range userIDs { + for j := range orgIDs { + objects = append(objects, fakeObject{ + Owner: userIDs[i], + OrgOwner: orgIDs[j], + Type: ResourceWorkspace.Type, + Allowed: false, + }) + } + } + + testCases := []struct { + Name string + SubjectID string + Roles []string + Action Action + ObjectType string + }{ + { + Name: "NoRoles", + SubjectID: userIDs[0].String(), + Roles: []string{}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "Admin", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), "auditor", RoleAdmin(), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "OrgAdmin", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), RoleOrgAdmin(orgIDs[0]), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "OrgMember", + SubjectID: userIDs[0].String(), + Roles: []string{RoleOrgMember(orgIDs[0]), RoleOrgMember(orgIDs[1]), RoleMember()}, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "ManyRoles", + SubjectID: userIDs[0].String(), + Roles: []string{ + RoleOrgMember(orgIDs[0]), RoleOrgAdmin(orgIDs[0]), + RoleOrgMember(orgIDs[1]), RoleOrgAdmin(orgIDs[1]), + RoleOrgMember(orgIDs[2]), RoleOrgAdmin(orgIDs[2]), + RoleOrgMember(orgIDs[4]), + RoleOrgMember(orgIDs[5]), + RoleMember(), + }, + ObjectType: ResourceWorkspace.Type, + Action: ActionRead, + }, + { + Name: "SiteMember", + SubjectID: userIDs[0].String(), + Roles: []string{RoleMember()}, + ObjectType: ResourceUser.Type, + Action: ActionRead, + }, + { + Name: "ReadOrgs", + SubjectID: userIDs[0].String(), + Roles: []string{ + RoleOrgMember(orgIDs[0]), + RoleOrgMember(orgIDs[1]), + RoleOrgMember(orgIDs[2]), + RoleOrgMember(orgIDs[3]), + RoleMember(), + }, + ObjectType: ResourceOrganization.Type, + Action: ActionRead, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + localObjects := make([]fakeObject, len(objects)) + copy(localObjects, objects) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + auth, err := NewAuthorizer() + require.NoError(t, err, "new auth") + + // Run auth 1 by 1 + var allowedCount int + for i, obj := range localObjects { + obj.Type = tc.ObjectType + err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, ActionRead, obj.RBACObject()) + obj.Allowed = err == nil + if err == nil { + allowedCount++ + } + localObjects[i] = obj + } + + // Run by filter + list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, localObjects) + require.NoError(t, err) + require.Equal(t, allowedCount, len(list), "expected number of allowed") + for _, obj := range list { + require.True(t, obj.Allowed, "expected allowed") + } + }) + } +} + +// TestAuthorizeDomain test the very basic roles that are commonly used. +func TestAuthorizeDomain(t *testing.T) { + t.Parallel() + defOrg := uuid.New() + unuseID := uuid.New() + + user := subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleMember())), + must(RoleByName(RoleOrgMember(defOrg))), + }, + } + + testAuthorize(t, "Member", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + + // Other org + other us + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{{ + Name: "deny-all", + // List out deny permissions explicitly + Site: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }}, + } + + testAuthorize(t, "DeletedMember", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleOrgAdmin(defOrg))), + must(RoleByName(RoleMember())), + }, + } + + testAuthorize(t, "OrgAdmin", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, + }) + + user = subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleAdmin())), + must(RoleByName(RoleMember())), + }, + } + + testAuthorize(t, "SiteAdmin", user, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.All(), actions: allActions(), allow: true}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, + }) + + // In practice this is a token scope on a regular subject. + // So this unit test does not represent a practical role. It is just + // testing the capabilities of the RBAC system. + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "WorkspaceToken", + // This is at the site level to prevent the token from losing access if the user + // is kicked from the org + Site: []Permission{ + { + Negate: false, + ResourceType: ResourceWorkspace.Type, + Action: ActionRead, + }, + }, + }, + }, + } + + testAuthorize(t, "WorkspaceToken", user, + // Read Actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionRead} + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: true}, + + {resource: ResourceWorkspace.All(), allow: true}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: true}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: true}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: true}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: true}, + }), + // Not read actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionCreate, ActionUpdate, ActionDelete} + c.allow = false + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + }), + ) + + // In practice this is a token scope on a regular subject + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "ReadOnlyOrgAndUser", + Site: []Permission{}, + Org: map[string][]Permission{ + defOrg.String(): {{ + Negate: false, + ResourceType: "*", + Action: ActionRead, + }}, + }, + User: []Permission{ + { + Negate: false, + ResourceType: "*", + Action: ActionRead, + }, + }, + }, + }, + } + + testAuthorize(t, "ReadOnly", user, + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionRead} + return c + }, []authTestCase{ + // Read + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: true}, + + {resource: ResourceWorkspace.All(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, + {resource: ResourceWorkspace.InOrg(unuseID), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + }), + + // Pass non-read actions + cases(func(c authTestCase) authTestCase { + c.actions = []Action{ActionCreate, ActionUpdate, ActionDelete} + c.allow = false + return c + }, []authTestCase{ + // Read + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unuseID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + })) +} + +// TestAuthorizeLevels ensures level overrides are acting appropriately +//nolint:paralleltest +func TestAuthorizeLevels(t *testing.T) { + defOrg := uuid.New() + unusedID := uuid.New() + + user := subject{ + UserID: "me", + Roles: []Role{ + must(RoleByName(RoleAdmin())), + { + Name: "org-deny:" + defOrg.String(), + Org: map[string][]Permission{ + defOrg.String(): { + { + Negate: true, + ResourceType: "*", + Action: "*", + }, + }, + }, + }, + { + Name: "user-deny-all", + // List out deny permissions explicitly + User: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }, + }, + } + + testAuthorize(t, "AdminAlwaysAllow", user, + cases(func(c authTestCase) authTestCase { + c.actions = allActions() + c.allow = true + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(defOrg)}, + + {resource: ResourceWorkspace.WithOwner(user.UserID)}, + + {resource: ResourceWorkspace.All()}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, + {resource: ResourceWorkspace.InOrg(unusedID)}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, + {resource: ResourceWorkspace.InOrg(unusedID)}, + + {resource: ResourceWorkspace.WithOwner("not-me")}, + })) + + user = subject{ + UserID: "me", + Roles: []Role{ + { + Name: "site-noise", + Site: []Permission{ + { + Negate: true, + ResourceType: "random", + Action: WildcardSymbol, + }, + }, + }, + must(RoleByName(RoleOrgAdmin(defOrg))), + { + Name: "user-deny-all", + // List out deny permissions explicitly + User: []Permission{ + { + Negate: true, + ResourceType: WildcardSymbol, + Action: WildcardSymbol, + }, + }, + }, + }, + } + + testAuthorize(t, "OrgAllowAll", user, + cases(func(c authTestCase) authTestCase { + c.actions = allActions() + return c + }, []authTestCase{ + // Org + me + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: ResourceWorkspace.InOrg(defOrg), allow: true}, + + {resource: ResourceWorkspace.WithOwner(user.UserID), allow: false}, + + {resource: ResourceWorkspace.All(), allow: false}, + + // Other org + me + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, + {resource: ResourceWorkspace.InOrg(unusedID), allow: false}, + + // Other org + other user + {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + + // Other org + other use + {resource: ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, + {resource: ResourceWorkspace.InOrg(unusedID), allow: false}, + + {resource: ResourceWorkspace.WithOwner("not-me"), allow: false}, + })) +} + +// cases applies a given function to all test cases. This makes generalities easier to create. +func cases(opt func(c authTestCase) authTestCase, cases []authTestCase) []authTestCase { + if opt == nil { + return cases + } + for i := range cases { + cases[i] = opt(cases[i]) + } + return cases +} + +type authTestCase struct { + resource Object + actions []Action + allow bool +} + +func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { + t.Helper() + authorizer, err := NewAuthorizer() + require.NoError(t, err) + for _, cases := range sets { + for _, c := range cases { + t.Run(name, func(t *testing.T) { + for _, a := range c.actions { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + t.Cleanup(cancel) + authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource) + // Logging only + if authError != nil { + var uerr *UnauthorizedError + xerrors.As(authError, &uerr) + d, _ := json.Marshal(uerr.Input()) + t.Logf("input: %s", string(d)) + t.Logf("internal error: %+v", uerr.Internal().Error()) + t.Logf("output: %+v", uerr.Output()) + } else { + d, _ := json.Marshal(map[string]interface{}{ + "subject": subject, + "object": c.resource, + "action": a, + }) + t.Log(string(d)) + } + + if c.allow { + assert.NoError(t, authError, "expected no error for testcase action %s", a) + } else { + assert.Error(t, authError, "expected unauthorized") + } + + partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, a, c.resource.Type) + require.NoError(t, err, "make prepared authorizer") + + // Also check the rego policy can form a valid partial query result. + // This ensures we can convert the queries into SQL WHERE clauses in the future. + // If this function returns 'Support' sections, then we cannot convert the query into SQL. + if len(partialAuthz.partialQueries.Support) > 0 { + d, _ := json.Marshal(partialAuthz.input) + t.Logf("input: %s", string(d)) + for _, q := range partialAuthz.partialQueries.Queries { + t.Logf("query: %+v", q.String()) + } + for _, s := range partialAuthz.partialQueries.Support { + t.Logf("support: %+v", s.String()) + } + } + require.Equal(t, 0, len(partialAuthz.partialQueries.Support), "expected 0 support rules") + + partialErr := partialAuthz.Authorize(ctx, c.resource) + if authError != nil { + assert.Error(t, partialErr, "partial error blocked valid request (false negative)") + } else { + assert.NoError(t, partialErr, "partial allowed invalid request (false positive)") + } + } + }) + } + } +} + +// allActions is a helper function to return all the possible actions types. +func allActions() []Action { + return []Action{ActionCreate, ActionRead, ActionUpdate, ActionDelete} +} + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go deleted file mode 100644 index 4c714e52e5844..0000000000000 --- a/coderd/rbac/authz_test.go +++ /dev/null @@ -1,605 +0,0 @@ -package rbac_test - -import ( - "context" - "encoding/json" - "testing" - - "github.com/google/uuid" - "github.com/stretchr/testify/require" - "golang.org/x/xerrors" - - "github.com/coder/coder/coderd/rbac" -) - -// subject is required because rego needs -type subject struct { - UserID string `json:"id"` - // For the unit test we want to pass in the roles directly, instead of just - // by name. This allows us to test custom roles that do not exist in the product, - // but test edge cases of the implementation. - Roles []rbac.Role `json:"roles"` -} - -func TestFilter(t *testing.T) { - t.Parallel() - - objectList := make([]rbac.Object, 0) - workspaceList := make([]rbac.Object, 0) - fileList := make([]rbac.Object, 0) - for i := 0; i < 10; i++ { - workspace := rbac.ResourceWorkspace.WithOwner("me") - file := rbac.ResourceFile.WithOwner("me") - - workspaceList = append(workspaceList, workspace) - fileList = append(fileList, file) - - objectList = append(objectList, workspace) - objectList = append(objectList, file) - } - - // copyList is to prevent tests from sharing the same slice - copyList := func(list []rbac.Object) []rbac.Object { - tmp := make([]rbac.Object, len(list)) - copy(tmp, list) - return tmp - } - - testCases := []struct { - Name string - List []rbac.Object - Expected []rbac.Object - Auth func(o rbac.Object) error - }{ - { - Name: "FilterWorkspaceType", - List: copyList(objectList), - Expected: copyList(workspaceList), - Auth: func(o rbac.Object) error { - if o.Type != rbac.ResourceWorkspace.Type { - return xerrors.New("only workspace") - } - return nil - }, - }, - { - Name: "FilterFileType", - List: copyList(objectList), - Expected: copyList(fileList), - Auth: func(o rbac.Object) error { - if o.Type != rbac.ResourceFile.Type { - return xerrors.New("only file") - } - return nil - }, - }, - { - Name: "FilterAll", - List: copyList(objectList), - Expected: []rbac.Object{}, - Auth: func(o rbac.Object) error { - return xerrors.New("always fail") - }, - }, - { - Name: "FilterNone", - List: copyList(objectList), - Expected: copyList(objectList), - Auth: func(o rbac.Object) error { - return nil - }, - }, - } - - for _, c := range testCases { - c := c - t.Run(c.Name, func(t *testing.T) { - t.Parallel() - authorizer := fakeAuthorizer{ - AuthFunc: func(_ context.Context, _ string, _ []string, _ rbac.Action, object rbac.Object) error { - return c.Auth(object) - }, - } - - filtered := rbac.Filter(context.Background(), authorizer, "me", []string{}, rbac.ActionRead, c.List) - require.ElementsMatch(t, c.Expected, filtered, "expect same list") - require.Equal(t, len(c.Expected), len(filtered), "same length list") - }) - } -} - -// TestAuthorizeDomain test the very basic roles that are commonly used. -func TestAuthorizeDomain(t *testing.T) { - t.Parallel() - defOrg := uuid.New() - unuseID := uuid.New() - - user := subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgMember(defOrg))), - }, - } - - testAuthorize(t, "Member", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other us - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{{ - Name: "deny-all", - // List out deny permissions explicitly - Site: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }}, - } - - testAuthorize(t, "DeletedMember", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), - must(rbac.RoleByName(rbac.RoleMember())), - }, - } - - testAuthorize(t, "OrgAdmin", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - }) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleAdmin())), - must(rbac.RoleByName(rbac.RoleMember())), - }, - } - - testAuthorize(t, "SiteAdmin", user, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: true}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - }) - - // In practice this is a token scope on a regular subject. - // So this unit test does not represent a practical role. It is just - // testing the capabilities of the RBAC system. - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "WorkspaceToken", - // This is at the site level to prevent the token from losing access if the user - // is kicked from the org - Site: []rbac.Permission{ - { - Negate: false, - ResourceType: rbac.ResourceWorkspace.Type, - Action: rbac.ActionRead, - }, - }, - }, - }, - } - - testAuthorize(t, "WorkspaceToken", user, - // Read Actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionRead} - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), allow: true}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, - }), - // Not read actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete} - c.allow = false - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - }), - ) - - // In practice this is a token scope on a regular subject - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "ReadOnlyOrgAndUser", - Site: []rbac.Permission{}, - Org: map[string][]rbac.Permission{ - defOrg.String(): {{ - Negate: false, - ResourceType: "*", - Action: rbac.ActionRead, - }}, - }, - User: []rbac.Permission{ - { - Negate: false, - ResourceType: "*", - Action: rbac.ActionRead, - }, - }, - }, - }, - } - - testAuthorize(t, "ReadOnly", user, - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionRead} - return c - }, []authTestCase{ - // Read - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - - {resource: rbac.ResourceWorkspace.All(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - }), - - // Pass non-read actions - cases(func(c authTestCase) authTestCase { - c.actions = []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete} - c.allow = false - return c - }, []authTestCase{ - // Read - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - })) -} - -// TestAuthorizeLevels ensures level overrides are acting appropriately -//nolint:paralleltest -func TestAuthorizeLevels(t *testing.T) { - defOrg := uuid.New() - unusedID := uuid.New() - - user := subject{ - UserID: "me", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleAdmin())), - { - Name: "org-deny:" + defOrg.String(), - Org: map[string][]rbac.Permission{ - defOrg.String(): { - { - Negate: true, - ResourceType: "*", - Action: "*", - }, - }, - }, - }, - { - Name: "user-deny-all", - // List out deny permissions explicitly - User: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }, - }, - } - - testAuthorize(t, "AdminAlwaysAllow", user, - cases(func(c authTestCase) authTestCase { - c.actions = allActions() - c.allow = true - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - - {resource: rbac.ResourceWorkspace.All()}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - })) - - user = subject{ - UserID: "me", - Roles: []rbac.Role{ - { - Name: "site-noise", - Site: []rbac.Permission{ - { - Negate: true, - ResourceType: "random", - Action: rbac.WildcardSymbol, - }, - }, - }, - must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), - { - Name: "user-deny-all", - // List out deny permissions explicitly - User: []rbac.Permission{ - { - Negate: true, - ResourceType: rbac.WildcardSymbol, - Action: rbac.WildcardSymbol, - }, - }, - }, - }, - } - - testAuthorize(t, "OrgAllowAll", user, - cases(func(c authTestCase) authTestCase { - c.actions = allActions() - return c - }, []authTestCase{ - // Org + me - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: false}, - - {resource: rbac.ResourceWorkspace.All(), allow: false}, - - // Other org + me - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - - // Other org + other user - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - // Other org + other use - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - })) -} - -// cases applies a given function to all test cases. This makes generalities easier to create. -func cases(opt func(c authTestCase) authTestCase, cases []authTestCase) []authTestCase { - if opt == nil { - return cases - } - for i := range cases { - cases[i] = opt(cases[i]) - } - return cases -} - -type authTestCase struct { - resource rbac.Object - actions []rbac.Action - allow bool -} - -func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { - t.Helper() - authorizer, err := rbac.NewAuthorizer() - require.NoError(t, err) - for _, cases := range sets { - for _, c := range cases { - t.Run(name, func(t *testing.T) { - for _, a := range c.actions { - err := authorizer.Authorize(context.Background(), subject.UserID, subject.Roles, a, c.resource) - if c.allow { - if err != nil { - var uerr *rbac.UnauthorizedError - xerrors.As(err, &uerr) - d, _ := json.Marshal(uerr.Input()) - t.Logf("input: %s", string(d)) - t.Logf("internal error: %+v", uerr.Internal().Error()) - t.Logf("output: %+v", uerr.Output()) - } - require.NoError(t, err, "expected no error for testcase action %s", a) - continue - } - - if err == nil { - d, _ := json.Marshal(map[string]interface{}{ - "subject": subject, - "object": c.resource, - "action": a, - }) - t.Log(string(d)) - } - require.Error(t, err, "expected unauthorized") - } - }) - } - } -} - -// allActions is a helper function to return all the possible actions types. -func allActions() []rbac.Action { - return []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete} -} diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index cd51d88361636..539d74275d417 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -221,6 +221,18 @@ func RoleByName(name string) (Role, error) { return role, nil } +func RolesByNames(roleNames []string) ([]Role, error) { + roles := make([]Role, 0, len(roleNames)) + for _, n := range roleNames { + r, err := RoleByName(n) + if err != nil { + return nil, xerrors.Errorf("get role permissions: %w", err) + } + roles = append(roles, r) + } + return roles, nil +} + func IsOrgRole(roleName string) (string, bool) { _, orgID, err := roleSplit(roleName) if err == nil && orgID != "" { diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 9026dd7b213eb..231780e9eed56 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -62,7 +62,8 @@ func BenchmarkRBACFilter(b *testing.B) { rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), rbac.RoleOrgMember(orgs[1]), rbac.RoleOrgAdmin(orgs[1]), rbac.RoleOrgMember(orgs[2]), rbac.RoleOrgAdmin(orgs[2]), - rbac.RoleMember()}, + rbac.RoleMember(), + }, UserID: users[0], }, } @@ -75,7 +76,8 @@ func BenchmarkRBACFilter(b *testing.B) { b.Run(c.Name, func(b *testing.B) { objects := benchmarkSetup(orgs, users, b.N) b.ResetTimer() - allowed := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects) + allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects) + require.NoError(b, err) var _ = allowed }) } diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go deleted file mode 100644 index 98b23b9303c86..0000000000000 --- a/coderd/rbac/example_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package rbac_test - -import ( - "context" - "testing" - - "github.com/google/uuid" - - "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/rbac" -) - -// TestExample gives some examples on how to use the authz library. -// This serves to test syntax more than functionality. -func TestExample(t *testing.T) { - t.Parallel() - ctx := context.Background() - authorizer, err := rbac.NewAuthorizer() - require.NoError(t, err) - defaultOrg := uuid.New() - - // user will become an authn object, and can even be a database.User if it - // fulfills the interface. Until then, use a placeholder. - user := subject{ - UserID: "alice", - Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgAdmin(defaultOrg))), - }, - } - - //nolint:paralleltest - t.Run("ReadAllWorkspaces", func(t *testing.T) { - // To read all workspaces on the site - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.All()) - var _ = err - require.Error(t, err, "this user cannot read all workspaces") - }) - - //nolint:paralleltest - t.Run("ReadOrgWorkspaces", func(t *testing.T) { - // To read all workspaces on the org 'default' - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg)) - require.NoError(t, err, "this user can read all org workspaces in 'default'") - }) - - //nolint:paralleltest - t.Run("ReadMyWorkspace", func(t *testing.T) { - // Note 'database.Workspace' could fulfill the object interface and be passed in directly - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID)) - require.NoError(t, err, "this user can their workspace") - }) -} - -func must[T any](value T, err error) T { - if err != nil { - panic(err) - } - return value -} diff --git a/coderd/rbac/fake_test.go b/coderd/rbac/fake_test.go deleted file mode 100644 index e8f6a90394cc7..0000000000000 --- a/coderd/rbac/fake_test.go +++ /dev/null @@ -1,15 +0,0 @@ -package rbac_test - -import ( - "context" - - "github.com/coder/coder/coderd/rbac" -) - -type fakeAuthorizer struct { - AuthFunc func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error -} - -func (f fakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { - return f.AuthFunc(ctx, subjectID, roleNames, action, object) -} diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go new file mode 100644 index 0000000000000..8ff0f1d17593f --- /dev/null +++ b/coderd/rbac/partial.go @@ -0,0 +1,147 @@ +package rbac + +import ( + "context" + + "golang.org/x/xerrors" + + "github.com/open-policy-agent/opa/rego" +) + +type PartialAuthorizer struct { + // partialQueries is mainly used for unit testing to assert our rego policy + // can always be compressed into a set of queries. + partialQueries *rego.PartialQueries + // input is used purely for debugging and logging. + input map[string]interface{} + // preparedQueries are the compiled set of queries after partial evaluation. + // Cache these prepared queries to avoid re-compiling the queries. + // If alwaysTrue is true, then ignore these. + preparedQueries []rego.PreparedEvalQuery + // alwaysTrue is if the subject can always perform the action on the + // resource type, regardless of the unknown fields. + alwaysTrue bool +} + +func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*PartialAuthorizer, error) { + input := map[string]interface{}{ + "subject": authSubject{ + ID: subjectID, + Roles: roles, + }, + "object": map[string]string{ + "type": objectType, + }, + "action": action, + } + + // Run the rego policy with a few unknown fields. This should simplify our + // policy to a set of queries. + partialQueries, err := rego.New( + rego.Query("true = data.authz.allow"), + rego.Module("policy.rego", policy), + rego.Unknowns([]string{ + "input.object.owner", + "input.object.org_owner", + }), + rego.Input(input), + ).Partial(ctx) + if err != nil { + return nil, xerrors.Errorf("prepare: %w", err) + } + + pAuth := &PartialAuthorizer{ + partialQueries: partialQueries, + preparedQueries: []rego.PreparedEvalQuery{}, + input: input, + } + + // Prepare each query to optimize the runtime when we iterate over the objects. + preparedQueries := make([]rego.PreparedEvalQuery, 0, len(partialQueries.Queries)) + for _, q := range partialQueries.Queries { + if q.String() == "" { + // No more work needed. An empty query is the same as + // 'WHERE true' + // This is likely an admin. We don't even need to use rego going + // forward. + pAuth.alwaysTrue = true + preparedQueries = []rego.PreparedEvalQuery{} + break + } + results, err := rego.New( + rego.ParsedQuery(q), + ).PrepareForEval(ctx) + if err != nil { + return nil, xerrors.Errorf("prepare query %s: %w", q.String(), err) + } + preparedQueries = append(preparedQueries, results) + } + pAuth.preparedQueries = preparedQueries + + return pAuth, nil +} + +// Authorize authorizes a single object using the partially prepared queries. +func (a PartialAuthorizer) Authorize(ctx context.Context, object Object) error { + if a.alwaysTrue { + return nil + } + + // How to interpret the results of the partial queries. + // We have a list of queries that are along the lines of: + // `input.object.org_owner = ""; "me" = input.object.owner` + // `input.object.org_owner in {"feda2e52-8bf1-42ce-ad75-6c5595cb297a"} ` + // All these queries are joined by an 'OR'. So we need to run through each + // query, and evaluate it. + // + // In each query, we have a list of the expressions, which should be + // all boolean expressions. In the above 1st example, there are 2. + // These expressions within a single query are `AND` together by rego. +EachQueryLoop: + for _, q := range a.preparedQueries { + // We need to eval each query with the newly known fields. + results, err := q.Eval(ctx, rego.EvalInput(map[string]interface{}{ + "object": object, + })) + if err != nil { + continue EachQueryLoop + } + + // If there are no results, then the query is false. This is because rego + // treats false queries as "undefined". So if any expression is false, the + // result is an empty list. + if len(results) == 0 { + continue EachQueryLoop + } + + // If there is more than 1 result, that means there is more than 1 rule. + // This should not happen, because our query should always be an expression. + // If this every occurs, it is likely the original query was not an expression. + if len(results) > 1 { + continue EachQueryLoop + } + + // Our queries should be simple, and should not yield any bindings. + // A binding is something like 'x := 1'. This binding as an expression is + // 'true', but in our case is unhelpful. We are not analyzing this ast to + // map bindings. So just error out. Similar to above, our queries should + // always be boolean expressions. + if len(results[0].Bindings) > 0 { + continue EachQueryLoop + } + + // We have a valid set of boolean expressions! All expressions are 'AND'd + // together. This is automatic by rego, so we should not actually need to + // inspect this any further. But just in case, we will verify each expression + // did resolve to 'true'. This is purely defensive programming. + for _, exp := range results[0].Expressions { + if exp.String() != "true" { + continue EachQueryLoop + } + } + + return nil + } + + return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), a.input, nil) +} diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 8f5208aca138e..485def49f38fb 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -1,11 +1,31 @@ package authz -import future.keywords.in -import future.keywords.every - +import future.keywords # A great playground: https://play.openpolicyagent.org/ -# TODO: Add debug instructions to do in the cli. Running really short on time, the -# playground is sufficient for now imo. In the future we can provide a tidy bash -# script for running this against predefined input. +# Helpful cli commands to debug. +# opa eval --format=pretty 'data.authz.allow = true' -d policy.rego -i input.json +# opa eval --partial --format=pretty 'data.authz.allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner -i input.json + +# +# This policy is specifically constructed to compress to a set of queries if the +# object's 'owner' and 'org_owner' fields are unknown. There is no specific set +# of rules that will guarantee that this policy has this property. However, there +# are some tricks. A unit test will enforce this property, so any edits that pass +# the unit test will be ok. +# +# Tricks: (It's hard to really explain this, fiddling is required) +# 1. Do not use unknown fields in any comprehension or iteration. +# 2. Use the unknown fields as minimally as possible. +# 3. Avoid making code branches based on the value of the unknown field. +# Unknown values are like a "set" of possible values. +# (This is why rule 1 usually breaks things) +# For example: +# In the org section, we calculate the 'allow' number for all orgs, rather +# than just the input.object.org_owner. This is because if the org_owner +# changes, then we don't need to recompute any 'allow' sets. We already have +# the 'allow' for the changed value. So the answer is in a lookup table. +# The final statement 'num := allow[input.object.org_owner]' does not have +# different code branches based on the org_owner. 'num's value does, but +# that is the whole point of partial evaluation. # bool_flip lets you assign a value to an inverted bool. # You cannot do 'x := !false', but you can do 'x := bool_flip(false)' @@ -19,121 +39,120 @@ bool_flip(b) = flipped { flipped = true } -# perms_grant returns a set of boolean values {true, false}. -# True means a positive permission in the set, false is a negative permission. -# It will only return `bool_flip(perm.negate)` for permissions that affect a given -# resource_type, and action. -# The empty set is returned if no relevant permissions are found. -perms_grant(permissions) = grants { - # If there are no permissions, this value is the empty set {}. - grants := { x | - # All permissions ... - perm := permissions[_] - # Such that the permission action, and type matches - perm.action in [input.action, "*"] - perm.resource_type in [input.object.type, "*"] - x := bool_flip(perm.negate) - } +# number is a quick way to get a set of {true, false} and convert it to +# -1: {false, true} or {false} +# 0: {} +# 1: {true} +number(set) = c { + count(set) == 0 + c := 0 } -# Site & User are both very simple. We default both to the empty set '{}'. If no permissions are present, then the -# result is the default value. -default site = {} -site = grant { - # Boolean set for all site wide permissions. - grant = { v | # Use set comprehension to remove duplicate values - # For each role, grab the site permission. - # Find the grants on this permission list. - v = perms_grant(input.subject.roles[_].site)[_] - } +number(set) = c { + false in set + c := -1 } -default user = {} -user = grant { - # Only apply user permissions if the user owns the resource - input.object.owner != "" - input.object.owner == input.subject.id - grant = { v | - # For each role, grab the user permissions. - # Find the grants on this permission list. - v = perms_grant(input.subject.roles[_].user)[_] - } +number(set) = c { + not false in set + set[_] + c := 1 } -# Organizations are more complex. If the user has no roles that specifically indicate the org_id of the object, -# then we want to block the action. This is because that means the user is not a member of the org. -# A non-member cannot access any org resources. -# org_member returns the set of permissions associated with a user if the user is a member of the -# organization -org_member = grant { - input.object.org_owner != "" - grant = { v | - v = perms_grant(input.subject.roles[_].org[input.object.org_owner])[_] +# site, org, and user rules are all similar. Each rule should return a number +# from [-1, 1]. The number corresponds to "negative", "abstain", and "positive" +# for the given level. See the 'allow' rules for how these numbers are used. +default site = 0 +site := num { + # allow is a set of boolean values without duplicates. + allow := { x | + # Iterate over all site permissions in all roles + perm := input.subject.roles[_].site[_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + # x is either 'true' or 'false' if a matching permission exists. + x := bool_flip(perm.negate) } + num := number(allow) } -# If a user is not part of an organization, 'org_non_member' is set to true -org_non_member { - input.object.org_owner != "" - # Identify if the user is in the org - roles := input.subject.roles - every role in roles { - not role.org[input.object.org_owner] - } +# org_members is the list of organizations the actor is apart of. +org_members := { orgID | + input.subject.roles[_].org[orgID] } -# org is two rules that equate to the following -# if org_non_member { return {false} } -# else { org_member } -# -# It is important both rules cannot be true, as the `org` rules cannot produce multiple outputs. -default org = {} -org = set { - # We have to do !org_non_member because rego rules must evaluate to 'true' - # to have a value set. - # So we do "not not-org-member" which means "subject is in org" - not org_non_member - set = org_member +# org is the same as 'site' except we need to iterate over each organization +# that the actor is a member of. +default org = 0 +org := num { + allow := { id: num | + id := org_members[_] + set := { x | + perm := input.subject.roles[_].org[id][_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + x := bool_flip(perm.negate) + } + num := number(set) + } + + # Return only the org value of the input's org. + # The reason why we do not do this up front, is that we need to make sure + # this policy compresses down to simple queries. One way to ensure this is + # to keep unknown values out of comprehensions. + # (https://www.openpolicyagent.org/docs/latest/policy-language/#comprehensions) + num := allow[input.object.org_owner] +} + +# 'org_mem' is set to true if the user is an org member +org_mem := true { + input.object.org_owner != "" + input.object.org_owner in org_members } -org = set { - org_non_member - set = {false} +# If the oject has no organization, then the user is also considered part of +# the non-existent org. +org_mem := true { + input.object.org_owner == "" +} + +# User is the same as the site, except it only applies if the user owns the object and +# the user is apart of the org (if the object has an org). +default user = 0 +user := num { + input.object.owner != "" + input.subject.id = input.object.owner + allow := { x | + perm := input.subject.roles[_].user[_] + perm.action in [input.action, "*"] + perm.resource_type in [input.object.type, "*"] + x := bool_flip(perm.negate) + } + num := number(allow) } -# The allow block is quite simple. Any set with `false` cascades down in levels. +# The allow block is quite simple. Any set with `-1` cascades down in levels. # Authorization looks for any `allow` statement that is true. Multiple can be true! # Note that the absence of `allow` means "unauthorized". # An explicit `"allow": true` is required. -# site allow + +default allow = false allow { - # No site wide deny - not false in site - # And all permissions are positive - site[_] + site = 1 } -# OR - -# org allow allow { - # No site or org deny - not false in site - not false in org - # And all permissions are positive - org[_] + not site = -1 + org = 1 } -# OR - -# user allow allow { - # No site, org, or user deny - not false in site - not false in org - not false in user - # And all permissions are positive - user[_] + not site = -1 + not org = -1 + # If we are not a member of an org, and the object has an org, then we are + # not authorized. This is an "implied -1" for not being in the org. + org_mem + user = 1 } diff --git a/coderd/templates.go b/coderd/templates.go index 884b26fabe385..e6bab1c25c927 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -273,7 +273,14 @@ func (api *API) templatesByOrganization(rw http.ResponseWriter, r *http.Request) } // Filter templates based on rbac permissions - templates = AuthorizeFilter(api, r, rbac.ActionRead, templates) + templates, err = AuthorizeFilter(api, r, rbac.ActionRead, templates) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching templates.", + Detail: err.Error(), + }) + return + } templateIDs := make([]uuid.UUID, 0, len(templates)) diff --git a/coderd/users.go b/coderd/users.go index 51509b0cc800e..15ba09dfffcbe 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -155,7 +155,15 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { return } - users = AuthorizeFilter(api, r, rbac.ActionRead, users) + users, err = AuthorizeFilter(api, r, rbac.ActionRead, users) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching users.", + Detail: err.Error(), + }) + return + } + userIDs := make([]uuid.UUID, 0, len(users)) for _, user := range users { userIDs = append(userIDs, user.ID) @@ -489,7 +497,14 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { } // Only include ones we can read from RBAC. - memberships = AuthorizeFilter(api, r, rbac.ActionRead, memberships) + memberships, err = AuthorizeFilter(api, r, rbac.ActionRead, memberships) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching memberships.", + Detail: err.Error(), + }) + return + } for _, mem := range memberships { // If we can read the org member, include the roles. @@ -610,7 +625,14 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) { } // Only return orgs the user can read. - organizations = AuthorizeFilter(api, r, rbac.ActionRead, organizations) + organizations, err = AuthorizeFilter(api, r, rbac.ActionRead, organizations) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organizations.", + Detail: err.Error(), + }) + return + } publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) for _, organization := range organizations { diff --git a/coderd/users_test.go b/coderd/users_test.go index 5c7777282541d..0ff422d237ac1 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1196,7 +1196,6 @@ func TestPaginatedUsers(t *testing.T) { {name: "all users", limit: 10, allUsers: allUsers}, {name: "all users", limit: 5, allUsers: allUsers}, {name: "all users", limit: 3, allUsers: allUsers}, - {name: "all users", limit: 1, allUsers: allUsers}, {name: "gmail search", limit: 3, allUsers: specialUsers, opt: gmailSearch}, {name: "gmail search", limit: 7, allUsers: specialUsers, opt: gmailSearch}, {name: "username search", limit: 3, allUsers: specialUsers, opt: usernameSearch}, diff --git a/coderd/workspaces.go b/coderd/workspaces.go index fd696d5f0edb1..07fbb27641031 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -145,7 +145,14 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } // Only return workspaces the user can read - workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) + workspaces, err = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspaces.", + Detail: err.Error(), + }) + return + } apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) if err != nil {