- Notifications
You must be signed in to change notification settings - Fork927
chore: join owner, template, and org in new workspace view#15116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
af796d7 toe1cb2cfCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
submitting partial review now
| | 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> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we keep the old resource name? I don't think admins care if there is a difference between a change to aWorkspaceTable or a change to aWorkspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The type is stillworkspace to admins:
Lines 184 to 185 inb05b00c
| case database.WorkspaceTable: | |
| returndatabase.ResourceTypeWorkspace |
It is justWorkspaceTable in Go
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
testutil/reflect.go Outdated
| // 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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion: use a non-UTC timezone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am going to be lazy here.
Previously this data was fetched via seperate queries. This causedan issue in orgs, as users are site wide scoped. So user readaccess was required to read another user's username.Now it is all joined into the workspace, implcitly giving read permsto some fields in related objects.
This reverts commite23704c.
b05b00c to6e448d8Compare343f8ec intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#15114
What this does
Joins in fields like
username,avatar_url,organization_name,template_nametoworkspacesvia aview.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
READpermission the related object, whichgets even more complicated with custom roles.Without these joins, if you give
READpermission to a workspace in an org, it also requires the permission toREADthe 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
READto a workspace now lets you see all these fields without needing perms to the related objects.Guardrails
Converting to
WorkspaceTableis 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 inbbbe7aa
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 inbbbe7aa
We use
WorkspaceTablefor audit logs, since fields likeUsernameare mutable, it would not make sense to include in the audit log with a workspace. It is sourced from the mutable field of Users.