Skip to content

Commit

Permalink
[server] fix deleteUser permissions
Browse files Browse the repository at this point in the history
  • Loading branch information
@svenefftinge
svenefftinge committed Oct 31, 2023
1 parent 15d2d34 commit 435969b
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 85 deletions.
95 changes: 15 additions & 80 deletions components/server/src/user/user-deletion-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
{},
Expand All @@ -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) {
Expand Down
33 changes: 33 additions & 0 deletions components/server/src/user/user-service.spec.db.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe("UserService", async () => {
let user: User;
let user2: User;
let nonOrgUser: User;
let orgOwner: User;

beforeEach(async () => {
container = createTestContainer();
Expand Down Expand Up @@ -69,6 +70,18 @@ describe("UserService", async () => {
primaryEmail: "[email protected]",
},
});

orgOwner = await userService.createUser({
organizationId: org.id,
identity: {
authId: "foo",
authName: "bar",
authProviderId: "github",
primaryEmail: "[email protected]",
},
});
await orgService.joinOrganization(orgOwner.id, invite.id);
await orgService.addOrUpdateMember(BUILTIN_INSTLLATION_ADMIN_USER_ID, org.id, orgOwner.id, "owner");
});

afterEach(async () => {
Expand Down Expand Up @@ -203,4 +216,24 @@ 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
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;
});
});
72 changes: 72 additions & 0 deletions components/server/src/user/user-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { UserDB } from "@gitpod/gitpod-db/lib";
import { Authorizer } from "../authorization/authorizer";
import {
AdditionalUserData,
Disposable,
Identity,
RoleOrPermission,
TokenEntry,
Expand Down Expand Up @@ -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",
},
});
}
}
6 changes: 2 additions & 4 deletions components/server/src/workspace/gitpod-server-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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> {
Expand Down
2 changes: 1 addition & 1 deletion components/spicedb/schema/schema.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 435969b

Please sign in to comment.