Skip to content

Commit 735bf0e

Browse files
svenefftingesvenefftinge
authored
[fga] more FGA checks and service use (#18517)
1 parent 614007d commit 735bf0e

File tree

7 files changed

+80
-37
lines changed

7 files changed

+80
-37
lines changed

‎components/server/src/authorization/definitions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export type UserResourceType = "user";
3232

3333
export type UserRelation = "self" | "organization" | "installation";
3434

35-
export type UserPermission = "read_info" | "write_info" | "make_admin" | "read_ssh" | "write_ssh";
35+
export type UserPermission = "read_info" | "write_info" | "delete" | "make_admin" | "read_ssh" | "write_ssh";
3636

3737
export type InstallationResourceType = "installation";
3838

‎components/server/src/billing/entitlement-service.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import {
1414
import { AttributionId } from "@gitpod/gitpod-protocol/lib/attribution";
1515
import { BillingTier } from "@gitpod/gitpod-protocol/lib/protocol";
1616
import { inject, injectable } from "inversify";
17-
import { Config } from "../config";
1817
import { BillingModes } from "./billing-mode";
1918
import { EntitlementServiceUBP } from "./entitlement-service-ubp";
2019
import { VerificationService } from "../auth/verification-service";
@@ -94,10 +93,11 @@ export interface EntitlementService {
9493
*/
9594
@injectable()
9695
export class EntitlementServiceImpl implements EntitlementService {
97-
@inject(Config) protected readonly config: Config;
98-
@inject(BillingModes) protected readonly billingModes: BillingModes;
99-
@inject(EntitlementServiceUBP) protected readonly ubp: EntitlementServiceUBP;
100-
@inject(VerificationService) protected readonly verificationService: VerificationService;
96+
constructor(
97+
@inject(BillingModes) private readonly billingModes: BillingModes,
98+
@inject(EntitlementServiceUBP) private readonly ubp: EntitlementServiceUBP,
99+
@inject(VerificationService) private readonly verificationService: VerificationService,
100+
) {}
101101

102102
async mayStartWorkspace(
103103
user: User,

‎components/server/src/user/user-deletion-service.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { StopWorkspacePolicy } from "@gitpod/ws-manager/lib";
1313
import { AuthProviderService } from "../auth/auth-provider-service";
1414
import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
1515
import { WorkspaceService } from "../workspace/workspace-service";
16+
import { Authorizer } from "../authorization/authorizer";
1617

1718
@injectable()
1819
export class UserDeletionService {
@@ -25,6 +26,7 @@ export class UserDeletionService {
2526
@inject(WorkspaceService) private readonly workspaceService: WorkspaceService,
2627
@inject(AuthProviderService) private readonly authProviderService: AuthProviderService,
2728
@inject(IAnalyticsWriter) private readonly analytics: IAnalyticsWriter,
29+
@inject(Authorizer) private readonly authorizer: Authorizer,
2830
) {}
2931

3032
/**
@@ -34,6 +36,7 @@ export class UserDeletionService {
3436
* we anonymize data that might contain user related/relatable data and keep the entities itself (incl. ids).
3537
*/
3638
async deleteUser(userId: string, targetUserId: string): Promise<void> {
39+
await this.authorizer.checkPermissionOnUser(userId, "delete", targetUserId);
3740
const user = await this.db.findUserById(targetUserId);
3841
if (!user) {
3942
throw new Error(`No user with id ${targetUserId} found!`);
@@ -159,7 +162,7 @@ export class UserDeletionService {
159162
await Promise.all(ownedTeams.map((t) => this.teamDb.deleteTeam(t.id)));
160163
}
161164

162-
anonymizeWorkspace(ws: Workspace) {
165+
private anonymizeWorkspace(ws: Workspace) {
163166
ws.context.title = "deleted-title";
164167
ws.context.normalizedContextURL = "deleted-normalizedContextURL";
165168
ws.contextURL = "deleted-contextURL";

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

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
649649
public async maySetTimeout(ctx: TraceContext): Promise<boolean> {
650650
const user = await this.checkUser("maySetTimeout");
651651
await this.guardAccess({ kind: "user", subject: user }, "get");
652+
await this.auth.checkPermissionOnUser(user.id, "read_info", user.id);
652653

653654
return await this.entitlementService.maySetTimeout(user.id);
654655
}
@@ -874,13 +875,14 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
874875

875876
const user = await this.checkAndBlockUser("getOwnerToken");
876877

877-
const workspace = await this.workspaceDb.trace(ctx).findById(workspaceId);
878+
//TODO this requests are only here to populate the resource guard check
879+
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
878880
if (!workspace) {
879881
throw new Error("owner token not found");
880882
}
881883
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
882884

883-
const latestInstance = await this.workspaceDb.trace(ctx).findCurrentInstance(workspaceId);
885+
const latestInstance = await this.workspaceService.getCurrentInstance(user.id, workspaceId);
884886
await this.guardAccess({ kind: "workspaceInstance", subject: latestInstance, workspace }, "get");
885887

886888
return await this.workspaceService.getOwnerToken(user.id, workspaceId);
@@ -892,6 +894,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
892894

893895
const user = await this.checkAndBlockUser("getIDECredentials");
894896

897+
//TODO this requests are only here to populate the resource guard check
895898
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
896899
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
897900

@@ -913,18 +916,18 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
913916
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
914917

915918
// (gpl) We keep this check here for backwards compatibility, it should be superfluous in the future
916-
const runningInstance = await this.workspaceDb.trace(ctx).findRunningInstance(workspace.id);
917-
if (runningInstance) {
918-
traceWI(ctx, { instanceId: runningInstance.id });
919+
const instance = await this.workspaceService.getCurrentInstance(user.id, workspace.id);
920+
if (instance && instance.status.phase !== "stopped") {
921+
traceWI(ctx, { instanceId: instance.id });
919922

920923
// We already have a running workspace.
921924
// Note: ownership doesn't matter here as this is basically a noop. It's not StartWorkspace's concern
922925
// to guard workspace access - just to prevent non-owners from starting workspaces.
923926

924-
await this.guardAccess({ kind: "workspaceInstance", subject: runningInstance, workspace }, "get");
927+
await this.guardAccess({ kind: "workspaceInstance", subject: instance, workspace }, "get");
925928
return {
926-
instanceID: runningInstance.id,
927-
workspaceURL: runningInstance.ideUrl,
929+
instanceID: instance.id,
930+
workspaceURL: instance.ideUrl,
928931
};
929932
}
930933

@@ -1028,24 +1031,22 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10281031

10291032
const user = await this.checkAndBlockUser("updateWorkspaceUserPin");
10301033

1031-
await this.workspaceDb.trace(ctx).transaction(async (db) => {
1032-
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
1033-
await this.guardAccess({ kind: "workspace", subject: ws }, "update");
1034-
1035-
switch (action) {
1036-
case "pin":
1037-
ws.pinned = true;
1038-
break;
1039-
case "unpin":
1040-
ws.pinned = false;
1041-
break;
1042-
case "toggle":
1043-
ws.pinned = !ws.pinned;
1044-
break;
1045-
}
1034+
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
1035+
await this.guardAccess({ kind: "workspace", subject: ws }, "update");
10461036

1047-
await db.store(ws);
1048-
});
1037+
switch (action) {
1038+
case "pin":
1039+
ws.pinned = true;
1040+
break;
1041+
case "unpin":
1042+
ws.pinned = false;
1043+
break;
1044+
case "toggle":
1045+
ws.pinned = !ws.pinned;
1046+
break;
1047+
}
1048+
1049+
await this.workspaceService.setPinned(user.id, ws.id, ws.pinned);
10491050
}
10501051

10511052
public async deleteWorkspace(ctx: TraceContext, workspaceId: string): Promise<void> {
@@ -1057,9 +1058,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10571058
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
10581059
await this.guardAccess({ kind: "workspace", subject: ws }, "delete");
10591060

1060-
// for good measure, try and stop running instances
1061-
await this.internalStopWorkspace(ctx, user.id, ws, "deleted via API");
1062-
10631061
await this.workspaceService.deleteWorkspace(user.id, workspaceId, "user");
10641062
}
10651063

@@ -1070,9 +1068,9 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10701068
const user = await this.checkAndBlockUser("setWorkspaceDescription");
10711069

10721070
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1073-
10741071
await this.guardAccess({ kind: "workspace", subject: workspace }, "update");
1075-
await this.workspaceDb.trace(ctx).updatePartial(workspaceId, { description });
1072+
1073+
await this.workspaceService.setDescription(user.id, workspaceId, description);
10761074
}
10771075

10781076
public async getWorkspaces(

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,28 @@ describe("WorkspaceService", async () => {
237237
"getWorkspace should return NOT_FOUND after hard-deletion",
238238
);
239239
});
240+
241+
it("should setPinned", async () => {
242+
const svc = container.get(WorkspaceService);
243+
const ws = await createTestWorkspace(svc, org, owner, project);
244+
245+
await expectError(ErrorCodes.NOT_FOUND, svc.setPinned(stranger.id, ws.id, true));
246+
await svc.setPinned(owner.id, ws.id, true);
247+
const ws2 = await svc.getWorkspace(owner.id, ws.id);
248+
expect(ws2.pinned, "workspace should be pinned").to.equal(true);
249+
});
250+
251+
it("should setDescription", async () => {
252+
const svc = container.get(WorkspaceService);
253+
const ws = await createTestWorkspace(svc, org, owner, project);
254+
const desc = "Some description";
255+
256+
await svc.setDescription(owner.id, ws.id, desc);
257+
const ws2 = await svc.getWorkspace(owner.id, ws.id);
258+
expect(ws2.description).to.equal(desc);
259+
260+
await expectError(ErrorCodes.NOT_FOUND, svc.setDescription(stranger.id, ws.id, desc));
261+
});
240262
});
241263

242264
async function createTestWorkspace(svc: WorkspaceService, org: Organization, owner: User, project: Project) {

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,15 @@ export class WorkspaceService {
8888
return this.doGetWorkspace(userId, workspaceId);
8989
}
9090

91+
async getCurrentInstance(userId: string, workspaceId: string): Promise<WorkspaceInstance | undefined> {
92+
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
93+
const result = await this.db.findCurrentInstance(workspaceId);
94+
if (!result) {
95+
throw new ApplicationError(ErrorCodes.NOT_FOUND, "No workspace instance found.", { workspaceId });
96+
}
97+
return result;
98+
}
99+
91100
// Internal method for allowing for additional DBs to be passed in
92101
private async doGetWorkspace(userId: string, workspaceId: string, db: WorkspaceDB = this.db): Promise<Workspace> {
93102
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
@@ -378,4 +387,14 @@ export class WorkspaceService {
378387

379388
return targetRegion;
380389
}
390+
391+
public async setPinned(userId: string, workspaceId: string, pinned: boolean): Promise<void> {
392+
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
393+
await this.db.updatePartial(workspaceId, { pinned });
394+
}
395+
396+
public async setDescription(userId: string, workspaceId: string, description: string) {
397+
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
398+
await this.db.updatePartial(workspaceId, { description });
399+
}
381400
}

‎components/spicedb/schema/schema.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ schema: |-
1313
// permissions
1414
permission read_info = self + organization->member + organization->owner + installation->admin
1515
permission write_info = self
16+
permission delete = self
1617
permission make_admin = installation->admin + organization->installation_admin
1718
1819
permission read_ssh = self

0 commit comments

Comments
 (0)