Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
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
4 changes: 2 additions & 2 deletions components/server/src/workspace/workspace-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,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 +866,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: 3 additions & 3 deletions components/spicedb/schema/schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ schema: |-
permission read_billing = member + owner + installation->admin
permission write_billing = owner + installation->admin

permission read_prebuild = member + owner + installation->admin
permission read_prebuild = collaborator + member + owner + installation->admin

permission create_workspace = member + collaborator

Expand All @@ -118,10 +118,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