Skip to content

Commit 930fe77

Browse files
geroplgeropl
authored
[fga] WorkspaceService.getWorkspace(s) (#18565)
* [server] WorkspaceService.getWorkspace(s) * review comments
1 parent 25c949e commit 930fe77

File tree

3 files changed

+112
-60
lines changed

3 files changed

+112
-60
lines changed

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

Lines changed: 30 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -842,11 +842,11 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
842842

843843
const user = await this.checkUser("getWorkspace");
844844

845-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
846-
const latestInstancePromise = this.workspaceDb.trace(ctx).findCurrentInstance(workspaceId);
845+
const result = await this.workspaceService.getWorkspace(user.id, workspaceId);
846+
const { workspace, latestInstance } = result;
847+
847848
const teamMembers = await this.organizationService.listMembers(user.id, workspace.organizationId);
848849
await this.guardAccess({ kind: "workspace", subject: workspace, teamMembers: teamMembers }, "get");
849-
const latestInstance = await latestInstancePromise;
850850
if (!!latestInstance) {
851851
await this.guardAccess(
852852
{
@@ -860,8 +860,8 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
860860
}
861861

862862
return {
863-
workspace,
864-
latestInstance: this.censorInstance(latestInstance),
863+
...result,
864+
latestInstance: this.censorInstance(result.latestInstance),
865865
};
866866
}
867867

@@ -872,10 +872,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
872872
const user = await this.checkAndBlockUser("getOwnerToken");
873873

874874
//TODO this requests are only here to populate the resource guard check
875-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
876-
if (!workspace) {
877-
throw new Error("owner token not found");
878-
}
875+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
879876
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
880877

881878
const latestInstance = await this.workspaceService.getCurrentInstance(user.id, workspaceId);
@@ -891,7 +888,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
891888
const user = await this.checkAndBlockUser("getIDECredentials");
892889

893890
//TODO this requests are only here to populate the resource guard check
894-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
891+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
895892
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
896893

897894
return await this.workspaceService.getIDECredentials(user.id, workspaceId);
@@ -908,11 +905,10 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
908905
const user = await this.checkAndBlockUser("startWorkspace", undefined, { workspaceId });
909906

910907
// (gpl) We keep this check here for backwards compatibility, it should be superfluous in the future
911-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
908+
const { workspace, latestInstance: instance } = await this.workspaceService.getWorkspace(user.id, workspaceId);
912909
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
913910

914911
// (gpl) We keep this check here for backwards compatibility, it should be superfluous in the future
915-
const instance = await this.workspaceService.getCurrentInstance(user.id, workspace.id);
916912
if (instance && instance.status.phase !== "stopped") {
917913
traceWI(ctx, { instanceId: instance.id });
918914

@@ -947,7 +943,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
947943
const user = await this.checkUser("stopWorkspace", undefined, { workspaceId });
948944
const logCtx = { userId: user.id, workspaceId };
949945

950-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
946+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
951947
if (workspace.type === "prebuild") {
952948
// If this is a team prebuild, any team member can stop it.
953949
const teamMembers = await this.organizationService.listMembers(user.id, workspace.organizationId);
@@ -1027,22 +1023,22 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10271023

10281024
const user = await this.checkAndBlockUser("updateWorkspaceUserPin");
10291025

1030-
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
1031-
await this.guardAccess({ kind: "workspace", subject: ws }, "update");
1026+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
1027+
await this.guardAccess({ kind: "workspace", subject: workspace }, "update");
10321028

10331029
switch (action) {
10341030
case "pin":
1035-
ws.pinned = true;
1031+
workspace.pinned = true;
10361032
break;
10371033
case "unpin":
1038-
ws.pinned = false;
1034+
workspace.pinned = false;
10391035
break;
10401036
case "toggle":
1041-
ws.pinned = !ws.pinned;
1037+
workspace.pinned = !workspace.pinned;
10421038
break;
10431039
}
10441040

1045-
await this.workspaceService.setPinned(user.id, ws.id, ws.pinned);
1041+
await this.workspaceService.setPinned(user.id, workspace.id, workspace.pinned);
10461042
}
10471043

10481044
public async deleteWorkspace(ctx: TraceContext, workspaceId: string): Promise<void> {
@@ -1051,8 +1047,8 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10511047

10521048
const user = await this.checkAndBlockUser("deleteWorkspace");
10531049

1054-
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
1055-
await this.guardAccess({ kind: "workspace", subject: ws }, "delete");
1050+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
1051+
await this.guardAccess({ kind: "workspace", subject: workspace }, "delete");
10561052

10571053
await this.workspaceService.deleteWorkspace(user.id, workspaceId, "user");
10581054
}
@@ -1063,7 +1059,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10631059

10641060
const user = await this.checkAndBlockUser("setWorkspaceDescription");
10651061

1066-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1062+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
10671063
await this.guardAccess({ kind: "workspace", subject: workspace }, "update");
10681064

10691065
await this.workspaceService.setDescription(user.id, workspaceId, description);
@@ -1077,22 +1073,17 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
10771073

10781074
const user = await this.checkUser("getWorkspaces");
10791075

1080-
const res = await this.workspaceDb.trace(ctx).find({
1081-
limit: 20,
1082-
...options,
1083-
userId: user.id,
1084-
includeHeadless: false,
1085-
});
1086-
await Promise.all(res.map((ws) => this.guardAccess({ kind: "workspace", subject: ws.workspace }, "get")));
1076+
const result = await this.workspaceService.getWorkspaces(user.id, options);
1077+
await Promise.all(result.map((ws) => this.guardAccess({ kind: "workspace", subject: ws.workspace }, "get")));
10871078
await Promise.all(
1088-
res.map((ws) =>
1079+
result.map((ws) =>
10891080
this.guardAccess(
10901081
{ kind: "workspaceInstance", subject: ws.latestInstance, workspace: ws.workspace },
10911082
"get",
10921083
),
10931084
),
10941085
);
1095-
return res;
1086+
return result;
10961087
}
10971088

10981089
public async isWorkspaceOwner(ctx: TraceContext, workspaceId: string): Promise<boolean> {
@@ -1101,7 +1092,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
11011092

11021093
const user = await this.checkUser("isWorkspaceOwner", undefined, { workspaceId });
11031094

1104-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1095+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
11051096
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
11061097
return user.id == workspace.ownerId;
11071098
}
@@ -1124,7 +1115,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
11241115

11251116
const user = await this.checkUser("getWorkspaceOwner");
11261117

1127-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1118+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
11281119
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
11291120

11301121
try {
@@ -1145,7 +1136,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
11451136

11461137
const user = await this.checkAndBlockUser("getWorkspaceUsers", undefined, { workspaceId });
11471138

1148-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1139+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
11491140
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
11501141

11511142
// Note: there's no need to try and guard the users below, they're not complete users but just enough to
@@ -1798,7 +1789,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
17981789

17991790
const user = await this.checkAndBlockUser("updateGitStatus");
18001791

1801-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1792+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
18021793
const instance = await this.workspaceService.getCurrentInstance(user.id, workspaceId);
18031794
traceWI(ctx, { instanceId: instance.id });
18041795
await this.guardAccess({ kind: "workspaceInstance", subject: instance, workspace }, "update");
@@ -1816,7 +1807,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
18161807

18171808
const user = await this.checkAndBlockUser("openPort");
18181809

1819-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1810+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
18201811
const runningInstance = await this.workspaceDb.trace(ctx).findRunningInstance(workspaceId);
18211812
if (!runningInstance) {
18221813
log.debug({ userId: user.id, workspaceId }, "Cannot open port for workspace with no running instance", {
@@ -1887,12 +1878,13 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
18871878
});
18881879
}
18891880

1881+
// TODO(gpl): Remove after FGA rollout
18901882
private async internGetCurrentWorkspaceInstance(
18911883
ctx: TraceContext,
18921884
user: User,
18931885
workspaceId: string,
18941886
): Promise<{ workspace: Workspace; instance: WorkspaceInstance | undefined }> {
1895-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
1887+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
18961888

18971889
const instance = await this.workspaceDb.trace(ctx).findRunningInstance(workspaceId);
18981890
return { instance, workspace };
@@ -2017,7 +2009,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
20172009

20182010
async getWorkspaceEnvVars(ctx: TraceContext, workspaceId: string): Promise<EnvVarWithValue[]> {
20192011
const user = await this.checkUser("getWorkspaceEnvVars");
2020-
const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);
2012+
const { workspace } = await this.workspaceService.getWorkspace(user.id, workspaceId);
20212013
await this.guardAccess({ kind: "workspace", subject: workspace }, "get");
20222014
const envVars = await this.envVarService.resolveEnvVariables(
20232015
workspace.ownerId,

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

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,8 @@ describe("WorkspaceService", async () => {
183183
const svc = container.get(WorkspaceService);
184184
const ws = await createTestWorkspace(svc, org, owner, project);
185185

186-
const foundWorkspace = await svc.getWorkspace(owner.id, ws.id);
187-
expect(foundWorkspace?.id).to.equal(ws.id);
186+
const { workspace: ownerWs } = await svc.getWorkspace(owner.id, ws.id);
187+
expect(ownerWs.id).to.equal(ws.id);
188188

189189
await expectError(ErrorCodes.PERMISSION_DENIED, svc.getWorkspace(member.id, ws.id));
190190
await expectError(ErrorCodes.NOT_FOUND, svc.getWorkspace(stranger.id, ws.id));
@@ -196,14 +196,45 @@ describe("WorkspaceService", async () => {
196196

197197
await svc.controlAdmission(owner.id, ws.id, "everyone");
198198

199-
const foundWorkspace = await svc.getWorkspace(owner.id, ws.id);
200-
expect(foundWorkspace?.id, "owner has access to shared workspace").to.equal(ws.id);
199+
const { workspace: ownerWs } = await svc.getWorkspace(owner.id, ws.id);
200+
expect(ownerWs.id, "owner has access to shared workspace").to.equal(ws.id);
201201

202-
const memberFoundWorkspace = await svc.getWorkspace(member.id, ws.id);
203-
expect(memberFoundWorkspace?.id, "member has access to shared workspace").to.equal(ws.id);
202+
const { workspace: memberWs } = await svc.getWorkspace(member.id, ws.id);
203+
expect(memberWs.id, "member has access to shared workspace").to.equal(ws.id);
204204

205-
const strangerFoundWorkspace = await svc.getWorkspace(stranger.id, ws.id);
206-
expect(strangerFoundWorkspace?.id, "stranger has access to shared workspace").to.equal(ws.id);
205+
const { workspace: strangerWs } = await svc.getWorkspace(stranger.id, ws.id);
206+
expect(strangerWs.id, "stranger has access to shared workspace").to.equal(ws.id);
207+
});
208+
209+
it("should getWorkspaces", async () => {
210+
const svc = container.get(WorkspaceService);
211+
await createTestWorkspace(svc, org, owner, project);
212+
213+
const ownerResult = await svc.getWorkspaces(owner.id, {});
214+
expect(ownerResult).to.have.lengthOf(1);
215+
216+
const memberResult = await svc.getWorkspaces(member.id, {});
217+
expect(memberResult).to.have.lengthOf(0);
218+
219+
const strangerResult = await svc.getWorkspaces(stranger.id, {});
220+
expect(strangerResult).to.have.lengthOf(0);
221+
});
222+
223+
it("should getWorkspaces - shared", async () => {
224+
const svc = container.get(WorkspaceService);
225+
const ws = await createTestWorkspace(svc, org, owner, project);
226+
227+
await svc.controlAdmission(owner.id, ws.id, "everyone");
228+
229+
const ownerResult = await svc.getWorkspaces(owner.id, {});
230+
expect(ownerResult, "owner").to.have.lengthOf(1);
231+
232+
// getWorkspaces is limited to the user's own workspaces atm
233+
const memberResult = await svc.getWorkspaces(member.id, {});
234+
expect(memberResult, "member").to.have.lengthOf(0);
235+
236+
const strangerResult = await svc.getWorkspaces(stranger.id, {});
237+
expect(strangerResult, "stranger").to.have.lengthOf(0);
207238
});
208239

209240
it("should getOwnerToken", async () => {
@@ -299,7 +330,7 @@ describe("WorkspaceService", async () => {
299330
// svc.getWorkspace(owner.id, ws.id),
300331
// "getWorkspace should return NOT_FOUND after deletion",
301332
// );
302-
const ws2 = await svc.getWorkspace(owner.id, ws.id);
333+
const { workspace: ws2 } = await svc.getWorkspace(owner.id, ws.id);
303334
expect(ws2.softDeleted, "workspace should be marked as 'softDeleted'").to.equal("user");
304335
});
305336

@@ -353,7 +384,7 @@ describe("WorkspaceService", async () => {
353384

354385
await expectError(ErrorCodes.NOT_FOUND, svc.setPinned(stranger.id, ws.id, true));
355386
await svc.setPinned(owner.id, ws.id, true);
356-
const ws2 = await svc.getWorkspace(owner.id, ws.id);
387+
const { workspace: ws2 } = await svc.getWorkspace(owner.id, ws.id);
357388
expect(ws2.pinned, "workspace should be pinned").to.equal(true);
358389
});
359390

@@ -363,7 +394,7 @@ describe("WorkspaceService", async () => {
363394
const desc = "Some description";
364395

365396
await svc.setDescription(owner.id, ws.id, desc);
366-
const ws2 = await svc.getWorkspace(owner.id, ws.id);
397+
const { workspace: ws2 } = await svc.getWorkspace(owner.id, ws.id);
367398
expect(ws2.description).to.equal(desc);
368399

369400
await expectError(ErrorCodes.NOT_FOUND, svc.setDescription(stranger.id, ws.id, desc));
@@ -516,7 +547,7 @@ describe("WorkspaceService", async () => {
516547

517548
// owner can share workspace
518549
await svc.controlAdmission(owner.id, ws.id, "everyone");
519-
const wsActual = await svc.getWorkspace(owner.id, ws.id);
550+
const { workspace: wsActual } = await svc.getWorkspace(owner.id, ws.id);
520551
expect(wsActual.shareable, "owner should be able to share by default").to.equal(true);
521552
});
522553

@@ -540,7 +571,7 @@ describe("WorkspaceService", async () => {
540571
"invalid admission level should fail",
541572
);
542573

543-
const wsActual = await svc.getWorkspace(owner.id, ws.id);
574+
const { workspace: wsActual } = await svc.getWorkspace(owner.id, ws.id);
544575
expect(!!wsActual.shareable, "shareable should still be false").to.equal(false);
545576
});
546577

0 commit comments

Comments
 (0)