chore: join owner, template, and org in new workspace view #15116

Emyrk · 2024-10-16T21:35:03Z

What this does

Joins in fields like username, avatar_url, organization_name, template_name to workspaces via a view.

Views are a nuisance to maintain, however they have become the only clean way to handle RBAC related objects for:

If this is not done, the backend has to do multiple fetches when trying to retrieve information always displayed like the Owner's username. These fetches have the RBAC issue that they require the READ permission the related object, which gets even more complicated with custom roles.

Without these joins, if you give READ permission to a workspace in an org, it also requires the permission to READ the user only to fetch the username & avatar. When using custom roles, these implicit links are confusing to use, and have caused a reported error.

We can further make this better, as some overfetching is still done, but this change is already a lot.

What this costs

We have to maintain yet another view. Insert rant about SQLc supporting client-side views that update with the schema.

RBAC Change

Giving READ to a workspace now lets you see all these fields without needing perms to the related objects.

-- Owner
coalesce(visible_users.avatar_url, '') AS owner_avatar_url,
coalesce(visible_users.username, '') AS owner_username,
-- Organization
coalesce(organizations.name, '') AS organization_name,
coalesce(organizations.display_name, '') AS organization_display_name,
coalesce(organizations.icon, '') AS organization_icon,
coalesce(organizations.description, '') AS organization_description,
      -- Template
coalesce(templates.name, '') AS template_name,
coalesce(templates.display_name, '') AS template_display_name,
coalesce(templates.icon, '') AS template_icon,
coalesce(templates.description, '') AS template_description

Guardrails

Converting to WorkspaceTable is guarded to stay in sync with the view. If you update the view, but not the converter function, this test will fail.

coder/coderd/database/modelqueries_internal_test.go

Lines 19 to 22 in bbbe7aa

    
           // TestWorkspaceTableConvert verifies all workspace fields are converted 
        
           // when reducing a `Workspace` to a `WorkspaceTable`. 
        
           // This test is a guard rail to prevent developer oversight mistakes. 
        
           func TestWorkspaceTableConvert(t *testing.T) {

.

And static structs are also guarded. Does the same as above, but ignores the converter function.

coder/coderd/database/gentest/models_test.go

Line 69 in bbbe7aa

func TestViewSubsetWorkspace(t *testing.T) {

We use WorkspaceTable for audit logs, since fields like Username are mutable, it would not make sense to include in the audit log with a workspace. It is sourced from the mutable field of Users.

johnstcn

submitting partial review now

johnstcn · 2024-10-21T15:00:00Z

docs/admin/security/audit-logs.md

 | WorkspaceBuild<br><i>start, stop</i>                     | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody><tr><td>build_number</td><td>false</td></tr><tr><td>created_at</td><td>false</td></tr><tr><td>daily_cost</td><td>false</td></tr><tr><td>deadline</td><td>false</td></tr><tr><td>id</td><td>false</td></tr><tr><td>initiator_by_avatar_url</td><td>false</td></tr><tr><td>initiator_by_username</td><td>false</td></tr><tr><td>initiator_id</td><td>false</td></tr><tr><td>job_id</td><td>false</td></tr><tr><td>max_deadline</td><td>false</td></tr><tr><td>provisioner_state</td><td>false</td></tr><tr><td>reason</td><td>false</td></tr><tr><td>template_version_id</td><td>true</td></tr><tr><td>transition</td><td>false</td></tr><tr><td>updated_at</td><td>false</td></tr><tr><td>workspace_id</td><td>false</td></tr></tbody></table>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | WorkspaceProxy<br><i></i>                                | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody><tr><td>created_at</td><td>true</td></tr><tr><td>deleted</td><td>false</td></tr><tr><td>derp_enabled</td><td>true</td></tr><tr><td>derp_only</td><td>true</td></tr><tr><td>display_name</td><td>true</td></tr><tr><td>icon</td><td>true</td></tr><tr><td>id</td><td>true</td></tr><tr><td>name</td><td>true</td></tr><tr><td>region_id</td><td>true</td></tr><tr><td>token_hashed_secret</td><td>true</td></tr><tr><td>updated_at</td><td>false</td></tr><tr><td>url</td><td>true</td></tr><tr><td>version</td><td>true</td></tr><tr><td>wildcard_hostname</td><td>true</td></tr></tbody></table>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               |
+| WorkspaceTable<br><i></i>                                | <table><thead><tr><th>Field</th><th>Tracked</th></tr></thead><tbody><tr><td>automatic_updates</td><td>true</td></tr><tr><td>autostart_schedule</td><td>true</td></tr><tr><td>created_at</td><td>false</td></tr><tr><td>deleted</td><td>false</td></tr><tr><td>deleting_at</td><td>true</td></tr><tr><td>dormant_at</td><td>true</td></tr><tr><td>favorite</td><td>true</td></tr><tr><td>id</td><td>true</td></tr><tr><td>last_used_at</td><td>false</td></tr><tr><td>name</td><td>true</td></tr><tr><td>organization_id</td><td>false</td></tr><tr><td>owner_id</td><td>true</td></tr><tr><td>template_id</td><td>true</td></tr><tr><td>ttl</td><td>true</td></tr><tr><td>updated_at</td><td>false</td></tr></tbody></table>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |


Can we keep the old resource name? I don't think admins care if there is a difference between a change to a WorkspaceTable or a change to a Workspace?

The type is still workspace to admins:

coder/coderd/audit/request.go

Lines 184 to 185 in b05b00c

case database.WorkspaceTable:

return database.ResourceTypeWorkspace

It is just WorkspaceTable in Go

coderd/database/modelqueries_internal_test.go

testutil/reflect.go

johnstcn · 2024-10-22T08:46:41Z

testutil/reflect.go

+	// Handle some special cases
+	switch v.Type() {
+	case reflect.TypeOf(time.Time{}):
+		v.Set(reflect.ValueOf(time.Date(2020, 5, 2, 5, 19, 21, 30, time.UTC)))


suggestion: use a non-UTC timezone

I am going to be lazy here.

Previously this data was fetched via seperate queries. This caused an issue in orgs, as users are site wide scoped. So user read access was required to read another user's username. Now it is all joined into the workspace, implcitly giving read perms to some fields in related objects.

This reverts commit e23704c.

github-actions bot assigned Emyrk Oct 16, 2024

Emyrk force-pushed the stevenmasley/workspace_org_view branch from af796d7 to e1cb2cf Compare October 18, 2024 14:08

Emyrk marked this pull request as ready for review October 18, 2024 14:40

Emyrk changed the title ~~chore: new workspace view to join in owner, template, and org~~ chore: join owner, template, and org in new workspace view Oct 18, 2024

johnstcn reviewed Oct 21, 2024

View reviewed changes

Emyrk requested a review from johnstcn October 21, 2024 16:14

johnstcn approved these changes Oct 22, 2024

View reviewed changes

Emyrk added 21 commits October 22, 2024 09:00

random text got added

15d24fb

add unit test to verify subset of fields

a3ea2bd

remove unused

d3bebae

Revert "remove unused"

ba8077d

This reverts commit e23704c.

add guard rail

90f2474

more test fixes

50fb160

fix dgen calls

f18c801

fixup audit track and workspacebuildData call

b231af9

bump migration number

ba01ba0

fix more tests

b216efd

fmt

d13fd10

continue fixing references to Workspace/WorkspaceTable

d48c231

linting:

8be63e7

make gen

a80a01d

sql spaces...

5ad6a58

make gen

9f6f5ee

fix audit panic

c48f327

fix audit test

81a8aa2

populate sdk workspace

50638c7

rebase fixes

6e448d8

Emyrk force-pushed the stevenmasley/workspace_org_view branch from b05b00c to 6e448d8 Compare October 22, 2024 14:05

fix workspace reference

05ce1ae

static time

c8de158

Emyrk merged commit 343f8ec into main Oct 22, 2024
27 checks passed

Emyrk deleted the stevenmasley/workspace_org_view branch October 22, 2024 14:20

github-actions bot locked and limited conversation to collaborators Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: join owner, template, and org in new workspace view #15116

chore: join owner, template, and org in new workspace view #15116

Emyrk commented Oct 16, 2024 •

edited

Loading

johnstcn left a comment

johnstcn Oct 21, 2024

Emyrk Oct 21, 2024

johnstcn Oct 22, 2024

Emyrk Oct 22, 2024

	// TestWorkspaceTableConvert verifies all workspace fields are converted
	// when reducing a `Workspace` to a `WorkspaceTable`.
	// This test is a guard rail to prevent developer oversight mistakes.
	func TestWorkspaceTableConvert(t *testing.T) {

	case database.WorkspaceTable:
	return database.ResourceTypeWorkspace

chore: join owner, template, and org in new workspace view #15116

chore: join owner, template, and org in new workspace view #15116

Conversation

Emyrk commented Oct 16, 2024 • edited Loading

What this does

What this costs

RBAC Change

Guardrails

johnstcn left a comment

Choose a reason for hiding this comment

johnstcn Oct 21, 2024

Choose a reason for hiding this comment

Emyrk Oct 21, 2024

Choose a reason for hiding this comment

johnstcn Oct 22, 2024

Choose a reason for hiding this comment

Emyrk Oct 22, 2024

Choose a reason for hiding this comment

Emyrk commented Oct 16, 2024 •

edited

Loading