[server] WorkspaceService.(hard-)deleteWorkspace

Author: geropl

components/server/src/authorization/definitions.ts

@@ -73,7 +73,7 @@ export type WorkspaceResourceType = "workspace";
 
 export type WorkspaceRelation = "org" | "owner";
 
-export type WorkspacePermission = "access" | "stop" | "read_info";
+export type WorkspacePermission = "access" | "stop" | "delete" | "read_info";
 
 export const rel = {
     user(id: string) {

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

@@ -12,6 +12,7 @@ import { TracedWorkspaceDB, DBWithTracing, WorkspaceDB } from "@gitpod/gitpod-db
 import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";
 import { Config } from "../config";
 import { Job } from "./runner";
+import { WorkspaceService } from "../workspace/workspace-service";
 
 /**
  * The WorkspaceGarbageCollector has two tasks:
@@ -21,6 +22,7 @@ import { Job } from "./runner";
 @injectable()
 export class WorkspaceGarbageCollector implements Job {
     @inject(WorkspaceDeletionService) protected readonly deletionService: WorkspaceDeletionService;
+    @inject(WorkspaceService) protected readonly workspaceService: WorkspaceService;
     @inject(TracedWorkspaceDB) protected readonly workspaceDB: DBWithTracing<WorkspaceDB>;
     @inject(Config) protected readonly config: Config;
 
@@ -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
             );
             const afterDelete = new Date();
 
@@ -125,7 +127,7 @@ export class WorkspaceGarbageCollector implements Job {
                     now,
                 );
             const deletes = await Promise.all(
-                workspaces.map((ws) => this.deletionService.hardDeleteWorkspace({ span }, ws.id)),
+                workspaces.map((ws) => this.workspaceService.hardDeleteWorkspace(ws.ownerId, ws.id)), // TODO(gpl) This should be a system user/service account instead of ws owner
             );
 
             log.info(`workspace-gc: successfully purged ${deletes.length} workspaces`);

components/server/src/test/expect-utils.ts

@@ -7,14 +7,15 @@
 import { ApplicationError, ErrorCode } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { expect } from "chai";
 
-export async function expectError(errorCode: ErrorCode, code: Promise<any> | (() => Promise<any>)) {
+export async function expectError(errorCode: ErrorCode, code: Promise<any> | (() => Promise<any>), message?: string) {
+    const msg = "expected error: " + errorCode + (message ? " - " + message : "");
     try {
         await (code instanceof Function ? code() : code);
-        expect.fail("expected error: " + errorCode);
+        expect.fail(msg);
     } catch (err) {
         if (!ApplicationError.hasErrorCode(err)) {
             throw err;
         }
-        expect(err && ApplicationError.hasErrorCode(err) && err.code).to.equal(errorCode);
+        expect(err && ApplicationError.hasErrorCode(err) && err.code, msg).to.equal(errorCode);
     }
 }

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

@@ -130,7 +130,6 @@ import { UserDeletionService } from "../user/user-deletion-service";
 import { UserAuthentication } from "../user/user-authentication";
 import { ContextParser } from "./context-parser-service";
 import { GitTokenScopeGuesser } from "./git-token-scope-guesser";
-import { WorkspaceDeletionService } from "./workspace-deletion-service";
 import { WorkspaceStarter, isClusterMaintenanceError } from "./workspace-starter";
 import { HeadlessLogUrls } from "@gitpod/gitpod-protocol/lib/headless-workspace-log";
 import { HeadlessLogService, HeadlessLogEndpoint } from "./headless-log-service";
@@ -213,7 +212,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
     constructor(
         @inject(Config) private readonly config: Config,
         @inject(TracedWorkspaceDB) private readonly workspaceDb: DBWithTracing<WorkspaceDB>,
-        @inject(WorkspaceDeletionService) private readonly workspaceDeletionService: WorkspaceDeletionService,
         @inject(ContextParser) private contextParser: ContextParser,
         @inject(HostContextProvider) private readonly hostContextProvider: HostContextProvider,
 
@@ -1116,8 +1114,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         // for good measure, try and stop running instances
         await this.internalStopWorkspace(ctx, user.id, ws, "deleted via API");
 
-        // actually delete the workspace
-        await this.workspaceDeletionService.softDeleteWorkspace(ctx, ws, "user");
+        await this.workspaceService.deleteWorkspace(user.id, workspaceId, "user");
     }
 
     public async setWorkspaceDescription(ctx: TraceContext, workspaceId: string, description: string): Promise<void> {
@@ -1460,7 +1457,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
             try {
                 await this.guardAccess({ kind: "workspace", subject: workspace }, "create");
             } catch (err) {
-                await this.workspaceDeletionService.hardDeleteWorkspace(ctx, workspace.id);
+                await this.workspaceService.hardDeleteWorkspace(user.id, workspace.id);
                 throw err;
             }
 

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

@@ -5,7 +5,6 @@
  */
 
 import { inject, injectable } from "inversify";
-import { WorkspaceSoftDeletion } from "@gitpod/gitpod-protocol";
 import {
     WorkspaceDB,
     WorkspaceAndOwner,
@@ -17,7 +16,6 @@ import { StorageClient } from "../storage/storage-client";
 import { Config } from "../config";
 import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";
 import { WorkspaceManagerClientProvider } from "@gitpod/ws-manager/lib/client-provider";
-import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 
 @injectable()
 export class WorkspaceDeletionService {
@@ -27,36 +25,6 @@ export class WorkspaceDeletionService {
     @inject(WorkspaceManagerClientProvider)
     protected readonly workspaceManagerClientProvider: WorkspaceManagerClientProvider;
 
-    /**
-     * This method does nothing beyond marking the given workspace as 'softDeleted' with the given cause and sets the 'softDeletedTime' to now.
-     * The actual deletion happens as part of the regular workspace garbage collection.
-     * @param ctx
-     * @param ws
-     * @param softDeleted
-     */
-    public async softDeleteWorkspace(
-        ctx: TraceContext,
-        ws: WorkspaceAndOwner,
-        softDeleted: WorkspaceSoftDeletion,
-    ): Promise<void> {
-        await this.db.trace(ctx).updatePartial(ws.id, {
-            softDeleted,
-            softDeletedTime: new Date().toISOString(),
-        });
-    }
-
-    /**
-     * This *hard deletes* the workspace entry and all corresponding workspace-instances, by triggering a periodic deleter mechanism that purges it from the DB.
-     * Note: when this function returns that doesn't mean that the entries are actually gone yet, that might still take a short while until periodic deleter comes
-     *       around to deleting them.
-     * @param ctx
-     * @param workspaceId
-     */
-    public async hardDeleteWorkspace(ctx: TraceContext, workspaceId: string): Promise<void> {
-        await this.db.trace(ctx).hardDeleteWorkspace(workspaceId);
-        log.info(`Purged Workspace ${workspaceId} and all WorkspaceInstances for this workspace`, { workspaceId });
-    }
-
     /**
      * This method garbageCollects a workspace. It deletes its contents and sets the workspaces 'contentDeletedTime'
      * @param ctx

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

@@ -107,6 +107,24 @@ describe("WorkspaceService", async () => {
             svc.stopWorkspace(stranger.id, ws.id, "test stranger stopping stopped workspace"),
         );
     });
+
+    it("should deleteWorkspace", async () => {
+        const svc = container.get(WorkspaceService);
+        const ws = await createTestWorkspace(svc, org, owner, project);
+
+        await expectError(
+            ErrorCodes.NOT_FOUND,
+            () => svc.deleteWorkspace(stranger.id, ws.id),
+            "stranger can't delete workspace",
+        );
+
+        await svc.deleteWorkspace(owner.id, ws.id);
+        await expectError(
+            ErrorCodes.NOT_FOUND,
+            () => svc.getWorkspace(owner.id, ws.id),
+            "getWorkspace should return NOT_FOUND after deletion",
+        );
+    });
 });
 
 async function createTestWorkspace(svc: WorkspaceService, org: Organization, owner: User, project: Project) {

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

@@ -6,21 +6,20 @@
 
 import { inject, injectable } from "inversify";
 import { WorkspaceDB } from "@gitpod/gitpod-db/lib";
-import { Project, User, Workspace, WorkspaceContext } from "@gitpod/gitpod-protocol";
+import { Project, User, Workspace, WorkspaceContext, WorkspaceSoftDeletion } from "@gitpod/gitpod-protocol";
 import { ErrorCodes, ApplicationError } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { Authorizer } from "../authorization/authorizer";
 import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";
 import { WorkspaceFactory } from "./workspace-factory";
-import { WorkspaceDeletionService } from "./workspace-deletion-service";
 import { StopWorkspacePolicy } from "@gitpod/ws-manager/lib";
 import { WorkspaceStarter } from "./workspace-starter";
+import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 
 @injectable()
 export class WorkspaceService {
     constructor(
         @inject(WorkspaceFactory) private readonly factory: WorkspaceFactory,
         @inject(WorkspaceStarter) private readonly workspaceStarter: WorkspaceStarter,
-        @inject(WorkspaceDeletionService) private readonly workspaceDeletionService: WorkspaceDeletionService,
         @inject(WorkspaceDB) private readonly db: WorkspaceDB,
         @inject(Authorizer) private readonly auth: Authorizer,
     ) {}
@@ -50,7 +49,7 @@ export class WorkspaceService {
         try {
             await this.auth.createWorkspaceInOrg(organizationId, user.id, workspace.id);
         } catch (err) {
-            await this.workspaceDeletionService.hardDeleteWorkspace(ctx, workspace.id);
+            await this.hardDeleteWorkspace(user.id, workspace.id);
             throw err;
         }
 
@@ -61,7 +60,7 @@ export class WorkspaceService {
         await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
 
         const workspace = await this.db.findById(workspaceId);
-        if (!workspace) {
+        if (!workspace || !!workspace.softDeleted || workspace.deleted) {
             throw new ApplicationError(ErrorCodes.NOT_FOUND, "Workspace not found.");
         }
         return workspace;
@@ -83,4 +82,40 @@ export class WorkspaceService {
         }
         await this.workspaceStarter.stopWorkspaceInstance({}, instance.id, instance.region, reason, policy);
     }
+
+    /**
+     * This method does nothing beyond marking the given workspace as 'softDeleted' with the given cause and sets the 'softDeletedTime' to now.
+     * The actual deletion happens as part of the regular workspace garbage collection.
+     * @param ctx
+     * @param ws
+     * @param softDeleted
+     */
+    async deleteWorkspace(
+        userId: string,
+        workspaceId: string,
+        softDeleted: WorkspaceSoftDeletion = "user",
+    ): Promise<void> {
+        await this.auth.checkPermissionOnWorkspace(userId, "delete", workspaceId);
+
+        await this.stopWorkspace(userId, workspaceId, "deleted via WorkspaceService");
+        await this.db.updatePartial(workspaceId, {
+            softDeleted,
+            softDeletedTime: new Date().toISOString(),
+        });
+    }
+
+    /**
+     * This *hard deletes* the workspace entry and all corresponding workspace-instances, by triggering a periodic deleter mechanism that purges it from the DB.
+     * Note: when this function returns that doesn't mean that the entries are actually gone yet, that might still take a short while until periodic deleter comes
+     *       around to deleting them.
+     * @param ctx
+     * @param userId
+     * @param workspaceId
+     */
+    public async hardDeleteWorkspace(userId: string, workspaceId: string): Promise<void> {
+        await this.auth.checkPermissionOnWorkspace(userId, "delete", workspaceId);
+
+        await this.db.hardDeleteWorkspace(workspaceId);
+        log.info(`Purged Workspace ${workspaceId} and all WorkspaceInstances for this workspace`, { workspaceId });
+    }
 }

components/spicedb/schema/schema.yaml

@@ -92,8 +92,10 @@ schema: |-
     //+ (hasAccessToRepository && isPrebuild) + everyoneIfIsShared
     permission access = owner
 
-    // This is modelled after current behavior
+    // Note: All of this is modelled after current behavior.
+    // There are a lot of improvements we can make here in the light of Organizations, but we explicitly do that as a separate step
     permission stop = owner + org->installation_admin
+    permission delete = owner
 
     // Whether a user can read basic info/metadata of a workspace
     //+ (hasAccessToRepository && isPrebuild) + everyoneIfIsShared