[server] fix deleteUser permissions (#18989)

svenefftinge · web-flow · commit 0e897af45403 · 2023-10-31T21:30:38.000+02:00
diff --git a/components/server/src/user/user-deletion-service.ts b/components/server/src/user/user-deletion-service.ts
@@ -5,48 +5,33 @@
  */
 
 import { injectable, inject } from "inversify";
-import { UserDB, WorkspaceDB, TeamDB, ProjectDB } from "@gitpod/gitpod-db/lib";
+import { WorkspaceDB, TeamDB, ProjectDB } from "@gitpod/gitpod-db/lib";
 import { User, Workspace } from "@gitpod/gitpod-protocol";
 import { StorageClient } from "../storage/storage-client";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { StopWorkspacePolicy } from "@gitpod/ws-manager/lib";
 import { AuthProviderService } from "../auth/auth-provider-service";
-import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
 import { WorkspaceService } from "../workspace/workspace-service";
-import { Authorizer } from "../authorization/authorizer";
-import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
+import { UserService } from "./user-service";
+import { TransactionalContext } from "@gitpod/gitpod-db/lib/typeorm/transactional-db-impl";
 
 @injectable()
 export class UserDeletionService {
     constructor(
-        @inject(UserDB) private readonly db: UserDB,
+        @inject(UserService) private readonly userService: UserService,
         @inject(WorkspaceDB) private readonly workspaceDb: WorkspaceDB,
         @inject(TeamDB) private readonly teamDb: TeamDB,
         @inject(ProjectDB) private readonly projectDb: ProjectDB,
         @inject(StorageClient) private readonly storageClient: StorageClient,
         @inject(WorkspaceService) private readonly workspaceService: WorkspaceService,
         @inject(AuthProviderService) private readonly authProviderService: AuthProviderService,
-        @inject(IAnalyticsWriter) private readonly analytics: IAnalyticsWriter,
-        @inject(Authorizer) private readonly authorizer: Authorizer,
-    ) {}
-
-    /**
-     * This method deletes a User logically. The contract here is that after running this method without receiving an
-     * error, the system does not contain any data that is relatable to the actual person in the sense of the GDPR.
-     * To guarantee that, but also maintain traceability
-     * we anonymize data that might contain user related/relatable data and keep the entities itself (incl. ids).
-     */
-    async deleteUser(userId: string, targetUserId: string): Promise<void> {
-        await this.authorizer.checkPermissionOnUser(userId, "delete", targetUserId);
-        const user = await this.db.findUserById(targetUserId);
-        if (!user) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, `No user with id ${targetUserId} found!`);
-        }
-
-        if (user.markedDeleted === true) {
-            log.debug({ userId: targetUserId }, "Is deleted but markDeleted already set. Continuing.");
-        }
+    ) {
+        this.userService.onDeleteUser(async (subjectId, user, ctx) => {
+            await this.contributeToDeleteUser(subjectId, user, ctx);
+        });
+    }
 
+    private async contributeToDeleteUser(userId: string, user: User, ctx: TransactionalContext): Promise<void> {
         // Stop all workspaces
         await this.workspaceService.stopRunningWorkspacesForUser(
             {},
@@ -62,70 +47,20 @@ export class UserDeletionService {
             try {
                 await this.authProviderService.deleteAuthProvider(provider);
             } catch (error) {
-                log.error({ userId: targetUserId }, "Failed to delete user's auth provider.", error);
+                log.error({ userId: user.id }, "Failed to delete user's auth provider.", error);
             }
         }
 
-        // User
-        await this.db.transaction(async (db) => {
-            this.anonymizeUser(user);
-            this.deleteIdentities(user);
-            await this.deleteTokens(db, user);
-            user.lastVerificationTime = undefined;
-            user.markedDeleted = true;
-            await db.storeUser(user);
-        });
-
         await Promise.all([
             // Workspace
-            this.anonymizeAllWorkspaces(targetUserId),
+            this.anonymizeAllWorkspaces(user.id),
             // Bucket
-            this.deleteUserBucket(targetUserId),
+            this.deleteUserBucket(user.id),
             // Teams owned only by this user
-            this.deleteSoleOwnedTeams(targetUserId),
+            this.deleteSoleOwnedTeams(user.id),
             // Team memberships
-            this.deleteTeamMemberships(targetUserId),
+            this.deleteTeamMemberships(user.id),
         ]);
-
-        // Track the deletion Event for Analytics Purposes
-        this.analytics.track({
-            userId: user.id,
-            event: "deletion",
-            properties: {
-                deleted_at: new Date().toISOString(),
-            },
-        });
-        this.analytics.identify({
-            userId: user.id,
-            traits: {
-                github_slug: "deleted-user",
-                gitlab_slug: "deleted-user",
-                bitbucket_slug: "deleted-user",
-                email: "deleted-user",
-                full_name: "deleted-user",
-                name: "deleted-user",
-            },
-        });
-    }
-
-    private anonymizeUser(user: User) {
-        user.avatarUrl = "deleted-avatarUrl";
-        user.fullName = "deleted-fullName";
-        user.name = "deleted-Name";
-        if (user.verificationPhoneNumber) {
-            user.verificationPhoneNumber = "deleted-phoneNumber";
-        }
-    }
-
-    private deleteIdentities(user: User) {
-        for (const identity of user.identities) {
-            identity.deleted = true; // This triggers the HARD DELETION of the identity
-        }
-    }
-
-    private async deleteTokens(db: UserDB, user: User) {
-        const tokenDeletions = user.identities.map((identity) => db.deleteTokens(identity));
-        await Promise.all(tokenDeletions);
     }
 
     private async anonymizeAllWorkspaces(userId: string) {
diff --git a/components/server/src/user/user-service.spec.db.ts b/components/server/src/user/user-service.spec.db.ts
@@ -23,6 +23,7 @@ const expect = chai.expect;
 describe("UserService", async () => {
     let container: Container;
     let userService: UserService;
+    let orgService: OrganizationService;
     let auth: Authorizer;
     let org: Organization;
     let user: User;
@@ -36,7 +37,7 @@ describe("UserService", async () => {
         });
         userService = container.get<UserService>(UserService);
         auth = container.get(Authorizer);
-        const orgService = container.get<OrganizationService>(OrganizationService);
+        orgService = container.get<OrganizationService>(OrganizationService);
         org = await orgService.createOrganization(BUILTIN_INSTLLATION_ADMIN_USER_ID, "myOrg");
         const invite = await orgService.getOrCreateInvite(BUILTIN_INSTLLATION_ADMIN_USER_ID, org.id);
         user = await userService.createUser({
@@ -203,4 +204,35 @@ describe("UserService", async () => {
         expect(users.total).to.eq(1);
         expect(users.rows.some((u) => u.id === nonOrgUser.id)).to.be.true;
     });
+
+    it("should delete user", async () => {
+        await expectError(ErrorCodes.NOT_FOUND, userService.deleteUser(nonOrgUser.id, user2.id));
+        await expectError(ErrorCodes.PERMISSION_DENIED, userService.deleteUser(user.id, user2.id));
+        // user can delete themselves
+        await userService.deleteUser(user.id, user.id);
+        user = await userService.findUserById(user.id, user.id);
+        expect(user.markedDeleted).to.be.true;
+
+        // org owners can delete users owned by org
+        const orgOwner = await userService.createUser({
+            organizationId: org.id,
+            identity: {
+                authId: "foo",
+                authName: "bar",
+                authProviderId: "github",
+                primaryEmail: "yolo@yolo.com",
+            },
+        });
+        await orgService.addOrUpdateMember(BUILTIN_INSTLLATION_ADMIN_USER_ID, org.id, orgOwner.id, "owner");
+
+        await expectError(ErrorCodes.NOT_FOUND, userService.deleteUser(orgOwner.id, nonOrgUser.id));
+        await userService.deleteUser(orgOwner.id, user2.id);
+        user2 = await userService.findUserById(orgOwner.id, user2.id);
+        expect(user2.markedDeleted).to.be.true;
+
+        // admins can delete any user
+        await userService.deleteUser(BUILTIN_INSTLLATION_ADMIN_USER_ID, nonOrgUser.id);
+        nonOrgUser = await userService.findUserById(BUILTIN_INSTLLATION_ADMIN_USER_ID, nonOrgUser.id);
+        expect(nonOrgUser.markedDeleted).to.be.true;
+    });
 });
diff --git a/components/server/src/user/user-service.ts b/components/server/src/user/user-service.ts
@@ -10,6 +10,7 @@ import { UserDB } from "@gitpod/gitpod-db/lib";
 import { Authorizer } from "../authorization/authorizer";
 import {
     AdditionalUserData,
+    Disposable,
     Identity,
     RoleOrPermission,
     TokenEntry,
@@ -230,4 +231,75 @@ export class UserService {
 
         await this.userDb.updateUserPartial({ id: userId, fgaRelationshipsVersion: undefined });
     }
+
+    private onDeleteListeners = new Set<
+        (subjectId: string, user: User, transactionCtx: TransactionalContext) => Promise<void>
+    >();
+    public onDeleteUser(
+        handler: (subjectId: string, user: User, transactionCtx: TransactionalContext) => Promise<void>,
+    ): Disposable {
+        this.onDeleteListeners.add(handler);
+        return {
+            dispose: () => {
+                this.onDeleteListeners.delete(handler);
+            },
+        };
+    }
+
+    /**
+     * This method deletes a User logically. The contract here is that after running this method without receiving an
+     * error, the system does not contain any data that is relatable to the actual person in the sense of the GDPR.
+     * To guarantee that, but also maintain traceability
+     * we anonymize data that might contain user related/relatable data and keep the entities itself (incl. ids).
+     */
+    async deleteUser(subjectId: string, targetUserId: string) {
+        await this.authorizer.checkPermissionOnUser(subjectId, "delete", targetUserId);
+
+        await this.userDb.transaction(async (db, ctx) => {
+            const user = await this.userDb.findUserById(targetUserId);
+            if (!user) {
+                throw new ApplicationError(ErrorCodes.NOT_FOUND, `No user with id ${targetUserId} found!`);
+            }
+
+            if (user.markedDeleted === true) {
+                log.debug({ userId: targetUserId }, "Is deleted but markDeleted already set. Continuing.");
+            }
+            for (const listener of this.onDeleteListeners) {
+                await listener(subjectId, user, ctx);
+            }
+            user.avatarUrl = "deleted-avatarUrl";
+            user.fullName = "deleted-fullName";
+            user.name = "deleted-Name";
+            if (user.verificationPhoneNumber) {
+                user.verificationPhoneNumber = "deleted-phoneNumber";
+            }
+            for (const identity of user.identities) {
+                identity.deleted = true;
+                await db.deleteTokens(identity);
+            }
+            user.lastVerificationTime = undefined;
+            user.markedDeleted = true;
+            await db.storeUser(user);
+        });
+
+        // Track the deletion Event for Analytics Purposes
+        this.analytics.track({
+            userId: targetUserId,
+            event: "deletion",
+            properties: {
+                deleted_at: new Date().toISOString(),
+            },
+        });
+        this.analytics.identify({
+            userId: targetUserId,
+            traits: {
+                github_slug: "deleted-user",
+                gitlab_slug: "deleted-user",
+                bitbucket_slug: "deleted-user",
+                email: "deleted-user",
+                full_name: "deleted-user",
+                name: "deleted-user",
+            },
+        });
+    }
 }
diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts
@@ -111,7 +111,6 @@ import { NotFoundError, UnauthorizedError } from "../errors";
 import { RepoURL } from "../repohost/repo-url";
 import { AuthorizationService } from "../user/authorization-service";
 import { TokenProvider } from "../user/token-provider";
-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";
@@ -214,7 +213,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         @inject(TokenProvider) private readonly tokenProvider: TokenProvider,
         @inject(UserAuthentication) private readonly userAuthentication: UserAuthentication,
         @inject(UserService) private readonly userService: UserService,
-        @inject(UserDeletionService) private readonly userDeletionService: UserDeletionService,
         @inject(IAnalyticsWriter) private readonly analytics: IAnalyticsWriter,
         @inject(AuthorizationService) private readonly authorizationService: AuthorizationService,
         @inject(SSHKeyService) private readonly sshKeyservice: SSHKeyService,
@@ -710,7 +708,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         const user = await this.checkAndBlockUser("deleteAccount");
         await this.guardAccess({ kind: "user", subject: user! }, "delete");
 
-        await this.userDeletionService.deleteUser(user.id, user.id);
+        await this.userService.deleteUser(user.id, user.id);
     }
 
     public async getWorkspace(ctx: TraceContext, workspaceId: string): Promise<WorkspaceInfo> {
@@ -2689,7 +2687,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
 
         const admin = await this.guardAdminAccess("adminDeleteUser", { id: userId }, Permission.ADMIN_PERMISSIONS);
 
-        await this.userDeletionService.deleteUser(admin.id, userId);
+        await this.userService.deleteUser(admin.id, userId);
     }
 
     async adminVerifyUser(ctx: TraceContext, userId: string): Promise<User> {
diff --git a/components/spicedb/schema/schema.yaml b/components/spicedb/schema/schema.yaml
@@ -13,7 +13,7 @@ schema: |-
     // permissions
     permission read_info = self + organization->member + organization->owner + installation->admin
     permission write_info = self
-    permission delete = self
+    permission delete = self + organization->owner + installation->admin
 
     permission make_admin = installation->admin + organization->installation_admin
 
