From 015a6f9e2665ddc08687bd0a304a3f05439c2ffc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 16 Nov 2022 11:01:09 -0600 Subject: [PATCH] fix: RBAC should default deny missing variables. (#5105) * 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 --- coderd/rbac/query.go | 2 +- coderd/rbac/query_internal_test.go | 2 +- coderd/workspaces_test.go | 12 +++++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/coderd/rbac/query.go b/coderd/rbac/query.go index d2442153d7..8a046eb8ad 100644 --- 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 index 712b063787..f5c2a57715 100644 --- 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 index f4ced10d53..71cc655b7e 100644 --- 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) {