From 6351f1487ed9b972e8ddb581cc6a1a4685974497 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:20:23 +0000 Subject: [PATCH 1/4] feat(hub): recover after GitHub App transfers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../pages/github-app-page/stories.tsx | 12 + hub/app/models/github_app_configuration.rb | 69 +-- .../github_app_configuration/diagnostics.rb | 31 +- .../github_app_configuration/installation.rb | 97 +++- .../github_app_configuration/ownership.rb | 55 +++ .../github_app_configuration/repositories.rb | 5 +- hub/app/models/github_client/installations.rb | 11 + hub/app/models/github_webhook.rb | 6 +- hub/app/views/github_apps/show.json.props | 12 +- hub/config/locales/github_app.en.yml | 1 + ...on_account_to_github_app_configurations.rb | 20 + hub/db/schema.rb | 4 +- hub/db/seeds/test/setup.rb | 1 + hub/docs/github-app-setup.md | 12 + hub/lib/tasks/github_app.rake | 2 - .../github_app_installation_atomicity_test.rb | 6 +- hub/test/integration/github_app_setup_test.rb | 2 +- ...app_installation_account_migration_test.rb | 49 ++ ...hub_app_administration_concurrency_test.rb | 10 +- .../administration_test.rb | 1 + .../diagnostics_test.rb | 45 ++ .../repositories_test.rb | 2 +- .../transfer_network_test.rb | 36 ++ .../transfer_recovery_test.rb | 454 ++++++++++++++++++ ...ithub_app_dual_barrier_concurrency_test.rb | 5 +- hub/test/models/github_webhook_test.rb | 36 +- hub/test/support/github_request_stubs.rb | 29 ++ 27 files changed, 937 insertions(+), 76 deletions(-) create mode 100644 hub/app/models/github_app_configuration/ownership.rb create mode 100644 hub/db/migrate/20261021195500_add_installation_account_to_github_app_configurations.rb create mode 100644 hub/test/lib/github_app_installation_account_migration_test.rb create mode 100644 hub/test/models/github_app_configuration/diagnostics_test.rb create mode 100644 hub/test/models/github_app_configuration/transfer_network_test.rb create mode 100644 hub/test/models/github_app_configuration/transfer_recovery_test.rb diff --git a/hub/app/javascript/pages/github-app-page/stories.tsx b/hub/app/javascript/pages/github-app-page/stories.tsx index 5fe35af9..ebaba939 100644 --- a/hub/app/javascript/pages/github-app-page/stories.tsx +++ b/hub/app/javascript/pages/github-app-page/stories.tsx @@ -40,6 +40,18 @@ export default meta type Story = StoryObj export const Configured: Story = {} +export const InstallationRequired: Story = { + args: { + data: { + ...data, + installation: "Installation 456: removed", + paragraphs: [ + ...data.paragraphs, + "Install this App in the github account and select its repositories. Hub checks GitHub automatically and restores access after installation.", + ], + }, + }, +} export const NotConfigured: Story = { args: { data: { diff --git a/hub/app/models/github_app_configuration.rb b/hub/app/models/github_app_configuration.rb index 2d90a4a2..608cd9c3 100644 --- a/hub/app/models/github_app_configuration.rb +++ b/hub/app/models/github_app_configuration.rb @@ -3,31 +3,32 @@ # Table name: github_app_configurations # Database name: primary # -# id :integer not null, primary key -# app_slug :string not null -# client_secret :text not null -# credential_revision :bigint default(1), not null -# installation_checked_at :datetime -# installation_state :string default("not_installed"), not null -# installed_at :datetime -# lock_version :integer default(0), not null -# owner_login :string not null -# owner_type :string not null -# previous_webhook_secret :text -# private_key :text not null -# singleton_key :integer default(1), not null -# suspended_at :datetime -# webhook_request_expires_at :datetime -# webhook_request_token :string -# webhook_rotation_state :string default("idle"), not null -# webhook_secret :text not null -# webhook_secret_rotated_at :datetime -# created_at :datetime not null -# updated_at :datetime not null -# client_id :string not null -# github_app_id :bigint not null -# installation_id :bigint -# owner_github_id :bigint not null +# id :integer not null, primary key +# app_slug :string not null +# client_secret :text not null +# credential_revision :bigint default(1), not null +# installation_checked_at :datetime +# installation_state :string default("not_installed"), not null +# installed_at :datetime +# lock_version :integer default(0), not null +# owner_login :string not null +# owner_type :string not null +# previous_webhook_secret :text +# private_key :text not null +# singleton_key :integer default(1), not null +# suspended_at :datetime +# webhook_request_expires_at :datetime +# webhook_request_token :string +# webhook_rotation_state :string default("idle"), not null +# webhook_secret :text not null +# webhook_secret_rotated_at :datetime +# created_at :datetime not null +# updated_at :datetime not null +# client_id :string not null +# github_app_id :bigint not null +# installation_account_github_id :bigint +# installation_id :bigint +# owner_github_id :bigint not null # # Indexes # @@ -35,6 +36,7 @@ # # Check Constraints # +# github_app_configurations_installation_account ((installation_id IS NULL AND installation_account_github_id IS NULL) OR (installation_id IS NOT NULL AND installation_account_github_id IS NOT NULL AND installation_account_github_id > 0)) # github_app_configurations_installation_identity ((installation_state = 'not_installed' AND installation_id IS NULL) OR (installation_state <> 'not_installed' AND installation_id IS NOT NULL)) # github_app_configurations_installation_state (installation_state IN ('not_installed', 'active', 'suspended', 'removed')) # github_app_configurations_owner_type (owner_type IN ('Organization', 'User')) @@ -62,6 +64,7 @@ class Superseded < StandardError; end :webhook_secret, presence: true validates :github_app_id, :owner_github_id, numericality: { only_integer: true, greater_than: 0 } validates :installation_id, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true + validates :installation_account_github_id, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true validates :installation_state, inclusion: { in: INSTALLATION_STATES } validates :owner_type, inclusion: { in: OWNER_TYPES } validates :singleton_key, inclusion: { in: [ 1 ] }, uniqueness: true @@ -72,6 +75,7 @@ class Superseded < StandardError; end has_object :diagnostics has_object :installation has_object :oauth + has_object :ownership has_object :repositories has_object :recovery has_object :rotator @@ -129,9 +133,18 @@ def credential_snapshot def synchronize_repositories! return if LocalDevelopment.enabled? - return repositories.remove_all! if installation_id.nil? + ownership.refresh! + installation.synchronize! + end + + # Prevents work for a previous owner or installation from restoring repository access. + def with_installation! + identity = attributes.slice("owner_github_id", "installation_id", "installation_state") + self.class.with_credentials!(**credential_snapshot) do + raise Superseded if !self.class.uncached { self.class.where(id:, **identity).exists? } - installation.reconcile!(installation_id) + yield + end end # Polls GitHub for current prototype branch heads. @@ -159,5 +172,7 @@ def audit_setup def installation_identity_matches_state valid = installation_state == "not_installed" ? installation_id.nil? : installation_id.present? errors.add(:installation_id, "does not match installation state") if !valid + valid_account = installation_id.nil? ? installation_account_github_id.nil? : installation_account_github_id.present? + errors.add(:installation_account_github_id, "does not match installation") if !valid_account end end diff --git a/hub/app/models/github_app_configuration/diagnostics.rb b/hub/app/models/github_app_configuration/diagnostics.rb index ef14c398..099cd861 100644 --- a/hub/app/models/github_app_configuration/diagnostics.rb +++ b/hub/app/models/github_app_configuration/diagnostics.rb @@ -6,21 +6,37 @@ class GithubAppConfiguration::Diagnostics < ActiveRecord::AssociatedObject def call app = record.github_client.apps.get(token: record.tokens.app) verify_app!(app) + owner = app.fetch("owner") + if owner.fetch("id") != record.owner_github_id || owner.fetch("type") != record.owner_type + return Result.new \ + configuration: "configured", + app: "owner changed; waiting for synchronization", + installation: "needs rechecking" + end + remote_installation = if record.installation_id - record.github_client.installations.get \ - record.installation_id, - token: record.tokens.app + begin + record.github_client.installations.get \ + record.installation_id, + token: record.tokens.app + rescue GithubClient::NotFound + nil + end end Result.new(configuration: "configured", app: "reachable", installation: installation_status(remote_installation)) end private - # Confirms GitHub returned the App and owner that Hub expects. + # A changed owner is recoverable, but another App or malformed identity is not. def verify_app!(app) + raise GithubClient::InvalidResponse, "GitHub returned incomplete App identity" if + !app.is_a?(Hash) || !app["owner"].is_a?(Hash) + owner = app.fetch("owner") - valid = app.fetch("id").to_i == record.github_app_id && - owner.fetch("id").to_i == record.owner_github_id && owner.fetch("type") == record.owner_type + valid = app.fetch("id").is_a?(Integer) && app.fetch("id") == record.github_app_id && + owner.fetch("id").is_a?(Integer) && owner.fetch("id").positive? && + GithubAppConfiguration::OWNER_TYPES.include?(owner.fetch("type")) raise GithubClient::InvalidResponse, "GitHub App identity does not match" if !valid rescue KeyError, TypeError, ArgumentError raise GithubClient::InvalidResponse, "GitHub returned incomplete App identity" @@ -31,7 +47,8 @@ def installation_status(payload) return "not installed" if payload.nil? account = payload.fetch("account") - return "owner mismatch" if account.fetch("id").to_i != record.owner_github_id + return "owner mismatch" if account.fetch("id").to_i != record.owner_github_id || + account.fetch("id").to_i != record.installation_account_github_id payload["suspended_at"].present? ? "suspended" : "reachable" rescue KeyError, TypeError, ArgumentError diff --git a/hub/app/models/github_app_configuration/installation.rb b/hub/app/models/github_app_configuration/installation.rb index b048938d..1c69532d 100644 --- a/hub/app/models/github_app_configuration/installation.rb +++ b/hub/app/models/github_app_configuration/installation.rb @@ -2,11 +2,36 @@ class GithubAppConfiguration::Installation < ActiveRecord::AssociatedObject class OwnerMismatch < StandardError; end + # Recovers an owner's replacement installation even when GitHub notifications were missed. + def synchronize! + credentials = record.credential_snapshot + version = record.lock_version + payload = if record.installation_id && record.installation_state != "removed" && + record.installation_account_github_id == record.owner_github_id + begin + remote_installation(record.installation_id) + rescue GithubClient::NotFound + installation_for_owner + end + else + installation_for_owner + end + if payload + record_status!(payload, credentials:, expected_version: version) + else + record_missing!(credentials:, expected_version: version) + end + end + # Fetches the current installation from GitHub and saves its status. def reconcile!(installation_id) snapshot = record.credential_snapshot + version = record.lock_version GithubAppConfiguration.with_credentials!(**snapshot) { } - record_status!(remote_installation(installation_id), credentials: snapshot) + record_status! \ + remote_installation(installation_id), + credentials: snapshot, + expected_version: version end # Commits browser setup and its audit only after all GitHub requests finish. @@ -14,6 +39,7 @@ def install!(installation_id) credentials = record.credential_snapshot GithubAppConfiguration.with_credentials!(**credentials) { } payload = remote_installation(installation_id) + verify_app!(payload) verify_owner!(payload.fetch("account")) state = state_for(payload, nil) installation_id = payload.fetch("id").to_i @@ -33,12 +59,13 @@ def install!(installation_id) end # Retains verified webhook status even when its later repository refresh fails. - def record_status!(payload, action: nil, credentials: record.credential_snapshot) - verify_owner!(payload.fetch("account")) + def record_status!(payload, action: nil, credentials: record.credential_snapshot, + expected_version: record.lock_version) + verify_app!(payload) return if stale_removal?(payload, action) state = state_for(payload, action) - save_status!(payload, state:, credentials:) + save_status!(payload, state:, credentials:, expected_version:, action:) state == "active" ? record.repositories.synchronize! : record.repositories.remove_all! end @@ -46,20 +73,66 @@ def record_status!(payload, action: nil, credentials: record.credential_snapshot # Fetches the installation without holding a local write transaction. def remote_installation(installation_id) - record.github_client.installations.get \ + payload = record.github_client.installations.get \ installation_id, token: record.tokens.app + verify_app!(payload) + raise GithubClient::InvalidResponse, "GitHub returned another installation" if + payload.fetch("id") != installation_id.to_i + + payload + end + + # A missing installation is expected while the new owner arranges reinstallation. + def installation_for_owner + payload = record.github_client.installations.for_account \ + login: record.owner_login, + type: record.owner_type, + token: record.tokens.app + verify_app!(payload) + payload + rescue GithubClient::NotFound + nil + end + + # Confirms absence without treating an outage or permission failure as removal. + def record_missing!(credentials:, expected_version:) + GithubAppConfiguration.with_credentials!(**credentials) do + record.with_lock do + raise GithubAppConfiguration::Superseded if record.lock_version != expected_version + + record.installation_state = "removed" if record.installation_id + record.installation_checked_at = Time.current + if record.changed.excluding("installation_checked_at").any? + record.save! + else + record.update_columns(installation_checked_at: record.installation_checked_at) + end + record.repositories.remove_all! + end + end end # Applies the verified status without performing GitHub I/O. - def save_status!(payload, state:, credentials: record.credential_snapshot) + def save_status!(payload, state:, credentials: record.credential_snapshot, + expected_version: record.lock_version, action: nil) GithubAppConfiguration.with_credentials!(**credentials) do record.with_lock do + raise GithubAppConfiguration::Superseded if record.lock_version != expected_version + + account = payload.fetch("account") + if action == "deleted" + matches = record.installation_id == payload.fetch("id") && + record.installation_account_github_id == account.fetch("id") + raise OwnerMismatch, "Removal does not match the saved installation" if !matches + else + verify_owner!(account) + end checked_at = Time.current record.assign_attributes \ installation_id: payload.fetch("id").to_i, + installation_account_github_id: account.fetch("id"), installation_state: state, - owner_login: payload.fetch("account").fetch("login"), installed_at: state == "active" ? record.installed_at || checked_at : record.installed_at, suspended_at: state == "suspended" ? record.suspended_at || checked_at : nil if record.changed? @@ -73,6 +146,16 @@ def save_status!(payload, state:, credentials: record.credential_snapshot) end end + # A signed webhook or API response must still identify this App and a real installation. + def verify_app!(payload) + valid = payload.is_a?(Hash) && payload["app_id"].is_a?(Integer) && + payload["app_id"] == record.github_app_id && + payload["id"].is_a?(Integer) && payload["id"].positive? && + payload["account"].is_a?(Hash) && + payload["account"]["id"].is_a?(Integer) && payload["account"]["id"].positive? + raise GithubClient::InvalidResponse, "GitHub returned an invalid installation identity" if !valid + end + # Confirms the installation belongs to the configured GitHub owner. def verify_owner!(account) matches = account.fetch("id").to_i == record.owner_github_id && diff --git a/hub/app/models/github_app_configuration/ownership.rb b/hub/app/models/github_app_configuration/ownership.rb new file mode 100644 index 00000000..4e0ff9d7 --- /dev/null +++ b/hub/app/models/github_app_configuration/ownership.rb @@ -0,0 +1,55 @@ +# Refreshes App ownership without changing credentials or ending Hub sessions. +class GithubAppConfiguration::Ownership < ActiveRecord::AssociatedObject + # Accepts owner metadata only from GitHub's authenticated response for this App. + def refresh! + credentials = record.credential_snapshot + version = record.lock_version + app = record.github_client.apps.get(token: record.tokens.app) + attributes = attributes_for(app) + + GithubAppConfiguration.with_credentials!(**credentials) do + record.with_lock do + raise GithubAppConfiguration::Superseded if record.lock_version != version + + transferred = record.owner_github_id != attributes.fetch(:owner_github_id) || + record.owner_type != attributes.fetch(:owner_type) + record.assign_attributes(attributes) + record.installation_state = "removed" if transferred && record.installation_id + record.save! if record.changed? + if transferred + record.repositories.remove_all! + SecurityAuditEvent.record! \ + action: "github_app.transfer", outcome: "succeeded", actor: nil, + context: { source: "command", github_app_configuration_id: record.id } + end + end + end + end + + private + + # Rejects incomplete identity before it can replace a saved owner. + def attributes_for(app) + raise KeyError if !app.is_a?(Hash) || !app["id"].is_a?(Integer) || app["id"] != record.github_app_id + + owner = app.fetch("owner") + raise KeyError if !owner.is_a?(Hash) + + attributes = { + owner_github_id: owner.fetch("id"), + owner_login: owner.fetch("login"), + owner_type: owner.fetch("type"), + app_slug: app.fetch("slug") + } + valid = attributes[:owner_github_id].is_a?(Integer) && attributes[:owner_github_id].positive? && + GithubAppConfiguration::OWNER_TYPES.include?(attributes[:owner_type]) && + attributes.values_at(:owner_login, :app_slug).all? do |value| + value.is_a?(String) && value.match?(/\A[A-Za-z0-9][A-Za-z0-9-]*\z/) + end + raise KeyError if !valid + + attributes + rescue KeyError, TypeError + raise GithubClient::InvalidResponse, "GitHub returned an invalid App identity" + end +end diff --git a/hub/app/models/github_app_configuration/repositories.rb b/hub/app/models/github_app_configuration/repositories.rb index 5ccb01d5..96356661 100644 --- a/hub/app/models/github_app_configuration/repositories.rb +++ b/hub/app/models/github_app_configuration/repositories.rb @@ -10,11 +10,10 @@ def snapshot(token: record.tokens.installation) def synchronize!(snapshot: nil) return remove_all! if record.installation_state != "active" - credentials = record.credential_snapshot attributes = snapshot || self.snapshot now = Time.current - GithubAppConfiguration.with_credentials!(**credentials) do + record.with_installation! do active_ids = attributes.map { _1.fetch(:github_id) } attributes.each { synchronize_repository!(_1.merge(synchronized_at: now)) } Repository.active.where.not(github_id: active_ids).update_all \ @@ -25,7 +24,7 @@ def synchronize!(snapshot: nil) # Marks all repositories unavailable when the App installation is inactive. def remove_all! now = Time.current - GithubAppConfiguration.with_credentials!(**record.credential_snapshot) do + record.with_installation! do Repository.active.update_all \ installation_state: "removed", removed_at: now, synchronized_at: now, updated_at: now end diff --git a/hub/app/models/github_client/installations.rb b/hub/app/models/github_client/installations.rb index ef3b5bbc..48347f0a 100644 --- a/hub/app/models/github_client/installations.rb +++ b/hub/app/models/github_client/installations.rb @@ -10,6 +10,17 @@ def get(id, token:) transport.request(:get, "/app/installations/#{id.to_i}", token:) end + # Finds this App's installation in its current owner's account. + def for_account(login:, type:, token:) + unless %w[Organization User].include?(type) && + login.is_a?(String) && login.match?(/\A[A-Za-z0-9][A-Za-z0-9-]*\z/) + raise GithubClient::InvalidResponse, "GitHub returned an invalid installation account" + end + + scope = type == "Organization" ? "orgs" : "users" + transport.request(:get, "/#{scope}/#{CGI.escape(login)}/installation", token:) + end + # Creates one access token for a GitHub App installation. def create_token(id, token:) transport.request(:post, "/app/installations/#{id.to_i}/access_tokens", token:) diff --git a/hub/app/models/github_webhook.rb b/hub/app/models/github_webhook.rb index 78a249ef..098c2e25 100644 --- a/hub/app/models/github_webhook.rb +++ b/hub/app/models/github_webhook.rb @@ -148,12 +148,10 @@ def record_installation!(payload) installation = payload.fetch("installation") verify_app!(installation) action = payload.fetch("action") - if event == "installation" && action == "created" && recovered? - configuration.installation.reconcile!(installation.fetch("id")) - elsif event == "installation" && %w[created deleted].include?(action) + if event == "installation" && action == "deleted" configuration.installation.record_status!(installation, action:) else - configuration.installation.reconcile!(installation.fetch("id")) + configuration.synchronize_repositories! end nil end diff --git a/hub/app/views/github_apps/show.json.props b/hub/app/views/github_apps/show.json.props index cec9cd23..df8d6950 100644 --- a/hub/app/views/github_apps/show.json.props +++ b/hub/app/views/github_apps/show.json.props @@ -2,9 +2,15 @@ json.partial! partial: "github_apps/configuration" json.pageTitle t("github_app.heading") json.heading t("github_app.heading") json.description t("github_app.description") -json.paragraphs \ - @configuration ? [ t("github_app.webhook.#{@configuration.webhook_rotation_state}") ] : - [ t("github_app.not_configured") ] +paragraphs = if @configuration + [ t("github_app.webhook.#{@configuration.webhook_rotation_state}") ] +else + [ t("github_app.not_configured") ] +end +if @configuration&.installation_state.in?(%w[not_installed removed]) + paragraphs << t("github_app.installation_required", owner: @configuration.owner_login) +end +json.paragraphs paragraphs json.links \ @configuration ? [ { label: t("github_app.credentials.heading"), path: edit_github_app_credentials_path }, diff --git a/hub/config/locales/github_app.en.yml b/hub/config/locales/github_app.en.yml index 6cca5890..19b7b7e9 100644 --- a/hub/config/locales/github_app.en.yml +++ b/hub/config/locales/github_app.en.yml @@ -9,6 +9,7 @@ en: verification: GitHub must have verified this browser's account within the last five minutes. If asked to verify, return here and confirm again. Submitted credentials are discarded, never replayed. identity: "App %{app_id} - %{owner} (%{owner_type}, identity %{owner_id})" installation: "Installation %{installation_id}: %{state}" + installation_required: "Install this App in the %{owner} account and select its repositories. Hub checks GitHub automatically and restores access after installation." busy: A webhook update is in progress. Wait for it to finish before changing the App. credentials: heading: Replace App credentials diff --git a/hub/db/migrate/20261021195500_add_installation_account_to_github_app_configurations.rb b/hub/db/migrate/20261021195500_add_installation_account_to_github_app_configurations.rb new file mode 100644 index 00000000..e6e42c2f --- /dev/null +++ b/hub/db/migrate/20261021195500_add_installation_account_to_github_app_configurations.rb @@ -0,0 +1,20 @@ +class AddInstallationAccountToGithubAppConfigurations < ActiveRecord::Migration[8.1] + def up + add_column :github_app_configurations, :installation_account_github_id, :bigint + execute <<~SQL + UPDATE github_app_configurations + SET installation_account_github_id = owner_github_id + WHERE installation_id IS NOT NULL + SQL + add_check_constraint :github_app_configurations, + "(installation_id IS NULL AND installation_account_github_id IS NULL) OR " \ + "(installation_id IS NOT NULL AND installation_account_github_id IS NOT NULL " \ + "AND installation_account_github_id > 0)", + name: "github_app_configurations_installation_account" + end + + def down + remove_check_constraint :github_app_configurations, name: "github_app_configurations_installation_account" + remove_column :github_app_configurations, :installation_account_github_id + end +end diff --git a/hub/db/schema.rb b/hub/db/schema.rb index 97846c4d..11ea3fe6 100644 --- a/hub/db/schema.rb +++ b/hub/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_15_060000) do +ActiveRecord::Schema[8.1].define(version: 2026_10_21_195500) do create_table "analytics_deliveries", force: :cascade do |t| t.integer "attempts", default: 0, null: false t.datetime "created_at", null: false @@ -268,7 +268,9 @@ t.string "webhook_rotation_state", default: "idle", null: false t.string "webhook_request_token" t.datetime "webhook_request_expires_at" + t.bigint "installation_account_github_id" t.index ["singleton_key"], name: "index_github_app_configurations_on_singleton_key", unique: true + t.check_constraint "(installation_id IS NULL AND installation_account_github_id IS NULL) OR (installation_id IS NOT NULL AND installation_account_github_id IS NOT NULL AND installation_account_github_id > 0)", name: "github_app_configurations_installation_account" t.check_constraint "(installation_state = 'not_installed' AND installation_id IS NULL) OR (installation_state <> 'not_installed' AND installation_id IS NOT NULL)", name: "github_app_configurations_installation_identity" t.check_constraint "(webhook_request_token IS NULL AND webhook_request_expires_at IS NULL) OR (webhook_rotation_state = 'pending' AND webhook_request_token IS NOT NULL AND webhook_request_expires_at IS NOT NULL)", name: "github_app_configurations_webhook_request" t.check_constraint "(webhook_rotation_state = 'idle' AND previous_webhook_secret IS NULL) OR (webhook_rotation_state IN ('pending', 'confirmed') AND previous_webhook_secret IS NOT NULL)", name: "github_app_configurations_webhook_rotation" diff --git a/hub/db/seeds/test/setup.rb b/hub/db/seeds/test/setup.rb index 9c05ac9b..7655cb1f 100644 --- a/hub/db/seeds/test/setup.rb +++ b/hub/db/seeds/test/setup.rb @@ -38,6 +38,7 @@ def github_app_configurations.create_installed(label = nil, **overrides) attributes = { singleton_key: 1, installation_id: 456, + installation_account_github_id: overrides.fetch(:owner_github_id, attributes_for.fetch(:owner_github_id)), installation_state: "active", installed_at: Time.current }.merge(overrides) diff --git a/hub/docs/github-app-setup.md b/hub/docs/github-app-setup.md index d69e0c72..92210e25 100644 --- a/hub/docs/github-app-setup.md +++ b/hub/docs/github-app-setup.md @@ -76,6 +76,18 @@ An upgraded Hub does not automatically grant these write permissions to an exist Production usually receives GitHub notifications for pushes, workflows, and installation changes. Both environments also poll GitHub so Hub can recover missed changes; local development relies on polling because it has no public tunnel. +## App ownership transfers + +Hub detects an ownership transfer by asking GitHub for the configured App's current owner. This uses the existing App credentials, not organization audit-log access. Hub accepts the response only for the same App ID, updates the owner and App slug, and records the transfer in its security audit. + +An App transfer can uninstall the App. Hub marks the previous installation and its repositories unavailable without deleting Builds, files, reviews, users, or administrator roles. Existing browser Sessions and canvas grants are retained, but they do not bypass repository checks while the installation is unavailable. Hub does not promise uninterrupted repository access during a transfer. + +The new owner installs the same App in their own account and selects its repositories. Hub accepts only an installation in the App owner's account; installing it in another organization is not supported. Hub stores the installation account separately so delayed removal notices for the previous account cannot remove a replacement installation. + +Installation notifications and the existing five-minute Build checks restore repository access after GitHub confirms the replacement installation. This also recovers a transfer completed before upgrading Hub, or one whose notifications were missed. Run `replica:github:poll` to check immediately. Transient GitHub failures do not count as proof that an installation was removed; failed jobs remain visible in the queue and a later check retries. + +The **GitHub App** administration page shows installation state and instructions when installation is needed. `replica:github:diagnose` reports an owner awaiting synchronization or an installation that is no longer present without changing the saved configuration. If diagnosis cannot reach GitHub, it reports failure instead. No console edits, credential replacement, or local reset are needed for an ordinary ownership transfer. + ## Routine App administration An existing Hub administrator opens **GitHub App** from the deployment security audit, or `/github_app`. For example, choose **Replace App credentials**, supply replacements from the same App's GitHub settings, confirm, and save. Every App mutation requires current persisted administrator permission, deliberate confirmation, and GitHub verification of the same browser Session within the last five minutes. If verification is needed, return and submit again; Hub never replays the operation. The role does not grant repository access. diff --git a/hub/lib/tasks/github_app.rake b/hub/lib/tasks/github_app.rake index 70d4214b..2616ebe7 100644 --- a/hub/lib/tasks/github_app.rake +++ b/hub/lib/tasks/github_app.rake @@ -23,8 +23,6 @@ namespace :replica do configuration = GithubAppConfiguration.current abort "GitHub App: not configured" if configuration.nil? - abort "GitHub App installation: not installed" if configuration.installation_id.nil? - configuration.discover_builds! puts "Repositories synchronized: #{Repository.active.count} active" puts "Builds discovered: #{Build.count} total" diff --git a/hub/test/integration/github_app_installation_atomicity_test.rb b/hub/test/integration/github_app_installation_atomicity_test.rb index ffe83b20..8851e9ac 100644 --- a/hub/test/integration/github_app_installation_atomicity_test.rb +++ b/hub/test/integration/github_app_installation_atomicity_test.rb @@ -120,7 +120,9 @@ def start_installation def stub_installation stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") .with { assert_no_write_transaction } - .to_return(status: 200, body: JSON.generate(id: 456, account: @credentials.fetch("owner"))) + .to_return(status: 200, body: JSON.generate( + id: 456, app_id: @credentials.fetch("id"), account: @credentials.fetch("owner") + )) stub_request(:post, "#{GITHUB_API_URL}/app/installations/456/access_tokens") .with { assert_no_write_transaction } .to_return(status: 201, body: JSON.generate(token: "installation-token", expires_at: 1.hour.from_now.iso8601)) @@ -138,7 +140,7 @@ def assert_no_write_transaction def installation_state @configuration.reload.attributes.slice \ - "installation_id", "installation_state", "owner_login", "installed_at", + "installation_id", "installation_account_github_id", "installation_state", "owner_login", "installed_at", "suspended_at", "installation_checked_at", "updated_at" end diff --git a/hub/test/integration/github_app_setup_test.rb b/hub/test/integration/github_app_setup_test.rb index e56283ee..f84f954e 100644 --- a/hub/test/integration/github_app_setup_test.rb +++ b/hub/test/integration/github_app_setup_test.rb @@ -88,7 +88,7 @@ class GithubAppSetupTest < ActionDispatch::IntegrationTest stub_request(:get, "https://api.github.com/app/installations/456") .to_return \ status: 200, - body: JSON.generate(id: 456, account: credentials.fetch("owner")), + body: JSON.generate(id: 456, app_id: credentials.fetch("id"), account: credentials.fetch("owner")), headers: { "Content-Type" => "application/json" } stub_github_installation_token stub_github_repositories([]) diff --git a/hub/test/lib/github_app_installation_account_migration_test.rb b/hub/test/lib/github_app_installation_account_migration_test.rb new file mode 100644 index 00000000..c28693c6 --- /dev/null +++ b/hub/test/lib/github_app_installation_account_migration_test.rb @@ -0,0 +1,49 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261021195500_add_installation_account_to_github_app_configurations").to_s + +class GithubAppInstallationAccountMigrationTest < ActiveSupport::TestCase + class UpgradeRecord < ActiveRecord::Base + self.abstract_class = true + end + + test "backfills saved installation accounts and enforces their identity independently of current ownership" do + UpgradeRecord.establish_connection(adapter: "sqlite3", database: ":memory:") + connection = UpgradeRecord.connection + connection.create_table(:github_app_configurations) do |table| + table.bigint :owner_github_id, null: false + table.bigint :installation_id + end + connection.execute <<~SQL + INSERT INTO github_app_configurations (owner_github_id, installation_id) + VALUES (321, 456), (654, NULL) + SQL + migration = AddInstallationAccountToGithubAppConfigurations.new + migration.define_singleton_method(:connection) { connection } + migration.suppress_messages { migration.up } + + accounts = connection.select_values \ + "SELECT installation_account_github_id FROM github_app_configurations ORDER BY id" + assert_equal [ 321, nil ], accounts + connection.execute("UPDATE github_app_configurations SET owner_github_id = 789 WHERE id = 1") + assert_equal 321, connection.select_value( + "SELECT installation_account_github_id FROM github_app_configurations WHERE id = 1" + ) + [ "NULL", "0", "-1" ].each do |invalid_account| + assert_raises(ActiveRecord::StatementInvalid) do + connection.execute \ + "UPDATE github_app_configurations SET installation_account_github_id = #{invalid_account} WHERE id = 1" + end + end + assert_raises(ActiveRecord::StatementInvalid) do + connection.execute("UPDATE github_app_configurations SET installation_account_github_id = 654 WHERE id = 2") + end + + migration.suppress_messages { migration.down } + + assert_equal %w[id owner_github_id installation_id], + connection.columns(:github_app_configurations).map(&:name) + assert_equal 456, connection.select_value("SELECT installation_id FROM github_app_configurations WHERE id = 1") + ensure + UpgradeRecord.connection_pool.disconnect! + end +end diff --git a/hub/test/models/github_app_administration_concurrency_test.rb b/hub/test/models/github_app_administration_concurrency_test.rb index 2d535937..1798b227 100644 --- a/hub/test/models/github_app_administration_concurrency_test.rb +++ b/hub/test/models/github_app_administration_concurrency_test.rb @@ -216,7 +216,9 @@ class GithubAppAdministrationConcurrencyTest < ActiveSupport::TestCase %w[browser host].each do |entrypoint| test "#{entrypoint} credential replacement survives an unchanged poll during key verification" do - @configuration.update!(installation_id: 456, installation_state: "active", installed_at: Time.current) + @configuration.update! \ + installation_id: 456, installation_account_github_id: 321, + installation_state: "active", installed_at: Time.current version = @configuration.lock_version dispatch = Queue.new finish = Queue.new @@ -264,11 +266,14 @@ class GithubAppAdministrationConcurrencyTest < ActiveSupport::TestCase end test "overlapping unchanged polls both synchronize repositories without stale writes" do - @configuration.update!(installation_id: 456, installation_state: "active", installed_at: Time.current) + @configuration.update! \ + installation_id: 456, installation_account_github_id: 321, + installation_state: "active", installed_at: Time.current version = @configuration.lock_version dispatch = Queue.new finish = Queue.new connections = Queue.new + stub_github_app(configuration: @configuration) stub_github_installation_token stub_github_repositories([]) stub_request(:get, "#{GITHUB_API_URL}/app/installations/456").to_return do @@ -305,6 +310,7 @@ class GithubAppAdministrationConcurrencyTest < ActiveSupport::TestCase def installation_payload { "id" => 456, + "app_id" => 123, "account" => { "id" => 321, "login" => "github", "type" => "Organization" } } end diff --git a/hub/test/models/github_app_configuration/administration_test.rb b/hub/test/models/github_app_configuration/administration_test.rb index 2885e9de..1e3a616c 100644 --- a/hub/test/models/github_app_configuration/administration_test.rb +++ b/hub/test/models/github_app_configuration/administration_test.rb @@ -362,6 +362,7 @@ class GithubAppConfiguration::AdministrationTest < ActiveSupport::TestCase def installation_payload { "id" => 456, + "app_id" => 123, "account" => { "id" => 321, "login" => "github", "type" => "Organization" } } end diff --git a/hub/test/models/github_app_configuration/diagnostics_test.rb b/hub/test/models/github_app_configuration/diagnostics_test.rb new file mode 100644 index 00000000..0468696e --- /dev/null +++ b/hub/test/models/github_app_configuration/diagnostics_test.rb @@ -0,0 +1,45 @@ +require "test_helper" + +class GithubAppConfiguration::DiagnosticsTest < ActiveSupport::TestCase + test "reports transferred ownership without changing saved App or installation state" do + configuration = github_app_configurations.create_installed + repository = repositories.create + version = configuration.lock_version + stub_github_app \ + configuration:, owner: { id: 654, login: "new-team", type: "Organization" } + + result = configuration.diagnostics.call + + assert_equal "configured", result.configuration + assert_equal "owner changed; waiting for synchronization", result.app + assert_equal "needs rechecking", result.installation + assert_equal 321, configuration.reload.owner_github_id + assert_equal version, configuration.lock_version + assert_equal "active", repository.reload.installation_state + assert_not_requested :get, "#{GITHUB_API_URL}/app/installations/456" + end + + test "reports a missing remote installation without mutating the last saved status" do + configuration = github_app_configurations.create_installed + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 404, body: "{}") + + result = configuration.diagnostics.call + + assert_equal "reachable", result.app + assert_equal "not installed", result.installation + assert_equal "active", configuration.reload.installation_state + end + + test "rejects another App identity instead of reporting an accepted owner transfer" do + configuration = github_app_configurations.create_installed + stub_github_app \ + configuration:, app_id: 999, + owner: { id: 654, login: "new-team", type: "Organization" } + + assert_raises(GithubClient::InvalidResponse) { configuration.diagnostics.call } + + assert_equal 321, configuration.reload.owner_github_id + end +end diff --git a/hub/test/models/github_app_configuration/repositories_test.rb b/hub/test/models/github_app_configuration/repositories_test.rb index dbe2bdae..480e1c86 100644 --- a/hub/test/models/github_app_configuration/repositories_test.rb +++ b/hub/test/models/github_app_configuration/repositories_test.rb @@ -27,7 +27,7 @@ class GithubAppConfiguration::RepositoriesTest < ActiveSupport::TestCase test "marks every repository removed for an inactive installation" do configuration = github_app_configurations.create \ - installation_id: 456, installation_state: "suspended" + installation_id: 456, installation_account_github_id: 321, installation_state: "suspended" repository = repositories.create configuration.repositories.synchronize! diff --git a/hub/test/models/github_app_configuration/transfer_network_test.rb b/hub/test/models/github_app_configuration/transfer_network_test.rb new file mode 100644 index 00000000..7c08183c --- /dev/null +++ b/hub/test/models/github_app_configuration/transfer_network_test.rb @@ -0,0 +1,36 @@ +require "test_helper" + +class GithubAppConfiguration::TransferNetworkTest < ActiveSupport::TestCase + self.use_transactional_tests = false + + test "transfer recovery performs every GitHub request outside write transactions" do + configuration = github_app_configurations.create_installed + owner = { id: 654, login: "new-team", type: "Organization" } + responses = { + [ :get, "/app" ] => { id: 123, slug: "transferred-hub", owner: }, + [ :get, "/orgs/new-team/installation" ] => { id: 789, app_id: 123, account: owner }, + [ :post, "/app/installations/789/access_tokens" ] => { + token: "replacement-token", expires_at: 1.hour.from_now.iso8601 + }, + [ :get, "/installation/repositories?per_page=100&page=1" ] => { repositories: [] } + } + responses.each do |(method, path), payload| + stub_request(method, "#{GITHUB_API_URL}#{path}").to_return do + assert_not ActiveRecord::Base.connection.transaction_open?, path + { status: 200, body: JSON.generate(payload), headers: { "Content-Type" => "application/json" } } + end + end + + configuration.synchronize_repositories! + + assert_equal 789, configuration.reload.installation_id + assert_equal 654, configuration.installation_account_github_id + ensure + if configuration&.persisted? + SecurityAuditEvent + .where("json_extract(context, '$.github_app_configuration_id') = ?", configuration.id) + .delete_all + configuration.destroy! + end + end +end diff --git a/hub/test/models/github_app_configuration/transfer_recovery_test.rb b/hub/test/models/github_app_configuration/transfer_recovery_test.rb new file mode 100644 index 00000000..318e88f6 --- /dev/null +++ b/hub/test/models/github_app_configuration/transfer_recovery_test.rb @@ -0,0 +1,454 @@ +require "test_helper" + +class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase + test "polling recovers a personal App transfer without replacing credentials or live access" do + configuration = github_app_configurations.create_installed \ + owner_github_id: 1001, owner_login: "octocat", owner_type: "User" + user = users.create + session = sessions.create(user:, fresh_authorized_at: Time.current) + repository = repositories.create + build = builds.create(prototype: prototypes.create(repository:)) + grant = preview_grants.create(build:, session:) + credentials = configuration.slice \ + :github_app_id, :client_id, :client_secret, :private_key, :webhook_secret, :credential_revision + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:, slug: "transferred-hub") + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(repository.github_id, "new-team/prototype") ]) + + configuration.synchronize_repositories! + + assert_equal credentials, configuration.reload.slice(*credentials.keys) + assert_equal [ 654, "new-team", "Organization", "transferred-hub" ], + configuration.slice(:owner_github_id, :owner_login, :owner_type, :app_slug).values + assert_equal 789, configuration.installation_id + assert_equal 654, configuration.installation_account_github_id + assert_equal "active", configuration.installation_state + assert_equal "new-team/prototype", repository.reload.name_with_owner + assert_equal session.fresh_authorized_at, session.reload.fresh_authorized_at + assert_equal session.id, grant.reload.session_id + assert User.exists?(user.id) + assert Build.exists?(build.id) + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" + end + + test "polling discovers an organization transfer with the old owner's backfilled installation account" do + configuration = github_app_configurations.create_installed + owner = { id: 654, login: "new-team", type: "Organization" } + repository = repositories.create + stub_github_app(configuration:, owner:) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(repository.github_id, "new-team/prototype") ]) + + configuration.synchronize_repositories! + + assert_equal 654, configuration.reload.owner_github_id + assert_equal 654, configuration.installation_account_github_id + assert_equal 789, configuration.installation_id + assert_equal "active", repository.reload.installation_state + end + + test "polling discovers a missed installation even when no installation was saved" do + configuration = github_app_configurations.create + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(2001, "new-team/prototype") ]) + + configuration.synchronize_repositories! + + assert_equal 789, configuration.reload.installation_id + assert_equal "active", configuration.installation_state + assert Repository.active.exists?(github_id: 2001) + end + + test "polling can discover a personal owner's installation without a saved installation ID" do + configuration = github_app_configurations.create \ + owner_github_id: 1001, owner_login: "octocat", owner_type: "User" + owner = { id: 1001, login: "octocat", type: "User" } + stub_github_app(configuration:) + stub_github_account_installation(account: owner) + stub_github_installation_token + stub_github_repositories([]) + + configuration.synchronize_repositories! + + assert_equal 456, configuration.reload.installation_id + assert_equal 1001, configuration.installation_account_github_id + assert_requested :get, "#{GITHUB_API_URL}/users/octocat/installation", times: 1 + end + + test "confirmed absence after a transfer retains history and permits later polling recovery" do + configuration = github_app_configurations.create_installed + repository = repositories.create + prototype = prototypes.create(repository:) + build = builds.create_successful(prototype:) + user = users.create + comment = ReviewComment.create! \ + prototype:, build:, user:, body: "Retain this review.", + anchor: { + schemaVersion: 1, kind: "element", + route: { pathname: "/", search: "", hash: "" }, + selector: "#save", accessibleName: "Save", tagName: "button", selectedText: "", + rect: { x: 20, y: 30, width: 100, height: 32 }, + viewport: { width: 1280, height: 720, scrollX: 0, scrollY: 0 }, + elementOffset: { x: 0.5, y: 0.5 } + } + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:) + account_request = stub_github_account_installation(account: owner, status: 404) + + configuration.synchronize_repositories! + + assert_equal 654, configuration.reload.owner_github_id + assert_not_equal "active", configuration.installation_state + assert_equal "removed", repository.reload.installation_state + assert Build.exists?(build.id) + assert Prototype.exists?(prototype.id) + assert User.exists?(user.id) + assert_equal "Retain this review.", comment.reload.body + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" + + remove_request_stub(account_request) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(repository.github_id, "new-team/prototype") ]) + + configuration.synchronize_repositories! + + assert_equal 789, configuration.reload.installation_id + assert_equal 654, configuration.installation_account_github_id + assert_equal "active", repository.reload.installation_state + assert Build.exists?(build.id) + end + + test "polling rejects another authenticated App identity before accepting its owner" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app \ + configuration:, app_id: 999, + owner: { id: 654, login: "new-team", type: "Organization" } + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 321, configuration.reload.owner_github_id + assert_equal 456, configuration.installation_id + assert_equal "active", repository.reload.installation_state + assert_not_requested :get, "#{GITHUB_API_URL}/orgs/new-team/installation" + end + + test "an installation in another account cannot replace the current owner's installation" do + configuration = github_app_configurations.create_installed + owner = { id: 321, login: "github", type: "Organization" } + stub_github_app(configuration:) + stub_missing_saved_installation + stub_github_account_installation \ + account: owner, payload: installation_payload(owner: owner.merge(id: 999), id: 789) + + assert_raises(GithubAppConfiguration::Installation::OwnerMismatch) do + configuration.synchronize_repositories! + end + + assert_equal 456, configuration.reload.installation_id + assert_equal 321, configuration.installation_account_github_id + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" + end + + test "an installation belonging to another App cannot be activated" do + configuration = github_app_configurations.create_installed + owner = { id: 321, login: "github", type: "Organization" } + stub_github_app(configuration:) + stub_missing_saved_installation + stub_github_account_installation(account: owner, installation_id: 789, app_id: 999) + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 456, configuration.reload.installation_id + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" + end + + test "a forbidden account lookup is not evidence that the installation was removed" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app(configuration:) + stub_missing_saved_installation + stub_github_account_installation \ + account: { id: 321, login: "github", type: "Organization" }, status: 403 + + assert_raises(GithubClient::Forbidden) { configuration.synchronize_repositories! } + + assert_equal "active", configuration.reload.installation_state + assert_equal 456, configuration.installation_id + assert_equal "active", repository.reload.installation_state + end + + test "a malformed App response cannot replace ownership or remove repositories" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_request(:get, "#{GITHUB_API_URL}/app") + .to_return(status: 200, body: JSON.generate(id: 123, slug: "transferred-hub", owner: nil)) + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 321, configuration.reload.owner_github_id + assert_equal "active", configuration.installation_state + assert_equal "active", repository.reload.installation_state + end + + test "an App API timeout leaves the last known installation available" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_request(:get, "#{GITHUB_API_URL}/app").to_timeout + + assert_raises(GithubClient::Error) { configuration.synchronize_repositories! } + + assert_equal 321, configuration.reload.owner_github_id + assert_equal 456, configuration.installation_id + assert_equal "active", repository.reload.installation_state + end + + test "a malformed installation response cannot be treated as confirmed absence" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app(configuration:) + stub_missing_saved_installation + stub_github_account_installation \ + account: { id: 321, login: "github", type: "Organization" }, payload: {} + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal "active", configuration.reload.installation_state + assert_equal "active", repository.reload.installation_state + end + + test "a null account installation response cannot remove the saved installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app(configuration:) + stub_missing_saved_installation + stub_request(:get, "#{GITHUB_API_URL}/orgs/github/installation") + .to_return(status: 200, body: "null") + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 456, configuration.reload.installation_id + assert_equal "active", configuration.installation_state + assert_equal "active", repository.reload.installation_state + end + + test "a null saved installation response cannot be treated as a missing installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 200, body: "null") + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 456, configuration.reload.installation_id + assert_equal "active", configuration.installation_state + assert_equal "active", repository.reload.installation_state + assert_not_requested :get, "#{GITHUB_API_URL}/orgs/github/installation" + end + + test "a saved installation lookup cannot return another installation in the same account" do + configuration = github_app_configurations.create_installed + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 200, body: JSON.generate( + installation_payload(owner: { id: 321, login: "github", type: "Organization" }, id: 789) + )) + + assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } + + assert_equal 456, configuration.reload.installation_id + assert_equal "active", configuration.installation_state + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" + end + + test "an owner rename and App slug change preserve the installation without recording a transfer" do + configuration = github_app_configurations.create_installed + repository = repositories.create + owner = { id: 321, login: "renamed-team", type: "Organization" } + installed_at = configuration.installed_at + stub_github_app(configuration:, owner:, slug: "renamed-hub") + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 456))) + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/installation/repositories") + .with(query: { "page" => "1", "per_page" => "100" }).to_return do + assert_equal "active", GithubAppConfiguration.find(configuration.id).installation_state + assert_equal "active", repository.reload.installation_state + { + status: 200, + body: JSON.generate(repositories: [ + repository_payload(repository.github_id, "renamed-team/prototype") + ]) + } + end + + assert_no_difference -> { SecurityAuditEvent.where(action: "github_app.transfer").count } do + configuration.synchronize_repositories! + end + + assert_equal "renamed-team", configuration.reload.owner_login + assert_equal "renamed-hub", configuration.app_slug + assert_equal 456, configuration.installation_id + assert_equal 321, configuration.installation_account_github_id + assert_equal installed_at, configuration.installed_at + assert_equal "renamed-team/prototype", repository.reload.name_with_owner + end + + test "an installation webhook recovers transferred ownership before accepting the new installation" do + configuration = github_app_configurations.create_installed + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") + .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(2001, "new-team/prototype") ]) + + deliver_installation(configuration:, owner:, id: 789, action: "created") + + assert_equal 654, configuration.reload.owner_github_id + assert_equal 654, configuration.installation_account_github_id + assert_equal 789, configuration.installation_id + assert Repository.active.exists?(github_id: 2001) + end + + test "a delayed old-owner uninstall cannot clear the recovered installation" do + configuration = github_app_configurations.create_installed \ + owner_github_id: 654, owner_login: "new-team", installation_id: 789 + repository = repositories.create(name_with_owner: "new-team/prototype") + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") + .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([ repository_payload(repository.github_id, repository.name_with_owner) ]) + + deliver_installation \ + configuration:, owner: { id: 321, login: "github", type: "Organization" }, + id: 456, action: "deleted" + + assert_equal 789, configuration.reload.installation_id + assert_equal 654, configuration.owner_github_id + assert_equal "active", configuration.installation_state + assert_equal "active", repository.reload.installation_state + end + + test "a delayed old-owner creation cannot restore an installation after a transfer" do + configuration = github_app_configurations.create_installed \ + owner_github_id: 654, owner_login: "new-team", installation_id: 789 + owner = { id: 654, login: "new-team", type: "Organization" } + stub_github_app(configuration:, owner:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") + .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) + stub_github_account_installation(account: owner, installation_id: 789) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([]) + + deliver_installation \ + configuration:, owner: { id: 321, login: "github", type: "Organization" }, + id: 456, action: "created" + + assert_equal 789, configuration.reload.installation_id + assert_equal 654, configuration.owner_github_id + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" + end + + test "a stale App response cannot roll ownership back after a newer transfer was recorded" do + configuration = github_app_configurations.create_installed + stub_request(:get, "#{GITHUB_API_URL}/app").to_return do + GithubAppConfiguration.find(configuration.id).update! \ + owner_github_id: 654, owner_login: "new-team", installation_id: 789, + installation_account_github_id: 654 + { + status: 200, + body: JSON.generate(id: 123, slug: "replica-hub-test", + owner: { id: 321, login: "github", type: "Organization" }) + } + end + + assert_raises(GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError) do + configuration.synchronize_repositories! + end + + assert_equal 654, configuration.reload.owner_github_id + assert_equal 789, configuration.installation_id + assert_not_requested :get, "#{GITHUB_API_URL}/orgs/github/installation" + end + + test "an old repository response cannot reactivate repositories after a concurrent transfer" do + configuration = github_app_configurations.create_installed + repository = repositories.create + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 200, body: JSON.generate( + installation_payload(owner: { id: 321, login: "github", type: "Organization" }, id: 456) + )) + stub_github_account_installation(account: { id: 321, login: "github", type: "Organization" }) + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/installation/repositories") + .with(query: { "page" => "1", "per_page" => "100" }).to_return do + GithubAppConfiguration.find(configuration.id).update! \ + owner_github_id: 654, owner_login: "new-team", installation_id: 789, + installation_account_github_id: 654 + repository.update!(installation_state: "removed", removed_at: Time.current) + { status: 200, body: JSON.generate(repositories: [ + repository_payload(repository.github_id, repository.name_with_owner) + ]) } + end + + assert_raises(GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError) do + configuration.synchronize_repositories! + end + + assert_equal 654, configuration.reload.owner_github_id + assert_equal 789, configuration.installation_id + assert_equal "removed", repository.reload.installation_state + end + + test "a stale installation lookup cannot replace a newer owner's installation" do + configuration = github_app_configurations.create + owner = { id: 321, login: "github", type: "Organization" } + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/orgs/github/installation").to_return do + GithubAppConfiguration.find(configuration.id).update! \ + owner_github_id: 654, owner_login: "new-team", + installation_id: 789, installation_state: "active", installation_account_github_id: 654 + { status: 200, body: JSON.generate(installation_payload(owner:, id: 456)) } + end + + assert_raises(GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError) do + configuration.synchronize_repositories! + end + + assert_equal 654, configuration.reload.owner_github_id + assert_equal 789, configuration.installation_id + assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" + end + + private + + def stub_missing_saved_installation + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 404, body: "{}") + end + + def installation_payload(owner:, id:) + { id:, app_id: 123, account: owner, suspended_at: nil } + end + + def deliver_installation(configuration:, owner:, id:, action:) + body = JSON.generate(action:, installation: installation_payload(owner:, id:)) + signature = "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", configuration.webhook_secret, body)}" + GithubWebhook.process! \ + configuration:, body:, signature:, delivery_id: SecureRandom.uuid, event: "installation" + end +end diff --git a/hub/test/models/github_app_dual_barrier_concurrency_test.rb b/hub/test/models/github_app_dual_barrier_concurrency_test.rb index b221c855..98ab5f0c 100644 --- a/hub/test/models/github_app_dual_barrier_concurrency_test.rb +++ b/hub/test/models/github_app_dual_barrier_concurrency_test.rb @@ -88,9 +88,10 @@ class GithubAppDualBarrierConcurrencyTest < ActiveSupport::TestCase end test "credential repair during prepared installation cannot apply status repositories or success audit" do - @configuration.update!(installation_id: nil, installation_state: "not_installed") + @configuration.update! \ + installation_id: nil, installation_account_github_id: nil, installation_state: "not_installed" stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") - .to_return(status: 200, body: JSON.generate(id: 456, + .to_return(status: 200, body: JSON.generate(id: 456, app_id: @configuration.github_app_id, account: github_app_configurations.manifest_credentials.fetch("owner"))) before = @repository.attributes url = "#{GITHUB_API_URL}/installation/repositories?page=1&per_page=100" diff --git a/hub/test/models/github_webhook_test.rb b/hub/test/models/github_webhook_test.rb index 8a4e7bf0..37855063 100644 --- a/hub/test/models/github_webhook_test.rb +++ b/hub/test/models/github_webhook_test.rb @@ -13,6 +13,9 @@ class GithubWebhookTest < ActiveSupport::TestCase payload = webhook_payload payload["installation"] = payload.fetch("installation").merge("id" => 789) body = JSON.generate(payload) + stub_github_app(configuration:) + stub_github_account_installation \ + account: { id: 321, login: "github", type: "Organization" }, installation_id: 789 installation_repositories = configuration.repositories installation_repositories.define_singleton_method(:synchronize!) do raise GithubClient::Error, "GitHub is temporarily unavailable" @@ -30,17 +33,20 @@ class GithubWebhookTest < ActiveSupport::TestCase test "reconciles a repository selection update before recording it" do configuration = github_app_configurations.create payload = webhook_payload.merge("action" => "added") - reconciled = nil - installation = Object.new - installation.define_singleton_method(:reconcile!) { |id| reconciled = id } - configuration.define_singleton_method(:installation) { installation } + stub_github_app(configuration:) + request = stub_github_account_installation \ + account: { id: 321, login: "github", type: "Organization" } + stub_github_installation_token + stub_github_repositories([ repository_payload(2001, "github/prototype") ]) process_webhook \ configuration:, body: JSON.generate(payload), event: "installation_repositories" - assert_equal 456, reconciled + assert_requested request, times: 1 + assert Repository.active.exists?(github_id: 2001) + assert WebhookDelivery.sole.processed? end test "rejects an invalid signature" do @@ -428,20 +434,21 @@ class GithubWebhookTest < ActiveSupport::TestCase configuration = installed_configuration configuration.update!(installation_id: 789) track_recovery(configuration:, event: "installation", installation_id: 456) + stub_github_app(configuration:) stub_request(:get, "https://api.github.com/app/installations/456") .to_return(status: 404, body: "{}") + replacement = webhook_payload.fetch("installation").merge("id" => 789) + stub_request(:get, "https://api.github.com/app/installations/789") + .to_return(status: 200, body: JSON.generate(replacement)) + stub_github_installation_token(installation_id: 789) + stub_github_repositories([]) - assert_raises(GithubClient::NotFound) do - process_webhook(configuration:, body: JSON.generate(webhook_payload)) - end + process_webhook(configuration:, body: JSON.generate(webhook_payload)) assert_equal 789, configuration.reload.installation_id assert_equal "active", configuration.installation_state - assert_not WebhookDelivery.sole.processed? + assert WebhookDelivery.sole.processed? assert_not_requested :post, "https://api.github.com/app/installations/456/access_tokens" - replacement = webhook_payload.fetch("installation").merge("id" => 789) - stub_request(:get, "https://api.github.com/app/installations/789") - .to_return(status: 200, body: JSON.generate(replacement)) installation_repositories = configuration.repositories installation_repositories.define_singleton_method(:synchronize!) do raise GithubClient::Error, "GitHub is temporarily unavailable" @@ -491,8 +498,9 @@ class GithubWebhookTest < ActiveSupport::TestCase payload = webhook_payload payload["installation"] = payload.fetch("installation").merge("id" => 789) track_recovery(configuration:, event: "installation", installation_id: 789) - request = stub_request(:get, "https://api.github.com/app/installations/789") - .to_return(status: 200, body: JSON.generate(payload.fetch("installation"))) + stub_github_app(configuration:) + request = stub_github_account_installation \ + account: { id: 321, login: "github", type: "Organization" }, installation_id: 789 installation_repositories = configuration.repositories installation_repositories.define_singleton_method(:synchronize!) do raise GithubClient::Error, "GitHub is temporarily unavailable" diff --git a/hub/test/support/github_request_stubs.rb b/hub/test/support/github_request_stubs.rb index 1e5ea5f5..04ffc4c7 100644 --- a/hub/test/support/github_request_stubs.rb +++ b/hub/test/support/github_request_stubs.rb @@ -4,6 +4,35 @@ module GithubRequestStubs private + # Stubs the authenticated App identity without granting access to other endpoints. + def stub_github_app(configuration: GithubAppConfiguration.current, owner: nil, app_id: nil, slug: nil) + stub_request(:get, "#{GITHUB_API_URL}/app") + .with { |request| request.headers["Authorization"].to_s.start_with?("Bearer ") } + .to_return \ + status: 200, + body: JSON.generate( + id: app_id || configuration.github_app_id, + slug: slug || configuration.app_slug, + owner: owner || { + id: configuration.owner_github_id, + login: configuration.owner_login, + type: configuration.owner_type + } + ), + headers: { "Content-Type" => "application/json" } + end + + # Stubs the configured App's installation in one exact account. + def stub_github_account_installation(account:, installation_id: 456, app_id: 123, status: 200, payload: nil) + scope = account.fetch(:type) == "Organization" ? "orgs" : "users" + stub_request(:get, "#{GITHUB_API_URL}/#{scope}/#{account.fetch(:login)}/installation") + .with { |request| request.headers["Authorization"].to_s.start_with?("Bearer ") } + .to_return \ + status:, + body: JSON.generate(payload || { id: installation_id, app_id:, account:, suspended_at: nil }), + headers: { "Content-Type" => "application/json" } + end + # Checks the account-specific landing page while completing an already stubbed sign-in. def sign_in_to_hub(github_id: 1001) user = User.find_by(github_id:) From 3009673e51b3fe1bf66665b85de3959de25bbbeb Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:29:41 +0000 Subject: [PATCH 2/4] test(hub): focus App transfer coverage on user outcomes Keep transfer recovery and critical failure coverage while removing duplicate scenarios, unlikely response variations, and internal-step assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...app_installation_account_migration_test.rb | 29 +-- .../diagnostics_test.rb | 14 -- .../transfer_network_test.rb | 36 --- .../transfer_recovery_test.rb | 233 +----------------- 4 files changed, 14 insertions(+), 298 deletions(-) delete mode 100644 hub/test/models/github_app_configuration/transfer_network_test.rb diff --git a/hub/test/lib/github_app_installation_account_migration_test.rb b/hub/test/lib/github_app_installation_account_migration_test.rb index c28693c6..4b16f8d8 100644 --- a/hub/test/lib/github_app_installation_account_migration_test.rb +++ b/hub/test/lib/github_app_installation_account_migration_test.rb @@ -6,7 +6,7 @@ class UpgradeRecord < ActiveRecord::Base self.abstract_class = true end - test "backfills saved installation accounts and enforces their identity independently of current ownership" do + test "upgrading preserves saved installations and records their accounts" do UpgradeRecord.establish_connection(adapter: "sqlite3", database: ":memory:") connection = UpgradeRecord.connection connection.create_table(:github_app_configurations) do |table| @@ -21,28 +21,11 @@ class UpgradeRecord < ActiveRecord::Base migration.define_singleton_method(:connection) { connection } migration.suppress_messages { migration.up } - accounts = connection.select_values \ - "SELECT installation_account_github_id FROM github_app_configurations ORDER BY id" - assert_equal [ 321, nil ], accounts - connection.execute("UPDATE github_app_configurations SET owner_github_id = 789 WHERE id = 1") - assert_equal 321, connection.select_value( - "SELECT installation_account_github_id FROM github_app_configurations WHERE id = 1" - ) - [ "NULL", "0", "-1" ].each do |invalid_account| - assert_raises(ActiveRecord::StatementInvalid) do - connection.execute \ - "UPDATE github_app_configurations SET installation_account_github_id = #{invalid_account} WHERE id = 1" - end - end - assert_raises(ActiveRecord::StatementInvalid) do - connection.execute("UPDATE github_app_configurations SET installation_account_github_id = 654 WHERE id = 2") - end - - migration.suppress_messages { migration.down } - - assert_equal %w[id owner_github_id installation_id], - connection.columns(:github_app_configurations).map(&:name) - assert_equal 456, connection.select_value("SELECT installation_id FROM github_app_configurations WHERE id = 1") + installations = connection.select_rows <<~SQL + SELECT owner_github_id, installation_id, installation_account_github_id + FROM github_app_configurations ORDER BY id + SQL + assert_equal [ [ 321, 456, 321 ], [ 654, nil, nil ] ], installations ensure UpgradeRecord.connection_pool.disconnect! end diff --git a/hub/test/models/github_app_configuration/diagnostics_test.rb b/hub/test/models/github_app_configuration/diagnostics_test.rb index 0468696e..355f69ce 100644 --- a/hub/test/models/github_app_configuration/diagnostics_test.rb +++ b/hub/test/models/github_app_configuration/diagnostics_test.rb @@ -4,7 +4,6 @@ class GithubAppConfiguration::DiagnosticsTest < ActiveSupport::TestCase test "reports transferred ownership without changing saved App or installation state" do configuration = github_app_configurations.create_installed repository = repositories.create - version = configuration.lock_version stub_github_app \ configuration:, owner: { id: 654, login: "new-team", type: "Organization" } @@ -14,9 +13,7 @@ class GithubAppConfiguration::DiagnosticsTest < ActiveSupport::TestCase assert_equal "owner changed; waiting for synchronization", result.app assert_equal "needs rechecking", result.installation assert_equal 321, configuration.reload.owner_github_id - assert_equal version, configuration.lock_version assert_equal "active", repository.reload.installation_state - assert_not_requested :get, "#{GITHUB_API_URL}/app/installations/456" end test "reports a missing remote installation without mutating the last saved status" do @@ -31,15 +28,4 @@ class GithubAppConfiguration::DiagnosticsTest < ActiveSupport::TestCase assert_equal "not installed", result.installation assert_equal "active", configuration.reload.installation_state end - - test "rejects another App identity instead of reporting an accepted owner transfer" do - configuration = github_app_configurations.create_installed - stub_github_app \ - configuration:, app_id: 999, - owner: { id: 654, login: "new-team", type: "Organization" } - - assert_raises(GithubClient::InvalidResponse) { configuration.diagnostics.call } - - assert_equal 321, configuration.reload.owner_github_id - end end diff --git a/hub/test/models/github_app_configuration/transfer_network_test.rb b/hub/test/models/github_app_configuration/transfer_network_test.rb deleted file mode 100644 index 7c08183c..00000000 --- a/hub/test/models/github_app_configuration/transfer_network_test.rb +++ /dev/null @@ -1,36 +0,0 @@ -require "test_helper" - -class GithubAppConfiguration::TransferNetworkTest < ActiveSupport::TestCase - self.use_transactional_tests = false - - test "transfer recovery performs every GitHub request outside write transactions" do - configuration = github_app_configurations.create_installed - owner = { id: 654, login: "new-team", type: "Organization" } - responses = { - [ :get, "/app" ] => { id: 123, slug: "transferred-hub", owner: }, - [ :get, "/orgs/new-team/installation" ] => { id: 789, app_id: 123, account: owner }, - [ :post, "/app/installations/789/access_tokens" ] => { - token: "replacement-token", expires_at: 1.hour.from_now.iso8601 - }, - [ :get, "/installation/repositories?per_page=100&page=1" ] => { repositories: [] } - } - responses.each do |(method, path), payload| - stub_request(method, "#{GITHUB_API_URL}#{path}").to_return do - assert_not ActiveRecord::Base.connection.transaction_open?, path - { status: 200, body: JSON.generate(payload), headers: { "Content-Type" => "application/json" } } - end - end - - configuration.synchronize_repositories! - - assert_equal 789, configuration.reload.installation_id - assert_equal 654, configuration.installation_account_github_id - ensure - if configuration&.persisted? - SecurityAuditEvent - .where("json_extract(context, '$.github_app_configuration_id') = ?", configuration.id) - .delete_all - configuration.destroy! - end - end -end diff --git a/hub/test/models/github_app_configuration/transfer_recovery_test.rb b/hub/test/models/github_app_configuration/transfer_recovery_test.rb index 318e88f6..f6fb0ef8 100644 --- a/hub/test/models/github_app_configuration/transfer_recovery_test.rb +++ b/hub/test/models/github_app_configuration/transfer_recovery_test.rb @@ -1,7 +1,7 @@ require "test_helper" class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase - test "polling recovers a personal App transfer without replacing credentials or live access" do + test "polling recovers a personal App transfer while retaining credentials and sessions" do configuration = github_app_configurations.create_installed \ owner_github_id: 1001, owner_login: "octocat", owner_type: "User" user = users.create @@ -30,24 +30,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal session.id, grant.reload.session_id assert User.exists?(user.id) assert Build.exists?(build.id) - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" - end - - test "polling discovers an organization transfer with the old owner's backfilled installation account" do - configuration = github_app_configurations.create_installed - owner = { id: 654, login: "new-team", type: "Organization" } - repository = repositories.create - stub_github_app(configuration:, owner:) - stub_github_account_installation(account: owner, installation_id: 789) - stub_github_installation_token(installation_id: 789) - stub_github_repositories([ repository_payload(repository.github_id, "new-team/prototype") ]) - - configuration.synchronize_repositories! - - assert_equal 654, configuration.reload.owner_github_id - assert_equal 654, configuration.installation_account_github_id - assert_equal 789, configuration.installation_id - assert_equal "active", repository.reload.installation_state end test "polling discovers a missed installation even when no installation was saved" do @@ -65,23 +47,7 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert Repository.active.exists?(github_id: 2001) end - test "polling can discover a personal owner's installation without a saved installation ID" do - configuration = github_app_configurations.create \ - owner_github_id: 1001, owner_login: "octocat", owner_type: "User" - owner = { id: 1001, login: "octocat", type: "User" } - stub_github_app(configuration:) - stub_github_account_installation(account: owner) - stub_github_installation_token - stub_github_repositories([]) - - configuration.synchronize_repositories! - - assert_equal 456, configuration.reload.installation_id - assert_equal 1001, configuration.installation_account_github_id - assert_requested :get, "#{GITHUB_API_URL}/users/octocat/installation", times: 1 - end - - test "confirmed absence after a transfer retains history and permits later polling recovery" do + test "an organization transfer retains history while waiting for reinstallation and recovers afterward" do configuration = github_app_configurations.create_installed repository = repositories.create prototype = prototypes.create(repository:) @@ -104,13 +70,13 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase configuration.synchronize_repositories! assert_equal 654, configuration.reload.owner_github_id + assert_equal 321, configuration.installation_account_github_id assert_not_equal "active", configuration.installation_state assert_equal "removed", repository.reload.installation_state assert Build.exists?(build.id) assert Prototype.exists?(prototype.id) assert User.exists?(user.id) assert_equal "Retain this review.", comment.reload.body - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" remove_request_stub(account_request) stub_github_account_installation(account: owner, installation_id: 789) @@ -137,7 +103,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal 321, configuration.reload.owner_github_id assert_equal 456, configuration.installation_id assert_equal "active", repository.reload.installation_state - assert_not_requested :get, "#{GITHUB_API_URL}/orgs/new-team/installation" end test "an installation in another account cannot replace the current owner's installation" do @@ -154,20 +119,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal 456, configuration.reload.installation_id assert_equal 321, configuration.installation_account_github_id - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" - end - - test "an installation belonging to another App cannot be activated" do - configuration = github_app_configurations.create_installed - owner = { id: 321, login: "github", type: "Organization" } - stub_github_app(configuration:) - stub_missing_saved_installation - stub_github_account_installation(account: owner, installation_id: 789, app_id: 999) - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal 456, configuration.reload.installation_id - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" end test "a forbidden account lookup is not evidence that the installation was removed" do @@ -185,129 +136,31 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal "active", repository.reload.installation_state end - test "a malformed App response cannot replace ownership or remove repositories" do - configuration = github_app_configurations.create_installed - repository = repositories.create - stub_request(:get, "#{GITHUB_API_URL}/app") - .to_return(status: 200, body: JSON.generate(id: 123, slug: "transferred-hub", owner: nil)) - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal 321, configuration.reload.owner_github_id - assert_equal "active", configuration.installation_state - assert_equal "active", repository.reload.installation_state - end - - test "an App API timeout leaves the last known installation available" do - configuration = github_app_configurations.create_installed - repository = repositories.create - stub_request(:get, "#{GITHUB_API_URL}/app").to_timeout - - assert_raises(GithubClient::Error) { configuration.synchronize_repositories! } - - assert_equal 321, configuration.reload.owner_github_id - assert_equal 456, configuration.installation_id - assert_equal "active", repository.reload.installation_state - end - - test "a malformed installation response cannot be treated as confirmed absence" do - configuration = github_app_configurations.create_installed - repository = repositories.create - stub_github_app(configuration:) - stub_missing_saved_installation - stub_github_account_installation \ - account: { id: 321, login: "github", type: "Organization" }, payload: {} - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal "active", configuration.reload.installation_state - assert_equal "active", repository.reload.installation_state - end - - test "a null account installation response cannot remove the saved installation" do - configuration = github_app_configurations.create_installed - repository = repositories.create - stub_github_app(configuration:) - stub_missing_saved_installation - stub_request(:get, "#{GITHUB_API_URL}/orgs/github/installation") - .to_return(status: 200, body: "null") - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal 456, configuration.reload.installation_id - assert_equal "active", configuration.installation_state - assert_equal "active", repository.reload.installation_state - end - - test "a null saved installation response cannot be treated as a missing installation" do - configuration = github_app_configurations.create_installed - repository = repositories.create - stub_github_app(configuration:) - stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") - .to_return(status: 200, body: "null") - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal 456, configuration.reload.installation_id - assert_equal "active", configuration.installation_state - assert_equal "active", repository.reload.installation_state - assert_not_requested :get, "#{GITHUB_API_URL}/orgs/github/installation" - end - - test "a saved installation lookup cannot return another installation in the same account" do - configuration = github_app_configurations.create_installed - stub_github_app(configuration:) - stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") - .to_return(status: 200, body: JSON.generate( - installation_payload(owner: { id: 321, login: "github", type: "Organization" }, id: 789) - )) - - assert_raises(GithubClient::InvalidResponse) { configuration.synchronize_repositories! } - - assert_equal 456, configuration.reload.installation_id - assert_equal "active", configuration.installation_state - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/789/access_tokens" - end - - test "an owner rename and App slug change preserve the installation without recording a transfer" do + test "an owner rename and App slug change keep repository access available" do configuration = github_app_configurations.create_installed repository = repositories.create owner = { id: 321, login: "renamed-team", type: "Organization" } - installed_at = configuration.installed_at stub_github_app(configuration:, owner:, slug: "renamed-hub") stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 456))) stub_github_installation_token - stub_request(:get, "#{GITHUB_API_URL}/installation/repositories") - .with(query: { "page" => "1", "per_page" => "100" }).to_return do - assert_equal "active", GithubAppConfiguration.find(configuration.id).installation_state - assert_equal "active", repository.reload.installation_state - { - status: 200, - body: JSON.generate(repositories: [ - repository_payload(repository.github_id, "renamed-team/prototype") - ]) - } - end + stub_github_repositories([ repository_payload(repository.github_id, "renamed-team/prototype") ]) - assert_no_difference -> { SecurityAuditEvent.where(action: "github_app.transfer").count } do - configuration.synchronize_repositories! - end + configuration.synchronize_repositories! assert_equal "renamed-team", configuration.reload.owner_login assert_equal "renamed-hub", configuration.app_slug assert_equal 456, configuration.installation_id assert_equal 321, configuration.installation_account_github_id - assert_equal installed_at, configuration.installed_at + assert_equal "active", configuration.installation_state assert_equal "renamed-team/prototype", repository.reload.name_with_owner + assert_equal "active", repository.installation_state end test "an installation webhook recovers transferred ownership before accepting the new installation" do configuration = github_app_configurations.create_installed owner = { id: 654, login: "new-team", type: "Organization" } stub_github_app(configuration:, owner:) - stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") - .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) stub_github_account_installation(account: owner, installation_id: 789) stub_github_installation_token(installation_id: 789) stub_github_repositories([ repository_payload(2001, "new-team/prototype") ]) @@ -324,13 +177,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase configuration = github_app_configurations.create_installed \ owner_github_id: 654, owner_login: "new-team", installation_id: 789 repository = repositories.create(name_with_owner: "new-team/prototype") - owner = { id: 654, login: "new-team", type: "Organization" } - stub_github_app(configuration:, owner:) - stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") - .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) - stub_github_account_installation(account: owner, installation_id: 789) - stub_github_installation_token(installation_id: 789) - stub_github_repositories([ repository_payload(repository.github_id, repository.name_with_owner) ]) deliver_installation \ configuration:, owner: { id: 321, login: "github", type: "Organization" }, @@ -342,48 +188,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal "active", repository.reload.installation_state end - test "a delayed old-owner creation cannot restore an installation after a transfer" do - configuration = github_app_configurations.create_installed \ - owner_github_id: 654, owner_login: "new-team", installation_id: 789 - owner = { id: 654, login: "new-team", type: "Organization" } - stub_github_app(configuration:, owner:) - stub_request(:get, "#{GITHUB_API_URL}/app/installations/789") - .to_return(status: 200, body: JSON.generate(installation_payload(owner:, id: 789))) - stub_github_account_installation(account: owner, installation_id: 789) - stub_github_installation_token(installation_id: 789) - stub_github_repositories([]) - - deliver_installation \ - configuration:, owner: { id: 321, login: "github", type: "Organization" }, - id: 456, action: "created" - - assert_equal 789, configuration.reload.installation_id - assert_equal 654, configuration.owner_github_id - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" - end - - test "a stale App response cannot roll ownership back after a newer transfer was recorded" do - configuration = github_app_configurations.create_installed - stub_request(:get, "#{GITHUB_API_URL}/app").to_return do - GithubAppConfiguration.find(configuration.id).update! \ - owner_github_id: 654, owner_login: "new-team", installation_id: 789, - installation_account_github_id: 654 - { - status: 200, - body: JSON.generate(id: 123, slug: "replica-hub-test", - owner: { id: 321, login: "github", type: "Organization" }) - } - end - - assert_raises(GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError) do - configuration.synchronize_repositories! - end - - assert_equal 654, configuration.reload.owner_github_id - assert_equal 789, configuration.installation_id - assert_not_requested :get, "#{GITHUB_API_URL}/orgs/github/installation" - end - test "an old repository response cannot reactivate repositories after a concurrent transfer" do configuration = github_app_configurations.create_installed repository = repositories.create @@ -392,7 +196,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase .to_return(status: 200, body: JSON.generate( installation_payload(owner: { id: 321, login: "github", type: "Organization" }, id: 456) )) - stub_github_account_installation(account: { id: 321, login: "github", type: "Organization" }) stub_github_installation_token stub_request(:get, "#{GITHUB_API_URL}/installation/repositories") .with(query: { "page" => "1", "per_page" => "100" }).to_return do @@ -414,26 +217,6 @@ class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase assert_equal "removed", repository.reload.installation_state end - test "a stale installation lookup cannot replace a newer owner's installation" do - configuration = github_app_configurations.create - owner = { id: 321, login: "github", type: "Organization" } - stub_github_app(configuration:) - stub_request(:get, "#{GITHUB_API_URL}/orgs/github/installation").to_return do - GithubAppConfiguration.find(configuration.id).update! \ - owner_github_id: 654, owner_login: "new-team", - installation_id: 789, installation_state: "active", installation_account_github_id: 654 - { status: 200, body: JSON.generate(installation_payload(owner:, id: 456)) } - end - - assert_raises(GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError) do - configuration.synchronize_repositories! - end - - assert_equal 654, configuration.reload.owner_github_id - assert_equal 789, configuration.installation_id - assert_not_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens" - end - private def stub_missing_saved_installation From 760f2606078334656dc8d3493beb06d221f82654 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:30:27 +0000 Subject: [PATCH 3/4] test(hub): assert repository selection outcomes Check repository availability instead of counting GitHub requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- hub/test/models/github_webhook_test.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hub/test/models/github_webhook_test.rb b/hub/test/models/github_webhook_test.rb index 37855063..b6403f8d 100644 --- a/hub/test/models/github_webhook_test.rb +++ b/hub/test/models/github_webhook_test.rb @@ -30,11 +30,11 @@ class GithubWebhookTest < ActiveSupport::TestCase assert_equal "active", configuration.installation_state end - test "reconciles a repository selection update before recording it" do + test "a repository selection notification makes selected repositories available" do configuration = github_app_configurations.create payload = webhook_payload.merge("action" => "added") stub_github_app(configuration:) - request = stub_github_account_installation \ + stub_github_account_installation \ account: { id: 321, login: "github", type: "Organization" } stub_github_installation_token stub_github_repositories([ repository_payload(2001, "github/prototype") ]) @@ -44,7 +44,6 @@ class GithubWebhookTest < ActiveSupport::TestCase body: JSON.generate(payload), event: "installation_repositories" - assert_requested request, times: 1 assert Repository.active.exists?(github_id: 2001) assert WebhookDelivery.sole.processed? end From 09fb24f9d4b21a52e6c1e7b44a80b7c5194201aa Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:37:09 +0000 Subject: [PATCH 4/4] fix(hub): reject results from replaced installations Bind branch and workflow discovery writes to the owner and installation captured before GitHub work, including webhook receipts and missing-run results after repository reactivation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- hub/app/models/github_app_configuration.rb | 18 +- .../build_discovery.rb | 21 +- .../github_app_configuration/repositories.rb | 3 +- hub/app/models/github_webhook.rb | 37 ++- hub/app/models/repository/branch_presence.rb | 16 +- hub/docs/github-app-setup.md | 2 + .../github_app_transfer_discovery_test.rb | 250 ++++++++++++++++++ 7 files changed, 311 insertions(+), 36 deletions(-) create mode 100644 hub/test/models/github_app_transfer_discovery_test.rb diff --git a/hub/app/models/github_app_configuration.rb b/hub/app/models/github_app_configuration.rb index 608cd9c3..097596d0 100644 --- a/hub/app/models/github_app_configuration.rb +++ b/hub/app/models/github_app_configuration.rb @@ -129,6 +129,12 @@ def credential_snapshot { configuration_id: id, credential_revision: } end + # Binds GitHub results to the owner and installation that requested them. + def installation_snapshot + credential_snapshot.merge \ + owner_github_id:, owner_type:, installation_id:, installation_account_github_id:, installation_state: + end + # Refreshes the installation and its repositories from GitHub. def synchronize_repositories! return if LocalDevelopment.enabled? @@ -137,11 +143,13 @@ def synchronize_repositories! installation.synchronize! end - # Prevents work for a previous owner or installation from restoring repository access. - def with_installation! - identity = attributes.slice("owner_github_id", "installation_id", "installation_state") - self.class.with_credentials!(**credential_snapshot) do - raise Superseded if !self.class.uncached { self.class.where(id:, **identity).exists? } + # Rejects old installation results in the transaction that would persist them. + def with_installation!(snapshot: installation_snapshot) + credentials = snapshot.slice(:configuration_id, :credential_revision) + identity = snapshot.except(:configuration_id, :credential_revision) + self.class.with_credentials!(**credentials) do + valid = self.class.uncached { self.class.where(id: snapshot.fetch(:configuration_id), **identity).exists? } + raise Superseded, "GitHub App installation changed; start again" if !valid yield end diff --git a/hub/app/models/github_app_configuration/build_discovery.rb b/hub/app/models/github_app_configuration/build_discovery.rb index 1c49a1d2..a002438d 100644 --- a/hub/app/models/github_app_configuration/build_discovery.rb +++ b/hub/app/models/github_app_configuration/build_discovery.rb @@ -5,15 +5,15 @@ def poll!(repository = nil) record.synchronize_repositories! return if record.installation_state != "active" + installation = record.installation_snapshot repositories = repository ? Repository.active.where(id: repository.id) : Repository.active - repositories.find_each { discover_repository!(_1) } + repositories.find_each { discover_repository!(_1, installation:) } end private # Records current branch commits and workflow states for one repository. - def discover_repository!(repository) - credentials = record.credential_snapshot + def discover_repository!(repository, installation:) observations = repository.branch_presence.observations current_branches = observations.values.select { |prototype| prototype.branch_deleted_at.nil? && prototype.reviewable? } .map(&:branch_name) @@ -25,18 +25,19 @@ def discover_repository!(repository) branch_name:, commit_sha:, observed: observations[branch_name], - configuration: record + configuration: record, + installation: end observations.each do |branch_name, observed| next if heads.key?(branch_name) - GithubAppConfiguration.with_credentials!(**credentials) do + record.with_installation!(snapshot: installation) do repository.branch_presence.record_missing!(branch_name:, observed:) end end runs = workflow_runs(repository) observed_run_ids = runs.filter_map do |run| - build = GithubAppConfiguration.with_credentials!(**credentials) do + build = record.with_installation!(snapshot: installation) do repository.workflow_run_discoveries.record!(run) end if build&.github_workflow_run_id == run.fetch("id") && @@ -44,7 +45,7 @@ def discover_repository!(repository) build.github_workflow_run_id end end - reconcile_saved_workflow_runs!(repository, observed_run_ids, credentials:) + reconcile_saved_workflow_runs!(repository, observed_run_ids, installation:) rescue KeyError, TypeError, ArgumentError, Repository::WorkflowRunDiscoveries::InvalidRun raise \ GithubClient::InvalidResponse, @@ -64,7 +65,7 @@ def workflow_runs(repository) end # Fetches exact saved runs whose completion was not present in the newest run list. - def reconcile_saved_workflow_runs!(repository, observed_run_ids, credentials:) + def reconcile_saved_workflow_runs!(repository, observed_run_ids, installation:) builds = Build.joins(:prototype).where \ prototypes: { repository_id: repository.id }, state: %w[queued running], @@ -76,11 +77,11 @@ def reconcile_saved_workflow_runs!(repository, observed_run_ids, credentials:) repository, build.github_workflow_run_id, token: record.tokens.installation - GithubAppConfiguration.with_credentials!(**credentials) do + record.with_installation!(snapshot: installation) do repository.workflow_run_discoveries.record!(run) end rescue GithubClient::NotFound - GithubAppConfiguration.with_credentials!(**credentials) do + record.with_installation!(snapshot: installation) do build.workflow_run.fail_missing! end end diff --git a/hub/app/models/github_app_configuration/repositories.rb b/hub/app/models/github_app_configuration/repositories.rb index 96356661..0e453557 100644 --- a/hub/app/models/github_app_configuration/repositories.rb +++ b/hub/app/models/github_app_configuration/repositories.rb @@ -10,10 +10,11 @@ def snapshot(token: record.tokens.installation) def synchronize!(snapshot: nil) return remove_all! if record.installation_state != "active" + installation = record.installation_snapshot attributes = snapshot || self.snapshot now = Time.current - record.with_installation! do + record.with_installation!(snapshot: installation) do active_ids = attributes.map { _1.fetch(:github_id) } attributes.each { synchronize_repository!(_1.merge(synchronized_at: now)) } Repository.active.where.not(github_id: active_ids).update_all \ diff --git a/hub/app/models/github_webhook.rb b/hub/app/models/github_webhook.rb index 098c2e25..573d3c5c 100644 --- a/hub/app/models/github_webhook.rb +++ b/hub/app/models/github_webhook.rb @@ -29,6 +29,7 @@ def supported_event?(event:, action:) def initialize(configuration:, body:, delivery_id:, event:, signature:) @configuration = configuration @credential_snapshot = configuration.credential_snapshot + @installation_snapshot = configuration.installation_snapshot @body = body @delivery_id = delivery_id @event = event @@ -50,12 +51,12 @@ def process! end delivery = find_delivery! if delivery.processed? - GithubAppConfiguration.with_credentials!(**credential_snapshot) do + with_delivery_configuration! do WebhookRedelivery.complete!(configuration:, guid: delivery_id) end return delivery.build end - claimed = GithubAppConfiguration.with_credentials!(**credential_snapshot) { delivery.start_processing! } + claimed = with_delivery_configuration! { delivery.start_processing! } return if !claimed if block_given? && !yield(delivery, installation.fetch("id").to_i) delivery.allow_retry! @@ -67,7 +68,7 @@ def process! when "workflow_run" then record_workflow_run!(payload) else record_installation!(payload) end - GithubAppConfiguration.with_credentials!(**credential_snapshot) do + with_delivery_configuration! do if delivery.finish_processing!(build:) WebhookRedelivery.complete!(configuration:, guid: delivery_id) end @@ -83,7 +84,16 @@ def process! private - attr_reader :body, :configuration, :credential_snapshot, :delivery_id, :event, :signature + attr_reader :body, :configuration, :credential_snapshot, :installation_snapshot, :delivery_id, :event, :signature + + # Installation notifications can change identity; pushes and workflow runs cannot. + def with_delivery_configuration!(&block) + if event.in?(%w[push workflow_run]) + configuration.with_installation!(snapshot: installation_snapshot, &block) + else + GithubAppConfiguration.with_credentials!(**credential_snapshot, &block) + end + end # Confirms the delivery body matches a signature made with a saved secret. # @see https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries @@ -121,7 +131,7 @@ def find_delivery! raise Rejected, "Invalid webhook delivery ID" if !valid_id digest = Digest::SHA256.hexdigest(body) - delivery = GithubAppConfiguration.with_credentials!(**credential_snapshot) do + delivery = with_delivery_configuration! do WebhookDelivery.find_or_create_by!(github_delivery_id: delivery_id) do |record| record.event = event record.payload_sha256 = digest @@ -169,7 +179,7 @@ def record_push!(payload) return if branch_name.nil? || !repository.build_branch?(branch_name) if payload.fetch("deleted") == true - repository.branch_presence.record_deletion!(branch_name:, configuration:) + repository.branch_presence.record_deletion!(branch_name:, configuration:, installation: installation_snapshot) return end return if payload.fetch("deleted") != false @@ -193,7 +203,7 @@ def record_push!(payload) expected_current_build_id = recovery ? head&.current_build_id : :any expected_head_observation_revision = head&.head_observation_revision || 0 branch_head = !recovery || current_branch_head?(repository, branch_name, commit_sha) - GithubAppConfiguration.with_credentials!(**credential_snapshot) do + configuration.with_installation!(snapshot: installation_snapshot) do repository.build_discoveries.record! \ branch_name:, commit_sha:, @@ -218,6 +228,7 @@ def record_retained_push!(repository, branch_name, commit_sha, previous_commit_s branch_name:, observed:, configuration:, + installation: installation_snapshot, default_branch: default_branch matches = Build.joins(:prototype).where( prototypes: { repository_id: repository.id, branch_name: }, commit_sha: @@ -237,7 +248,7 @@ def record_retained_push!(repository, branch_name, commit_sha, previous_commit_s previous_commit_sha.present? && observed.current_build&.commit_sha == previous_commit_sha return build if !accepted_head_change && (matches.many? || build.prototype.branch_deleted_at.nil?) - GithubAppConfiguration.with_credentials!(**credential_snapshot) do + configuration.with_installation!(snapshot: installation_snapshot) do repository.build_discoveries.record! \ branch_name:, commit_sha:, @@ -279,7 +290,7 @@ def record_workflow_run!(payload) repository = Repository.active.find_by(github_id: repository_payload.fetch("id").to_i) return if repository.nil? - GithubAppConfiguration.with_credentials!(**credential_snapshot) do + configuration.with_installation!(snapshot: installation_snapshot) do repository.workflow_run_discoveries.record! \ payload.fetch("workflow_run"), action: payload.fetch("action"), @@ -289,9 +300,11 @@ def record_workflow_run!(payload) # Confirms a push belongs to this Hub's current App installation. def verify_installation!(installation) - matches = configuration.installation_id.present? && - installation.fetch("id").to_i == configuration.installation_id - raise Rejected, "Webhook is for another GitHub App installation" if !matches + configuration.with_installation!(snapshot: installation_snapshot) do + installation_id = installation_snapshot.fetch(:installation_id) + matches = installation_id.present? && installation.fetch("id").to_i == installation_id + raise Rejected, "Webhook is for another GitHub App installation" if !matches + end end # Extracts only Git branch refs and leaves tags or malformed refs ineligible. diff --git a/hub/app/models/repository/branch_presence.rb b/hub/app/models/repository/branch_presence.rb index bddb112d..e49ba826 100644 --- a/hub/app/models/repository/branch_presence.rb +++ b/hub/app/models/repository/branch_presence.rb @@ -61,12 +61,12 @@ def current_head(branch_name, configuration:) end # Checks ancestry outside locks, then accepts it only against the captured owner. - def record_head!(branch_name:, commit_sha:, observed:, configuration:, default_branch: record.default_branch) - credentials = configuration.credential_snapshot + def record_head!(branch_name:, commit_sha:, observed:, configuration:, + installation: configuration.installation_snapshot, default_branch: record.default_branch) resume = if observed&.branch_deleted_at.present? observed.retired_head_sha.present? && ancestor?(observed.retired_head_sha, commit_sha, configuration:) end - GithubAppConfiguration.with_credentials!(**credentials) do + configuration.with_installation!(snapshot: installation) do record.build_discoveries.record! \ branch_name:, commit_sha:, @@ -98,23 +98,23 @@ def record_missing!(branch_name:, observed:) end # Confirms a deletion notification against GitHub, not its delivery order. - def record_deletion!(branch_name:, configuration:) + def record_deletion!(branch_name:, configuration:, installation: configuration.installation_snapshot) return if record.local? - credentials = configuration.credential_snapshot observed = observations[branch_name] return if observed.nil? || observed.branch_deleted_at.present? || !observed.reviewable? return if heads(configuration:, corroborate: [ branch_name ]).key?(branch_name) - GithubAppConfiguration.with_credentials!(**credentials) { record_missing!(branch_name:, observed:) } + configuration.with_installation!(snapshot: installation) { record_missing!(branch_name:, observed:) } end # Delayed events may refresh ownership, but only a confirmed current head revives it. - def refresh!(branch_name:, observed:, configuration:, default_branch: record.default_branch) + def refresh!(branch_name:, observed:, configuration:, installation: configuration.installation_snapshot, + default_branch: record.default_branch) sha = current_head(branch_name, configuration:) return if sha.nil? - record_head!(branch_name:, commit_sha: sha, observed:, configuration:, default_branch:) + record_head!(branch_name:, commit_sha: sha, observed:, configuration:, installation:, default_branch:) end private diff --git a/hub/docs/github-app-setup.md b/hub/docs/github-app-setup.md index 92210e25..cc52119f 100644 --- a/hub/docs/github-app-setup.md +++ b/hub/docs/github-app-setup.md @@ -86,6 +86,8 @@ The new owner installs the same App in their own account and selects its reposit Installation notifications and the existing five-minute Build checks restore repository access after GitHub confirms the replacement installation. This also recovers a transfer completed before upgrading Hub, or one whose notifications were missed. Run `replica:github:poll` to check immediately. Transient GitHub failures do not count as proof that an installation was removed; failed jobs remain visible in the queue and a later check retries. +Branch and workflow results remain bound to the owner and installation that started the poll or webhook processing. If a transfer replaces that installation before a result is saved, Hub rejects the old work even when the same repository is already available again. + The **GitHub App** administration page shows installation state and instructions when installation is needed. `replica:github:diagnose` reports an owner awaiting synchronization or an installation that is no longer present without changing the saved configuration. If diagnosis cannot reach GitHub, it reports failure instead. No console edits, credential replacement, or local reset are needed for an ordinary ownership transfer. ## Routine App administration diff --git a/hub/test/models/github_app_transfer_discovery_test.rb b/hub/test/models/github_app_transfer_discovery_test.rb new file mode 100644 index 00000000..2669941d --- /dev/null +++ b/hub/test/models/github_app_transfer_discovery_test.rb @@ -0,0 +1,250 @@ +require "test_helper" + +class GithubAppTransferDiscoveryTest < ActiveSupport::TestCase + test "an old branch poll cannot replace a head retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + head = repository.build_discoveries.record! \ + branch_name: "prototype/welcome", commit_sha: "c" * 40, branch_head: true + stub_poll(configuration, repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { per_page: 100, page: 1 }).to_return do + replace_installation!(configuration, repository) + { status: 200, body: JSON.generate([ { name: "prototype/welcome", commit: { sha: "b" * 40 } } ]) } + end + + allow_obsolete_work_to_stop { configuration.discover_builds!(repository) } + + assert_replacement_retained(configuration, repository) + assert_equal head, head.prototype.reload.current_build + assert_equal [ "c" * 40 ], head.prototype.builds.pluck(:commit_sha) + end + + test "an old branch absence cannot retire a branch retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + head = repository.build_discoveries.record! \ + branch_name: "prototype/welcome", commit_sha: "c" * 40, branch_head: true + stub_poll(configuration, repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { per_page: 100, page: 1 }) + .to_return(status: 200, body: "[]") + stub_absent_branch(repository) { replace_installation!(configuration, repository) } + + allow_obsolete_work_to_stop { configuration.discover_builds!(repository) } + + assert_replacement_retained(configuration, repository) + assert_nil head.prototype.reload.branch_deleted_at + assert_equal head, head.prototype.current_build + end + + test "an old workflow list cannot fail a build retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + build = repository.workflow_run_discoveries.record!(workflow_run(status: "in_progress")) + stub_poll(configuration, repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/actions/workflows/replica.yml/runs") + .with(query: { event: "push", per_page: 100, page: 1 }).to_return do + replace_installation!(configuration, repository) + { status: 200, body: JSON.generate(workflow_runs: [ workflow_run(status: "completed", conclusion: "failure") ]) } + end + + allow_obsolete_work_to_stop { configuration.discover_builds!(repository) } + + assert_replacement_retained(configuration, repository) + assert_equal "running", build.reload.state + assert_nil build.failure_summary + assert_nil build.workflow_completed_at + end + + test "an old exact workflow lookup cannot complete a build retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + build = repository.workflow_run_discoveries.record!(workflow_run(status: "in_progress")) + stub_poll(configuration, repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/actions/runs/9001").to_return do + replace_installation!(configuration, repository) + { status: 200, body: JSON.generate(workflow_run(status: "completed", conclusion: "success")) } + end + + allow_obsolete_work_to_stop { configuration.discover_builds!(repository) } + + assert_replacement_retained(configuration, repository) + assert_equal "running", build.reload.state + assert_nil build.workflow_completed_at + end + + test "an old missing workflow lookup cannot fail a build retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + build = repository.workflow_run_discoveries.record!(workflow_run(status: "in_progress")) + stub_poll(configuration, repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/actions/runs/9001").to_return do + replace_installation!(configuration, repository) + { status: 404, body: "{}" } + end + + allow_obsolete_work_to_stop { configuration.discover_builds!(repository) } + + assert_replacement_retained(configuration, repository) + assert_equal "running", build.reload.state + assert_nil build.failure_summary + end + + test "an admitted old push cannot replace the replacement installation's accepted head" do + configuration = github_app_configurations.create_installed + repository = repositories.create + head = nil + payload = push_payload(repository) + + allow_obsolete_work_to_stop do + deliver(configuration, payload, event: "push") do + replace_installation!(configuration, repository) + head = repository.build_discoveries.record! \ + branch_name: "prototype/welcome", commit_sha: "c" * 40, branch_head: true + true + end + end + + assert_replacement_retained(configuration, repository) + assert_equal head, head.prototype.reload.current_build + assert_equal [ "c" * 40 ], head.prototype.builds.pluck(:commit_sha) + end + + test "an admitted old workflow cannot fail the replacement installation's accepted run" do + configuration = github_app_configurations.create_installed + repository = repositories.create + build = nil + payload = { + installation: { id: 456 }, + repository: { id: repository.github_id, default_branch: "main" }, + action: "completed", + workflow_run: workflow_run(status: "completed", conclusion: "failure") + } + + allow_obsolete_work_to_stop do + deliver(configuration, payload, event: "workflow_run") do + replace_installation!(configuration, repository) + build = repository.workflow_run_discoveries.record!(workflow_run(status: "in_progress")) + true + end + end + + assert_replacement_retained(configuration, repository) + assert_equal "running", build.reload.state + assert_equal 9001, build.github_workflow_run_id + assert_nil build.failure_summary + end + + test "an old deletion lookup cannot retire a branch retained by the replacement installation" do + configuration = github_app_configurations.create_installed + repository = repositories.create + head = repository.build_discoveries.record! \ + branch_name: "prototype/welcome", commit_sha: "c" * 40, branch_head: true + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { per_page: 100, page: 1 }) + .to_return(status: 200, body: "[]") + stub_absent_branch(repository) { replace_installation!(configuration, repository) } + payload = push_payload(repository).merge(deleted: true, after: "0" * 40) + + allow_obsolete_work_to_stop { deliver(configuration, payload, event: "push") } + + assert_replacement_retained(configuration, repository) + assert_nil head.prototype.reload.branch_deleted_at + assert_equal head, head.prototype.current_build + end + + private + + def stub_poll(configuration, repository) + stub_github_app(configuration:) + stub_request(:get, "#{GITHUB_API_URL}/app/installations/456") + .to_return(status: 200, body: JSON.generate( + id: 456, app_id: configuration.github_app_id, + account: { id: 321, login: "github", type: "Organization" }, suspended_at: nil + )) + stub_github_installation_token + stub_github_repositories([ repository_payload(repository.github_id, repository.name_with_owner) ]) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { per_page: 100, page: 1 }) + .to_return(status: 200, body: JSON.generate( + [ { name: "prototype/welcome", commit: { sha: "c" * 40 } } ] + )) + [ repository.github_path, "new-team/prototype" ].each do |path| + stub_request(:get, "#{GITHUB_API_URL}/repos/#{path}/actions/workflows/replica.yml/runs") + .with(query: { event: "push", per_page: 100, page: 1 }) + .to_return(status: 200, body: JSON.generate(workflow_runs: [])) + end + end + + def stub_absent_branch(repository) + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/git/ref/heads/prototype/welcome") + .to_return(status: 404, body: "{}") + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}").to_return do + yield + { status: 200, body: JSON.generate(id: repository.github_id) } + end + end + + def replace_installation!(configuration, repository) + replacement = GithubAppConfiguration.find(configuration.id) + replacement.update! \ + owner_github_id: 654, owner_login: "new-team", installation_id: 789, + installation_account_github_id: 654 + replacement.repositories.remove_all! + replacement.repositories.synchronize! \ + snapshot: [ + { github_id: repository.github_id, name_with_owner: "new-team/prototype", default_branch: "main" } + ] + end + + def assert_replacement_retained(configuration, repository) + assert_equal configuration.credential_revision, configuration.reload.credential_revision + assert_equal 654, configuration.owner_github_id + assert_equal 789, configuration.installation_id + assert_equal 654, configuration.installation_account_github_id + assert_equal "active", configuration.installation_state + assert_equal "active", repository.reload.installation_state + assert_equal "new-team/prototype", repository.name_with_owner + end + + def allow_obsolete_work_to_stop + yield + rescue GithubAppConfiguration::Superseded, ActiveRecord::StaleObjectError, GithubWebhook::Rejected + nil + end + + def deliver(configuration, payload, event:, &admission) + body = JSON.generate(payload) + signature = "sha256=#{OpenSSL::HMAC.hexdigest("SHA256", configuration.webhook_secret, body)}" + GithubWebhook.process! \ + configuration:, body:, signature:, delivery_id: SecureRandom.uuid, event:, &admission + end + + def push_payload(repository) + { + installation: { id: 456 }, + repository: { id: repository.github_id, default_branch: "main" }, + ref: "refs/heads/prototype/welcome", + before: "a" * 40, + after: "b" * 40, + deleted: false + } + end + + def workflow_run(status:, conclusion: nil) + { + "id" => 9001, + "run_attempt" => 1, + "path" => ".github/workflows/replica.yml", + "event" => "push", + "head_branch" => "prototype/welcome", + "head_sha" => "c" * 40, + "status" => status, + "conclusion" => conclusion, + "html_url" => "https://github.com/github/prototype/actions/runs/9001", + "created_at" => "2026-09-17T18:00:00Z" + } + end +end