Skip to content

[fga] more FGA checks and service use #18517

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

Merged
merged 1 commit into from
Aug 15, 2023
Merged

[fga] more FGA checks and service use #18517

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
2 changes: 1 addition & 1 deletion components/server/src/authorization/definitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export type UserResourceType = "user";

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

export type UserPermission = "read_info" | "write_info" | "make_admin" | "read_ssh" | "write_ssh";
export type UserPermission = "read_info" | "write_info" | "delete" | "make_admin" | "read_ssh" | "write_ssh";

export type InstallationResourceType = "installation";

Expand Down
10 changes: 5 additions & 5 deletions components/server/src/billing/entitlement-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
import { AttributionId } from "@gitpod/gitpod-protocol/lib/attribution";
import { BillingTier } from "@gitpod/gitpod-protocol/lib/protocol";
import { inject, injectable } from "inversify";
import { Config } from "../config";
import { BillingModes } from "./billing-mode";
import { EntitlementServiceUBP } from "./entitlement-service-ubp";
import { VerificationService } from "../auth/verification-service";
Expand Down Expand Up @@ -94,10 +93,11 @@ export interface EntitlementService {
*/
@injectable()
export class EntitlementServiceImpl implements EntitlementService {
@inject(Config) protected readonly config: Config;
@inject(BillingModes) protected readonly billingModes: BillingModes;
@inject(EntitlementServiceUBP) protected readonly ubp: EntitlementServiceUBP;
@inject(VerificationService) protected readonly verificationService: VerificationService;
constructor(
@inject(BillingModes) private readonly billingModes: BillingModes,
@inject(EntitlementServiceUBP) private readonly ubp: EntitlementServiceUBP,
@inject(VerificationService) private readonly verificationService: VerificationService,
) {}

async mayStartWorkspace(
user: User,
Expand Down
5 changes: 4 additions & 1 deletion 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 @@ -13,6 +13,7 @@ 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";

@injectable()
export class UserDeletionService {
Expand All @@ -25,6 +26,7 @@ export class UserDeletionService {
@inject(WorkspaceService) private readonly workspaceService: WorkspaceService,
@inject(AuthProviderService) private readonly authProviderService: AuthProviderService,
@inject(IAnalyticsWriter) private readonly analytics: IAnalyticsWriter,
@inject(Authorizer) private readonly authorizer: Authorizer,
) {}

/**
Expand All @@ -34,6 +36,7 @@ export class UserDeletionService {
* 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 Error(`No user with id ${targetUserId} found!`);
Expand Down Expand Up @@ -159,7 +162,7 @@ export class UserDeletionService {
await Promise.all(ownedTeams.map((t) => this.teamDb.deleteTeam(t.id)));
}

anonymizeWorkspace(ws: Workspace) {
private anonymizeWorkspace(ws: Workspace) {
ws.context.title = "deleted-title";
ws.context.normalizedContextURL = "deleted-normalizedContextURL";
ws.contextURL = "deleted-contextURL";
Expand Down
58 changes: 28 additions & 30 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 @@ -649,6 +649,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
public async maySetTimeout(ctx: TraceContext): Promise<boolean> {
const user = await this.checkUser("maySetTimeout");
await this.guardAccess({ kind: "user", subject: user }, "get");
await this.auth.checkPermissionOnUser(user.id, "read_info", user.id);

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

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

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

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

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

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

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

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

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

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

await this.guardAccess({ kind: "workspaceInstance", subject: runningInstance, workspace }, "get");
await this.guardAccess({ kind: "workspaceInstance", subject: instance, workspace }, "get");
return {
instanceID: runningInstance.id,
workspaceURL: runningInstance.ideUrl,
instanceID: instance.id,
workspaceURL: instance.ideUrl,
};
}

Expand Down Expand Up @@ -1028,24 +1031,22 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {

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

await this.workspaceDb.trace(ctx).transaction(async (db) => {
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
await this.guardAccess({ kind: "workspace", subject: ws }, "update");

switch (action) {
case "pin":
ws.pinned = true;
break;
case "unpin":
ws.pinned = false;
break;
case "toggle":
ws.pinned = !ws.pinned;
break;
}
const ws = await this.workspaceService.getWorkspace(user.id, workspaceId);
await this.guardAccess({ kind: "workspace", subject: ws }, "update");

await db.store(ws);
});
switch (action) {
case "pin":
ws.pinned = true;
break;
case "unpin":
ws.pinned = false;
break;
case "toggle":
ws.pinned = !ws.pinned;
break;
}

await this.workspaceService.setPinned(user.id, ws.id, ws.pinned);
}

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

// for good measure, try and stop running instances
await this.internalStopWorkspace(ctx, user.id, ws, "deleted via API");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geropl I deleted this here because it happens as part of deleteWorkspace below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only difference I see is the "delete via API" is replaced by "deleted via WorkspaceService". But not sure how we used that anyway. 👍


await this.workspaceService.deleteWorkspace(user.id, workspaceId, "user");
}

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

const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId);

await this.guardAccess({ kind: "workspace", subject: workspace }, "update");
await this.workspaceDb.trace(ctx).updatePartial(workspaceId, { description });

await this.workspaceService.setDescription(user.id, workspaceId, description);
}

public async getWorkspaces(
Expand Down
22 changes: 22 additions & 0 deletions components/server/src/workspace/workspace-service.spec.db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,28 @@ describe("WorkspaceService", async () => {
"getWorkspace should return NOT_FOUND after hard-deletion",
);
});

it("should setPinned", async () => {
const svc = container.get(WorkspaceService);
const ws = await createTestWorkspace(svc, org, owner, project);

await expectError(ErrorCodes.NOT_FOUND, svc.setPinned(stranger.id, ws.id, true));
await svc.setPinned(owner.id, ws.id, true);
const ws2 = await svc.getWorkspace(owner.id, ws.id);
expect(ws2.pinned, "workspace should be pinned").to.equal(true);
});

it("should setDescription", async () => {
const svc = container.get(WorkspaceService);
const ws = await createTestWorkspace(svc, org, owner, project);
const desc = "Some description";

await svc.setDescription(owner.id, ws.id, desc);
const ws2 = await svc.getWorkspace(owner.id, ws.id);
expect(ws2.description).to.equal(desc);

await expectError(ErrorCodes.NOT_FOUND, svc.setDescription(stranger.id, ws.id, desc));
});
});

async function createTestWorkspace(svc: WorkspaceService, org: Organization, owner: User, project: Project) {
Expand Down
19 changes: 19 additions & 0 deletions components/server/src/workspace/workspace-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ export class WorkspaceService {
return this.doGetWorkspace(userId, workspaceId);
}

async getCurrentInstance(userId: string, workspaceId: string): Promise<WorkspaceInstance | undefined> {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
const result = await this.db.findCurrentInstance(workspaceId);
if (!result) {
throw new ApplicationError(ErrorCodes.NOT_FOUND, "No workspace instance found.", { workspaceId });
}
return result;
}

// Internal method for allowing for additional DBs to be passed in
private async doGetWorkspace(userId: string, workspaceId: string, db: WorkspaceDB = this.db): Promise<Workspace> {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
Expand Down Expand Up @@ -378,4 +387,14 @@ export class WorkspaceService {

return targetRegion;
}

public async setPinned(userId: string, workspaceId: string, pinned: boolean): Promise<void> {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
await this.db.updatePartial(workspaceId, { pinned });
}

public async setDescription(userId: string, workspaceId: string, description: string) {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
await this.db.updatePartial(workspaceId, { description });
}
}
1 change: 1 addition & 0 deletions components/spicedb/schema/schema.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ schema: |-
// permissions
permission read_info = self + organization->member + organization->owner + installation->admin
permission write_info = self
permission delete = self
permission make_admin = installation->admin + organization->installation_admin

permission read_ssh = self
Expand Down