cleanup

coder · Emyrk · May 15, 2024 · May 9, 2024 · May 9, 2024 · May 9, 2024
commit e4d59aece59e4f3358f89cb5ca69c732017c93e6
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -165,12 +165,12 @@ var (
 				Site: rbac.Permissions(map[string][]policy.Action{
 					// TODO: Add ProvisionerJob resource type.
 					rbac.ResourceFile.Type:     {policy.ActionRead},
-					rbac.ResourceSystem.Type:   {rbac.WildcardSymbol},
+					rbac.ResourceSystem.Type:   {policy.WildcardSymbol},
 					rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate},
 					// Unsure why provisionerd needs update and read personal
 					rbac.ResourceUser.Type:      {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal},
 					rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceBuild},
-					rbac.ResourceApiKey.Type:    {rbac.WildcardSymbol},
+					rbac.ResourceApiKey.Type:    {policy.WildcardSymbol},
 					// When org scoped provisioner credentials are implemented,
 					// this can be reduced to read a specific org.
 					rbac.ResourceOrganization.Type: {policy.ActionRead},
@@ -191,7 +191,7 @@ var (
 				Name:        "autostart",
 				DisplayName: "Autostart Daemon",
 				Site: rbac.Permissions(map[string][]policy.Action{
-					rbac.ResourceSystem.Type:    {rbac.WildcardSymbol},
+					rbac.ResourceSystem.Type:    {policy.WildcardSymbol},
 					rbac.ResourceTemplate.Type:  {policy.ActionRead, policy.ActionUpdate},
 					rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceBuild},
 					rbac.ResourceUser.Type:      {policy.ActionRead},
@@ -212,7 +212,7 @@ var (
 				Name:        "hangdetector",
 				DisplayName: "Hang Detector Daemon",
 				Site: rbac.Permissions(map[string][]policy.Action{
-					rbac.ResourceSystem.Type:    {rbac.WildcardSymbol},
+					rbac.ResourceSystem.Type:    {policy.WildcardSymbol},
 					rbac.ResourceTemplate.Type:  {policy.ActionRead},
 					rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate},
 				}),
@@ -234,10 +234,11 @@ var (
 					rbac.ResourceWildcard.Type:           {policy.ActionRead},
 					rbac.ResourceApiKey.Type:             {policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete},
 					rbac.ResourceGroup.Type:              {policy.ActionCreate, policy.ActionUpdate},
-					rbac.ResourceAssignRole.Type:         {policy.ActionCreate, policy.ActionDelete},
-					rbac.ResourceSystem.Type:             {rbac.WildcardSymbol},
+					rbac.ResourceAssignRole.Type:         rbac.ResourceAssignRole.AvailableActions(),
+					rbac.ResourceSystem.Type:             {policy.WildcardSymbol},
 					rbac.ResourceOrganization.Type:       {policy.ActionCreate, policy.ActionRead},
 					rbac.ResourceOrganizationMember.Type: {policy.ActionCreate},
+					rbac.ResourceAssignOrgRole.Type:      {policy.ActionRead, policy.ActionCreate, policy.ActionDelete},
 					rbac.ResourceProvisionerDaemon.Type:  {policy.ActionCreate, policy.ActionUpdate},
 					rbac.ResourceUser.Type:               rbac.ResourceUser.AvailableActions(),
 					rbac.ResourceWorkspace.Type:          {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceBuild, policy.ActionSSH},
@@ -607,7 +608,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
 	}
 
 	if len(added) > 0 {
-		if err := q.authorizeContext(ctx, policy.ActionCreate, roleAssign); err != nil {
+		if err := q.authorizeContext(ctx, policy.ActionAssign, roleAssign); err != nil {
 			return err
 		}
 	}
@@ -1775,7 +1776,7 @@ func (q *querier) GetUserActivityInsights(ctx context.Context, arg database.GetU
 				return nil, err
 			}
 
-			if err := q.authorizeContext(ctx, policy.ActionViewInsights, template.RBACObject()); err != nil {
+			if err := q.authorizeContext(ctx, policy.ActionViewInsights, template); err != nil {
 				return nil, err
 			}
 		}

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -636,7 +636,7 @@ func (s *MethodTestSuite) TestOrganization() {
 			UserID:         u.ID,
 			Roles:          []string{rbac.RoleOrgAdmin(o.ID)},
 		}).Asserts(
-			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionCreate,
+			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign,
 			rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate)
 	}))
 	s.Run("UpdateMemberRoles", s.Subtest(func(db database.Store, check *expects) {
@@ -656,7 +656,7 @@ func (s *MethodTestSuite) TestOrganization() {
 			OrgID:        o.ID,
 		}).Asserts(
 			mem, policy.ActionRead,
-			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionCreate, // org-mem
+			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign, // org-mem
 			rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionDelete, // org-admin
 		).Returns(out)
 	}))
@@ -1023,7 +1023,7 @@ func (s *MethodTestSuite) TestUser() {
 		check.Args(database.InsertUserParams{
 			ID:        uuid.New(),
 			LoginType: database.LoginTypePassword,
-		}).Asserts(rbac.ResourceAssignRole, policy.ActionCreate, rbac.ResourceUser, policy.ActionCreate)
+		}).Asserts(rbac.ResourceAssignRole, policy.ActionAssign, rbac.ResourceUser, policy.ActionCreate)
 	}))
 	s.Run("InsertUserLink", s.Subtest(func(db database.Store, check *expects) {
 		u := dbgen.User(s.T(), db, database.User{})
@@ -1158,7 +1158,7 @@ func (s *MethodTestSuite) TestUser() {
 			ID:           u.ID,
 		}).Asserts(
 			u, policy.ActionRead,
-			rbac.ResourceAssignRole, policy.ActionCreate,
+			rbac.ResourceAssignRole, policy.ActionAssign,
 			rbac.ResourceAssignRole, policy.ActionDelete,
 		).Returns(o)
 	}))

diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 	"github.com/coder/coder/v2/coderd/rbac/regosql"
+	"github.com/coder/coder/v2/coderd/util/slice"
 	"github.com/coder/coder/v2/testutil"
 )
 
@@ -310,7 +311,7 @@ func TestAuthorizeDomain(t *testing.T) {
 		},
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(unuseID).WithACLUserList(map[string][]policy.Action{
-				user.ID: {WildcardSymbol},
+				user.ID: {policy.WildcardSymbol},
 			}),
 			actions: ResourceWorkspace.AvailableActions(),
 			allow:   true,
@@ -342,7 +343,7 @@ func TestAuthorizeDomain(t *testing.T) {
 		},
 		{
 			resource: ResourceWorkspace.WithOwner(unuseID.String()).InOrg(defOrg).WithGroupACL(map[string][]policy.Action{
-				allUsersGroup: {WildcardSymbol},
+				allUsersGroup: {policy.WildcardSymbol},
 			}),
 			actions: ResourceWorkspace.AvailableActions(),
 			allow:   true,
@@ -398,8 +399,8 @@ func TestAuthorizeDomain(t *testing.T) {
 			Site: []Permission{
 				{
 					Negate:       true,
-					ResourceType: WildcardSymbol,
-					Action:       WildcardSymbol,
+					ResourceType: policy.WildcardSymbol,
+					Action:       policy.WildcardSymbol,
 				},
 			},
 		}},
@@ -439,10 +440,13 @@ func TestAuthorizeDomain(t *testing.T) {
 		},
 	}
 
+	workspaceExceptConnect := slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH)
+	workspaceConnect := []policy.Action{policy.ActionApplicationConnect, policy.ActionSSH}
 	testAuthorize(t, "OrgAdmin", user, []authTestCase{
 		// Org + me
 		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: ResourceWorkspace.AvailableActions(), allow: true},
-		{resource: ResourceWorkspace.InOrg(defOrg), actions: ResourceWorkspace.AvailableActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: workspaceExceptConnect, allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg), actions: workspaceConnect, allow: false},
 
 		{resource: ResourceWorkspace.WithOwner(user.ID), actions: ResourceWorkspace.AvailableActions(), allow: true},
 
@@ -453,7 +457,8 @@ func TestAuthorizeDomain(t *testing.T) {
 		{resource: ResourceWorkspace.InOrg(unuseID), actions: ResourceWorkspace.AvailableActions(), allow: false},
 
 		// Other org + other user
-		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: workspaceExceptConnect, allow: true},
+		{resource: ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: workspaceConnect, allow: false},
 
 		{resource: ResourceWorkspace.WithOwner("not-me"), actions: ResourceWorkspace.AvailableActions(), allow: false},
 
@@ -713,8 +718,8 @@ func TestAuthorizeLevels(t *testing.T) {
 				User: []Permission{
 					{
 						Negate:       true,
-						ResourceType: WildcardSymbol,
-						Action:       WildcardSymbol,
+						ResourceType: policy.WildcardSymbol,
+						Action:       policy.WildcardSymbol,
 					},
 				},
 			},
@@ -761,7 +766,7 @@ func TestAuthorizeLevels(t *testing.T) {
 					{
 						Negate:       true,
 						ResourceType: "random",
-						Action:       WildcardSymbol,
+						Action:       policy.WildcardSymbol,
 					},
 				},
 			},
@@ -772,8 +777,8 @@ func TestAuthorizeLevels(t *testing.T) {
 				User: []Permission{
 					{
 						Negate:       true,
-						ResourceType: WildcardSymbol,
-						Action:       WildcardSymbol,
+						ResourceType: policy.WildcardSymbol,
+						Action:       policy.WildcardSymbol,
 					},
 				},
 			},
@@ -782,7 +787,8 @@ func TestAuthorizeLevels(t *testing.T) {
 
 	testAuthorize(t, "OrgAllowAll", user,
 		cases(func(c authTestCase) authTestCase {
-			c.actions = ResourceWorkspace.AvailableActions()
+			// SSH and app connect are not implied here.
+			c.actions = slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH)
 			return c
 		}, []authTestCase{
 			// Org + me

diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go
@@ -8,8 +8,6 @@ import (
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 )
 
-const WildcardSymbol = "*"
-
 // ResourceUserObject is a helper function to create a user object for authz checks.
 func ResourceUserObject(userID uuid.UUID) Object {
 	return ResourceUser.WithID(userID).WithOwner(userID.String())

diff --git a/coderd/rbac/policy/policy.go b/coderd/rbac/policy/policy.go
@@ -75,9 +75,8 @@ var RBACPermissions = map[string]PermissionDefinition{
 			ActionUpdate: actDef("update an existing user"),
 			ActionDelete: actDef("delete an existing user"),
 
-			ActionReadPersonal:   actDef("read personal user data like password"),
+			ActionReadPersonal:   actDef("read personal user data like user settings and auth links"),
 			ActionUpdatePersonal: actDef("update personal data"),
-			// ActionReadPublic: actDef(fieldOwner, "read public user data"),
 		},
 	},
 	"workspace": {
@@ -168,7 +167,7 @@ var RBACPermissions = map[string]PermissionDefinition{
 		Actions: map[Action]ActionDefinition{
 			ActionCreate: actDef("create an organization member"),
 			ActionRead:   actDef("read member"),
-			ActionUpdate: actDef("update a organization member"),
+			ActionUpdate: actDef("update an organization member"),
 			ActionDelete: actDef("delete member"),
 		},
 	},

diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go
@@ -92,7 +92,7 @@ func allPermsExcept(excepts ...Objecter) []Permission {
 		perms = append(perms, Permission{
 			Negate:       false,
 			ResourceType: r.RBACObject().Type,
-			Action:       WildcardSymbol,
+			Action:       policy.WildcardSymbol,
 		})
 	}
 	return perms

diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go
@@ -56,6 +56,7 @@ func TestOwnerExec(t *testing.T) {
 	})
 }
 
+// nolint:tparallel,paralleltest -- subtests share a map, just run sequentially.
 func TestRolePermissions(t *testing.T) {
 	t.Parallel()
 
@@ -523,9 +524,10 @@ func TestRolePermissions(t *testing.T) {
 	}
 
 	passed := true
+	// nolint:tparallel,paralleltest
 	for _, c := range testCases {
 		c := c
-		// nolint:tparallel -- These share the same remainingPermissions map
+		// nolint:tparallel,paralleltest -- These share the same remainingPermissions map
 		t.Run(c.Name, func(t *testing.T) {
 			remainingSubjs := make(map[string]struct{})
 			for _, subj := range requiredSubjects {
@@ -568,7 +570,7 @@ func TestRolePermissions(t *testing.T) {
 	// Only run these if the tests on top passed. Otherwise, the error output is too noisy.
 	if passed {
 		for rtype, v := range remainingPermissions {
-			// nolint:tparallel -- Making a subtest for easier diagnosing failures.
+			// nolint:tparallel,paralleltest -- Making a subtest for easier diagnosing failures.
 			t.Run(fmt.Sprintf("%s-AllActions", rtype), func(t *testing.T) {
 				if len(v) > 0 {
 					assert.Equal(t, map[policy.Action]bool{}, v, "remaining permissions should be empty for type %q", rtype)

diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go
@@ -61,12 +61,12 @@ var builtinScopes = map[ScopeName]Scope{
 			Name:        fmt.Sprintf("Scope_%s", ScopeAll),
 			DisplayName: "All operations",
 			Site: Permissions(map[string][]policy.Action{
-				ResourceWildcard.Type: {WildcardSymbol},
+				ResourceWildcard.Type: {policy.WildcardSymbol},
 			}),
 			Org:  map[string][]Permission{},
 			User: []Permission{},
 		},
-		AllowIDList: []string{WildcardSymbol},
+		AllowIDList: []string{policy.WildcardSymbol},
 	},
 
 	ScopeApplicationConnect: {
@@ -79,7 +79,7 @@ var builtinScopes = map[ScopeName]Scope{
 			Org:  map[string][]Permission{},
 			User: []Permission{},
 		},
-		AllowIDList: []string{WildcardSymbol},
+		AllowIDList: []string{policy.WildcardSymbol},
 	},
 }
 

diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go
@@ -541,32 +541,31 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 					appTokenAPIClient.HTTPClient.Transport = appDetails.SDKClient.HTTPClient.Transport
 
 					var (
-						canCreateApplicationConnect = "can-create-application_connect"
-						canReadUserMe               = "can-read-user-me"
+						canApplicationConnect = "can-create-application_connect"
+						canReadUserMe         = "can-read-user-me"
 					)
 					authRes, err := appTokenAPIClient.AuthCheck(ctx, codersdk.AuthorizationRequest{
 						Checks: map[string]codersdk.AuthorizationCheck{
-							canCreateApplicationConnect: {
+							canApplicationConnect: {
 								Object: codersdk.AuthorizationObject{
-									ResourceType:   "application_connect",
-									OwnerID:        "me",
+									ResourceType:   "workspace",
+									OwnerID:        appDetails.FirstUser.UserID.String(),
 									OrganizationID: appDetails.FirstUser.OrganizationID.String(),
 								},
-								Action: "create",
+								Action: codersdk.ActionApplicationConnect,
 							},
 							canReadUserMe: {
 								Object: codersdk.AuthorizationObject{
 									ResourceType: "user",
-									OwnerID:      "me",
 									ResourceID:   appDetails.FirstUser.UserID.String(),
 								},
-								Action: "read",
+								Action: codersdk.ActionRead,
 							},
 						},
 					})
 					require.NoError(t, err)
 
-					require.True(t, authRes[canCreateApplicationConnect])
+					require.True(t, authRes[canApplicationConnect])
 					require.False(t, authRes[canReadUserMe])
 
 					// Load the application page with the API key set.

diff --git a/codersdk/authorization.go b/codersdk/authorization.go
@@ -32,7 +32,7 @@ type AuthorizationCheck struct {
 	// Omitting the 'OrganizationID' could produce the incorrect value, as
 	// workspaces have both `user` and `organization` owners.
 	Object AuthorizationObject `json:"object"`
-	Action string              `json:"action" enums:"create,read,update,delete"`
+	Action RBACAction          `json:"action" enums:"create,read,update,delete"`
 }
 
 // AuthorizationObject can represent a "set" of objects, such as: all workspaces in an organization, all workspaces owned by me,

diff --git a/enterprise/coderd/authorize_test.go b/enterprise/coderd/authorize_test.go
@@ -59,7 +59,7 @@ func TestCheckACLPermissions(t *testing.T) {
 				ResourceType: codersdk.ResourceTemplate,
 				ResourceID:   template.ID.String(),
 			},
-			Action: "write",
+			Action: codersdk.ActionUpdate,
 		},
 	}
 

diff --git a/enterprise/coderd/coderd_test.go b/enterprise/coderd/coderd_test.go
@@ -500,7 +500,7 @@ func testDBAuthzRole(ctx context.Context) context.Context {
 				Name:        "testing",
 				DisplayName: "Unit Tests",
 				Site: rbac.Permissions(map[string][]policy.Action{
-					rbac.ResourceWildcard.Type: {rbac.WildcardSymbol},
+					rbac.ResourceWildcard.Type: {policy.WildcardSymbol},
 				}),
 				Org:  map[string][]rbac.Permission{},
 				User: []rbac.Permission{},

diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go
@@ -15,7 +15,6 @@ import (
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
-	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 	"github.com/coder/coder/v2/codersdk"
 )
@@ -320,7 +319,7 @@ func convertToTemplateRole(actions []policy.Action) codersdk.TemplateRole {
 func convertSDKTemplateRole(role codersdk.TemplateRole) []policy.Action {
 	switch role {
 	case codersdk.TemplateRoleAdmin:
-		return []policy.Action{rbac.WildcardSymbol}
+		return []policy.Action{policy.WildcardSymbol}
 	case codersdk.TemplateRoleUse:
 		return []policy.Action{policy.ActionRead}
 	}

diff --git a/enterprise/tailnet/pgcoord.go b/enterprise/tailnet/pgcoord.go
@@ -103,7 +103,7 @@ var pgCoordSubject = rbac.Subject{
 			Name:        "tailnetcoordinator",
 			DisplayName: "Tailnet Coordinator",
 			Site: rbac.Permissions(map[string][]policy.Action{
-				rbac.ResourceTailnetCoordinator.Type: {rbac.WildcardSymbol},
+				rbac.ResourceTailnetCoordinator.Type: {policy.WildcardSymbol},
 			}),
 			Org:  map[string][]rbac.Permission{},
 			User: []rbac.Permission{},