Skip to content

Fix and re-enable automated prebuilds for GitHub Enterprise #8757

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
Mar 14, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Mar 11, 2022

Description

Okay, here is my theory on why project creation sometimes failed with 404 errors when webhooks were enabled. The short version is: We never ensured that the user's GHE token is actually allowed to install a webhook.

  1. User connects with GitHub Enterprise. Gitpod receives a token with minimum scopes (i.e. user:email)
  2. User goes to /new to create a new Project, and sees all their repositories as available, because gh.repos.listForAuthenticatedUser works without special scopes
  3. User selects a repository, Project gets created, then onDidCreateProject looks if it can/should install a webhook
    a. canInstallAutomatedPrebuilds returns true, because the user has indeed ADMIN permission on the repo (and GraphQL tells you that without special scopes)
    b. However, installAutomatedPrebuilds then fails, presumably with a 404 error (to not leak the existence of a repo you shouldn't be able to access?), because you do need at least public_repo or repo scope to actually install a webhook

Related Issue(s)

Fixes #8756

How to test

Release Notes

NONE

Documentation

@jankeromnes jankeromnes requested a review from a team March 11, 2022 14:07
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 11, 2022
@jankeromnes
Copy link
Contributor Author

jankeromnes commented Mar 11, 2022

@andrew-farries @AlexTugarev What do you think about this approach, i.e. ensuring at least public_repo scope when attempting to select GitHub Enterprise in the New Project flow?

Also, now I wonder whether the default GitLab scopes are actually sufficient to install a webhook on GitLab. 🤔 Maybe this authorize flow should always request more than just default scope. EDIT: Seems like the default GitLab scopes read_user + api are actually sufficient to install GitLab webhooks.

Hint: One commit contains auto-format changes from our pre-commit hook. Please feel free to ignore it to limit noise. Relevant commit.

@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #8757 (6c89311) into main (e3a89f3) will not change coverage.
The diff coverage is n/a.

❗ Current head 6c89311 differs from pull request most recent head fdc323a. Consider uploading reports for the commit fdc323a to get more accurate results

@@           Coverage Diff           @@
##             main    #8757   +/-   ##
=======================================
  Coverage   11.17%   11.17%           
=======================================
  Files          18       18           
  Lines         993      993           
=======================================
  Hits          111      111           
  Misses        880      880           
  Partials        2        2           
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jldec
Copy link
Contributor

jldec commented Mar 11, 2022

Amended the related issue to #8756

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Mar 11, 2022

Amended the related issue to #8756

I saw 😂 thanks! (When I noticed the new issue, I started editing my PR description, then wondered "wait this is the same number?!" -- so I cancelled my edit and saw the "edited by jldec" 😁 🙌)

jldec
jldec previously requested changes Mar 11, 2022
Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

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

Default oauth scope shows list of public repos only in /new, and adding a project for a public repo does not appear to configure the webhook properly for triggering prebiulds (even though I see the webhook on the public repo). There is no indicator in the /new project flow to suggest adding repo scope.

Opening a workspace on a private repo DOES ask for repo scope, and then the /new project page shows both public and private repos, and the private repo project appears to receive webhooks properly.

I think it would be better for /new to ask for repo scope before listing repos.

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Mar 11, 2022

Many thanks for your feedback @jldec!

Default oauth scope shows list of public repos only in /new

Yes, that's expected (if you don't grant repo scope, you won't see private repos in /new). Although, I agree with you that there is no clear UX for how to "see my private repositories" when you don't.

and adding a project for a public repo does not appear to configure the webhook properly for triggering prebiulds (even though I see the webhook on the public repo).

That's unexpected. Testing my PR, I was able to successfully:

  1. Connect GitHub Enterprise with minimum scope
  2. Use the /new flow to add a public repository
  3. Gitpod then detected new pushes to the public repository, and successfully started new Prebuilds for them

Are you saying that for your public repo, my PR did not trigger new prebuilds?

I think it would be better for /new to ask for repo scope before listing repos.

That's indeed a product choice we can make. I believe our options are:

  1. Make the minimum scope for New Projects public_repo (thus you'll see only public repos by default, and webhooks should work on them, and you can additionally grant repo if you want to add private repos)
  2. Make the minimum scope for New Projects repo (thus forcing users to also grant private repo access, which they might not want, but makes for a simpler UX)

EDIT: We decided to go with 2. because using only public_repo with GitHub Enterprise doesn't make much sense.

andrew-farries
andrew-farries previously approved these changes Mar 14, 2022
andrew-farries
andrew-farries previously approved these changes Mar 14, 2022
@roboquat roboquat merged commit 5578e23 into main Mar 14, 2022
@roboquat roboquat deleted the jx/fix-ghe-prebuilds branch March 14, 2022 16:33
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prebuilds not triggered on GitHub Enterprise projects
4 participants