Add tests for GuardedResources: Workspace, WorkspaceInstance and WorkspaceLog (1/2) #10939
Conversation
geropl
changed the title
| @@ -1539,7 +1539,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |||
| } | |||
| traceWI(ctx, { instanceId: instance.id }); | |||
| const teamMembers = await this.getTeamMembersByProject(workspace.projectId); | |||
| await this.guardAccess({ kind: "workspaceInstance", subject: instance, workspace, teamMembers }, "get"); | |||
| await this.guardAccess({ kind: "workspaceLog", subject: workspace, teamMembers }, "get"); | |||
There was a problem hiding this comment.
This is fine, because image build logs should be the same access-level as prebuild logs.
| @@ -1664,7 +1664,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |||
| const wsiPromise = this.workspaceDb.trace(ctx).findInstanceById(instanceId); | |||
| const teamMembers = await this.getTeamMembersByProject(ws.projectId); | |||
|
|
|||
| await this.guardAccess({ kind: "workspace", subject: ws, teamMembers }, "get"); | |||
| await this.guardAccess({ kind: "workspaceLog", subject: ws, teamMembers }, "get"); | |||
There was a problem hiding this comment.
For workspace logs we have a separate resource kind "workspaceLog".
Hey, that's awesome, many thanks for adding tests @geropl! 🥇 💯
If the tests already run as part of the build, I guess this could be simply: "1. Should build:"
FYI, I think this should rather be Source: Slack announcement (internal) |
Yes, or
Thx, was not aware - adjusted. |
| workspace = resource.workspace; | ||
| break; | ||
| default: | ||
| // We do not handle resource kinds here! |
There was a problem hiding this comment.
| // We do not handle resource kinds here! | |
| // We do not handle other resource kinds here! |
There was a problem hiding this comment.
Many thanks!
Code looks good to me, especially if it does what we expect it to. 👍
Holding because of two (optional) open questions:
- Shouldn't we run a new build with-preview to give the new guards a quick sanity check?
- I see in the build logs that
workspaceLikeResourceGuardsCanAcccessran and did pass ✅ however, out of abundance of caution, did you observe actual failures? (E.g. while developing, or by flipping one random expected boolean to see if the test actually fails)
/hold
|
Lol, and my hold? 😆 |
Wow, never saw that happen 🙈
Yes, developed against my expectations - which did not always matched reality at first 😉 👍 |
roboquat
added
deployed: webapp
Description
This PR adds tests for workspace-like GuardedResources:
Workspace,WorkspaceInstanceandWorkspaceLog.It's a precursor for a follow-up PR.
Related Issue(s)
Context: #10696
How to test
cd components/server && yarn testRelease Notes
Documentation
Werft options: