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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
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
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.

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

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
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 org-wide read prebuild permission I flipped on accidentally when looking for the project-specific one, and it ended up raising a question: do we want to let collaborators view the list of prebuilds of the org? I'm guessing not, because we'd have to change those the related pages quite a bit (to signal that you can't start / stop prebuilds or view any of their settings), but it very theoretically could be useful if they wanted to at check what's wrong with a repo's prebuild config and if it's something they can fix themselves.
The detail of a single prebuild still will need some small tweaks to be properly accessible (currently, it just errors), but it probably wouldn't take too long to change.


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