Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[server] fix deleteUser permissions #18989

Merged
merged 1 commit into from
Oct 31, 2023
Merged

[server] fix deleteUser permissions #18989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 15 additions & 80 deletions components/server/src/user/user-deletion-service.ts
View file
Open in desktop
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
34 changes: 33 additions & 1 deletion components/server/src/user/user-service.spec.db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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({
Expand Down Expand Up @@ -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 () => {
svenefftinge marked this conversation as resolved.
Show resolved Hide resolved
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: "[email protected]",
},
});
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;
});
});
72 changes: 72 additions & 0 deletions components/server/src/user/user-service.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading