diff --git a/hub/app/javascript/pages/github-app-page/stories.tsx b/hub/app/javascript/pages/github-app-page/stories.tsx index fdd541f0..8f820dd7 100644 --- a/hub/app/javascript/pages/github-app-page/stories.tsx +++ b/hub/app/javascript/pages/github-app-page/stories.tsx @@ -41,6 +41,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..097596d0 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 @@ -125,13 +129,30 @@ 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? - return repositories.remove_all! if installation_id.nil? + ownership.refresh! + installation.synchronize! + end - installation.reconcile!(installation_id) + # 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 end # Polls GitHub for current prototype branch heads. @@ -159,5 +180,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/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/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..0e453557 100644 --- a/hub/app/models/github_app_configuration/repositories.rb +++ b/hub/app/models/github_app_configuration/repositories.rb @@ -10,11 +10,11 @@ def snapshot(token: record.tokens.installation) def synchronize!(snapshot: nil) return remove_all! if record.installation_state != "active" - credentials = record.credential_snapshot + installation = record.installation_snapshot attributes = snapshot || self.snapshot now = Time.current - GithubAppConfiguration.with_credentials!(**credentials) 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 \ @@ -25,7 +25,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..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 @@ -148,12 +158,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 @@ -171,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 @@ -195,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:, @@ -220,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: @@ -239,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:, @@ -281,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"), @@ -291,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/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 6323d0c1..6208d703 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 verification is required, Hub temporarily encrypts your submitted settings and continues your confirmed change after it succeeds. 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 5dffbef6..63689674 100644 --- a/hub/db/schema.rb +++ b/hub/db/schema.rb @@ -271,7 +271,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 f332278a..396888ea 100644 --- a/hub/docs/github-app-setup.md +++ b/hub/docs/github-app-setup.md @@ -76,6 +76,20 @@ 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. + +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 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, Hub continues the confirmed operation after it succeeds, rechecking permissions and the App's saved version. 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..4b16f8d8 --- /dev/null +++ b/hub/test/lib/github_app_installation_account_migration_test.rb @@ -0,0 +1,32 @@ +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 "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| + 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 } + + 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 +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..355f69ce --- /dev/null +++ b/hub/test/models/github_app_configuration/diagnostics_test.rb @@ -0,0 +1,31 @@ +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 + 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 "active", repository.reload.installation_state + 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 +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_recovery_test.rb b/hub/test/models/github_app_configuration/transfer_recovery_test.rb new file mode 100644 index 00000000..f6fb0ef8 --- /dev/null +++ b/hub/test/models/github_app_configuration/transfer_recovery_test.rb @@ -0,0 +1,237 @@ +require "test_helper" + +class GithubAppConfiguration::TransferRecoveryTest < ActiveSupport::TestCase + 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 + 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) + 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 "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:) + 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_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 + + 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 + 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 + 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 "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" } + 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_github_repositories([ repository_payload(repository.github_id, "renamed-team/prototype") ]) + + 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 "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_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") + + 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 "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_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 + + 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_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 diff --git a/hub/test/models/github_webhook_test.rb b/hub/test/models/github_webhook_test.rb index 8a4e7bf0..b6403f8d 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" @@ -27,20 +30,22 @@ 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") - reconciled = nil - installation = Object.new - installation.define_singleton_method(:reconcile!) { |id| reconciled = id } - configuration.define_singleton_method(:installation) { installation } + stub_github_app(configuration:) + 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 Repository.active.exists?(github_id: 2001) + assert WebhookDelivery.sole.processed? end test "rejects an invalid signature" do @@ -428,20 +433,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 +497,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:)