[fga] Workspace: create, get, stop and delete #18403

geropl · 2023-08-01T16:11:43Z

Description

This PR introduces a first version of a WorkspaceService to encapsulate the CRUD operations on Workspaces, and do authorization with FGA/SpiceDB. It currently has the following methods:

createWorkspace
getWorkspace
stopWorkspace
deleteWorkspace
hardDeleteWorkspace

There is a lot of code-movement happening, so here some highlights interesting for reviewers:

the schema
WorkspaceService (specifically the places where we check and add/remove relations)
this change in semantics for from internalGetWorkspace
the tests

Future work:

update RelationshipUpdate for adding user -> workspace.owner and org -> workspace.org
merge WorkspaceDeletionService into WorkspaceGC (follow up PR)
snapshots: captured in https://linear.app/gitpod/issue/EXP-348/migrate-snapshotservice-takesnapshot-waitforsnapshot-getsnapshots
we might want to improve the testability of WorkspaceFactory by extracting all external calls (ConfigProvider)
we very likely want to re-think the GitpodServerImpl.createWorkspace API. I did not include this here because:
- if turned into a distraction from FGA very quickly
- it requires API changes to be done properly

Summary generated by Copilot

`🤖 Generated by Copilot at 75c3415`

This pull request introduces a centralized permissions feature for workspaces and organizations, using Spicedb as the backend. It adds a new WorkspaceService class to handle workspace operations, and updates the Authorizer, PrebuildManager, and GitpodServerImpl classes to use it. It also updates the schema, code generation, and tests to support the new feature, and fixes some bugs related to tracing and testing.

Related Issue(s)

Fixes EXP-317, EXP-320, EXP-323, EXP-322

How to test

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - gpl-fga-workspace-1
🔗 URL - gpl-fga-workspace-1.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - gpl-fga-workspace-1-gha.14518

Build Options

Build

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer

analytics=segment
with-dedicated-emulation
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment / Integration Tests

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
with-integration-tests=webapp
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
with-monitoring

/hold

geropl · 2023-08-02T09:51:42Z

components/server/src/prebuilds/prebuild-manager.ts

@@ -77,21 +77,18 @@ export class PrebuildManager {
            const results: Promise<any>[] = [];
            for (const prebuild of prebuilds) {
                try {
-                    for (const instance of prebuild.instances) {


No clue why we would cycle over instances here before. I could imagine this was to mitigate some weird behavior we had in the past. 🤔
We only every have one WorkspaceInstance for every Workspace of type = 'prebuilt'. And even if we'd change that in the future: There is always ever one WorkspaceInstance "running" for each given Workspace. 🤷

Also, I just noticed that same code paths did not emit ApplicationError but just Error instead. But I don't expect big issues there.

geropl · 2023-08-02T09:54:37Z

components/server/src/workspace/workspace-service.ts

+        await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
+
+        const workspace = await this.db.findById(workspaceId);
+        if (!workspace || !!workspace.softDeleted || workspace.deleted) {


THIS is the only thing that should have changed in comparison to the old code: Before, we would ignore softDeleted and workspace.deleted, and simply return the workspace.

Of course I reviewed all (transient) call sites and they should be fine, but I might have missed sth.

I'm torn here: If anybody raises there hand, I'm happy to add a feature flag here, just to reduce the risk of rolling this change out. 👍

I would have been more concerned about the contract to throw NOT_FOUND instead of undefined but since you only replaced usages of GitpodServerImpl#internalGetWorkspace which has the same contract I believe this is good.
That said, we will want to call this for adminGetWorkspace(s) as well, which should return deleted workspaces.

Yes, I thought I can postpone that problem, but that might not mamke sense. Let me check if we can remove softDeleted here.

wrong thread, removed

I checked again, and before, only startWorkspace checked for softDeleted. To keep the change in this PR as low as possible, I removed that check here.
I would leave it and monitor if I was there on Monday, but as I'm not, I really don't want to add extra noise to the point of these changes (FGA). 👍

geropl · 2023-08-02T09:55:47Z

components/spicedb/schema/schema.yaml

+    relation owner: user
+
+    // Whether a user can access a workspace (with an IDE)
+    //+ (hasAccessToRepository && isPrebuild) + everyoneIfIsShared


This is missing, and will be added with a follow-up PR.

We just discussed with Sven and Anton: This will break "prebuild access" (for everybody except owners) and "workspace sharing" everywhere we have the FGA feature flag enabled, namely preview environments.

components/server/src/workspace/gitpod-server-impl.ts

geropl · 2023-08-03T16:00:25Z

Going to rebase this now to resolve the conflicts, and align with the authorizer changes.

- createWorkspace - getWorkspace - stopWorkspace - deleteWorkspace - hardDeleteWorkspace

svenefftinge

Looks all very good! Just a few comments.

components/gitpod-protocol/src/util/tracing.ts

components/server/src/authorization/authorizer.ts

svenefftinge · 2023-08-03T16:28:55Z

components/server/src/jobs/workspace-gc.ts

@@ -70,7 +72,7 @@ export class WorkspaceGarbageCollector implements Job {
                );
            const afterSelect = new Date();
            const deletes = await Promise.all(
-                workspaces.map((ws) => this.deletionService.softDeleteWorkspace({ span }, ws, "gc")),
+                workspaces.map((ws) => this.workspaceService.deleteWorkspace(ws.ownerId, ws.id, "gc")), // TODO(gpl) This should be a system user/service account instead of ws owner


This would work, but is not correct as this is not triggered by the owner but by the system.
In another place (UserService#findById) I have used undefined as the userId to indicate the request is coming from the system.

Ok, I was not aware that we have a way (undefined) to express "the system does it".

Yeah, I'm not particularly satisfied with that choice for the protocol. But I liked it better than introducing a system ID or anything else I could think of. 🤷

@svenefftinge Could you shoot me a link to an example? I can't find one on main atm, and wonder if you meant setting all userId types to string | undefined? 🤔

gitpod/components/server/src/iam/iam-session-app.ts

Line 150 in 9b4cc85

await this.orgService.addOrUpdateMember(undefined, organizationId, user.id, "member", ctx);

After looking at the change, I really thing we should make this a separate argument, instead of overriding userId for the security reasons. Let's discuss in sync. 👍

components/server/src/workspace/gitpod-server-impl.ts

components/server/src/workspace/workspace-service.spec.db.ts

svenefftinge · 2023-08-03T16:46:49Z

components/server/src/workspace/workspace-service.ts

+        await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
+
+        const workspace = await this.db.findById(workspaceId);
+        if (!workspace || !!workspace.softDeleted || workspace.deleted) {


I would have been more concerned about the contract to throw NOT_FOUND instead of undefined but since you only replaced usages of GitpodServerImpl#internalGetWorkspace which has the same contract I believe this is good.
That said, we will want to call this for adminGetWorkspace(s) as well, which should return deleted workspaces.

components/spicedb/codegen/codegen.go

geropl · 2023-08-04T13:47:28Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/hold size/XL size/XXL and removed size/XL labels Aug 1, 2023

geropl force-pushed the gpl/fga-workspace-1 branch 2 times, most recently from 499141f to edc7282 Compare August 2, 2023 09:48

geropl commented Aug 2, 2023

View reviewed changes

geropl force-pushed the gpl/fga-workspace-1 branch from edc7282 to 8da43bb Compare August 2, 2023 10:00

geropl marked this pull request as ready for review August 2, 2023 12:13

geropl requested a review from a team as a code owner August 2, 2023 12:13

roboquat removed the do-not-merge/work-in-progress label Aug 2, 2023

github-actions bot added the team: team-experience label Aug 2, 2023

geropl mentioned this pull request Aug 2, 2023

[server] Merge WorkspaceDeletionService into WorkspaceGC #18410

Merged

14 tasks

geropl commented Aug 2, 2023

View reviewed changes

components/server/src/workspace/gitpod-server-impl.ts Show resolved Hide resolved

geropl force-pushed the gpl/fga-workspace-1 branch from 8da43bb to 72ab824 Compare August 3, 2023 16:01

geropl added 4 commits August 3, 2023 16:01

[server] Cleanup to make tests pass

b4f85c3

[server] Introduce WorkspaceService with:

eb1e4cc

- createWorkspace - getWorkspace - stopWorkspace - deleteWorkspace - hardDeleteWorkspace

add log info

470d6c2

fix after rebase

a1e5387

geropl force-pushed the gpl/fga-workspace-1 branch from 72ab824 to a1e5387 Compare August 3, 2023 16:07

svenefftinge approved these changes Aug 3, 2023

View reviewed changes

review comments

6b8acb5

geropl changed the title ~~[fga] Workspace: create + start~~ [fga] Workspace: create, get, stop and delet Aug 4, 2023

geropl changed the title ~~[fga] Workspace: create, get, stop and delet~~ [fga] Workspace: create, get, stop and delete Aug 4, 2023

address review comment: system user

9e4adba

roboquat removed the do-not-merge/hold label Aug 4, 2023

roboquat merged commit 0d36c68 into main Aug 4, 2023

roboquat deleted the gpl/fga-workspace-1 branch August 4, 2023 13:48

JennPlothow mentioned this pull request Aug 5, 2023

✌️ #18442

Closed

[fga] Workspace: create, get, stop and delete #18403

[fga] Workspace: create, get, stop and delete #18403

Uh oh!

Conversation

geropl commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

🤖 Generated by Copilot at 75c3415

Related Issue(s)

How to test

Documentation

Preview status

Build Options

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geropl Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geropl Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geropl commented Aug 3, 2023

Uh oh!

svenefftinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenefftinge Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geropl commented Aug 4, 2023

Uh oh!

Uh oh!

geropl commented Aug 1, 2023 •

edited

Loading

`🤖 Generated by Copilot at 75c3415`

geropl Aug 2, 2023 •

edited

Loading

geropl Aug 4, 2023 •

edited

Loading

svenefftinge Aug 4, 2023 •

edited

Loading