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.

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

Enable collaborators #20353

Merged
merged 9 commits into from
Nov 22, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,12 @@ export function deduplicateAndFilterRepositories(
if (results.length === 0) {
// If the searchString is a URL, and it's not present in the proposed results, "artificially" add it here.
if (isValidGitUrl(searchString)) {
console.log("It's valid man");
Copy link
Member Author

Choose a reason for hiding this comment

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

These super rad debug logs I accidentally left in with #20287. Yikes!

results.push(
new SuggestedRepository({
url: searchString,
}),
);
}

console.log("Valid after man");
}

// Limit what we show to 200 results
Expand All @@ -145,7 +142,7 @@ export function deduplicateAndFilterRepositories(

const ALLOWED_GIT_PROTOCOLS = ["ssh:", "git:", "http:", "https:"];
/**
* An opionated git URL validator
* An opinionated git URL validator
*
* Assumptions:
* - Git hosts are not themselves TLDs (like .com) or reserved names like `localhost`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,15 +361,13 @@ export const PrebuildDetailPage: FC = () => {
>
View Prebuild Settings
</LinkButton>
<Button
<LinkButton
disabled={prebuild?.status?.phase?.name !== PrebuildPhase_Phase.AVAILABLE}
onClick={() =>
(window.location.href = `/#open-prebuild/${prebuild?.id}/${prebuild?.contextUrl}`)
}
href={`/#open-prebuild/${prebuild?.id}/${prebuild?.contextUrl}`}
variant="secondary"
>
Open Debug Workspace
</Button>
</LinkButton>
</div>
</div>
</div>
Expand Down
18 changes: 15 additions & 3 deletions components/server/src/prebuilds/prebuild-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export interface StartPrebuildParams {
commitInfo?: CommitInfo;
forcePrebuild?: boolean;
trigger?: keyof ProjectUsage;
assumeProjectActive?: boolean;
}

export interface PrebuildFilter {
Expand Down Expand Up @@ -330,7 +331,15 @@ export class PrebuildManager {

async startPrebuild(
ctx: TraceContext,
{ context, project, user, commitInfo, forcePrebuild, trigger = "lastWebhookReceived" }: StartPrebuildParams,
{
context,
project,
user,
commitInfo,
forcePrebuild,
trigger = "lastWebhookReceived",
assumeProjectActive,
}: StartPrebuildParams,
): Promise<StartPrebuildResult> {
const span = TraceContext.startSpan("startPrebuild", ctx);
const cloneURL = context.repository.cloneUrl;
Expand Down Expand Up @@ -461,7 +470,10 @@ export class PrebuildManager {
"Prebuild is rate limited. Please contact Gitpod if you believe this happened in error.";
await this.workspaceDB.trace({ span }).storePrebuiltWorkspace(prebuild);
span.setTag("ratelimited", true);
} else if (await this.projectService.isProjectConsideredInactive(user.id, project.id)) {
} else if (
!assumeProjectActive &&
(await this.projectService.isProjectConsideredInactive(user.id, project.id))
) {
prebuild.state = "aborted";
prebuild.error =
"Project is inactive. Please start a new workspace for this project to re-enable prebuilds.";
Expand Down Expand Up @@ -661,7 +673,7 @@ export class PrebuildManager {
if (!prebuild || !organizationId) {
throw new ApplicationError(ErrorCodes.PRECONDITION_FAILED, "prebuild workspace not found");
}
await this.auth.checkPermissionOnOrganization(userId, "read_prebuild", organizationId);
await this.auth.checkPermissionOnProject(userId, "read_prebuild", organizationId);

const instance = await this.workspaceService.getCurrentInstance(userId, prebuild.workspace.id, {
skipPermissionCheck: true,
Expand Down
21 changes: 17 additions & 4 deletions components/server/src/projects/projects-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,14 @@ export class ProjectsService {
@inject(InstallationService) private readonly installationService: InstallationService,
) {}

async getProject(userId: string, projectId: string): Promise<Project> {
await this.auth.checkPermissionOnProject(userId, "read_info", projectId);
/**
* Returns a project by its ID.
* @param skipPermissionCheck useful either when the caller already checked permissions or when we need to do something purely server-side (e.g. looking up a project when starting a workspace by a collaborator)
*/
async getProject(userId: string, projectId: string, skipPermissionCheck?: boolean): Promise<Project> {
if (!skipPermissionCheck) {
await this.auth.checkPermissionOnProject(userId, "read_info", projectId);
}
const project = await this.projectDB.findProjectById(projectId);
if (!project) {
throw new ApplicationError(ErrorCodes.NOT_FOUND, `Project ${projectId} not found.`);
Expand Down Expand Up @@ -132,11 +138,18 @@ export class ProjectsService {
return filteredProjects;
}

async findProjectsByCloneUrl(userId: string, cloneUrl: string, organizationId?: string): Promise<Project[]> {
async findProjectsByCloneUrl(
userId: string,
cloneUrl: string,
organizationId?: string,
skipPermissionCheck?: boolean,
): Promise<Project[]> {
const projects = await this.projectDB.findProjectsByCloneUrl(cloneUrl, organizationId);
const result: Project[] = [];
for (const project of projects) {
if (await this.auth.hasPermissionOnProject(userId, "read_info", project.id)) {
const hasPermission =
skipPermissionCheck || (await this.auth.hasPermissionOnProject(userId, "read_info", project.id));
if (hasPermission) {
result.push(project);
}
}
Expand Down
2 changes: 2 additions & 0 deletions components/server/src/workspace/context-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ export class ContextService {
user.id,
context.repository.cloneUrl,
options?.organizationId,
true,
);
// todo(ft): solve for this case with collaborators who can't select projects directly
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love ideas around this: how can we not block collaborators if a repo they're working on gets imported twice? Or, in the very least, how do we educate members and owners about this possibility?

It's admittedly a case on the edge™, but IMO worth considering.

Copy link
Member Author

Choose a reason for hiding this comment

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

todo: file an issue for this

if (projects.length > 1) {
throw new ApplicationError(ErrorCodes.BAD_REQUEST, "Multiple projects found for clone URL.");
}
Expand Down
4 changes: 2 additions & 2 deletions components/server/src/workspace/suggested-repos-sorter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const sortSuggestedRepositories = (repos: SuggestedRepositoryWithSorting[
// This allows us to consider the lastUse of a recently used project when sorting
// as it will may have an entry for the project (no lastUse), and another for recent workspaces (w/ lastUse)

const projectURLs: string[] = [];
let projectURLs: string[] = [];
let uniqueRepositories: SuggestedRepositoryWithSorting[] = [];

for (const repo of repos) {
Expand Down Expand Up @@ -88,7 +88,7 @@ export const sortSuggestedRepositories = (repos: SuggestedRepositoryWithSorting[
uniqueRepositories = uniqueRepositories.map((repo) => {
if (repo.projectId && !repo.projectName) {
delete repo.projectId;
delete projectURLs[projectURLs.indexOf(repo.url)];
projectURLs = projectURLs.filter((url) => url !== repo.url);
Copy link
Member Author

Choose a reason for hiding this comment

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

This deletion would not work properly when you had two repos with the same project URL. Because we're iterating over the unique repos, it would clean up only the first occurrence of the project URL, instead of cleaning up all of them, leaving an incorrect state in projectURLs.

}

return repo;
Expand Down
5 changes: 3 additions & 2 deletions components/server/src/workspace/workspace-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ export class WorkspaceService {
forcePrebuild: false,
context,
trigger: "lastWorkspaceStart",
assumeProjectActive: true,
});
log.info(logCtx, "starting prebuild after workspace creation", {
projectId: project.id,
Expand Down Expand Up @@ -819,7 +820,7 @@ export class WorkspaceService {
}

const projectPromise = workspace.projectId
? ApplicationError.notFoundToUndefined(this.projectsService.getProject(user.id, workspace.projectId))
? ApplicationError.notFoundToUndefined(this.projectsService.getProject(user.id, workspace.projectId, true))
: Promise.resolve(undefined);

await mayStartPromise;
Expand Down Expand Up @@ -866,7 +867,7 @@ export class WorkspaceService {
result = await this.entitlementService.mayStartWorkspace(user, organizationId, runningInstances);
TraceContext.addNestedTags(ctx, { mayStartWorkspace: { result } });
} catch (err) {
log.error({ userId: user.id }, "EntitlementSerivce.mayStartWorkspace error", err);
log.error({ userId: user.id }, "EntitlementService.mayStartWorkspace error", err);
TraceContext.setError(ctx, err);
return; // we don't want to block workspace starts because of internal errors
}
Expand Down
14 changes: 8 additions & 6 deletions components/server/src/workspace/workspace-starter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,8 @@ export class WorkspaceStarter {
return;
}

// implicit project (existing on the same clone URL)
const projects = await this.projectService.findProjectsByCloneUrl(user.id, contextURL, organizationId);
// implicit project (existing on the same clone URL). We skip the permission check so that collaborators are not stuck
const projects = await this.projectService.findProjectsByCloneUrl(user.id, contextURL, organizationId, true);
if (projects.length === 0) {
throw new ApplicationError(
ErrorCodes.PRECONDITION_FAILED,
Expand Down Expand Up @@ -1951,10 +1951,12 @@ export class WorkspaceStarter {
{},
);
if (isEnabledPrebuildFullClone) {
const project = await this.projectService.getProject(user.id, workspace.projectId).catch((err) => {
log.error("failed to get project", err);
return undefined;
});
const project = await this.projectService
.getProject(user.id, workspace.projectId, true)
.catch((err) => {
log.error("failed to get project", err);
return undefined;
});
if (project && project.settings?.prebuilds?.cloneSettings?.fullClone) {
result.setFullClone(true);
}
Expand Down
6 changes: 4 additions & 2 deletions components/spicedb/schema/schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ schema: |-
permission read_billing = member + owner + installation->admin
permission write_billing = owner + installation->admin

// Note that there are two different read_prebuild permissions: this one, guarding primarily the listPrebuilds method in the API and then the other under projects, which is the actual permission used for checking if a user can use a prebuild for a repository.
// Today, the difference is that collaborators can't read prebuilds on an org but can on a repository (project).
permission read_prebuild = member + owner + installation->admin

permission create_workspace = member + collaborator
Expand All @@ -118,10 +120,10 @@ schema: |-
permission write_info = editor + org->owner + org->installation_admin
permission delete = editor + org->owner + org->installation_admin

permission read_env_var = viewer + editor + org->owner + org->installation_admin
permission read_env_var = viewer + editor + org->collaborator + org->owner + org->installation_admin
permission write_env_var = editor + org->owner + org->installation_admin

permission read_prebuild = viewer + editor + org->owner + org->installation_admin
permission read_prebuild = viewer + editor + org->collaborator + org->owner + org->installation_admin
permission write_prebuild = editor + org->owner
}

Expand Down
Loading