fix: RBAC should default deny missing variables. (#5105)

Emyrk · web-flow · commit 015a6f9e2665 · 2022-11-16T11:01:09.000-06:00
* fix: RBAC should default deny missing variables.

The default behavior was to use 'true' for missing variables. This
was an incorrect assumption. If the variable is missing, the new
default is to deny (fail secure).

* Assert 1 workspace is returned for the owners
diff --git a/coderd/rbac/query.go b/coderd/rbac/query.go
@@ -470,7 +470,7 @@ func (t opInternalMember2) SQLString(cfg SQLConfig) string {
 		}
 
 		if sqlType == VarTypeSkip {
-			return "true"
+			return "false"
 		}
 	}
 
diff --git a/coderd/rbac/query_internal_test.go b/coderd/rbac/query_internal_test.go
@@ -84,7 +84,7 @@ func TestCompileQuery(t *testing.T) {
 			`"*" in input.object.acl_group_list["4d30d4a8-b87d-45ac-b0d4-51b2e68e7e75"]`,
 		))
 		require.NoError(t, err, "compile")
-		require.Equal(t, `true`,
+		require.Equal(t, `false`,
 			expression.SQLString(NoACLConfig()), "literal dereference")
 	})
 }
diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go
@@ -170,14 +170,20 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
 
 	// This other user is not in the first user's org. Since other is an admin, they can
 	// still see the "first" user's workspace.
-	other := coderdtest.CreateAnotherUser(t, client, otherOrg.ID, rbac.RoleOwner())
-	otherWorkspaces, err := other.Workspaces(ctx, codersdk.WorkspaceFilter{})
+	otherOwner := coderdtest.CreateAnotherUser(t, client, otherOrg.ID, rbac.RoleOwner())
+	otherWorkspaces, err := otherOwner.Workspaces(ctx, codersdk.WorkspaceFilter{})
 	require.NoError(t, err, "(other) fetch workspaces")
 
-	firstWorkspaces, err := other.Workspaces(ctx, codersdk.WorkspaceFilter{})
+	firstWorkspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
 	require.NoError(t, err, "(first) fetch workspaces")
 
 	require.ElementsMatch(t, otherWorkspaces.Workspaces, firstWorkspaces.Workspaces)
+	require.Equal(t, len(firstWorkspaces.Workspaces), 1, "should be 1 workspace present")
+
+	memberView := coderdtest.CreateAnotherUser(t, client, otherOrg.ID)
+	memberViewWorkspaces, err := memberView.Workspaces(ctx, codersdk.WorkspaceFilter{})
+	require.NoError(t, err, "(member) fetch workspaces")
+	require.Equal(t, 0, len(memberViewWorkspaces.Workspaces), "member in other org should see 0 workspaces")
 }
 
 func TestPostWorkspacesByOrganization(t *testing.T) {

Original file line number	Diff line number	Diff line change
`@@ -470,7 +470,7 @@ func (t opInternalMember2) SQLString(cfg SQLConfig) string {`
`470`	`470`	`}`
`471`	`471`
`472`	`472`	`if sqlType == VarTypeSkip {`
`473`		`- return "true"`
	`473`	`+ return "false"`
`474`	`474`	`}`
`475`	`475`	`}`
`476`	`476`
Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ func TestCompileQuery(t *testing.T) {`
`84`	`84`	`"*" in input.object.acl_group_list["4d30d4a8-b87d-45ac-b0d4-51b2e68e7e75"]`,
`85`	`85`	`))`
`86`	`86`	`require.NoError(t, err, "compile")`
`87`		- require.Equal(t, `true`,
	`87`	+ require.Equal(t, `false`,
`88`	`88`	`expression.SQLString(NoACLConfig()), "literal dereference")`
`89`	`89`	`})`
`90`	`90`	`}`