Skip to content

[fga] prebuild access #18560

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.

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 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 23 additions & 2 deletions components/server/src/workspace/gitpod-server-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,23 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {

private forwardInstanceUpdateToClient(ctx: TraceContext, instance: WorkspaceInstance) {
// gpl: We decided against tracing updates here, because it create far too much noise (cmp. history)
this.client?.onInstanceUpdate(this.censorInstance(instance));
if (this.userID) {
this.workspaceService
.getWorkspace(this.userID, instance.workspaceId)
.then((ws) => {
this.client?.onInstanceUpdate(this.censorInstance(instance));
})
.catch((err) => {
if (
ApplicationError.hasErrorCode(err) &&
(err.code === ErrorCodes.NOT_FOUND || err.code === ErrorCodes.PERMISSION_DENIED)
) {
// ignore
} else {
log.error("error forwarding instance update to client", err);
}
});
}
}

setClient(ctx: TraceContext, client: GitpodApiClient | undefined): void {
Expand Down Expand Up @@ -1197,12 +1213,16 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
public async isPrebuildDone(ctx: TraceContext, pwsId: string): Promise<boolean> {
traceAPIParams(ctx, { pwsId });

const user = await this.checkUser("isPrebuildDone");

const pws = await this.workspaceDb.trace(ctx).findPrebuildByID(pwsId);
if (!pws) {
if (!pws || !pws.projectId) {
// there is no prebuild - that's as good one being done
return true;
}

await this.auth.checkPermissionOnProject(user.id, "read_prebuild", pws.projectId);

return PrebuiltWorkspace.isDone(pws);
}

Expand Down Expand Up @@ -1486,6 +1506,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {

const project = await this.projectsService.getProject(user.id, projectId);
await this.guardProjectOperation(user, projectId, "get");
await this.auth.checkPermissionOnProject(user.id, "read_prebuild", projectId);

const events = await this.projectsService.getPrebuildEvents(user.id, project.cloneUrl);
return events;
Expand Down
21 changes: 15 additions & 6 deletions components/server/src/workspace/workspace-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,14 @@ export class WorkspaceService {

// 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);

const workspace = await db.findById(workspaceId);

if (workspace?.type === "prebuild" && workspace.projectId) {
await this.auth.checkPermissionOnProject(userId, "read_prebuild", workspace.projectId);
} else {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);
}

// TODO(gpl) We might want to add || !!workspace.softDeleted here in the future, but we were unsure how that would affect existing clients
// In order to reduce risk, we leave it for a future changeset.
if (!workspace || workspace.deleted) {
Expand Down Expand Up @@ -678,9 +683,13 @@ export class WorkspaceService {
): Promise<HeadlessLogUrls> {
const workspace = await this.db.findByInstanceId(instanceId);
if (!workspace) {
throw new ApplicationError(ErrorCodes.NOT_FOUND, `Workspace for instanceId ${instanceId} not found`);
throw new ApplicationError(ErrorCodes.NOT_FOUND, `Prebuild for instanceId ${instanceId} not found`);
}
await this.auth.checkPermissionOnWorkspace(userId, "access", workspace.id);
if (workspace.type !== "prebuild" || !workspace.projectId) {
throw new ApplicationError(ErrorCodes.CONFLICT, `Workspace is not a prebuild`);
}

await this.auth.checkPermissionOnProject(userId, "read_prebuild", workspace.projectId);

const wsiPromise = this.db.findInstanceById(instanceId);
await check(workspace);
Expand All @@ -703,8 +712,8 @@ export class WorkspaceService {
workspaceId: string,
client: Pick<GitpodClient, "onWorkspaceImageBuildLogs">,
): Promise<void> {
await this.auth.checkPermissionOnWorkspace(userId, "access", workspaceId);

// check access
await this.getWorkspace(userId, workspaceId);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it still works, but I wonder what the motivation for this change is? Feels a bit like hiding the check somewhat. πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

Ah, missed the indirection.

Can we maybe work around this pattern? E.g., by moving the "is prebuild" check in doGetWorkspace into Authorizer, and call it here as well? πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

Resolved sync: As it's only two call-sites, we go with this approach for now.

const logCtx: LogContext = { userId, workspaceId };
let instance = await this.db.findCurrentInstance(workspaceId);
if (!instance || instance.status.phase === "stopped") {
Expand Down