batches: implement commit signing via GitHub apps#52333
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits b148eb6 and 9ee59be or learn more. Open explanation
|
Co-authored-by: Becca Steinbrecher <becca.steinbrecher@sourcegraph.com> Co-authored-by: Milan Freml <kopancek@users.noreply.github.com> close #52264
Closes #52270 Exposes new `domain` property from #52268 ## Test plan - Make sure there is a GitHub App on your instance - In the API Console ensure the following query doesn't fail and returns the `domain` property: ``` query { gitHubApps { nodes { name domain } } } ```` --------- Co-authored-by: Milan Freml <kopancek@users.noreply.github.com> Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Closes [#52346](https://github.com/sourcegraph/sourcegraph/issues/52346) This PR allows the domain to be specified when creating a GitHub app. Right now it's hard coded to only work with the `repos` domain till we figure out the UX for creating a Batch Changes GitHub App. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Create a GitHub app. * inspect the `github_apps` table for the newly created GitHub app - the app should have the value of `repos` in the `domain` column.
…52473) Closes #[52465](https://github.com/sourcegraph/sourcegraph/issues/52465), setup for #[52220](https://github.com/sourcegraph/sourcegraph/issues/52220) Adds new `supportsCommitSigning ` field to the `BatchChangesCodeHosts` resolver. Only GitHub external services have commit signing for now. Later on we will support more code hosts. Even though different mechanisms will support the commit signing (ie SSH keys), that will be handled downstream. ## Test plan  - Nav to `/site-admin/batch-changes` - Ensure `/graphql?GlobalBatchChangesCodeHosts` returns the correct value for `supportsCommitSigning ` --------- Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Becca Steinbrecher <beccasteinbrecher@gmail.com> Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com> Co-authored-by: Becca Steinbrecher <becca.steinbrecher@sourcegraph.com> Closes [#52324](https://github.com/sourcegraph/sourcegraph/issues/52324)
e356e40 to
b148eb6
Compare
This PR is a revert of [52136](https://github.com/sourcegraph/sourcegraph/pull/52136). It creates a `github_app_installs` table that tracks the installation of every GitHub app on the Sourcegraph instance. Closes #52494 ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Execute the latest migrations with the command `sg migration up` * Install a GitHub app via the GitHub apps page the attempt to install the app. * if the installation is successful, the installation ID will be saved in the `installation_id` column in the `github_app_installs` table.
…52898) We have a named type called `GitHubAppDomain`. However, the constants that we create of the named type are called `Domain`; this can lead to confusion in the future when there's a named type called `Domain`. I've renamed `BatchesDomain` and `ReposDomain` to `BatchesGitHubAppDomain` and `ReposGitHubAppDomain` to ensure the constants follow the naming of the named type. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Every GitHub app-related operation should work as is. * Just a variable name change.
…t` type (#52733) Closes #52484. - Surfaces a `GitHubAppConfiguration` property on `BatchChangesCodeHost` type - Normalizes `github_app.base_url` - Adds migration to fix past entries - Adds a check to not create duplicate `batches` GH Apps on the same `base_url` - Follow up issue: #[52792](https://github.com/sourcegraph/sourcegraph/issues/52792)  ## Test plan - Query new field ``` query { batchChangesCodeHosts { nodes { externalServiceURL supportsCommitSigning commitSigningConfiguration { __typename ... on GitHubAppConfiguration { appID name appURL logo } } } } } ``` - `sg migration up` successfully adds a trailing `/` on any `github_apps.base_url` missing it - Change [this line](https://github.com/sourcegraph/sourcegraph/pull/52733/files#diff-e8fe19664bf9f7c5e69983ecad802bdf6610083828a4d3faee9eddf430ab068fR108) to `if domain == types.ReposDomain {`, go through the UI to add a new GitHub App on an existing `base_url`, observe it error --------- Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> Co-authored-by: Milan Freml <kopancek@users.noreply.github.com> Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
This PR introduces a background worker that runs every 24 hours. This background worker adds data for new installations of a GitHub app to the database and removes installations that no longer exist on GitHub. This ensures that Sourcegraph has some level of sync consistency with GitHub. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Create a GitHub app and ensure the installation information isn't saved in the database. If you have pre-existing apps before the introduction of #52618 then you already have GitHub apps without installation data saved. * Starting your SG instance instantiates a background worker that'll backfill the installation information and save it in the `github_app_installs` table.
courier-new
left a comment
There was a problem hiding this comment.
Hey @sourcegraph/security-code-review would love a quick review of a couple things here! This PR enables commit signing with Batch Changes via GitHub Apps. We were already able to connect GitHub Apps to Sourcegraph prior to this, so this PR just builds off of that a bit. As such, there's not a whole lot that I think warrants your attention, but I called out just a few areas of interest in the hopes you could give it some eyes! Notably:
- The middleware layer we use to facilitate the GitHub App connection process, keep state, and handle the installation flow redirects
- A new background worker used for recording and syncing additional metadata about individual GitHub App Installations
- In the Batch Changes code, authenticating and pushing commits via a GitHub App Installation, if we find one that works for a given repo
There was a problem hiding this comment.
Here we're passing a couple more parameters back and forth between the frontend, the middleware, and GitHub and store a couple of these in the redis cache as well. We collect a bit more information on the new GitHub App installation, and we handle errors a bit more gracefully.
There was a problem hiding this comment.
This background worker requests the current set of installations for a GitHub App on GitHub and then syncs our records in the DB to match it. We also opportunistically perform this sync when an admin visits the GitHub App page in the web UI.
There was a problem hiding this comment.
Here we now detect if we're pushing commits to a GitHub code host and try to authenticate with a GitHub App installation. If we can, we'll push a commit with that authentication in order to sign it. If we can't, we just return.
evict
left a comment
There was a problem hiding this comment.
Looks good! I only have one recommendation for the caching bit. 👍
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 1a5376d...6c20cff.
|
Co-authored-by: Becca Steinbrecher <beccasteinbrecher@gmail.com> Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Closes https://github.com/sourcegraph/sourcegraph/issues/48611.
Test plan
Reminder to tag security-team before merging this into
main.