-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[server] createProject should not query all repositories β EXP-459 #18532
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
Changes from all commits
5306aea
e025ceb
a15a073
1015169
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/** | ||
* Copyright (c) 2023 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License.AGPL.txt in the project root for license information. | ||
*/ | ||
|
||
import { Project, User } from "@gitpod/gitpod-protocol"; | ||
import { RepoURL } from "../repohost"; | ||
import { inject, injectable } from "inversify"; | ||
import { HostContextProvider } from "../auth/host-context-provider"; | ||
import { Config } from "../config"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
|
||
@injectable() | ||
export class ScmService { | ||
constructor( | ||
@inject(HostContextProvider) private readonly hostContextProvider: HostContextProvider, | ||
@inject(Config) private readonly config: Config, | ||
) {} | ||
|
||
async canInstallWebhook(currentUser: User, cloneURL: string) { | ||
try { | ||
const parsedUrl = RepoURL.parseRepoUrl(cloneURL); | ||
const hostContext = parsedUrl?.host ? this.hostContextProvider.get(parsedUrl?.host) : undefined; | ||
const authProvider = hostContext && hostContext.authProvider.info; | ||
const type = authProvider && authProvider.authProviderType; | ||
const host = authProvider?.host; | ||
if (!type || !host) { | ||
throw Error("Unknown host: " + parsedUrl?.host); | ||
} | ||
if ( | ||
type === "GitLab" || | ||
type === "Bitbucket" || | ||
type === "BitbucketServer" || | ||
(type === "GitHub" && (host !== "github.com" || !this.config.githubApp?.enabled)) | ||
) { | ||
const repositoryService = hostContext?.services?.repositoryService; | ||
if (repositoryService) { | ||
return await repositoryService.canInstallAutomatedPrebuilds(currentUser, cloneURL); | ||
} | ||
} | ||
// The GitHub App case isn't handled here due to a circular dependency problem. | ||
} catch (error) { | ||
log.error("Failed to check precondition for creating a project."); | ||
} | ||
return false; | ||
} | ||
|
||
async installWebhookForPrebuilds(project: Project, installer: User) { | ||
// Install the prebuilds webhook if possible | ||
const { teamId, cloneUrl } = project; | ||
const parsedUrl = RepoURL.parseRepoUrl(project.cloneUrl); | ||
const hostContext = parsedUrl?.host ? this.hostContextProvider.get(parsedUrl?.host) : undefined; | ||
const authProvider = hostContext && hostContext.authProvider.info; | ||
const type = authProvider && authProvider.authProviderType; | ||
if ( | ||
type === "GitLab" || | ||
type === "Bitbucket" || | ||
type === "BitbucketServer" || | ||
(type === "GitHub" && (authProvider?.host !== "github.com" || !this.config.githubApp?.enabled)) | ||
) { | ||
const repositoryService = hostContext?.services?.repositoryService; | ||
if (repositoryService) { | ||
// Note: For GitLab, we expect .canInstallAutomatedPrebuilds() to always return true, because earlier | ||
// in the project creation flow, we only propose repositories where the user is actually allowed to | ||
// install a webhook. | ||
if (await repositoryService.canInstallAutomatedPrebuilds(installer, cloneUrl)) { | ||
log.info( | ||
{ organizationId: teamId, userId: installer.id }, | ||
"Update prebuild installation for project.", | ||
); | ||
await repositoryService.installAutomatedPrebuilds(installer, cloneUrl); | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,7 @@ import { SSHKeyService } from "../user/sshkey-service"; | |
import { StartWorkspaceOptions, WorkspaceService, mapGrpcError } from "./workspace-service"; | ||
import { GitpodTokenService } from "../user/gitpod-token-service"; | ||
import { EnvVarService } from "../user/env-var-service"; | ||
import { ScmService } from "../projects/scm-service"; | ||
|
||
// shortcut | ||
export const traceWI = (ctx: TraceContext, wi: Omit<LogContext, "userId">) => TraceContext.setOWI(ctx, wi); // userId is already taken care of in WebsocketConnectionManager | ||
|
@@ -243,6 +244,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |
@inject(HeadlessLogService) private readonly headlessLogService: HeadlessLogService, | ||
|
||
@inject(ProjectsService) private readonly projectsService: ProjectsService, | ||
@inject(ScmService) private readonly scmService: ScmService, | ||
|
||
@inject(IDEService) private readonly ideService: IDEService, | ||
|
||
|
@@ -2638,16 +2640,13 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |
await this.auth.checkPermissionOnOrganization(user.id, "create_project", params.teamId); | ||
|
||
// Check if provided clone URL is accessible for the current user, and user has admin permissions. | ||
let url; | ||
try { | ||
url = new URL(params.cloneUrl); | ||
new URL(params.cloneUrl); | ||
} catch (err) { | ||
throw new ApplicationError(ErrorCodes.BAD_REQUEST, "Clone URL must be a valid URL."); | ||
} | ||
const availableRepositories = await this.getProviderRepositoriesForUser(ctx, { provider: url.host }); | ||
if (!availableRepositories.some((r) => r.cloneUrl === params.cloneUrl)) { | ||
// The error message is derived from internals of `getProviderRepositoriesForUser` and | ||
// `getRepositoriesForAutomatedPrebuilds`, which require admin permissions to be present. | ||
const canCreateProject = await this.canCreateProject(user, params.cloneUrl); | ||
if (!canCreateProject) { | ||
throw new ApplicationError( | ||
ErrorCodes.BAD_REQUEST, | ||
"Repository URL seems to be inaccessible, or admin permissions are missing.", | ||
|
@@ -2676,6 +2675,37 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |
return project; | ||
} | ||
|
||
/** | ||
* Checks if a project can be created, i.e. the current user has the required permissions | ||
* to install webhooks for the given repository. | ||
*/ | ||
private async canCreateProject(currentUser: User, cloneURL: string) { | ||
try { | ||
const parsedUrl = RepoURL.parseRepoUrl(cloneURL); | ||
const host = parsedUrl?.host; | ||
if (!host) { | ||
throw Error("Unknown host: " + parsedUrl?.host); | ||
} | ||
if (host === "github.com" && this.config.githubApp?.enabled) { | ||
const availableRepositories = await this.githubAppSupport.getProviderRepositoriesForUser({ | ||
user: currentUser, | ||
provider: "github.com", | ||
}); | ||
return availableRepositories.some( | ||
(r) => r?.cloneUrl?.toLocaleLowerCase() === cloneURL?.toLocaleLowerCase(), | ||
); | ||
} else { | ||
return await this.scmService.canInstallWebhook(currentUser, cloneURL); | ||
|
||
// note: the GitHub App based check is not included in the ProjectService due | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me this is kind of a red flag to not but both things togehter in a SCMService, honestly. Also, most of that functionality seems util functions around HostContext and the likes, maybe we can place them in such a place (without injection) which resolves/avoids the cycle. I would be ok to merge this today in favor of turnarounds, if we resolve it tomorrow maybe? π There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's resolve this in a different context ππ» |
||
// to a circular dependency problem which would otherwise occur. | ||
} | ||
} catch (error) { | ||
log.error("Failed to check precondition for creating a project."); | ||
} | ||
return false; | ||
} | ||
|
||
public async updateProjectPartial(ctx: TraceContext, partialProject: PartialProject): Promise<void> { | ||
traceAPIParams(ctx, { | ||
// censor everything irrelevant | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we are checking for all types we support explicitly. I wonder for which cases this code is not applicable...? π€
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one https://github.com/gitpod-io/gitpod/pull/18532/files#diff-d6fdb9bb15400f4fbba00db81b2ce6cb7f7bb566dd97b93b9fd4e3fc9a79f479R2689