From db550204bebb9d3d78ff72a4a6cf70f8a4997da5 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Thu, 24 Sep 2026 16:47:27 -0400 Subject: [PATCH 1/5] Add direct repository access backend Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5b0e8ea9-7fd3-4e74-87d7-c3b7e69d8a7b --- README.md | 100 +++- lib/entitlements/backend/github_repository.rb | 32 ++ .../github_repository/configuration.rb | 99 ++++ .../backend/github_repository/controller.rb | 38 ++ .../models/repository_access.rb | 45 ++ .../backend/github_repository/provider.rb | 77 +++ .../backend/github_repository/service.rb | 148 ++++++ .../backend/github_repository_spec.rb | 487 ++++++++++++++++++ .../repositories/entitlements-app/read.txt | 2 + .../repositories/entitlements-app/triage.yaml | 2 + .../repositories/entitlements-app/write.rb | 13 + .../repositories/other.repo/admin.yaml | 3 + .../repositories/other.repo/maintain.txt | 1 + spec/unit/spec_helper.rb | 1 + 14 files changed, 1047 insertions(+), 1 deletion(-) create mode 100644 lib/entitlements/backend/github_repository.rb create mode 100644 lib/entitlements/backend/github_repository/configuration.rb create mode 100644 lib/entitlements/backend/github_repository/controller.rb create mode 100644 lib/entitlements/backend/github_repository/models/repository_access.rb create mode 100644 lib/entitlements/backend/github_repository/provider.rb create mode 100644 lib/entitlements/backend/github_repository/service.rb create mode 100644 spec/unit/entitlements/backend/github_repository_spec.rb create mode 100644 spec/unit/fixtures/repositories/entitlements-app/read.txt create mode 100644 spec/unit/fixtures/repositories/entitlements-app/triage.yaml create mode 100644 spec/unit/fixtures/repositories/entitlements-app/write.rb create mode 100644 spec/unit/fixtures/repositories/other.repo/admin.yaml create mode 100644 spec/unit/fixtures/repositories/other.repo/maintain.txt diff --git a/README.md b/README.md index dffde10..e49f437 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![acceptance](https://github.com/github/entitlements-github-plugin/actions/workflows/acceptance.yml/badge.svg)](https://github.com/github/entitlements-github-plugin/actions/workflows/acceptance.yml) [![test](https://github.com/github/entitlements-github-plugin/actions/workflows/test.yml/badge.svg)](https://github.com/github/entitlements-github-plugin/actions/workflows/test.yml) [![lint](https://github.com/github/entitlements-github-plugin/actions/workflows/lint.yml/badge.svg)](https://github.com/github/entitlements-github-plugin/actions/workflows/lint.yml) [![release](https://github.com/github/entitlements-github-plugin/actions/workflows/release.yml/badge.svg)](https://github.com/github/entitlements-github-plugin/actions/workflows/release.yml) [![build](https://github.com/github/entitlements-github-plugin/actions/workflows/build.yml/badge.svg)](https://github.com/github/entitlements-github-plugin/actions/workflows/build.yml) [![coverage](https://img.shields.io/badge/coverage-100%25-success)](https://img.shields.io/badge/coverage-100%25-success) [![style](https://img.shields.io/badge/code%20style-rubocop--github-blue)](https://github.com/github/rubocop-github) -`entitlements-github-plugin` is an [entitlements-app](https://github.com/github/entitlements-app) plugin allowing entitlements configs to be used to manage membership of GitHub.com Organizations and Teams. +`entitlements-github-plugin` is an [entitlements-app](https://github.com/github/entitlements-app) plugin allowing entitlements configs to manage GitHub organization and team membership, and direct repository access. ## Usage @@ -38,6 +38,7 @@ require "entitlements" # require entitlements plugins here require "entitlements/backend/github_org" require "entitlements/backend/github_team" +require "entitlements/backend/github_repository" require "entitlements/service/github" ``` @@ -85,6 +86,103 @@ Entitlements configs can contain metadata which the plugin will use to make furt `metadata_parent_team_name` - when defined in an entitlements config, the defined team will be made the parent team of this GitHub.com Team. +### GitHub repositories + +The `github_repository` backend manages **direct, user-only repository grants for active, non-owner organization members**. It does not create or delete repositories. Load `entitlements/backend/github_repository` in your plugin loader and add this entry under `groups`: + +```yaml +github.com/github/repositories: + type: github_repository + dir: repositories/github + base: ou=repositories,ou=github,ou=GitHub,dc=github,dc=com + org: github + token: <%= ENV.fetch("GITHUB_REPOSITORY_TOKEN") %> + addr: <%= ENV["GITHUB_API_BASE"] %> + allowed_types: [txt] + allowed_methods: [username, group] + features: [add, update, remove] + ignore: [] + ignore_not_found: false +``` + +`dir`, `base`, `org`, and `token` are required, nonempty strings. Relative directories resolve against Entitlements' `configuration_path`. Omit `addr` (or set it to null) for GitHub.com. For GitHub Enterprise Server, use `https://HOST/api/v3`; repository GraphQL requests use `https://HOST/api/graphql`. The server must support collaborator permission sources and source-specific role names. + +#### Repository and role files + +Each immediate subdirectory opts one repository into management: + +```text +repositories/github/ + entitlements-app/ + read.txt + write.txt + maintain.txt + another.repository/ + admin.txt +``` + +Role files use standard Entitlements syntax, not bare lists of logins. For example, `entitlements-app/write.txt`: + +```text +username = alice +username = bob; expiration = 2027-01-01 +group = engineering/platform +``` + +Group references, filters, and expiration are evaluated by the normal Entitlements rules engine. People must resolve through the configured people data source, with their `uid` equal to their GitHub login. As with other backends, the underlying username rule omits people absent from that data source; validate such references in your configuration CI. `ignore_not_found` applies to evaluated people missing active GitHub organization membership, not to missing repositories or API failures. + +YAML and Ruby role files are also supported when enabled in `allowed_types`, e.g. `[txt, yaml, rb]`. If omitted, all three formats are allowed. Ruby files use the standard Entitlements Ruby rule-class convention (repository directory and role filename determine the class); enable them only for trusted configuration authors. `allowed_methods` constrains declarative rules, not arbitrary Ruby code. + +| Role filename (without extension) | REST permission | +|----------------------------------|-----------------| +| `read` | `pull` | +| `triage` | `triage` | +| `write` | `push` | +| `maintain` | `maintain` | +| `admin` | `admin` | + +Custom roles are not supported. Unsupported direct roles or incomplete API responses abort reconciliation rather than falling back to effective permissions. A user cannot occur in multiple roles, even with different capitalization. Comparisons are case-insensitive; difference logs preserve login capitalization and show old and new roles. Duplicate role files, unsupported extensions, symlinks, nested directories, and unexpected files (including README and hidden files) are rejected. Keep documentation outside the managed root. + +**A missing role file means no desired direct members for that role.** An empty repository directory therefore requests removal of all managed direct grants if `remove` is enabled. To keep an explicitly empty role file, use the standard `metadata_no_conditions_ok = true` text directive. **Deleting the entire repository directory opts that repository out without cleanup**; existing access is untouched. The configured root must still exist. Git does not track empty directories, so keep an explicit empty role file when intending to remove every managed grant. + +#### Ownership boundary and feature flags + +Only a `Repository` permission source is used to determine a current direct role, even when a team or organization gives the person a higher effective permission. Team, organization, enterprise-team, and owner grants are not managed. Outside collaborators and organization owners are excluded entirely; desired owners and non-members fail validation by default. With `ignore_not_found: true`, they are skipped with a warning. This backend does not invite people into the organization or manage pending organization invitations. + +`ignore` is an array of logins removed from both sides of the diff, case-insensitively. Ignored users' grants are never mutated. Ignoring a user does not bypass schema/response validation when reading the repository. No fallback to effective permissions is performed. + +`features` defaults to `[add, update, remove]`. `add` permits new direct grants, `update` permits role changes, and `remove` permits deleting direct grants absent from desired state. Disabled operations are suppressed in both actions and displayed state. `features: []` performs reads and validation but produces no changes. To inspect the full proposed diff without applying it, use Entitlements' no-op mode with all features enabled. + +Role changes issue one `PUT`, never a remove followed by an add. All upserts for a repository precede removals. Removing a direct grant does not remove inherited access; it also has GitHub's documented side effects on forks and other resources. Review the [collaborator API documentation](https://docs.github.com/en/rest/collaborators/collaborators) before enabling removal. GitHub may reject a direct role below organization base permissions. + +#### GitHub App permissions and validation gate + +Install the App on every managed repository with: + +| Scope | Permission | Use | +|-------|------------|-----| +| Repository | **Administration: write** | Add, change, and remove collaborator grants | +| Repository | **Metadata: read** (automatically granted) | Repository visibility | +| Organization | **Members: read** | Active organization members and owners | + +These REST requirements are listed in [GitHub's App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps). Repository reads require access to `collaborators(affiliation: DIRECT)`, `permissionSources`, and each direct source's `roleName`; see the [GraphQL schema](https://docs.github.com/en/graphql/reference/repos). **Live installation-token access to these fields has not been validated by the unit suite.** Before deploying, verify it using the actual App installation and GitHub/GHES version. If those fields are unavailable, do not enable mutations or substitute effective REST permissions. + +In a designated disposable repository, give an active organization member a direct role and a different, higher inherited role. Verify the direct source reports the lower role, then add/change/remove the direct grant using the installation token. Expect `204` for organization-member adds, updates, and removals. `201` indicates an invitation rather than active access; the backend warns and does not cache it as a completed grant. Confirm inherited and outside access remain unchanged and a subsequent run has no diff. Do not run this check against production accounts or repositories. + +#### API usage, failure behavior, and rollout + +Repository reads use one GraphQL request per page of up to 100 direct collaborators, at least one per managed repository. Pagination follows `hasNextPage` and rejects missing/repeated cursors. Snapshots are cached in memory for the service's lifetime and invalidated after successful or partial applies. There is no persistent repository cache or `entitlements-caches` integration. + +Organization membership uses the shared per-run cache (paginated REST reads for `admin` and `member`, 100 users per page); predictive membership is refreshed before authorizing repository grants. Each added or changed grant requires one REST `PUT`, and each removed grant one `DELETE`, excluding retries. Octokit's existing middleware retries server errors on idempotent mutations; authorization, validation, and abuse/rate-limit responses abort without application-level retries. GraphQL uses the shared bounded retry transport. A partial failure stops application, leaves already-applied grants in place, and invalidates the snapshot. Re-run after resolving the failure; changes are not rolled back automatically. + +1. Complete the disposable-repository App validation above. +2. Start with a small set of repository directories and no-op mode; compare direct roles with repository settings. `features: []` is also safe for read/validation checks but suppresses the diff. +3. Enable only `add` and `update`, then confirm successive runs converge. +4. Review ignored accounts, inherited access, and outside collaborators before enabling `remove`. +5. Expand gradually while measuring GraphQL cost, REST rate usage, runtime, and failure rates. Consider persistent caching only if measurements justify it. + +Production convergence and API budgets require this live rollout; mocked tests do not establish them. + ## Release 🚀 To release a new version of this Gem, do the following: diff --git a/lib/entitlements/backend/github_repository.rb b/lib/entitlements/backend/github_repository.rb new file mode 100644 index 0000000..39a102a --- /dev/null +++ b/lib/entitlements/backend/github_repository.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require_relative "github_org" +require_relative "../service/github" + +module Entitlements + class Backend + class GitHubRepository + ROLES = { + "read" => "pull", + "triage" => "triage", + "write" => "push", + "maintain" => "maintain", + "admin" => "admin" + }.freeze + FEATURES = %w[add update remove].freeze + + class Error < RuntimeError; end + + def self.fail!(message) + Entitlements.logger.error(message) + raise Error, message + end + end + end +end + +require_relative "github_repository/models/repository_access" +require_relative "github_repository/configuration" +require_relative "github_repository/service" +require_relative "github_repository/provider" +require_relative "github_repository/controller" diff --git a/lib/entitlements/backend/github_repository/configuration.rb b/lib/entitlements/backend/github_repository/configuration.rb new file mode 100644 index 0000000..dae4f8d --- /dev/null +++ b/lib/entitlements/backend/github_repository/configuration.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + class Configuration + # Underscores are used by Enterprise Managed User logins. + LOGIN = /\A[a-zA-Z0-9][a-zA-Z0-9_-]*\z/ + REPOSITORY = /\A[a-zA-Z0-9_.-]{1,100}\z/ + + def self.validate!(key, data) + spec = Entitlements::Backend::BaseController::COMMON_GROUP_CONFIG.merge( + "dir" => { required: true, type: String }, + "base" => { required: true, type: String }, + "org" => { required: true, type: String }, + "token" => { required: true, type: String }, + "addr" => { required: false, type: [String, NilClass] }, + "features" => { required: false, type: Array }, + "ignore" => { required: false, type: Array }, + "ignore_not_found" => { required: false, type: [TrueClass, FalseClass] } + ) + Entitlements::Util::Util.validate_attr!(spec, data, "GitHub repository backend #{key}") + %w[dir base org token].each do |name| + GitHubRepository.fail!("#{key}: #{name} must not be empty") if data.fetch(name).strip.empty? + end + validate_login!(data.fetch("org")) + { "features" => FEATURES, "allowed_types" => %w[txt yaml rb], + "allowed_methods" => Entitlements::Data::Groups::Calculated.rules_index.keys }.each do |name, allowed| + invalid = data.fetch(name, []) - allowed + GitHubRepository.fail!("#{key}: invalid #{name}: #{invalid.inspect}") unless invalid.empty? + end + data.fetch("ignore", []).each { |login| validate_login!(login) } + return if data["addr"].nil? + + uri = URI.parse(data.fetch("addr")) + unless %w[http https].include?(uri.scheme) && uri.host && !uri.userinfo && !uri.query && !uri.fragment + GitHubRepository.fail!("#{key}: addr must be an HTTP(S) API base URL without credentials, query or fragment") + end + rescue URI::InvalidURIError => e + GitHubRepository.fail!("#{key}: invalid addr: #{e.message}") + end + + def self.validate_login!(login) + GitHubRepository.fail!("Invalid GitHub login: #{login.inspect}") unless login.is_a?(String) && LOGIN.match?(login) + end + + def self.validate_repository!(repository) + unless repository.is_a?(String) && REPOSITORY.match?(repository) && !%w[. ..].include?(repository) + GitHubRepository.fail!("Invalid GitHub repository name: #{repository.inspect}") + end + end + + def initialize(config) + @config = config + end + + def load + root = File.expand_path(@config.fetch("dir"), Entitlements.config_path) + seen = Set.new + Dir.children(root).sort.map do |repository| + self.class.validate_repository!(repository) + path = File.join(root, repository) + unless File.directory?(path) && !File.symlink?(path) && seen.add?(repository.downcase) + GitHubRepository.fail!("Unexpected or duplicate repository directory: #{path}") + end + load_repository(repository, path) + end + end + + private + + def load_repository(repository, path) + roles = {} + seen_roles = Set.new + seen_users = Set.new + Dir.children(path).sort.each do |entry| + filename = File.join(path, entry) + role = File.basename(entry, File.extname(entry)) + extension = File.extname(entry).delete_prefix(".") + unless File.file?(filename) && !File.symlink?(filename) && ROLES.key?(role) && + @config.fetch("allowed_types", %w[txt yaml rb]).include?(extension) && seen_roles.add?(role) + GitHubRepository.fail!("Unexpected or duplicate repository role file: #{filename}") + end + ruleset = Entitlements::Data::Groups::Calculated.ruleset(filename: filename, config: @config) + ruleset.modified_filtered_members.each do |person| + login = person.uid + self.class.validate_login!(login) + unless seen_users.add?(login.downcase) + GitHubRepository.fail!("#{repository}: duplicate user across roles: #{login}") + end + roles[login] = role + end + end + Models::RepositoryAccess.new(repository: repository, roles: roles, ou: @config.fetch("base")) + end + end + end + end +end diff --git a/lib/entitlements/backend/github_repository/controller.rb b/lib/entitlements/backend/github_repository/controller.rb new file mode 100644 index 0000000..f4877bd --- /dev/null +++ b/lib/entitlements/backend/github_repository/controller.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + class Controller < Entitlements::Backend::BaseController + def self.priority + 50 + end + + register + + def initialize(group_name, config = nil) + super + @provider = Provider.new(config: @config) + end + + def validate_config!(key, data) + Configuration.validate!(key, data) + end + + def validate + @repositories = Configuration.new(config).load + end + + def calculate + # Evaluate every local file before making the first GitHub request. + validate + @actions = @repositories.filter_map { |repository| @provider.action_for(repository, group_name) } + end + + def apply(action) + @provider.commit(action) + end + end + end + end +end diff --git a/lib/entitlements/backend/github_repository/models/repository_access.rb b/lib/entitlements/backend/github_repository/models/repository_access.rb new file mode 100644 index 0000000..df5530b --- /dev/null +++ b/lib/entitlements/backend/github_repository/models/repository_access.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + module Models + class RepositoryAccess < Entitlements::Models::Group + attr_reader :repository, :roles + + def initialize(repository:, roles:, ou:) + Configuration.validate_repository!(repository) + @repository = repository + @roles = {} + @logins = {} + roles.sort_by { |login, _| login.downcase }.each do |login, role| + Configuration.validate_login!(login) + GitHubRepository.fail!("Unsupported repository role: #{role.inspect}") unless ROLES.key?(role) + key = login.downcase + GitHubRepository.fail!("Duplicate repository user: #{login}") if @roles.key?(key) + @roles[key] = role + @logins[key] = login + end + @roles.freeze + @logins.freeze + super(dn: "cn=#{repository},#{ou}", members: Set.new(@logins.values)) + end + + def role_for(login) + roles[login.downcase] + end + + def login_for(login) + @logins.fetch(login.downcase) + end + + def equals?(other) + other.is_a?(self.class) && dn.casecmp?(other.dn) && roles == other.roles + end + + alias_method :==, :equals? + end + end + end + end +end diff --git a/lib/entitlements/backend/github_repository/provider.rb b/lib/entitlements/backend/github_repository/provider.rb new file mode 100644 index 0000000..bf27297 --- /dev/null +++ b/lib/entitlements/backend/github_repository/provider.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + class Provider < Entitlements::Backend::BaseProvider + def initialize(config:) + @config = config + @github = Service.new(org: config.fetch("org"), token: config.fetch("token"), + ou: config.fetch("base"), addr: config["addr"]) + end + + def action_for(desired, group_name) + ignored = Set.new(@config.fetch("ignore", []).map(&:downcase)) + validate_members(desired, ignored) + existing = @github.read_repository(desired.repository) + current = existing.roles.reject { |login, _| ignored.include?(login) } + target = desired.roles.reject { |login, _| ignored.include?(login) } + effective = current.dup + instructions = [] + (current.keys | target.keys).sort.each do |login| + before = current[login] + after = target[login] + next if before == after + feature = if before.nil? + "add" + elsif after.nil? + "remove" + else + "update" + end + next unless @config.fetch("features", FEATURES).include?(feature) + if after + effective[login] = after + instructions << { action: :upsert, login: desired.login_for(login), permission: ROLES.fetch(after) } + else + effective.delete(login) + instructions << { action: :remove, login: existing.login_for(login) } + end + name = after ? desired.login_for(login) : existing.login_for(login) + Entitlements.logger.info "CHANGE #{desired.repository}: #{name} #{before || '(none)'} -> #{after || '(none)'}" + end + return if instructions.empty? + action = Entitlements::Models::Action.new(desired.dn, + snapshot(existing, current), snapshot(desired, effective), group_name, ignored_users: ignored) + instructions.sort_by { |instruction| [instruction[:action] == :upsert ? 0 : 1, instruction[:login].downcase] }.each do |instruction| + action.add_implementation(instruction) + end + action + end + + def commit(action) + unless action.existing.is_a?(Models::RepositoryAccess) && action.updated.is_a?(Models::RepositoryAccess) && + action.existing.dn == action.updated.dn && action.implementation.is_a?(Array) + GitHubRepository.fail!("Invalid repository action") + end + @github.apply(action.updated.repository, action.implementation) + end + + private + + def snapshot(source, roles) + Models::RepositoryAccess.new(repository: source.repository, roles: roles, ou: @config.fetch("base")) + end + + def validate_members(desired, ignored) + invalid = desired.roles.keys - @github.active_members.keys - ignored.to_a + return if invalid.empty? + message = "#{desired.repository}: not active non-owner organization members: #{invalid.join(', ')}" + GitHubRepository.fail!(message) unless @config.fetch("ignore_not_found", false) + Entitlements.logger.warn("#{message}; ignored") + ignored.merge(invalid) + end + end + end + end +end diff --git a/lib/entitlements/backend/github_repository/service.rb b/lib/entitlements/backend/github_repository/service.rb new file mode 100644 index 0000000..9b9136b --- /dev/null +++ b/lib/entitlements/backend/github_repository/service.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + class Service < Entitlements::Service::GitHub + def active_members + # Never use predictive membership to authorize a new direct grant. + @active_members ||= begin + invalidate_org_members_predictive_cache + org_members.transform_keys(&:downcase).select { |_, role| role == "member" } + end + end + + def read_repository(repository) + Configuration.validate_repository!(repository) + @repositories ||= {} + @repositories[repository.downcase] ||= begin + roles = {} + cursor = nil + cursors = Set.new + loop do + connection = collaborators(repository, cursor) + connection.fetch("edges").each { |edge| read_edge(edge, roles) } + page = connection.fetch("pageInfo") + more = page.fetch("hasNextPage") + GitHubRepository.fail!("Malformed repository pagination") unless [true, false].include?(more) + break unless more + cursor = page.fetch("endCursor") + unless cursor.is_a?(String) && !cursor.empty? && cursors.add?(cursor) + GitHubRepository.fail!("Missing or repeated repository pagination cursor") + end + end + Models::RepositoryAccess.new(repository: repository, roles: roles, ou: ou) + end + rescue KeyError, TypeError => e + GitHubRepository.fail!("Malformed repository response for #{repository}: #{e.message}") + end + + def apply(repository, instructions) + Configuration.validate_repository!(repository) + instructions.sort_by { |instruction| [instruction.fetch(:action) == :upsert ? 0 : 1, instruction.fetch(:login).downcase] }.each do |instruction| + login = instruction.fetch(:login) + Configuration.validate_login!(login) + unless active_members.key?(login.downcase) + GitHubRepository.fail!("#{repository}: #{login} is not an active non-owner organization member") + end + mutate(repository, instruction) + end + ensure + # A partial apply must not leave a successful-looking cached snapshot. + @repositories&.delete(repository.downcase) + end + + private + + def collaborators(repository, cursor) + query = <<~GRAPHQL + { + repository(owner: #{JSON.generate(org)}, name: #{JSON.generate(repository)}) { + collaborators(affiliation: DIRECT, first: 100, after: #{JSON.generate(cursor)}) { + edges { + node { login } + permission + permissionSources { roleName source { __typename } } + } + pageInfo { hasNextPage endCursor } + } + } + } + GRAPHQL + response = graphql_http_post(query) + unless response[:code] == 200 && response[:data].is_a?(Hash) && !response[:data].key?("errors") + GitHubRepository.fail!("Repository GraphQL query failed for #{org}/#{repository}: #{response.inspect}") + end + data = response[:data].fetch("data") + repo = data.is_a?(Hash) && data["repository"] + connection = repo.is_a?(Hash) && repo["collaborators"] + unless connection.is_a?(Hash) && connection["edges"].is_a?(Array) && connection["pageInfo"].is_a?(Hash) + GitHubRepository.fail!("Missing or malformed collaborator data for #{org}/#{repository}") + end + connection + end + + def read_edge(edge, roles) + unless edge.is_a?(Hash) && edge["node"].is_a?(Hash) && edge["permissionSources"].is_a?(Array) + GitHubRepository.fail!("Missing or malformed repository permission sources") + end + login = edge.fetch("node").fetch("login") + Configuration.validate_login!(login) + return unless active_members.key?(login.downcase) + direct = edge.fetch("permissionSources").select do |source| + unless source.is_a?(Hash) && source["source"].is_a?(Hash) + GitHubRepository.fail!("Malformed repository permission source") + end + type = source.fetch("source").fetch("__typename") + unless %w[Repository Team Organization EnterpriseTeam].include?(type) + GitHubRepository.fail!("Unknown repository permission source: #{type.inspect}") + end + type == "Repository" + end + return if direct.empty? + GitHubRepository.fail!("Ambiguous direct repository permissions for #{login}") unless direct.size == 1 + role = direct.first.fetch("roleName") + GitHubRepository.fail!("Unsupported direct repository role for #{login}: #{role.inspect}") unless role.is_a?(String) && ROLES.key?(role.downcase) + GitHubRepository.fail!("Duplicate repository collaborator: #{login}") if roles.keys.any? { |key| key.casecmp?(login) } + roles[login] = role.downcase + end + + def mutate(repository, instruction) + path = "repos/#{org}/#{repository}/collaborators/#{instruction.fetch(:login)}" + action = instruction.fetch(:action) + case action + when :upsert + permission = instruction.fetch(:permission) + GitHubRepository.fail!("Unsupported REST repository permission: #{permission.inspect}") unless ROLES.value?(permission) + when :remove + permission = nil + else + GitHubRepository.fail!("Unknown repository instruction: #{action.inspect}") + end + # Octokit's middleware already retries server errors on idempotent requests. + result = if action == :upsert + octokit.put(path, permission: permission) + else + octokit.delete(path) + end + status = octokit.last_response.status + unless (action == :upsert ? [201, 204] : [204]).include?(status) + GitHubRepository.fail!("Unexpected repository mutation response: HTTP #{status}") + end + if status == 201 + unless result.is_a?(Sawyer::Resource) && result[:id].is_a?(Integer) && result[:id] > 0 + GitHubRepository.fail!("Malformed repository invitation response") + end + Entitlements.logger.warn("#{repository}: invitation created for #{instruction.fetch(:login)}; access is not yet active") + end + rescue Octokit::Error => e + GitHubRepository.fail!("#{action} #{org}/#{repository}/#{instruction.fetch(:login)} failed: #{e.message}") + end + + def graphql_uri + @graphql_uri ||= URI.parse(octokit.api_endpoint.sub(%r{/api/v3/?\z}, "/api/").sub(%r{/?\z}, "/") + "graphql") + end + end + end + end +end diff --git a/spec/unit/entitlements/backend/github_repository_spec.rb b/spec/unit/entitlements/backend/github_repository_spec.rb new file mode 100644 index 0000000..34b97f8 --- /dev/null +++ b/spec/unit/entitlements/backend/github_repository_spec.rb @@ -0,0 +1,487 @@ +# frozen_string_literal: true + +require_relative "../../spec_helper" +require "tmpdir" +require "fileutils" + +describe Entitlements::Backend::GitHubRepository do + let(:backend) { described_class } + let(:base) { "ou=repositories,dc=example,dc=com" } + let(:config) do + { "dir" => fixture("repositories"), "base" => base, "org" => "example", "token" => "test-token" } + end + let(:service) { backend::Service.new(org: "example", token: "test-token", ou: base) } + let(:members) { { "alice" => "member", "bob" => "member", "carol" => "member", "owner" => "admin" } } + + def access(roles = {}, repository: "app", **inline_roles) + backend::Models::RepositoryAccess.new(repository: repository, roles: roles.merge(inline_roles), ou: base) + end + + def edge(login, role = "write", sources: nil) + { "node" => { "login" => login }, "permission" => "ADMIN", + "permissionSources" => sources || [{ "roleName" => role, "source" => { "__typename" => "Repository" } }] } + end + + def page(edges, more: false, cursor: nil) + { "data" => { "repository" => { "collaborators" => { + "edges" => edges, "pageInfo" => { "hasNextPage" => more, "endCursor" => cursor } + } } } } + end + + def stub_page(body, endpoint: "https://api.github.com/graphql") + stub_request(:post, endpoint).to_return(status: 200, body: JSON.generate(body)) + end + + describe "configuration validation" do + it "registers and loads a minimal backend without requesting GitHub" do + expect(backend::Controller.identifier).to eq("github_repository") + expect(backend::Controller.priority).to eq(50) + expect(backend::Controller.new("repos", config).actions).to eq([]) + end + + %w[dir base org token].each do |key| + it "requires a nonempty #{key}" do + expect { backend::Controller.new("repos", config.reject { |k, _| k == key }) }.to raise_error(RuntimeError, /missing attribute/) + expect { backend::Controller.new("repos", config.merge(key => " ")) }.to raise_error(backend::Error, /must not be empty/) + end + end + + [ + ["features", ["invite"]], ["features", nil], ["ignore", "alice"], ["ignore", [7]], + ["ignore", ["../alice"]], ["org", "bad/org"], ["allowed_types", ["json"]], + ["allowed_methods", ["unknown"]], ["ignore_not_found", "yes"], ["token", 1], + ["addr", "ftp://github.test"], ["addr", "https://user:pass@github.test"], + ["addr", "https://github.test?query=yes"], ["addr", "not a url"] + ].each do |key, value| + it "rejects #{key}=#{value.inspect}" do + expect { backend::Controller.new("repos", config.merge(key => value)) }.to raise_error(RuntimeError) + end + end + + it "accepts nil and valid enterprise addresses, flags, and managed-user logins" do + [nil, "https://github.test/api/v3/"].each do |addr| + expect { backend::Controller.new("repos", config.merge("addr" => addr, "features" => [], + "ignore" => ["alice_enterprise"], "allowed_methods" => %w[username group])) + }.not_to raise_error + end + end + end + + describe "repository model" do + it "compares roles and names case insensitively, preserving display logins and sorted keys" do + model = access("Bob" => "maintain", "ALIce" => "read") + expect(model.roles.keys).to eq(%w[alice bob]) + expect(model.role_for("ALICE")).to eq("read") + expect(model.login_for("alice")).to eq("ALIce") + expect(model.member?("bOB")).to be(true) + expect(model.member_strings).to eq(Set.new(%w[Bob ALIce])) + expect(model).to eq(access({ "alice" => "read", "bob" => "maintain" }, repository: "APP")) + expect(model.equals?(access("alice" => "write", "bob" => "maintain"))).to be(false) + expect(model.equals?(access({ "alice" => "read", "bob" => "maintain" }, repository: "other"))).to be(false) + expect(model.equals?(:none)).to be(false) + end + + ["", ".", "..", "bad/repo", "bad repo", "a" * 101, nil].each do |name| + it "rejects repository name #{name.inspect}" do + expect { access({}, repository: name) }.to raise_error(backend::Error, /repository name/) + end + end + + it "rejects custom roles, invalid logins, and duplicate case variants" do + expect { access("alice" => "custom") }.to raise_error(backend::Error, /Unsupported/) + expect { access("../alice" => "write") }.to raise_error(backend::Error, /login/) + expect { access("alice" => "read", "ALICE" => "write") }.to raise_error(backend::Error, /Duplicate/) + end + end + + describe "recursive loader" do + before do + cache[:people_obj] = Entitlements::Data::People::YAML.new(filename: fixture("people.yaml")) + cache[:file_objects] = {} + end + + it "evaluates text, YAML, Ruby, group references and expiration deterministically" do + result = backend::Configuration.new(config).load + expect(result.map(&:repository)).to eq(["entitlements-app", "other.repo"]) + expect(result.first.roles).to eq("balinese" => "read", "chartreux" => "triage", "dwelf" => "write") + expect(result.last.roles.values.uniq).to eq(["maintain"]) + expect(result.last.roles).not_to have_key("bengal") + end + + it "applies standard filters" do + filter = Class.new do + def initialize(**); end + + def filtered?(person) + person.uid.downcase == "balinese" + end + end + Entitlements::Data::Groups::Calculated.register_filter("exclude", { class: filter, config: {} }) + expect(backend::Configuration.new(config).load.first.roles).not_to have_key("balinese") + end + + it "resolves a relative dir against the configuration root" do + expect(backend::Configuration.new(config.merge("dir" => "../repositories")).load.size).to eq(2) + end + + it "rejects a disallowed extension" do + expect { backend::Configuration.new(config.merge("allowed_types" => ["txt"])).load }.to raise_error(backend::Error, /role file/) + end + + it "honors allowed rule methods" do + expect { backend::Configuration.new(config.merge("allowed_methods" => ["group"])).load }.to raise_error(RuntimeError, /not a valid function/) + end + + it "fails when the configured root is absent" do + expect { backend::Configuration.new(config.merge("dir" => fixture("missing-repositories"))).load }.to raise_error(Errno::ENOENT) + end + + it "treats an empty repository directory as empty desired access and a deleted directory as unmanaged" do + Dir.mktmpdir do |root| + Dir.mkdir("#{root}/app") + loader = backend::Configuration.new(config.merge("dir" => root)) + expect(loader.load.first.roles).to eq({}) + Dir.rmdir("#{root}/app") + expect(loader.load).to eq([]) + end + end + + ["README.md", "custom.txt", "write.json", "write", ".hidden", "write.txt/nested.txt"].each do |entry| + it "rejects unexpected role entry #{entry}" do + Dir.mktmpdir do |root| + FileUtils.mkdir_p(File.dirname("#{root}/app/#{entry}")) + File.write("#{root}/app/#{entry}", "username = balinese\n") + expect { backend::Configuration.new(config.merge("dir" => root)).load }.to raise_error(backend::Error, /role file/) + end + end + end + + it "rejects root files, symlinks, duplicate role files and duplicate users" do + Dir.mktmpdir do |root| + loader = backend::Configuration.new(config.merge("dir" => root)) + File.write("#{root}/README", "") + expect { loader.load }.to raise_error(backend::Error, /directory/) + File.unlink("#{root}/README") + File.symlink(config.fetch("dir"), "#{root}/app") + expect { loader.load }.to raise_error(backend::Error, /directory/) + File.unlink("#{root}/app") + Dir.mkdir("#{root}/app") + File.symlink("#{config.fetch('dir')}/entitlements-app/read.txt", "#{root}/app/read.txt") + expect { loader.load }.to raise_error(backend::Error, /role file/) + File.unlink("#{root}/app/read.txt") + File.write("#{root}/app/read.txt", "username = balinese\n") + File.write("#{root}/app/read.yaml", "rules:\n username: bengal\n") + expect { loader.load }.to raise_error(backend::Error, /role file/) + File.rename("#{root}/app/read.yaml", "#{root}/app/write.yaml") + File.write("#{root}/app/write.yaml", "rules:\n username: BALINESE\n") + expect { loader.load }.to raise_error(backend::Error, /duplicate user/) + end + end + end + + describe "diff and controller" do + let(:provider) { backend::Provider.new(config: config) } + before do + allow(backend::Service).to receive(:new).and_return(service) + allow(service).to receive(:active_members).and_return(members.reject { |_, role| role == "admin" }) + end + + described_class::FEATURES.length.succ.times.flat_map { |size| described_class::FEATURES.combination(size).to_a }.each do |features| + it "honors feature combination #{features.inspect} in instructions and displayed state" do + config["features"] = features + allow(service).to receive(:read_repository).with("app").and_return(access("alice" => "read", "bob" => "write")) + action = provider.action_for(access("ALICE" => "admin", "carol" => "triage"), "repos") + if features.empty? + expect(action).to be_nil + else + expected = [] + expected << { action: :upsert, login: "ALICE", permission: "admin" } if features.include?("update") + expected << { action: :upsert, login: "carol", permission: "triage" } if features.include?("add") + expected << { action: :remove, login: "bob" } if features.include?("remove") + expect(action.implementation).to eq(expected) + effective = { "alice" => features.include?("update") ? "admin" : "read" } + effective["carol"] = "triage" if features.include?("add") + effective["bob"] = "write" unless features.include?("remove") + expect(action.updated.roles).to eq(effective) + expect(action.existing.equals?(action.updated)).to be(false) + end + end + end + + it "ignores configured users on both sides and handles case-only changes as no-op" do + config["ignore"] = ["OWNER", "Bob"] + allow(service).to receive(:read_repository).and_return(access("alice" => "read", "bob" => "admin")) + expect(provider.action_for(access("ALICE" => "read", "owner" => "write"), "repos")).to be_nil + end + + it "rejects desired non-members and owners before reading a repository" do + expect(service).not_to receive(:read_repository) + expect { provider.action_for(access("outsider" => "read"), "repos") }.to raise_error(backend::Error, /not active/) + expect { provider.action_for(access("owner" => "read"), "repos") }.to raise_error(backend::Error, /not active/) + end + + it "warns and ignores non-members when explicitly configured" do + config["ignore_not_found"] = true + expect(logger).to receive(:warn).with(/outsider.*ignored/) + allow(service).to receive(:read_repository).and_return(access) + expect(provider.action_for(access("outsider" => "read"), "repos")).to be_nil + end + + it "validates all files before API requests, calculates and applies one action per repository" do + desired = access("alice" => "maintain") + loader = instance_double(backend::Configuration, load: [desired]) + allow(backend::Configuration).to receive(:new).and_return(loader) + allow(service).to receive(:read_repository).and_return(access("alice" => "write")) + controller = backend::Controller.new("repos", config) + actions = controller.calculate + expect(actions.size).to eq(1) + expect(controller.change_count).to eq(1) + expect(service).to receive(:apply).with("app", [{ action: :upsert, login: "alice", permission: "maintain" }]) + controller.apply(actions.first) + allow(loader).to receive(:load).and_raise(backend::Error, "invalid file") + expect(service).not_to receive(:read_repository) + expect { controller.calculate }.to raise_error(backend::Error, /invalid file/) + end + + it "does not calculate destructive cleanup for removed repository directories" do + Dir.mktmpdir do |root| + expect(service).not_to receive(:read_repository) + expect(backend::Controller.new("repos", config.merge("dir" => root)).calculate).to eq([]) + end + end + + it "rejects invalid actions" do + action = Entitlements::Models::Action.new("app", access, nil, "repos") + expect { provider.commit(action) }.to raise_error(backend::Error, /Invalid repository action/) + end + end + + describe "end-to-end reconciliation" do + it "converges on a second calculation, preserving inherited and outside access" do + cache[:people_obj] = Entitlements::Data::People::YAML.new(filename: fixture("people.yaml")) + cache[:file_objects] = {} + stub_request(:get, "https://api.github.com/orgs/example/members") + .with(query: { role: "admin", per_page: 100 }) + .to_return(status: 200, body: '[{"login":"owner"}]', headers: { "Content-Type" => "application/json" }) + members_request = stub_request(:get, "https://api.github.com/orgs/example/members") + .with(query: { role: "member", per_page: 100 }) + .to_return(status: 200, body: '[{"login":"balinese"},{"login":"bob"},{"login":"carol"}]', + headers: { "Content-Type" => "application/json" }) + inherited = edge("carol", sources: [{ "roleName" => "admin", "source" => { "__typename" => "Team" } }]) + preserved = [edge("outsider"), edge("owner"), inherited] + stub_page(page([edge("balinese", "read"), edge("bob")] + preserved)) + put = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/balinese") + .with(body: { permission: "push" }).to_return(status: 204) + delete = stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/bob").to_return(status: 204) + Dir.mktmpdir do |root| + Dir.mkdir("#{root}/app") + File.write("#{root}/app/write.txt", "username = balinese\n") + controller = backend::Controller.new("repos", config.merge("dir" => root)) + actions = controller.calculate + expect(actions.size).to eq(1) + expect(actions.first.implementation.size).to eq(2) + controller.apply(actions.first) + stub_page(page([edge("balinese", "write")] + preserved)) + expect(controller.calculate).to eq([]) + end + expect(put).to have_been_requested.once + expect(delete).to have_been_requested.once + expect(members_request).to have_been_requested.once + %w[owner outsider carol].each do |login| + expect(a_request(:delete, "https://api.github.com/repos/example/app/collaborators/#{login}")).not_to have_been_made + end + end + end + + describe "GitHub transport" do + before do + allow(service).to receive(:org_members).and_return(members) + allow(service).to receive(:org_members_from_predictive_cache?).and_return(false) + end + + it "uses the organization membership cache and excludes owners" do + expect(service).to receive(:invalidate_org_members_predictive_cache) + expect(service.active_members).to eq(members.reject { |_, role| role == "admin" }) + end + + it "paginates, uses direct roles instead of effective permissions, and caches per repository" do + first = page([edge("Alice", "Read"), edge("outsider"), edge("owner")], more: true, cursor: 'a"b') + inherited = %w[Team Organization EnterpriseTeam].map { |type| { "roleName" => "admin", "source" => { "__typename" => type } } } + second = page([edge("Bob", "triage", sources: inherited), edge("Carol", "maintain")]) + request = stub_request(:post, "https://api.github.com/graphql") + .with(headers: { "Authorization" => "bearer test-token" }) + .to_return({ status: 200, body: JSON.generate(first) }, { status: 200, body: JSON.generate(second) }) + expect(service.read_repository("app").roles).to eq("alice" => "read", "carol" => "maintain") + expect(service.read_repository("APP").roles).to eq("alice" => "read", "carol" => "maintain") + expect(request).to have_been_requested.twice + expect(a_request(:post, "https://api.github.com/graphql").with { |req| + JSON.parse(req.body).fetch("query").include?('after: "a\\"b"') + }).to have_been_made.once + end + + it "selects the direct role even alongside a higher inherited grant" do + sources = [{ "roleName" => "admin", "source" => { "__typename" => "Team" } }, + { "roleName" => "triage", "source" => { "__typename" => "Repository" } }] + stub_page(page([edge("alice", sources: sources)])) + expect(service.read_repository("app").roles).to eq("alice" => "triage") + end + + described_class::ROLES.each_key do |role| + it "reads canonical role #{role}" do + stub_page(page([edge("alice", role)])) + expect(service.read_repository("app").role_for("ALICE")).to eq(role) + end + end + + [ + {}, { "data" => nil }, { "data" => { "repository" => nil } }, + { "data" => { "repository" => { "collaborators" => {} } } }, + { "errors" => [{ "message" => "denied" }] }, + { "data" => { "repository" => { "collaborators" => { "edges" => [], "pageInfo" => {} } } } } + ].each do |body| + it "fails closed on missing or partial GraphQL data #{body.inspect}" do + stub_page(body) + expect { service.read_repository("app") }.to raise_error(backend::Error) + end + end + + it "rejects missing sources, unsupported roles, malformed sources and duplicate direct grants" do + [ + edge("alice").merge("permissionSources" => nil), + edge("alice", nil), edge("alice", "custom"), + edge("alice", sources: [nil]), + edge("alice", sources: [{ "source" => {} }]), + edge("alice", sources: [{ "source" => { "__typename" => nil } }]), + edge("alice", sources: [edge("alice")["permissionSources"].first] * 2), + edge("../alice") + ].each do |invalid| + stub_page(page([invalid])) + expect { service.read_repository("app") }.to raise_error(backend::Error) + end + stub_page(page([edge("alice"), edge("ALICE")])) + expect { service.read_repository("app") }.to raise_error(backend::Error, /Duplicate/) + end + + it "rejects invalid or non-advancing pagination" do + [page([], more: nil), page([], more: true), page([], more: true, cursor: "")].each do |body| + stub_page(body) + expect { service.read_repository("app") }.to raise_error(backend::Error, /pagination/) + end + stub_page(page([], more: true, cursor: "repeat")) + expect { service.read_repository("app") }.to raise_error(backend::Error, /repeated/) + end + + it "surfaces HTTP and malformed JSON failures" do + [403, 500].each do |status| + stub_request(:post, "https://api.github.com/graphql").to_return(status: status, body: "denied") + expect { service.read_repository("app") }.to raise_error(backend::Error, /GraphQL/) + end + stub_request(:post, "https://api.github.com/graphql").to_return(status: 200, body: "{broken") + expect { service.read_repository("app") }.to raise_error(backend::Error, /GraphQL/) + end + + it "recovers from a transient GraphQL error" do + request = stub_request(:post, "https://api.github.com/graphql").to_return( + { status: 502 }, { status: 200, body: JSON.generate(page([])) } + ) + expect(service.read_repository("app").roles).to eq({}) + expect(request).to have_been_requested.twice + end + + described_class::ROLES.each do |role, permission| + it "upserts #{role} with REST #{permission} in exactly one PUT" do + request = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice") + .with(body: { permission: permission }).to_return(status: 204) + service.apply("app", [{ action: :upsert, login: "alice", permission: permission }]) + expect(request).to have_been_requested.once + end + end + + it "applies upserts before removals and invalidates a cached snapshot" do + stub_page(page([edge("alice")])) + service.read_repository("app") + order = [] + stub_request(:put, "https://api.github.com/repos/example/app/collaborators/bob").to_return do + order << :put + { status: 204 } + end + stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/alice").to_return do + order << :delete + { status: 204 } + end + service.apply("app", [{ action: :remove, login: "alice" }, { action: :upsert, login: "bob", permission: "push" }]) + expect(order).to eq([:put, :delete]) + stub_page(page([edge("bob")])) + expect(service.read_repository("app").roles).to eq("bob" => "write") + end + + it "reports invitations without claiming active access" do + stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice") + .to_return(status: 201, body: '{"id":1}', headers: { "Content-Type" => "application/json" }) + expect(logger).to receive(:warn).with(/invitation created.*not yet active/) + service.apply("app", [{ action: :upsert, login: "alice", permission: "pull" }]) + end + + it "rejects malformed invitation responses and unexpected deletion responses" do + ["null", "{}", '{"id":0}', '{"id":"1"}'].each do |body| + stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice") + .to_return(status: 201, body: body, headers: { "Content-Type" => "application/json" }) + expect { service.apply("app", [{ action: :upsert, login: "alice", permission: "pull" }]) } + .to raise_error(backend::Error, /Malformed repository invitation/) + end + stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/alice").to_return(status: 201) + expect { service.apply("app", [{ action: :remove, login: "alice" }]) } + .to raise_error(backend::Error, /Unexpected repository mutation/) + end + + [401, 403, 404, 422, 429, 200].each do |status| + it "surfaces REST HTTP #{status} without retry" do + request = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice") + .to_return(status: status, body: '{"message":"denied"}', headers: { "Content-Type" => "application/json" }) + expect { service.apply("app", [{ action: :upsert, login: "alice", permission: "push" }]) }.to raise_error(backend::Error) + expect(request).to have_been_requested.once + end + end + + it "retries server failures on idempotent REST mutations" do + request = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice") + .to_return({ status: 502 }, { status: 204 }) + service.apply("app", [{ action: :upsert, login: "alice", permission: "push" }]) + expect(request).to have_been_requested.twice + end + + it "stops on a partial failure without removing anyone or caching success" do + stub_page(page([edge("carol")])) + service.read_repository("app") + stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice").to_return(status: 204) + failed = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/bob").to_return(status: 500) + instructions = [{ action: :upsert, login: "alice", permission: "push" }, + { action: :upsert, login: "bob", permission: "push" }, { action: :remove, login: "carol" }] + expect { service.apply("app", instructions) }.to raise_error(backend::Error) + expect(failed).to have_been_requested.times(3) + expect(a_request(:delete, /collaborators/)).not_to have_been_made + stub_page(page([edge("alice"), edge("carol")])) + expect(service.read_repository("app").roles.keys).to eq(%w[alice carol]) + end + + it "rejects unknown instructions, invalid permissions, and non-member targets" do + [ + { action: :oops, login: "alice" }, + { action: :upsert, login: "alice", permission: "custom" }, + { action: :remove, login: "outsider" } + ].each do |instruction| + expect { service.apply("app", [instruction]) }.to raise_error(backend::Error) + end + end + + it "uses GHES REST and GraphQL API paths" do + enterprise = backend::Service.new(org: "example", token: "test-token", ou: base, addr: "https://github.test/api/v3/") + allow(enterprise).to receive(:active_members).and_return(members) + stub_page(page([edge("alice")]), endpoint: "https://github.test/api/graphql") + expect(enterprise.read_repository("app").role_for("alice")).to eq("write") + request = stub_request(:delete, "https://github.test/api/v3/repos/example/app/collaborators/alice").to_return(status: 204) + enterprise.apply("app", [{ action: :remove, login: "alice" }]) + expect(request).to have_been_requested.once + end + end +end diff --git a/spec/unit/fixtures/repositories/entitlements-app/read.txt b/spec/unit/fixtures/repositories/entitlements-app/read.txt new file mode 100644 index 0000000..e2e4d32 --- /dev/null +++ b/spec/unit/fixtures/repositories/entitlements-app/read.txt @@ -0,0 +1,2 @@ +username = balinese +username = bengal; expiration = 2001-01-01 diff --git a/spec/unit/fixtures/repositories/entitlements-app/triage.yaml b/spec/unit/fixtures/repositories/entitlements-app/triage.yaml new file mode 100644 index 0000000..eb82356 --- /dev/null +++ b/spec/unit/fixtures/repositories/entitlements-app/triage.yaml @@ -0,0 +1,2 @@ +rules: + username: chartreux diff --git a/spec/unit/fixtures/repositories/entitlements-app/write.rb b/spec/unit/fixtures/repositories/entitlements-app/write.rb new file mode 100644 index 0000000..51e7029 --- /dev/null +++ b/spec/unit/fixtures/repositories/entitlements-app/write.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Entitlements + class Rule + class EntitlementsApp + class Write < Entitlements::Rule::Base + def members + Set.new([Entitlements.cache[:people_obj].read("DwelF")]) + end + end + end + end +end diff --git a/spec/unit/fixtures/repositories/other.repo/admin.yaml b/spec/unit/fixtures/repositories/other.repo/admin.yaml new file mode 100644 index 0000000..088e881 --- /dev/null +++ b/spec/unit/fixtures/repositories/other.repo/admin.yaml @@ -0,0 +1,3 @@ +expiration: 2001-01-01 +rules: + username: bengal diff --git a/spec/unit/fixtures/repositories/other.repo/maintain.txt b/spec/unit/fixtures/repositories/other.repo/maintain.txt new file mode 100644 index 0000000..d02e07d --- /dev/null +++ b/spec/unit/fixtures/repositories/other.repo/maintain.txt @@ -0,0 +1 @@ +group = pizza_teams/from_username diff --git a/spec/unit/spec_helper.rb b/spec/unit/spec_helper.rb index 2a4ee2a..161947c 100644 --- a/spec/unit/spec_helper.rb +++ b/spec/unit/spec_helper.rb @@ -42,6 +42,7 @@ require_relative "../../lib/entitlements/backend/github_org" require_relative "../../lib/entitlements/backend/github_team" +require_relative "../../lib/entitlements/backend/github_repository" require_relative "../../lib/entitlements/service/github" def fixture(path) From c8c3584339bfc5ac0821019941975b2fd9641294 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Thu, 24 Sep 2026 16:51:19 -0400 Subject: [PATCH 2/5] Document live permission-source scope requirement Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5b0e8ea9-7fd3-4e74-87d7-c3b7e69d8a7b --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index e49f437..9f6db67 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,8 @@ Install the App on every managed repository with: These REST requirements are listed in [GitHub's App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps). Repository reads require access to `collaborators(affiliation: DIRECT)`, `permissionSources`, and each direct source's `roleName`; see the [GraphQL schema](https://docs.github.com/en/graphql/reference/repos). **Live installation-token access to these fields has not been validated by the unit suite.** Before deploying, verify it using the actual App installation and GitHub/GHES version. If those fields are unavailable, do not enable mutations or substitute effective REST permissions. +A live GitHub.com probe with a classic OAuth token confirmed that `permissionSources` and `roleName` require the `admin:org` scope; `repo` plus `read:org` was rejected. This is a classic-token scope requirement, not evidence that an App installation token has access. Validate the App separately rather than broadening an operator's token automatically. + In a designated disposable repository, give an active organization member a direct role and a different, higher inherited role. Verify the direct source reports the lower role, then add/change/remove the direct grant using the installation token. Expect `204` for organization-member adds, updates, and removals. `201` indicates an invitation rather than active access; the backend warns and does not cache it as a completed grant. Confirm inherited and outside access remain unchanged and a subsequent run has no diff. Do not run this check against production accounts or repositories. #### API usage, failure behavior, and rollout From d12e39acb4bdbff05b3829c7e847505d64428341 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Thu, 24 Sep 2026 17:38:17 -0400 Subject: [PATCH 3/5] Avoid requesting unused effective repository permissions The live entitlements-default demo proved edge.permission adds a public_repo scope requirement to an admin:org token. Read only the source-specific roles consumed by reconciliation, without broadening credentials or falling back to effective permissions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5b0e8ea9-7fd3-4e74-87d7-c3b7e69d8a7b --- README.md | 2 ++ lib/entitlements/backend/github_repository/service.rb | 1 - spec/unit/entitlements/backend/github_repository_spec.rb | 9 +++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 9f6db67..e98fc26 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,8 @@ These REST requirements are listed in [GitHub's App permission reference](https: A live GitHub.com probe with a classic OAuth token confirmed that `permissionSources` and `roleName` require the `admin:org` scope; `repo` plus `read:org` was rejected. This is a classic-token scope requirement, not evidence that an App installation token has access. Validate the App separately rather than broadening an operator's token automatically. +The backend deliberately does not request the unused effective `permission` field. A live CI calculation with an `admin:org`-only token showed that field requires an additional `public_repo` scope. Exact direct roles come from `permissionSources.roleName`, so requesting effective permissions would add an unnecessary credential requirement. + In a designated disposable repository, give an active organization member a direct role and a different, higher inherited role. Verify the direct source reports the lower role, then add/change/remove the direct grant using the installation token. Expect `204` for organization-member adds, updates, and removals. `201` indicates an invitation rather than active access; the backend warns and does not cache it as a completed grant. Confirm inherited and outside access remain unchanged and a subsequent run has no diff. Do not run this check against production accounts or repositories. #### API usage, failure behavior, and rollout diff --git a/lib/entitlements/backend/github_repository/service.rb b/lib/entitlements/backend/github_repository/service.rb index 9b9136b..6f8de28 100644 --- a/lib/entitlements/backend/github_repository/service.rb +++ b/lib/entitlements/backend/github_repository/service.rb @@ -61,7 +61,6 @@ def collaborators(repository, cursor) collaborators(affiliation: DIRECT, first: 100, after: #{JSON.generate(cursor)}) { edges { node { login } - permission permissionSources { roleName source { __typename } } } pageInfo { hasNextPage endCursor } diff --git a/spec/unit/entitlements/backend/github_repository_spec.rb b/spec/unit/entitlements/backend/github_repository_spec.rb index 34b97f8..55682ee 100644 --- a/spec/unit/entitlements/backend/github_repository_spec.rb +++ b/spec/unit/entitlements/backend/github_repository_spec.rb @@ -326,6 +326,15 @@ def filtered?(person) expect(service.read_repository("app").roles).to eq("alice" => "triage") end + it "does not request the unused effective permission field or its additional token scope" do + request = stub_request(:post, "https://api.github.com/graphql").with do |req| + query = JSON.parse(req.body).fetch("query") + query.include?("permissionSources { roleName") && !query.match?(/\bpermission\b/) + end.to_return(status: 200, body: JSON.generate(page([edge("alice", "read").reject { |key, _| key == "permission" }]))) + expect(service.read_repository("app").roles).to eq("alice" => "read") + expect(request).to have_been_requested.once + end + described_class::ROLES.each_key do |role| it "reads canonical role #{role}" do stub_page(page([edge("alice", role)])) From 224de61218a0721768b5c37c338f8a25aa88198c Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Fri, 25 Sep 2026 08:57:53 -0400 Subject: [PATCH 4/5] Enforce individual-only repository grants Remove all repository team associations and undeclared direct grants, including outside collaborators. Preserve team hierarchy and membership, order user upserts before cleanup, and verify snapshots before and after application. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a8107bce-2500-45f8-b046-3f50092d9fbd --- README.md | 28 +-- .../models/repository_access.rb | 32 +++- .../backend/github_repository/provider.rb | 31 +++- .../backend/github_repository/service.rb | 41 ++++- .../backend/github_repository_spec.rb | 172 ++++++++++++++++-- 5 files changed, 265 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index e98fc26..8a7fbf3 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ Entitlements configs can contain metadata which the plugin will use to make furt ### GitHub repositories -The `github_repository` backend manages **direct, user-only repository grants for active, non-owner organization members**. It does not create or delete repositories. Load `entitlements/backend/github_repository` in your plugin loader and add this entry under `groups`: +The `github_repository` backend enforces **individual-only repository grants**. Role files define the desired direct grants; undeclared direct user grants (including outside collaborators and explicit owner grants) and **all repository team grants** are removed when `remove` is enabled. New grants are restricted to active, non-owner organization members. It does not create or delete repositories. Load `entitlements/backend/github_repository` in your plugin loader and add this entry under `groups`: ```yaml github.com/github/repositories: @@ -129,7 +129,7 @@ username = bob; expiration = 2027-01-01 group = engineering/platform ``` -Group references, filters, and expiration are evaluated by the normal Entitlements rules engine. People must resolve through the configured people data source, with their `uid` equal to their GitHub login. As with other backends, the underlying username rule omits people absent from that data source; validate such references in your configuration CI. `ignore_not_found` applies to evaluated people missing active GitHub organization membership, not to missing repositories or API failures. +Group references, filters, and expiration are evaluated by the normal Entitlements rules engine. Group references expand to individual users, never GitHub team grants. People must resolve through the configured people data source, with their `uid` equal to their GitHub login. As with other backends, the underlying username rule omits people absent from that data source; validate such references in your configuration CI. `ignore_not_found` applies to evaluated people missing active GitHub organization membership, not to missing repositories or API failures. YAML and Ruby role files are also supported when enabled in `allowed_types`, e.g. `[txt, yaml, rb]`. If omitted, all three formats are allowed. Ruby files use the standard Entitlements Ruby rule-class convention (repository directory and role filename determine the class); enable them only for trusted configuration authors. `allowed_methods` constrains declarative rules, not arbitrary Ruby code. @@ -143,17 +143,21 @@ YAML and Ruby role files are also supported when enabled in `allowed_types`, e.g Custom roles are not supported. Unsupported direct roles or incomplete API responses abort reconciliation rather than falling back to effective permissions. A user cannot occur in multiple roles, even with different capitalization. Comparisons are case-insensitive; difference logs preserve login capitalization and show old and new roles. Duplicate role files, unsupported extensions, symlinks, nested directories, and unexpected files (including README and hidden files) are rejected. Keep documentation outside the managed root. -**A missing role file means no desired direct members for that role.** An empty repository directory therefore requests removal of all managed direct grants if `remove` is enabled. To keep an explicitly empty role file, use the standard `metadata_no_conditions_ok = true` text directive. **Deleting the entire repository directory opts that repository out without cleanup**; existing access is untouched. The configured root must still exist. Git does not track empty directories, so keep an explicit empty role file when intending to remove every managed grant. +**A missing role file means no desired direct members for that role.** An empty repository directory therefore requests removal of all managed direct user and team grants if `remove` is enabled. To keep an explicitly empty role file, use the standard `metadata_no_conditions_ok = true` text directive. **Deleting the entire repository directory opts that repository out without cleanup**; existing access is untouched. The configured root must still exist. Git does not track empty directories, so keep an explicit empty role file when intending to remove every managed grant. #### Ownership boundary and feature flags -Only a `Repository` permission source is used to determine a current direct role, even when a team or organization gives the person a higher effective permission. Team, organization, enterprise-team, and owner grants are not managed. Outside collaborators and organization owners are excluded entirely; desired owners and non-members fail validation by default. With `ignore_not_found: true`, they are skipped with a warning. This backend does not invite people into the organization or manage pending organization invitations. +Only a `Repository` permission source determines a current direct user role, even when a team or organization gives the person higher effective access. Direct grants are read regardless of organization membership. Desired owners and non-members still fail validation by default; with `ignore_not_found: true`, they are ignored with a warning. This backend does not invite people into the organization or manage pending organization invitations. + +Every managed repository also opts into removal of all organization-team repository associations, including empty teams. There is no team manifest or team allowlist. Team membership, hierarchy, and access to other repositories remain unchanged. Parent associations are removed before child associations; the backend re-reads the team list before each removal because inherited child access may disappear with its parent. Diffs report one removal per observed team, not a collaborator deletion for each team member. Exact team roles are unnecessary because no team grant is desired. + +Organization base permissions, organization-owner privileges, and public/internal repository visibility are outside repository-grant management and remain unchanged. The diff explicitly notes this boundary: removing a grant does not necessarily remove all of a person's effective access. Public repositories remain publicly readable. Encountering an `EnterpriseTeam` permission source aborts reconciliation because this backend cannot remove that association; it does not silently claim convergence. `ignore` is an array of logins removed from both sides of the diff, case-insensitively. Ignored users' grants are never mutated. Ignoring a user does not bypass schema/response validation when reading the repository. No fallback to effective permissions is performed. -`features` defaults to `[add, update, remove]`. `add` permits new direct grants, `update` permits role changes, and `remove` permits deleting direct grants absent from desired state. Disabled operations are suppressed in both actions and displayed state. `features: []` performs reads and validation but produces no changes. To inspect the full proposed diff without applying it, use Entitlements' no-op mode with all features enabled. +`features` defaults to `[add, update, remove]`. `add` permits new direct grants, `update` permits role changes, and `remove` permits deleting undeclared direct users and all repository team associations. Disabled operations are suppressed in both actions and displayed state. If removals are disabled while teams remain, the backend warns that individual-only grants are not enforced. `features: []` performs reads and validation but produces no changes. Feature restrictions and ignored users are policy exceptions; use all features and an empty ignore list for authoritative enforcement. To inspect the full proposed diff without applying it, use Entitlements' no-op mode with all features enabled. -Role changes issue one `PUT`, never a remove followed by an add. All upserts for a repository precede removals. Removing a direct grant does not remove inherited access; it also has GitHub's documented side effects on forks and other resources. Review the [collaborator API documentation](https://docs.github.com/en/rest/collaborators/collaborators) before enabling removal. GitHub may reject a direct role below organization base permissions. +Role changes issue one `PUT`, never a remove followed by an add. All desired user upserts for a repository precede user and team removals. Removing grants has GitHub's documented side effects on forks and other resources. Review the [collaborator API documentation](https://docs.github.com/en/rest/collaborators/collaborators) before enabling removal. GitHub may reject a direct role below organization base permissions. #### GitHub App permissions and validation gate @@ -161,9 +165,9 @@ Install the App on every managed repository with: | Scope | Permission | Use | |-------|------------|-----| -| Repository | **Administration: write** | Add, change, and remove collaborator grants | +| Repository | **Administration: write** | Add, change, and remove collaborator grants; remove team repository associations | | Repository | **Metadata: read** (automatically granted) | Repository visibility | -| Organization | **Members: read** | Active organization members and owners | +| Organization | **Members: read** | Active organization members, owners, and repository teams | These REST requirements are listed in [GitHub's App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps). Repository reads require access to `collaborators(affiliation: DIRECT)`, `permissionSources`, and each direct source's `roleName`; see the [GraphQL schema](https://docs.github.com/en/graphql/reference/repos). **Live installation-token access to these fields has not been validated by the unit suite.** Before deploying, verify it using the actual App installation and GitHub/GHES version. If those fields are unavailable, do not enable mutations or substitute effective REST permissions. @@ -171,18 +175,20 @@ A live GitHub.com probe with a classic OAuth token confirmed that `permissionSou The backend deliberately does not request the unused effective `permission` field. A live CI calculation with an `admin:org`-only token showed that field requires an additional `public_repo` scope. Exact direct roles come from `permissionSources.roleName`, so requesting effective permissions would add an unnecessary credential requirement. -In a designated disposable repository, give an active organization member a direct role and a different, higher inherited role. Verify the direct source reports the lower role, then add/change/remove the direct grant using the installation token. Expect `204` for organization-member adds, updates, and removals. `201` indicates an invitation rather than active access; the backend warns and does not cache it as a completed grant. Confirm inherited and outside access remain unchanged and a subsequent run has no diff. Do not run this check against production accounts or repositories. +In a designated disposable repository, give an active organization member a direct role and a different, higher team role. Include an undeclared outside collaborator and parent/child teams, including an empty team. Verify reads using the actual installation token, then reconcile individual-only access. Expect `204` for user upserts/removals and team association removals. `201` indicates an invitation rather than active access; the backend warns and post-apply verification must not mistake it for convergence. Confirm team memberships and other repositories remain unchanged, undeclared user/team grants disappear, and a subsequent run has no diff. Do not run this check against production accounts or repositories. #### API usage, failure behavior, and rollout -Repository reads use one GraphQL request per page of up to 100 direct collaborators, at least one per managed repository. Pagination follows `hasNextPage` and rejects missing/repeated cursors. Snapshots are cached in memory for the service's lifetime and invalidated after successful or partial applies. There is no persistent repository cache or `entitlements-caches` integration. +Repository reads use one GraphQL request per page of up to 100 direct collaborators, plus paginated REST repository-team reads (100 per page). GraphQL pagination follows `hasNextPage` and rejects missing/repeated cursors; team reads use Octokit's automatic Link pagination. Snapshots include team identities and hierarchy independently of user membership. Snapshots are cached in memory for the service's lifetime and invalidated after successful or partial applies. There is no persistent repository cache or `entitlements-caches` integration. + +Before application, a fresh snapshot must match the calculated existing state (excluding ignored users), otherwise the backend aborts and requires recalculation. After application, another fresh snapshot must match the feature-controlled target state. Residual grants or partial failures are explicit errors, never successful-looking convergence. These checks detect drift but are not an atomic transaction with GitHub; concurrent administrators can still change access during a run. Organization membership uses the shared per-run cache (paginated REST reads for `admin` and `member`, 100 users per page); predictive membership is refreshed before authorizing repository grants. Each added or changed grant requires one REST `PUT`, and each removed grant one `DELETE`, excluding retries. Octokit's existing middleware retries server errors on idempotent mutations; authorization, validation, and abuse/rate-limit responses abort without application-level retries. GraphQL uses the shared bounded retry transport. A partial failure stops application, leaves already-applied grants in place, and invalidates the snapshot. Re-run after resolving the failure; changes are not rolled back automatically. 1. Complete the disposable-repository App validation above. 2. Start with a small set of repository directories and no-op mode; compare direct roles with repository settings. `features: []` is also safe for read/validation checks but suppresses the diff. 3. Enable only `add` and `update`, then confirm successive runs converge. -4. Review ignored accounts, inherited access, and outside collaborators before enabling `remove`. +4. Review ignored accounts, organization-level access, outside collaborators, and all repository team associations before enabling `remove`. Existing team-based write/admin access will be removed even when team members are declared individually. 5. Expand gradually while measuring GraphQL cost, REST rate usage, runtime, and failure rates. Consider persistent caching only if measurements justify it. Production convergence and API budgets require this live rollout; mocked tests do not establish them. diff --git a/lib/entitlements/backend/github_repository/models/repository_access.rb b/lib/entitlements/backend/github_repository/models/repository_access.rb index df5530b..858c9ca 100644 --- a/lib/entitlements/backend/github_repository/models/repository_access.rb +++ b/lib/entitlements/backend/github_repository/models/repository_access.rb @@ -5,9 +5,9 @@ class Backend class GitHubRepository module Models class RepositoryAccess < Entitlements::Models::Group - attr_reader :repository, :roles + attr_reader :repository, :roles, :teams - def initialize(repository:, roles:, ou:) + def initialize(repository:, roles:, ou:, teams: []) Configuration.validate_repository!(repository) @repository = repository @roles = {} @@ -22,9 +22,35 @@ def initialize(repository:, roles:, ou:) end @roles.freeze @logins.freeze + @teams = {} + slugs = Set.new + teams.each do |team| + unless team.is_a?(Hash) && team[:id].is_a?(Integer) && team[:id].positive? && + team[:slug].is_a?(String) && /\A[a-zA-Z0-9_-]+\z/.match?(team[:slug]) && + (team[:parent_id].nil? || (team[:parent_id].is_a?(Integer) && team[:parent_id].positive?)) + GitHubRepository.fail!("Malformed repository team: #{team.inspect}") + end + if @teams.key?(team[:id]) || !slugs.add?(team[:slug].downcase) + GitHubRepository.fail!("Duplicate repository team: #{team[:slug]}") + end + @teams[team[:id]] = team.dup.freeze + end + @teams.freeze + ordered_teams super(dn: "cn=#{repository},#{ou}", members: Set.new(@logins.values)) end + def ordered_teams + remaining = teams.dup + ordered = [] + until remaining.empty? + roots = remaining.values.reject { |team| remaining.key?(team[:parent_id]) }.sort_by { |team| team[:slug].downcase } + GitHubRepository.fail!("Cyclic repository team hierarchy") if roots.empty? + roots.each { |team| ordered << remaining.delete(team[:id]) } + end + ordered + end + def role_for(login) roles[login.downcase] end @@ -34,7 +60,7 @@ def login_for(login) end def equals?(other) - other.is_a?(self.class) && dn.casecmp?(other.dn) && roles == other.roles + other.is_a?(self.class) && dn.casecmp?(other.dn) && roles == other.roles && teams == other.teams end alias_method :==, :equals? diff --git a/lib/entitlements/backend/github_repository/provider.rb b/lib/entitlements/backend/github_repository/provider.rb index bf27297..f6b50b7 100644 --- a/lib/entitlements/backend/github_repository/provider.rb +++ b/lib/entitlements/backend/github_repository/provider.rb @@ -40,10 +40,21 @@ def action_for(desired, group_name) name = after ? desired.login_for(login) : existing.login_for(login) Entitlements.logger.info "CHANGE #{desired.repository}: #{name} #{before || '(none)'} -> #{after || '(none)'}" end + teams = existing.teams.values + if @config.fetch("features", FEATURES).include?("remove") + existing.ordered_teams.each do |team| + instructions << { action: :remove_team, team_id: team[:id], slug: team[:slug] } + Entitlements.logger.info "CHANGE #{desired.repository}: team #{@config.fetch('org')}/#{team[:slug]} (granted) -> (none)" + end + teams = [] + elsif !teams.empty? + Entitlements.logger.warn("#{desired.repository}: remove disabled; individual-only repository grants are not enforced") + end + Entitlements.logger.info "#{desired.repository}: organization-level access and repository visibility are unchanged" return if instructions.empty? action = Entitlements::Models::Action.new(desired.dn, - snapshot(existing, current), snapshot(desired, effective), group_name, ignored_users: ignored) - instructions.sort_by { |instruction| [instruction[:action] == :upsert ? 0 : 1, instruction[:login].downcase] }.each do |instruction| + snapshot(existing, current), snapshot(desired, effective, teams: teams), group_name, ignored_users: ignored) + instructions.partition { |instruction| instruction[:action] == :upsert }.flatten.each do |instruction| action.add_implementation(instruction) end action @@ -54,13 +65,25 @@ def commit(action) action.existing.dn == action.updated.dn && action.implementation.is_a?(Array) GitHubRepository.fail!("Invalid repository action") end + current = @github.read_repository(action.updated.repository, refresh: true) + unless filtered_snapshot(current, action.ignored_users) == action.existing + GitHubRepository.fail!("Repository grants changed since calculation; recalculate before applying") + end @github.apply(action.updated.repository, action.implementation) + current = @github.read_repository(action.updated.repository, refresh: true) + unless filtered_snapshot(current, action.ignored_users) == action.updated + GitHubRepository.fail!("Repository grants did not converge for #{action.updated.repository}; recalculate before retrying") + end end private - def snapshot(source, roles) - Models::RepositoryAccess.new(repository: source.repository, roles: roles, ou: @config.fetch("base")) + def snapshot(source, roles, teams: source.teams.values) + Models::RepositoryAccess.new(repository: source.repository, roles: roles, teams: teams, ou: @config.fetch("base")) + end + + def filtered_snapshot(source, ignored) + snapshot(source, source.roles.reject { |login, _| ignored.include?(login) }) end def validate_members(desired, ignored) diff --git a/lib/entitlements/backend/github_repository/service.rb b/lib/entitlements/backend/github_repository/service.rb index 6f8de28..b0d542e 100644 --- a/lib/entitlements/backend/github_repository/service.rb +++ b/lib/entitlements/backend/github_repository/service.rb @@ -12,9 +12,10 @@ def active_members end end - def read_repository(repository) + def read_repository(repository, refresh: false) Configuration.validate_repository!(repository) @repositories ||= {} + @repositories.delete(repository.downcase) if refresh @repositories[repository.downcase] ||= begin roles = {} cursor = nil @@ -31,7 +32,7 @@ def read_repository(repository) GitHubRepository.fail!("Missing or repeated repository pagination cursor") end end - Models::RepositoryAccess.new(repository: repository, roles: roles, ou: ou) + Models::RepositoryAccess.new(repository: repository, roles: roles, teams: repository_teams(repository), ou: ou) end rescue KeyError, TypeError => e GitHubRepository.fail!("Malformed repository response for #{repository}: #{e.message}") @@ -39,10 +40,14 @@ def read_repository(repository) def apply(repository, instructions) Configuration.validate_repository!(repository) - instructions.sort_by { |instruction| [instruction.fetch(:action) == :upsert ? 0 : 1, instruction.fetch(:login).downcase] }.each do |instruction| + instructions.partition { |instruction| instruction.fetch(:action) == :upsert }.flatten.each do |instruction| + if instruction.fetch(:action) == :remove_team + remove_team(repository, instruction) + next + end login = instruction.fetch(:login) Configuration.validate_login!(login) - unless active_members.key?(login.downcase) + if instruction.fetch(:action) == :upsert && !active_members.key?(login.downcase) GitHubRepository.fail!("#{repository}: #{login} is not an active non-owner organization member") end mutate(repository, instruction) @@ -54,6 +59,32 @@ def apply(repository, instructions) private + def repository_teams(repository) + teams = octokit.repository_teams("#{org}/#{repository}") + GitHubRepository.fail!("Malformed repository teams for #{repository}") unless teams.is_a?(Array) + teams.map do |team| + unless team.is_a?(Sawyer::Resource) && team.key?(:parent) && + (team[:parent].nil? || (team[:parent].is_a?(Sawyer::Resource) && team[:parent][:id].is_a?(Integer))) + GitHubRepository.fail!("Malformed repository team response for #{repository}") + end + { id: team[:id], slug: team[:slug], parent_id: team[:parent]&.[](:id) } + end + rescue Octokit::Error => e + GitHubRepository.fail!("Reading teams for #{org}/#{repository} failed: #{e.message}") + end + + def remove_team(repository, instruction) + # Removing a parent association can also remove inherited child access. + current = Models::RepositoryAccess.new(repository: repository, roles: {}, teams: repository_teams(repository), ou: ou) + team = current.teams[instruction.fetch(:team_id)] + return unless team + GitHubRepository.fail!("Repository team identity changed") unless team[:slug] == instruction.fetch(:slug) + octokit.delete("orgs/#{org}/teams/#{team[:slug]}/repos/#{org}/#{repository}") + GitHubRepository.fail!("Unexpected team removal response: HTTP #{octokit.last_response.status}") unless octokit.last_response.status == 204 + rescue Octokit::Error => e + GitHubRepository.fail!("Removing team from #{org}/#{repository} failed: #{e.message}") + end + def collaborators(repository, cursor) query = <<~GRAPHQL { @@ -87,7 +118,6 @@ def read_edge(edge, roles) end login = edge.fetch("node").fetch("login") Configuration.validate_login!(login) - return unless active_members.key?(login.downcase) direct = edge.fetch("permissionSources").select do |source| unless source.is_a?(Hash) && source["source"].is_a?(Hash) GitHubRepository.fail!("Malformed repository permission source") @@ -96,6 +126,7 @@ def read_edge(edge, roles) unless %w[Repository Team Organization EnterpriseTeam].include?(type) GitHubRepository.fail!("Unknown repository permission source: #{type.inspect}") end + GitHubRepository.fail!("Unsupported enterprise-team access for #{login}") if type == "EnterpriseTeam" type == "Repository" end return if direct.empty? diff --git a/spec/unit/entitlements/backend/github_repository_spec.rb b/spec/unit/entitlements/backend/github_repository_spec.rb index 55682ee..4dc850b 100644 --- a/spec/unit/entitlements/backend/github_repository_spec.rb +++ b/spec/unit/entitlements/backend/github_repository_spec.rb @@ -13,10 +13,21 @@ let(:service) { backend::Service.new(org: "example", token: "test-token", ou: base) } let(:members) { { "alice" => "member", "bob" => "member", "carol" => "member", "owner" => "admin" } } - def access(roles = {}, repository: "app", **inline_roles) - backend::Models::RepositoryAccess.new(repository: repository, roles: roles.merge(inline_roles), ou: base) + def access(roles = {}, repository: "app", teams: [], **inline_roles) + backend::Models::RepositoryAccess.new(repository: repository, roles: roles.merge(inline_roles), teams: teams, ou: base) end + def team(id = 1, slug = "engineering", parent_id = nil) + { id: id, slug: slug, parent_id: parent_id } + end + + def stub_teams(teams = [], endpoint: "https://api.github.com/repos/example/app/teams") + stub_request(:get, endpoint).with(query: { per_page: 100 }) + .to_return(status: 200, body: JSON.generate(teams), headers: { "Content-Type" => "application/json" }) + end + + before { stub_teams } + def edge(login, role = "write", sources: nil) { "node" => { "login" => login }, "permission" => "ADMIN", "permissionSources" => sources || [{ "roleName" => role, "source" => { "__typename" => "Repository" } }] } @@ -92,6 +103,19 @@ def stub_page(body, endpoint: "https://api.github.com/graphql") expect { access("../alice" => "write") }.to raise_error(backend::Error, /login/) expect { access("alice" => "read", "ALICE" => "write") }.to raise_error(backend::Error, /Duplicate/) end + + it "tracks teams separately from users and orders parents before children" do + model = access("engineering" => "read", :teams => [team(2, "child", 1), team]) + expect(model.member_strings).to eq(Set.new(["engineering"])) + expect(model.ordered_teams.map { |entry| entry[:id] }).to eq([1, 2]) + expect(model).not_to eq(access("engineering" => "read")) + [team(nil), team(0), team(1, "../bad"), team(1, "valid", -1)].each do |invalid| + expect { access(teams: [invalid]) }.to raise_error(backend::Error, /Malformed/) + end + expect { access(teams: [team, team]) }.to raise_error(backend::Error, /Duplicate/) + expect { access(teams: [team, team(2, "ENGINEERING")]) }.to raise_error(backend::Error, /Duplicate/) + expect { access(teams: [team(1, "one", 2), team(2, "two", 1)]) }.to raise_error(backend::Error, /Cyclic/) + end end describe "recursive loader" do @@ -189,7 +213,7 @@ def filtered?(person) described_class::FEATURES.length.succ.times.flat_map { |size| described_class::FEATURES.combination(size).to_a }.each do |features| it "honors feature combination #{features.inspect} in instructions and displayed state" do config["features"] = features - allow(service).to receive(:read_repository).with("app").and_return(access("alice" => "read", "bob" => "write")) + allow(service).to receive(:read_repository).with("app").and_return(access({ "alice" => "read", "bob" => "write" }, teams: [team])) action = provider.action_for(access("ALICE" => "admin", "carol" => "triage"), "repos") if features.empty? expect(action).to be_nil @@ -198,11 +222,13 @@ def filtered?(person) expected << { action: :upsert, login: "ALICE", permission: "admin" } if features.include?("update") expected << { action: :upsert, login: "carol", permission: "triage" } if features.include?("add") expected << { action: :remove, login: "bob" } if features.include?("remove") + expected << { action: :remove_team, team_id: 1, slug: "engineering" } if features.include?("remove") expect(action.implementation).to eq(expected) effective = { "alice" => features.include?("update") ? "admin" : "read" } effective["carol"] = "triage" if features.include?("add") effective["bob"] = "write" unless features.include?("remove") expect(action.updated.roles).to eq(effective) + expect(action.updated.teams.empty?).to eq(features.include?("remove")) expect(action.existing.equals?(action.updated)).to be(false) end end @@ -236,7 +262,9 @@ def filtered?(person) actions = controller.calculate expect(actions.size).to eq(1) expect(controller.change_count).to eq(1) - expect(service).to receive(:apply).with("app", [{ action: :upsert, login: "alice", permission: "maintain" }]) + expect(service).to receive(:apply).with("app", [{ action: :upsert, login: "alice", permission: "maintain" }]) do + allow(service).to receive(:read_repository).and_return(desired) + end controller.apply(actions.first) allow(loader).to receive(:load).and_raise(backend::Error, "invalid file") expect(service).not_to receive(:read_repository) @@ -254,10 +282,51 @@ def filtered?(person) action = Entitlements::Models::Action.new("app", access, nil, "repos") expect { provider.commit(action) }.to raise_error(backend::Error, /Invalid repository action/) end + + it "calculates and counts team-only actions without pretending teams are users" do + desired = access + allow(backend::Configuration).to receive(:new).and_return(instance_double(backend::Configuration, load: [desired])) + allow(service).to receive(:read_repository).and_return(access(teams: [team])) + controller = backend::Controller.new("repos", config) + action = controller.calculate.first + expect(controller.change_count).to eq(1) + expect(action.implementation).to eq([{ action: :remove_team, team_id: 1, slug: "engineering" }]) + expect(action.existing.member_strings).to be_empty + expect(action.updated.teams).to be_empty + expect(action.existing).not_to eq(action.updated) + end + + it "preserves teams when remove is disabled and warns about the unenforced policy" do + config["features"] = %w[add update] + allow(service).to receive(:read_repository).and_return(access(teams: [team])) + expect(logger).to receive(:warn).with(/individual-only.*not enforced/) + action = provider.action_for(access("alice" => "read"), "repos") + expect(action.updated.teams).to eq(action.existing.teams) + expect(action.implementation.map { |instruction| instruction[:action] }).to eq([:upsert]) + end + + it "removes undeclared outside and owner direct grants as well as all teams" do + allow(service).to receive(:read_repository).and_return(access("outsider" => "read", "owner" => "admin", :teams => [team])) + action = provider.action_for(access("alice" => "read"), "repos") + expect(action.implementation.map { |instruction| instruction[:action] }).to eq([:upsert, :remove, :remove, :remove_team]) + expect(action.updated.roles).to eq("alice" => "read") + end + + it "rejects stale plans before mutation and rejects residual grants after apply" do + allow(service).to receive(:read_repository).and_return(access(teams: [team])) + action = provider.action_for(access, "repos") + allow(service).to receive(:read_repository).with("app", refresh: true).and_return(access) + expect(service).not_to receive(:apply) + expect { provider.commit(action) }.to raise_error(backend::Error, /changed since calculation/) + RSpec::Mocks.space.proxy_for(service).reset + allow(service).to receive(:read_repository).with("app", refresh: true).and_return(action.existing) + expect(service).to receive(:apply).with("app", action.implementation) + expect { provider.commit(action) }.to raise_error(backend::Error, /did not converge/) + end end describe "end-to-end reconciliation" do - it "converges on a second calculation, preserving inherited and outside access" do + it "converges to individual-only grants, removing teams and undeclared direct grants" do cache[:people_obj] = Entitlements::Data::People::YAML.new(filename: fixture("people.yaml")) cache[:file_objects] = {} stub_request(:get, "https://api.github.com/orgs/example/members") @@ -268,28 +337,39 @@ def filtered?(person) .to_return(status: 200, body: '[{"login":"balinese"},{"login":"bob"},{"login":"carol"}]', headers: { "Content-Type" => "application/json" }) inherited = edge("carol", sources: [{ "roleName" => "admin", "source" => { "__typename" => "Team" } }]) - preserved = [edge("outsider"), edge("owner"), inherited] - stub_page(page([edge("balinese", "read"), edge("bob")] + preserved)) + initial = page([edge("balinese", "read"), edge("bob"), edge("outsider"), edge("owner"), inherited]) + final = page([edge("balinese", "write")]) + stub_request(:post, "https://api.github.com/graphql").to_return( + { status: 200, body: JSON.generate(initial) }, + { status: 200, body: JSON.generate(initial) }, + { status: 200, body: JSON.generate(final) } + ) + stub_teams([{ id: 1, slug: "engineering", parent: nil }]) put = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/balinese") .with(body: { permission: "push" }).to_return(status: 204) delete = stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/bob").to_return(status: 204) + %w[outsider owner].each do |login| + stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/#{login}").to_return(status: 204) + end + remove_team = stub_request(:delete, "https://api.github.com/orgs/example/teams/engineering/repos/example/app").to_return do + stub_teams + { status: 204 } + end Dir.mktmpdir do |root| Dir.mkdir("#{root}/app") File.write("#{root}/app/write.txt", "username = balinese\n") controller = backend::Controller.new("repos", config.merge("dir" => root)) actions = controller.calculate expect(actions.size).to eq(1) - expect(actions.first.implementation.size).to eq(2) + expect(actions.first.implementation.size).to eq(5) controller.apply(actions.first) - stub_page(page([edge("balinese", "write")] + preserved)) expect(controller.calculate).to eq([]) end expect(put).to have_been_requested.once expect(delete).to have_been_requested.once expect(members_request).to have_been_requested.once - %w[owner outsider carol].each do |login| - expect(a_request(:delete, "https://api.github.com/repos/example/app/collaborators/#{login}")).not_to have_been_made - end + expect(remove_team).to have_been_requested.once + expect(a_request(:delete, "https://api.github.com/repos/example/app/collaborators/carol")).not_to have_been_made end end @@ -306,13 +386,13 @@ def filtered?(person) it "paginates, uses direct roles instead of effective permissions, and caches per repository" do first = page([edge("Alice", "Read"), edge("outsider"), edge("owner")], more: true, cursor: 'a"b') - inherited = %w[Team Organization EnterpriseTeam].map { |type| { "roleName" => "admin", "source" => { "__typename" => type } } } + inherited = %w[Team Organization].map { |type| { "roleName" => "admin", "source" => { "__typename" => type } } } second = page([edge("Bob", "triage", sources: inherited), edge("Carol", "maintain")]) request = stub_request(:post, "https://api.github.com/graphql") .with(headers: { "Authorization" => "bearer test-token" }) .to_return({ status: 200, body: JSON.generate(first) }, { status: 200, body: JSON.generate(second) }) - expect(service.read_repository("app").roles).to eq("alice" => "read", "carol" => "maintain") - expect(service.read_repository("APP").roles).to eq("alice" => "read", "carol" => "maintain") + expect(service.read_repository("app").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write", "owner" => "write") + expect(service.read_repository("APP").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write", "owner" => "write") expect(request).to have_been_requested.twice expect(a_request(:post, "https://api.github.com/graphql").with { |req| JSON.parse(req.body).fetch("query").include?('after: "a\\"b"') @@ -361,6 +441,7 @@ def filtered?(person) edge("alice", sources: [nil]), edge("alice", sources: [{ "source" => {} }]), edge("alice", sources: [{ "source" => { "__typename" => nil } }]), + edge("alice", sources: [{ "source" => { "__typename" => "EnterpriseTeam" } }]), edge("alice", sources: [edge("alice")["permissionSources"].first] * 2), edge("../alice") ].each do |invalid| @@ -477,7 +558,7 @@ def filtered?(person) [ { action: :oops, login: "alice" }, { action: :upsert, login: "alice", permission: "custom" }, - { action: :remove, login: "outsider" } + { action: :upsert, login: "outsider", permission: "pull" } ].each do |instruction| expect { service.apply("app", [instruction]) }.to raise_error(backend::Error) end @@ -486,11 +567,70 @@ def filtered?(person) it "uses GHES REST and GraphQL API paths" do enterprise = backend::Service.new(org: "example", token: "test-token", ou: base, addr: "https://github.test/api/v3/") allow(enterprise).to receive(:active_members).and_return(members) + stub_teams([], endpoint: "https://github.test/api/v3/repos/example/app/teams") stub_page(page([edge("alice")]), endpoint: "https://github.test/api/graphql") expect(enterprise.read_repository("app").role_for("alice")).to eq("write") request = stub_request(:delete, "https://github.test/api/v3/repos/example/app/collaborators/alice").to_return(status: 204) enterprise.apply("app", [{ action: :remove, login: "alice" }]) expect(request).to have_been_requested.once end + + it "paginates repository teams including empty teams, independent of collaborators" do + stub_page(page([])) + stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100 }) + .to_return(status: 200, body: '[{"id":1,"slug":"empty","parent":null}]', + headers: { "Content-Type" => "application/json", "Link" => '; rel="next"' }) + stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100, page: 2 }) + .to_return(status: 200, body: '[{"id":2,"slug":"child","parent":{"id":1}}]', headers: { "Content-Type" => "application/json" }) + snapshot = service.read_repository("app") + expect(snapshot.roles).to be_empty + expect(snapshot.ordered_teams).to eq([team(1, "empty"), team(2, "child", 1)]) + end + + it "rejects inaccessible or malformed repository team lists" do + stub_page(page([])) + ["{}", "[{}]", '[{"id":1,"slug":"team","parent":{}}]', '[{"id":0,"slug":"team","parent":null}]'].each do |body| + stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100 }) + .to_return(status: 200, body: body, headers: { "Content-Type" => "application/json" }) + expect { service.read_repository("app") }.to raise_error(backend::Error, /Malformed/) + end + stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100 }).to_return(status: 403) + expect { service.read_repository("app") }.to raise_error(backend::Error, /Reading teams/) + end + + it "removes parents before remaining direct child associations, skipping inherited access that disappeared" do + entries = [{ id: 1, slug: "parent", parent: nil }, { id: 2, slug: "child", parent: { id: 1 } }, + { id: 3, slug: "inherited", parent: { id: 1 } }] + stub_teams(entries) + order = [] + stub_request(:put, "https://api.github.com/repos/example/app/collaborators/alice").to_return do + order << :user + { status: 204 } + end + stub_request(:delete, "https://api.github.com/orgs/example/teams/parent/repos/example/app").to_return do + order << :parent + stub_teams([entries[1]]) + { status: 204 } + end + stub_request(:delete, "https://api.github.com/orgs/example/teams/child/repos/example/app").to_return do + order << :child + stub_teams + { status: 204 } + end + instructions = entries.map { |entry| { action: :remove_team, team_id: entry[:id], slug: entry[:slug] } } + service.apply("app", instructions + [{ action: :upsert, login: "alice", permission: "pull" }]) + expect(order).to eq([:user, :parent, :child]) + expect(a_request(:delete, %r{/teams/inherited/})).not_to have_been_made + end + + it "surfaces failed team removals and changed team identities" do + stub_teams([{ id: 1, slug: "engineering", parent: nil }]) + instruction = { action: :remove_team, team_id: 1, slug: "engineering" } + [403, 200].each do |status| + stub_request(:delete, "https://api.github.com/orgs/example/teams/engineering/repos/example/app").to_return(status: status) + expect { service.apply("app", [instruction]) }.to raise_error(backend::Error) + end + expect { service.apply("app", [instruction.merge(slug: "renamed")]) }.to raise_error(backend::Error, /identity changed/) + end end end From c522a17097b2f75fb5c3c15d7e70123823f38f8b Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Fri, 25 Sep 2026 10:38:28 -0400 Subject: [PATCH 5/5] Respect organization-wide access during repository reconciliation Discover base permissions and every organization role and assignment. Accept owners as desired users, defer ambiguous owner grants and roles below inherited access, and preserve organization/enterprise team sources using access_source. Refresh organization access for stale-plan and convergence checks, fail closed on incomplete metadata, and document required read permissions and policy exceptions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a8107bce-2500-45f8-b046-3f50092d9fbd --- README.md | 28 +- lib/entitlements/backend/github_repository.rb | 1 + .../models/organization_access.rb | 48 ++++ .../models/repository_access.rb | 15 +- .../backend/github_repository/provider.rb | 27 +- .../backend/github_repository/service.rb | 73 ++++- .../backend/github_repository_spec.rb | 263 ++++++++++++++++-- 7 files changed, 400 insertions(+), 55 deletions(-) create mode 100644 lib/entitlements/backend/github_repository/models/organization_access.rb diff --git a/README.md b/README.md index 8a7fbf3..53fc46a 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ Entitlements configs can contain metadata which the plugin will use to make furt ### GitHub repositories -The `github_repository` backend enforces **individual-only repository grants**. Role files define the desired direct grants; undeclared direct user grants (including outside collaborators and explicit owner grants) and **all repository team grants** are removed when `remove` is enabled. New grants are restricted to active, non-owner organization members. It does not create or delete repositories. Load `entitlements/backend/github_repository` in your plugin loader and add this entry under `groups`: +The `github_repository` backend manages **individual-only repository-local grants**, preserving organization-wide access. Role files define the desired direct grants; undeclared direct user grants (including outside collaborators) and **direct organization-team repository associations** are removed when `remove` is enabled. Active organization members, including owners, are valid desired users. Owner grants and requested roles below inherited organization-wide access are explicitly deferred as described below, not silently treated as satisfied direct grants. It does not create or delete repositories. Load `entitlements/backend/github_repository` in your plugin loader and add this entry under `groups`: ```yaml github.com/github/repositories: @@ -105,7 +105,7 @@ github.com/github/repositories: ignore_not_found: false ``` -`dir`, `base`, `org`, and `token` are required, nonempty strings. Relative directories resolve against Entitlements' `configuration_path`. Omit `addr` (or set it to null) for GitHub.com. For GitHub Enterprise Server, use `https://HOST/api/v3`; repository GraphQL requests use `https://HOST/api/graphql`. The server must support collaborator permission sources and source-specific role names. +`dir`, `base`, `org`, and `token` are required, nonempty strings. Relative directories resolve against Entitlements' `configuration_path`. Omit `addr` (or set it to null) for GitHub.com. For GitHub Enterprise Server, use `https://HOST/api/v3`; repository GraphQL requests use `https://HOST/api/graphql`. The server must expose collaborator permission sources, source-specific role names, repository-team `access_source`, organization base permissions, and the organization-role catalog and assignments. Missing data or unsupported APIs abort reconciliation; there is no assumption that unavailable inherited-access information means no inherited access. #### Repository and role files @@ -141,17 +141,23 @@ YAML and Ruby role files are also supported when enabled in `allowed_types`, e.g | `maintain` | `maintain` | | `admin` | `admin` | -Custom roles are not supported. Unsupported direct roles or incomplete API responses abort reconciliation rather than falling back to effective permissions. A user cannot occur in multiple roles, even with different capitalization. Comparisons are case-insensitive; difference logs preserve login capitalization and show old and new roles. Duplicate role files, unsupported extensions, symlinks, nested directories, and unexpected files (including README and hidden files) are rejected. Keep documentation outside the managed root. +Custom direct repository roles are not supported. Organization roles, including custom and enterprise-defined organization roles, are discovered dynamically without a role-name allowlist. Their `base_role` determines their inherited repository level; roles with no repository base still retain their organization capabilities and are reported without inventing a repository permission. Unsupported direct roles, unknown repository base roles, or incomplete API responses abort reconciliation rather than falling back to effective permissions. A user cannot occur in multiple roles, even with different capitalization. Comparisons are case-insensitive; difference logs preserve login capitalization and show old and new roles. Duplicate role files, unsupported extensions, symlinks, nested directories, and unexpected files (including README and hidden files) are rejected. Keep documentation outside the managed root. **A missing role file means no desired direct members for that role.** An empty repository directory therefore requests removal of all managed direct user and team grants if `remove` is enabled. To keep an explicitly empty role file, use the standard `metadata_no_conditions_ok = true` text directive. **Deleting the entire repository directory opts that repository out without cleanup**; existing access is untouched. The configured root must still exist. Git does not track empty directories, so keep an explicit empty role file when intending to remove every managed grant. #### Ownership boundary and feature flags -Only a `Repository` permission source determines a current direct user role, even when a team or organization gives the person higher effective access. Direct grants are read regardless of organization membership. Desired owners and non-members still fail validation by default; with `ignore_not_found: true`, they are ignored with a warning. This backend does not invite people into the organization or manage pending organization invitations. +For non-owners, only a `Repository` permission source determines a current direct user role, even when a team or organization gives the person higher effective access. Non-member direct grants are included in cleanup. Desired non-members fail validation by default; with `ignore_not_found: true`, they are ignored with a warning. Owners do not require this workaround. This backend does not invite people into the organization or manage pending organization invitations. -Every managed repository also opts into removal of all organization-team repository associations, including empty teams. There is no team manifest or team allowlist. Team membership, hierarchy, and access to other repositories remain unchanged. Parent associations are removed before child associations; the backend re-reads the team list before each removal because inherited child access may disappear with its parent. Diffs report one removal per observed team, not a collaborator deletion for each team member. Exact team roles are unnecessary because no team grant is desired. +Organization-wide access is read from live organization membership, `default_repository_permission`, and **every role and its user assignments** from the Organization Roles API, including direct, indirect/team-derived, and mixed assignments. All-repository read/triage/write/maintain/admin, security-manager, and custom/enterprise-defined organization roles follow the same metadata-driven path. No organization role or assignment is modified. -Organization base permissions, organization-owner privileges, and public/internal repository visibility are outside repository-grant management and remain unchanged. The diff explicitly notes this boundary: removing a grant does not necessarily remove all of a person's effective access. Public repositories remain publicly readable. Encountering an `EnterpriseTeam` permission source aborts reconciliation because this backend cannot remove that association; it does not silently claim convergence. +An upsert below the highest inherited organization repository level is logged as `DEFER`; the desired declaration is retained, but the current direct grant is left unchanged. This avoids base-permission API rejection and does not invent a higher desired role. Equal or higher direct roles can be provisioned normally. Undeclared non-owner direct grants can still be removed: their organization-wide access remains. A deferral is a policy exception, not full convergence to the role files, and can leave an existing higher direct grant in place until reviewed or the inherited role expires. + +Owners require an additional representation safeguard: GitHub can emit a synthetic `Repository` admin source for organization ownership, including alongside a real direct source. All direct-user reconciliation for current owners is deferred because these sources cannot safely be distinguished. The backend does not delete apparent owner grants or claim to downgrade owner access. After JIT ownership or another blocking organization role expires, a new calculation resumes the declared direct-grant reconciliation. This is periodic reconciliation, not an atomic JIT handoff; access can change between runs. + +Every managed repository opts into removal of team associations explicitly reported with `access_source: direct`, including empty teams. Entries reported as `organization` or `enterprise` are preserved and logged. A team with both a direct repository association and an organization role can lose its direct association while retaining inherited access. Missing/unknown source metadata is an error, never a reason to guess that access is direct. Enterprise-team associations are outside this backend's writable scope; their presence is explicitly reported as an unmanaged policy exception. There is no team manifest or team allowlist. Team membership, hierarchy, organization roles, and access to other repositories remain unchanged. Parent direct associations are removed before child direct associations, with a fresh source check before each deletion. + +Organization-wide privileges and public/internal repository visibility remain unchanged. Removing repository-local grants does not necessarily remove all of a person's effective access. Public repositories remain publicly readable. `ignore` is an array of logins removed from both sides of the diff, case-insensitively. Ignored users' grants are never mutated. Ignoring a user does not bypass schema/response validation when reading the repository. No fallback to effective permissions is performed. @@ -167,7 +173,9 @@ Install the App on every managed repository with: |-------|------------|-----| | Repository | **Administration: write** | Add, change, and remove collaborator grants; remove team repository associations | | Repository | **Metadata: read** (automatically granted) | Repository visibility | -| Organization | **Members: read** | Active organization members, owners, and repository teams | +| Organization | **Members: read** | Active organization members, owners, repository teams, and organization-role user assignments | +| Organization | **Administration: read** | Organization base repository permission and permission-source visibility | +| Organization | **Custom organization roles: read** | Complete organization-role catalog, including predefined and enterprise-defined roles | These REST requirements are listed in [GitHub's App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps). Repository reads require access to `collaborators(affiliation: DIRECT)`, `permissionSources`, and each direct source's `roleName`; see the [GraphQL schema](https://docs.github.com/en/graphql/reference/repos). **Live installation-token access to these fields has not been validated by the unit suite.** Before deploying, verify it using the actual App installation and GitHub/GHES version. If those fields are unavailable, do not enable mutations or substitute effective REST permissions. @@ -175,15 +183,15 @@ A live GitHub.com probe with a classic OAuth token confirmed that `permissionSou The backend deliberately does not request the unused effective `permission` field. A live CI calculation with an `admin:org`-only token showed that field requires an additional `public_repo` scope. Exact direct roles come from `permissionSources.roleName`, so requesting effective permissions would add an unnecessary credential requirement. -In a designated disposable repository, give an active organization member a direct role and a different, higher team role. Include an undeclared outside collaborator and parent/child teams, including an empty team. Verify reads using the actual installation token, then reconcile individual-only access. Expect `204` for user upserts/removals and team association removals. `201` indicates an invitation rather than active access; the backend warns and post-apply verification must not mistake it for convergence. Confirm team memberships and other repositories remain unchanged, undeclared user/team grants disappear, and a subsequent run has no diff. Do not run this check against production accounts or repositories. +In a designated disposable organization/repository, cover owners/JIT transitions, each all-repository base role, security-manager and custom role assignments (direct and through teams), organization base permissions, an undeclared outside collaborator, and empty/nested teams. Include a team with both direct and organization-wide access. Verify reads using the actual installation token, then reconcile repository-local grants. Expect `204` for user upserts/removals and team association removals. `201` indicates an invitation rather than active access; post-apply verification must not mistake it for convergence. Confirm organization-wide privileges, team memberships and other repositories remain unchanged, manageable undeclared grants disappear, and subsequent runs show either no diff or explicit deferrals. Do not run this mutation check against production accounts or repositories. #### API usage, failure behavior, and rollout Repository reads use one GraphQL request per page of up to 100 direct collaborators, plus paginated REST repository-team reads (100 per page). GraphQL pagination follows `hasNextPage` and rejects missing/repeated cursors; team reads use Octokit's automatic Link pagination. Snapshots include team identities and hierarchy independently of user membership. Snapshots are cached in memory for the service's lifetime and invalidated after successful or partial applies. There is no persistent repository cache or `entitlements-caches` integration. -Before application, a fresh snapshot must match the calculated existing state (excluding ignored users), otherwise the backend aborts and requires recalculation. After application, another fresh snapshot must match the feature-controlled target state. Residual grants or partial failures are explicit errors, never successful-looking convergence. These checks detect drift but are not an atomic transaction with GitHub; concurrent administrators can still change access during a run. +Before application, a fresh snapshot must match the calculated existing state (excluding ignored users), including organization membership, base permissions, role definitions and assignments; otherwise the backend aborts and requires recalculation. After application, another fresh snapshot must match the feature-controlled, deferral-aware target state. Team comparisons use only direct associations, so inherited access persisting after direct removal is not a false convergence failure. Residual manageable grants or partial failures are explicit errors. Deferrals remain visible policy exceptions. These checks detect drift but are not an atomic transaction with GitHub; concurrent administrators can still change access during a run. -Organization membership uses the shared per-run cache (paginated REST reads for `admin` and `member`, 100 users per page); predictive membership is refreshed before authorizing repository grants. Each added or changed grant requires one REST `PUT`, and each removed grant one `DELETE`, excluding retries. Octokit's existing middleware retries server errors on idempotent mutations; authorization, validation, and abuse/rate-limit responses abort without application-level retries. GraphQL uses the shared bounded retry transport. A partial failure stops application, leaves already-applied grants in place, and invalidates the snapshot. Re-run after resolving the failure; changes are not rolled back automatically. +Organization access is cached per service/installation during calculation, not shared with other backends' predictive/JIT membership caches. Reads include paginated REST membership, organization settings, the complete role catalog, and paginated user assignments for every role. It is refreshed before and after each repository apply; membership/role changes may require recalculation even if repository grants did not change. Each added or changed grant requires one REST `PUT`, and each removed grant one `DELETE`, excluding retries. Octokit's existing middleware retries server errors on idempotent mutations; authorization, validation, and abuse/rate-limit responses abort without application-level retries. GraphQL uses the shared bounded retry transport. A partial failure stops application, leaves already-applied grants in place, and invalidates the snapshot. Re-run after resolving the failure; changes are not rolled back automatically. 1. Complete the disposable-repository App validation above. 2. Start with a small set of repository directories and no-op mode; compare direct roles with repository settings. `features: []` is also safe for read/validation checks but suppresses the diff. diff --git a/lib/entitlements/backend/github_repository.rb b/lib/entitlements/backend/github_repository.rb index 39a102a..d3157cd 100644 --- a/lib/entitlements/backend/github_repository.rb +++ b/lib/entitlements/backend/github_repository.rb @@ -25,6 +25,7 @@ def self.fail!(message) end end +require_relative "github_repository/models/organization_access" require_relative "github_repository/models/repository_access" require_relative "github_repository/configuration" require_relative "github_repository/service" diff --git a/lib/entitlements/backend/github_repository/models/organization_access.rb b/lib/entitlements/backend/github_repository/models/organization_access.rb new file mode 100644 index 0000000..d90614f --- /dev/null +++ b/lib/entitlements/backend/github_repository/models/organization_access.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Entitlements + class Backend + class GitHubRepository + module Models + class OrganizationAccess + attr_reader :members, :base_role, :assignments + + def initialize(members:, base_role:, assignments:) + unless (ROLES.keys + ["none"]).include?(base_role) && + members.values.all? { |role| %w[member admin].include?(role) } + GitHubRepository.fail!("Malformed organization access") + end + @members = members.transform_keys(&:downcase).freeze + @base_role = base_role + @assignments = assignments.transform_keys(&:downcase).transform_values do |roles| + roles.sort_by { |role| role.fetch(:id) }.freeze + end.freeze + end + + def owner?(login) + members[login.downcase] == "admin" + end + + def inherited_role(login) + return unless members.key?(login.downcase) + roles = assignments.fetch(login.downcase, []).filter_map { |assignment| assignment[:base_role] } + roles << base_role unless base_role == "none" + roles << "admin" if owner?(login) + roles.max_by { |role| ROLES.keys.index(role) } + end + + def sources(login) + result = assignments.fetch(login.downcase, []).map { |role| "organization role #{role.fetch(:name).inspect}" } + result << "organization base #{base_role}" if members.key?(login.downcase) && base_role != "none" + result << "organization ownership" if owner?(login) + result + end + + def ==(other) + other.is_a?(self.class) && members == other.members && base_role == other.base_role && assignments == other.assignments + end + end + end + end + end +end diff --git a/lib/entitlements/backend/github_repository/models/repository_access.rb b/lib/entitlements/backend/github_repository/models/repository_access.rb index 858c9ca..b1867a3 100644 --- a/lib/entitlements/backend/github_repository/models/repository_access.rb +++ b/lib/entitlements/backend/github_repository/models/repository_access.rb @@ -5,11 +5,12 @@ class Backend class GitHubRepository module Models class RepositoryAccess < Entitlements::Models::Group - attr_reader :repository, :roles, :teams + attr_reader :repository, :roles, :teams, :organization_access - def initialize(repository:, roles:, ou:, teams: []) + def initialize(repository:, roles:, ou:, teams: [], organization_access: nil) Configuration.validate_repository!(repository) @repository = repository + @organization_access = organization_access @roles = {} @logins = {} roles.sort_by { |login, _| login.downcase }.each do |login, role| @@ -27,6 +28,7 @@ def initialize(repository:, roles:, ou:, teams: []) teams.each do |team| unless team.is_a?(Hash) && team[:id].is_a?(Integer) && team[:id].positive? && team[:slug].is_a?(String) && /\A[a-zA-Z0-9_-]+\z/.match?(team[:slug]) && + %w[direct organization enterprise].include?(team[:access_source]) && (team[:parent_id].nil? || (team[:parent_id].is_a?(Integer) && team[:parent_id].positive?)) GitHubRepository.fail!("Malformed repository team: #{team.inspect}") end @@ -41,7 +43,7 @@ def initialize(repository:, roles:, ou:, teams: []) end def ordered_teams - remaining = teams.dup + remaining = direct_teams.dup ordered = [] until remaining.empty? roots = remaining.values.reject { |team| remaining.key?(team[:parent_id]) }.sort_by { |team| team[:slug].downcase } @@ -51,6 +53,10 @@ def ordered_teams ordered end + def direct_teams + teams.select { |_, team| team[:access_source] == "direct" } + end + def role_for(login) roles[login.downcase] end @@ -60,7 +66,8 @@ def login_for(login) end def equals?(other) - other.is_a?(self.class) && dn.casecmp?(other.dn) && roles == other.roles && teams == other.teams + other.is_a?(self.class) && dn.casecmp?(other.dn) && roles == other.roles && + direct_teams == other.direct_teams && organization_access == other.organization_access end alias_method :==, :equals? diff --git a/lib/entitlements/backend/github_repository/provider.rb b/lib/entitlements/backend/github_repository/provider.rb index f6b50b7..5b1a8da 100644 --- a/lib/entitlements/backend/github_repository/provider.rb +++ b/lib/entitlements/backend/github_repository/provider.rb @@ -16,11 +16,20 @@ def action_for(desired, group_name) existing = @github.read_repository(desired.repository) current = existing.roles.reject { |login, _| ignored.include?(login) } target = desired.roles.reject { |login, _| ignored.include?(login) } + access = existing.organization_access + GitHubRepository.fail!("Missing organization access snapshot") unless access.is_a?(Models::OrganizationAccess) effective = current.dup instructions = [] (current.keys | target.keys).sort.each do |login| before = current[login] after = target[login] + sources = access.sources(login) + Entitlements.logger.info "#{desired.repository}: #{login} retains #{sources.join(', ')}" unless sources.empty? + floor = access.inherited_role(login) + if access.owner?(login) || (after && floor && ROLES.keys.index(after) < ROLES.keys.index(floor)) + Entitlements.logger.warn "DEFER #{desired.repository}: #{login} direct role #{after || '(none)'}; inherited #{floor} via #{sources.join(', ')}; direct grants unchanged" + next + end next if before == after feature = if before.nil? "add" @@ -46,14 +55,21 @@ def action_for(desired, group_name) instructions << { action: :remove_team, team_id: team[:id], slug: team[:slug] } Entitlements.logger.info "CHANGE #{desired.repository}: team #{@config.fetch('org')}/#{team[:slug]} (granted) -> (none)" end - teams = [] - elsif !teams.empty? + teams = teams.reject { |team| team[:access_source] == "direct" } + elsif !existing.direct_teams.empty? Entitlements.logger.warn("#{desired.repository}: remove disabled; individual-only repository grants are not enforced") end + existing.teams.each_value do |team| + next if team[:access_source] == "direct" + Entitlements.logger.info "#{desired.repository}: preserving #{team[:access_source]} access for team #{@config.fetch('org')}/#{team[:slug]}" + if team[:access_source] == "enterprise" + Entitlements.logger.warn "#{desired.repository}: enterprise team #{team[:slug]} is unmanaged; individual-only policy is not fully enforced" + end + end Entitlements.logger.info "#{desired.repository}: organization-level access and repository visibility are unchanged" return if instructions.empty? action = Entitlements::Models::Action.new(desired.dn, - snapshot(existing, current), snapshot(desired, effective, teams: teams), group_name, ignored_users: ignored) + snapshot(existing, current), snapshot(existing, effective, teams: teams), group_name, ignored_users: ignored) instructions.partition { |instruction| instruction[:action] == :upsert }.flatten.each do |instruction| action.add_implementation(instruction) end @@ -79,7 +95,8 @@ def commit(action) private def snapshot(source, roles, teams: source.teams.values) - Models::RepositoryAccess.new(repository: source.repository, roles: roles, teams: teams, ou: @config.fetch("base")) + Models::RepositoryAccess.new(repository: source.repository, roles: roles, teams: teams, + organization_access: source.organization_access, ou: @config.fetch("base")) end def filtered_snapshot(source, ignored) @@ -89,7 +106,7 @@ def filtered_snapshot(source, ignored) def validate_members(desired, ignored) invalid = desired.roles.keys - @github.active_members.keys - ignored.to_a return if invalid.empty? - message = "#{desired.repository}: not active non-owner organization members: #{invalid.join(', ')}" + message = "#{desired.repository}: not active organization members: #{invalid.join(', ')}" GitHubRepository.fail!(message) unless @config.fetch("ignore_not_found", false) Entitlements.logger.warn("#{message}; ignored") ignored.merge(invalid) diff --git a/lib/entitlements/backend/github_repository/service.rb b/lib/entitlements/backend/github_repository/service.rb index b0d542e..fb09aeb 100644 --- a/lib/entitlements/backend/github_repository/service.rb +++ b/lib/entitlements/backend/github_repository/service.rb @@ -5,11 +5,49 @@ class Backend class GitHubRepository class Service < Entitlements::Service::GitHub def active_members - # Never use predictive membership to authorize a new direct grant. - @active_members ||= begin - invalidate_org_members_predictive_cache - org_members.transform_keys(&:downcase).select { |_, role| role == "member" } + organization_access.members + end + + def organization_access(refresh: false) + @organization_access = nil if refresh + @organization_access ||= begin + # Use this installation's live view, never another backend's predictive/JIT cache. + members = members_and_roles_from_rest.transform_values(&:downcase) + organization = octokit.organization(org) + GitHubRepository.fail!("Missing organization access settings for #{org}") unless organization.is_a?(Sawyer::Resource) + base_role = organization[:default_repository_permission] + data = octokit.get("orgs/#{org}/organization-roles") + unless data.is_a?(Sawyer::Resource) && data[:roles].is_a?(Array) && + data[:total_count].is_a?(Integer) && data[:total_count] == data[:roles].length + GitHubRepository.fail!("Incomplete organization role catalog for #{org}") + end + assignments = {} + seen = Set.new + data[:roles].each do |role| + unless role.is_a?(Sawyer::Resource) && role[:id].is_a?(Integer) && role[:id].positive? && + seen.add?(role[:id]) && role[:name].is_a?(String) && !role[:name].empty? && + role.key?(:base_role) && (role[:base_role].nil? || ROLES.key?(role[:base_role])) && + role[:permissions].is_a?(Array) && role[:permissions].all? { |permission| permission.is_a?(String) } + GitHubRepository.fail!("Malformed organization role for #{org}") + end + grant = { id: role[:id], name: role[:name], base_role: role[:base_role], permissions: role[:permissions].sort.freeze }.freeze + users = octokit.paginate("orgs/#{org}/organization-roles/#{role[:id]}/users") + GitHubRepository.fail!("Malformed organization role assignments") unless users.is_a?(Array) + logins = Set.new + users.each do |user| + unless user.is_a?(Sawyer::Resource) && %w[direct indirect mixed].include?(user[:assignment]) + GitHubRepository.fail!("Malformed organization role assignee") + end + login = user[:login] + Configuration.validate_login!(login) + GitHubRepository.fail!("Duplicate organization role assignee: #{login}") unless logins.add?(login.downcase) + (assignments[login.downcase] ||= []) << grant + end + end + Models::OrganizationAccess.new(members: members, base_role: base_role, assignments: assignments) end + rescue Octokit::Error => e + GitHubRepository.fail!("Reading organization access for #{org} failed: #{e.message}") end def read_repository(repository, refresh: false) @@ -17,12 +55,13 @@ def read_repository(repository, refresh: false) @repositories ||= {} @repositories.delete(repository.downcase) if refresh @repositories[repository.downcase] ||= begin + access = organization_access(refresh: refresh) roles = {} cursor = nil cursors = Set.new loop do connection = collaborators(repository, cursor) - connection.fetch("edges").each { |edge| read_edge(edge, roles) } + connection.fetch("edges").each { |edge| read_edge(edge, roles, access) } page = connection.fetch("pageInfo") more = page.fetch("hasNextPage") GitHubRepository.fail!("Malformed repository pagination") unless [true, false].include?(more) @@ -32,7 +71,8 @@ def read_repository(repository, refresh: false) GitHubRepository.fail!("Missing or repeated repository pagination cursor") end end - Models::RepositoryAccess.new(repository: repository, roles: roles, teams: repository_teams(repository), ou: ou) + Models::RepositoryAccess.new(repository: repository, roles: roles, teams: repository_teams(repository), + organization_access: access, ou: ou) end rescue KeyError, TypeError => e GitHubRepository.fail!("Malformed repository response for #{repository}: #{e.message}") @@ -47,8 +87,11 @@ def apply(repository, instructions) end login = instruction.fetch(:login) Configuration.validate_login!(login) + if organization_access.owner?(login) + GitHubRepository.fail!("#{repository}: direct grants for owner #{login} are deferred; recalculate") + end if instruction.fetch(:action) == :upsert && !active_members.key?(login.downcase) - GitHubRepository.fail!("#{repository}: #{login} is not an active non-owner organization member") + GitHubRepository.fail!("#{repository}: #{login} is not an active organization member") end mutate(repository, instruction) end @@ -67,7 +110,12 @@ def repository_teams(repository) (team[:parent].nil? || (team[:parent].is_a?(Sawyer::Resource) && team[:parent][:id].is_a?(Integer))) GitHubRepository.fail!("Malformed repository team response for #{repository}") end - { id: team[:id], slug: team[:slug], parent_id: team[:parent]&.[](:id) } + source = team[:access_source] + unless %w[direct organization enterprise].include?(source) && %w[organization enterprise].include?(team[:type]) && + (source != "direct" || team[:type] == "organization") + GitHubRepository.fail!("Missing or unsupported repository team access_source") + end + { id: team[:id], slug: team[:slug], parent_id: team[:parent]&.[](:id), access_source: source } end rescue Octokit::Error => e GitHubRepository.fail!("Reading teams for #{org}/#{repository} failed: #{e.message}") @@ -79,6 +127,7 @@ def remove_team(repository, instruction) team = current.teams[instruction.fetch(:team_id)] return unless team GitHubRepository.fail!("Repository team identity changed") unless team[:slug] == instruction.fetch(:slug) + GitHubRepository.fail!("Repository team access source changed; recalculate") unless team[:access_source] == "direct" octokit.delete("orgs/#{org}/teams/#{team[:slug]}/repos/#{org}/#{repository}") GitHubRepository.fail!("Unexpected team removal response: HTTP #{octokit.last_response.status}") unless octokit.last_response.status == 204 rescue Octokit::Error => e @@ -112,8 +161,9 @@ def collaborators(repository, cursor) connection end - def read_edge(edge, roles) - unless edge.is_a?(Hash) && edge["node"].is_a?(Hash) && edge["permissionSources"].is_a?(Array) + def read_edge(edge, roles, access) + unless edge.is_a?(Hash) && edge["node"].is_a?(Hash) && + edge["permissionSources"].is_a?(Array) && !edge["permissionSources"].empty? GitHubRepository.fail!("Missing or malformed repository permission sources") end login = edge.fetch("node").fetch("login") @@ -126,9 +176,10 @@ def read_edge(edge, roles) unless %w[Repository Team Organization EnterpriseTeam].include?(type) GitHubRepository.fail!("Unknown repository permission source: #{type.inspect}") end - GitHubRepository.fail!("Unsupported enterprise-team access for #{login}") if type == "EnterpriseTeam" type == "Repository" end + # GitHub emits synthetic Repository admin sources for organization owners. + return if access.owner?(login) return if direct.empty? GitHubRepository.fail!("Ambiguous direct repository permissions for #{login}") unless direct.size == 1 role = direct.first.fetch("roleName") diff --git a/spec/unit/entitlements/backend/github_repository_spec.rb b/spec/unit/entitlements/backend/github_repository_spec.rb index 4dc850b..43d76d1 100644 --- a/spec/unit/entitlements/backend/github_repository_spec.rb +++ b/spec/unit/entitlements/backend/github_repository_spec.rb @@ -13,20 +13,40 @@ let(:service) { backend::Service.new(org: "example", token: "test-token", ou: base) } let(:members) { { "alice" => "member", "bob" => "member", "carol" => "member", "owner" => "admin" } } - def access(roles = {}, repository: "app", teams: [], **inline_roles) - backend::Models::RepositoryAccess.new(repository: repository, roles: roles.merge(inline_roles), teams: teams, ou: base) + def access(roles = {}, repository: "app", teams: [], organization_access: nil, **inline_roles) + backend::Models::RepositoryAccess.new(repository: repository, roles: roles.merge(inline_roles), teams: teams, + organization_access: organization_access || self.organization_access, ou: base) end - def team(id = 1, slug = "engineering", parent_id = nil) - { id: id, slug: slug, parent_id: parent_id } + def team(id = 1, slug = "engineering", parent_id = nil, source = "direct") + { id: id, slug: slug, parent_id: parent_id, access_source: source } end def stub_teams(teams = [], endpoint: "https://api.github.com/repos/example/app/teams") + teams = teams.map { |entry| { access_source: "direct", type: "organization" }.merge(entry) } stub_request(:get, endpoint).with(query: { per_page: 100 }) .to_return(status: 200, body: JSON.generate(teams), headers: { "Content-Type" => "application/json" }) end - before { stub_teams } + def organization_access(base_role: "none", assignments: {}, membership: members) + backend::Models::OrganizationAccess.new(members: membership, base_role: base_role, assignments: assignments) + end + + def organization_role(id = 10, base_role = "write", name = "all_repo_write") + { id: id, name: name, base_role: base_role, permissions: [] } + end + + def stub_organization(base_role: "none", roles: []) + stub_request(:get, "https://api.github.com/orgs/example") + .to_return(status: 200, body: JSON.generate(default_repository_permission: base_role), headers: { "Content-Type" => "application/json" }) + stub_request(:get, "https://api.github.com/orgs/example/organization-roles") + .to_return(status: 200, body: JSON.generate(total_count: roles.size, roles: roles), headers: { "Content-Type" => "application/json" }) + end + + before do + stub_teams + stub_organization + end def edge(login, role = "write", sources: nil) { "node" => { "login" => login }, "permission" => "ADMIN", @@ -92,6 +112,30 @@ def stub_page(body, endpoint: "https://api.github.com/graphql") expect(model.equals?(:none)).to be(false) end + describe "organization access model" do + it "combines base permissions, ownership and arbitrary assigned roles without matching names" do + role = organization_role(10, "maintain", "enterprise-defined-role") + context = organization_access(base_role: "read", assignments: { "ALICE" => [role] }) + expect(context.inherited_role("alice")).to eq("maintain") + expect(context.inherited_role("bob")).to eq("read") + expect(context.inherited_role("owner")).to eq("admin") + expect(context.inherited_role("outsider")).to be_nil + expect(context.sources("ALICE")).to include('organization role "enterprise-defined-role"', "organization base read") + expect(context.sources("owner")).to include("organization ownership") + expect(context).to eq(organization_access(base_role: "read", assignments: { "alice" => [role] })) + expect(context).not_to eq(organization_access) + expect(context).not_to eq(nil) + expect(organization_access.inherited_role("alice")).to be_nil + end + + it "rejects unavailable base settings or unsupported organization membership roles" do + [nil, "custom"].each do |role| + expect { organization_access(base_role: role) }.to raise_error(backend::Error, /Malformed organization access/) + end + expect { organization_access(membership: { "alice" => "unknown" }) }.to raise_error(backend::Error) + end + end + ["", ".", "..", "bad/repo", "bad repo", "a" * 101, nil].each do |name| it "rejects repository name #{name.inspect}" do expect { access({}, repository: name) }.to raise_error(backend::Error, /repository name/) @@ -207,7 +251,7 @@ def filtered?(person) let(:provider) { backend::Provider.new(config: config) } before do allow(backend::Service).to receive(:new).and_return(service) - allow(service).to receive(:active_members).and_return(members.reject { |_, role| role == "admin" }) + allow(service).to receive(:active_members).and_return(members) end described_class::FEATURES.length.succ.times.flat_map { |size| described_class::FEATURES.combination(size).to_a }.each do |features| @@ -240,10 +284,9 @@ def filtered?(person) expect(provider.action_for(access("ALICE" => "read", "owner" => "write"), "repos")).to be_nil end - it "rejects desired non-members and owners before reading a repository" do + it "rejects desired non-members before reading a repository" do expect(service).not_to receive(:read_repository) expect { provider.action_for(access("outsider" => "read"), "repos") }.to raise_error(backend::Error, /not active/) - expect { provider.action_for(access("owner" => "read"), "repos") }.to raise_error(backend::Error, /not active/) end it "warns and ignores non-members when explicitly configured" do @@ -283,6 +326,12 @@ def filtered?(person) expect { provider.commit(action) }.to raise_error(backend::Error, /Invalid repository action/) end + it "rejects observed state without organization access metadata" do + missing = backend::Models::RepositoryAccess.new(repository: "app", roles: {}, ou: base) + allow(service).to receive(:read_repository).and_return(missing) + expect { provider.action_for(access, "repos") }.to raise_error(backend::Error, /Missing organization access snapshot/) + end + it "calculates and counts team-only actions without pretending teams are users" do desired = access allow(backend::Configuration).to receive(:new).and_return(instance_double(backend::Configuration, load: [desired])) @@ -305,10 +354,10 @@ def filtered?(person) expect(action.implementation.map { |instruction| instruction[:action] }).to eq([:upsert]) end - it "removes undeclared outside and owner direct grants as well as all teams" do - allow(service).to receive(:read_repository).and_return(access("outsider" => "read", "owner" => "admin", :teams => [team])) + it "removes undeclared outside direct grants as well as all direct teams" do + allow(service).to receive(:read_repository).and_return(access("outsider" => "read", :teams => [team])) action = provider.action_for(access("alice" => "read"), "repos") - expect(action.implementation.map { |instruction| instruction[:action] }).to eq([:upsert, :remove, :remove, :remove_team]) + expect(action.implementation.map { |instruction| instruction[:action] }).to eq([:upsert, :remove, :remove_team]) expect(action.updated.roles).to eq("alice" => "read") end @@ -323,6 +372,71 @@ def filtered?(person) expect(service).to receive(:apply).with("app", action.implementation) expect { provider.commit(action) }.to raise_error(backend::Error, /did not converge/) end + + it "accepts desired owners without ignore_not_found and defers their ambiguous direct grants" do + allow(service).to receive(:read_repository).and_return(access(teams: [team], organization_access: organization_access)) + expect(logger).to receive(:warn).with(/DEFER app: owner.*inherited admin.*organization ownership/) + action = provider.action_for(access("owner" => "read"), "repos") + expect(action.implementation).to eq([{ action: :remove_team, team_id: 1, slug: "engineering" }]) + expect(action.ignored_users).to be_empty + end + + described_class::ROLES.each_key do |role| + it "handles all-repository #{role} assignments and provisions equal direct grants" do + inherited = organization_access(assignments: { "alice" => [organization_role(10, role, "arbitrary-#{role}")] }) + allow(service).to receive(:read_repository).and_return(access(organization_access: inherited)) + action = provider.action_for(access("alice" => role), "repos") + expect(action.implementation).to eq([{ action: :upsert, login: "alice", permission: backend::ROLES.fetch(role) }]) + end + end + + it "defers lower desired roles without inventing a successful direct grant or raising inherited privileges" do + inherited = organization_access(assignments: { "alice" => [organization_role] }) + current = access({ "alice" => "admin" }, teams: [team], organization_access: inherited) + allow(service).to receive(:read_repository).and_return(current) + expect(logger).to receive(:warn).with(/DEFER app: alice direct role read; inherited write/) + action = provider.action_for(access("alice" => "read"), "repos") + expect(action.updated.roles).to eq("alice" => "admin") + expect(action.implementation.map { |entry| entry[:action] }).to eq([:remove_team]) + end + + it "defers roles below organization base and continues to provision users above the base" do + allow(service).to receive(:read_repository).and_return(access(organization_access: organization_access(base_role: "write"))) + expect(logger).to receive(:warn).with(/DEFER app: alice.*organization base write/) + action = provider.action_for(access("alice" => "read", "bob" => "admin"), "repos") + expect(action.updated.roles).to eq("bob" => "admin") + end + + it "removes undeclared direct grants even when a non-owner retains organization-wide access" do + inherited = organization_access(assignments: { "alice" => [organization_role] }) + allow(service).to receive(:read_repository).and_return(access({ "alice" => "admin" }, organization_access: inherited)) + action = provider.action_for(access, "repos") + expect(action.implementation).to eq([{ action: :remove, login: "alice" }]) + end + + it "preserves organization and enterprise team sources while removing a direct association" do + teams = [team, team(2, "security", nil, "organization"), team(3, "enterprise", nil, "enterprise")] + allow(service).to receive(:read_repository).and_return(access(teams: teams, organization_access: organization_access)) + action = provider.action_for(access, "repos") + expect(action.implementation).to eq([{ action: :remove_team, team_id: 1, slug: "engineering" }]) + expect(action.updated.teams.keys).to eq([2, 3]) + # A direct association can mask an organization-wide source for the same team. + expect(action.updated).to eq(access(teams: teams.map { |entry| entry.merge(access_source: "organization") }, + organization_access: organization_access)) + end + + it "plans a direct grant after owner JIT expires and rejects plans if organization access changes" do + elevated = organization_access + demoted = organization_access(membership: members.merge("owner" => "member")) + allow(service).to receive(:read_repository).and_return(access(organization_access: elevated)) + expect(provider.action_for(access("owner" => "read"), "repos")).to be_nil + allow(service).to receive(:read_repository).and_return(access(organization_access: demoted)) + action = provider.action_for(access("owner" => "read"), "repos") + expect(action.implementation).to eq([{ action: :upsert, login: "owner", permission: "pull" }]) + allow(service).to receive(:read_repository).with("app", refresh: true).and_return(access(organization_access: elevated)) + expect(service).not_to receive(:apply) + expect { provider.commit(action) }.to raise_error(backend::Error, /changed since calculation/) + end end describe "end-to-end reconciliation" do @@ -348,7 +462,7 @@ def filtered?(person) put = stub_request(:put, "https://api.github.com/repos/example/app/collaborators/balinese") .with(body: { permission: "push" }).to_return(status: 204) delete = stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/bob").to_return(status: 204) - %w[outsider owner].each do |login| + %w[outsider].each do |login| stub_request(:delete, "https://api.github.com/repos/example/app/collaborators/#{login}").to_return(status: 204) end remove_team = stub_request(:delete, "https://api.github.com/orgs/example/teams/engineering/repos/example/app").to_return do @@ -361,27 +475,124 @@ def filtered?(person) controller = backend::Controller.new("repos", config.merge("dir" => root)) actions = controller.calculate expect(actions.size).to eq(1) - expect(actions.first.implementation.size).to eq(5) + expect(actions.first.implementation.size).to eq(4) controller.apply(actions.first) expect(controller.calculate).to eq([]) end expect(put).to have_been_requested.once expect(delete).to have_been_requested.once - expect(members_request).to have_been_requested.once + expect(members_request).to have_been_requested.times(3) expect(remove_team).to have_been_requested.once expect(a_request(:delete, "https://api.github.com/repos/example/app/collaborators/carol")).not_to have_been_made + expect(a_request(:delete, "https://api.github.com/repos/example/app/collaborators/owner")).not_to have_been_made end end describe "GitHub transport" do before do - allow(service).to receive(:org_members).and_return(members) - allow(service).to receive(:org_members_from_predictive_cache?).and_return(false) + allow(service).to receive(:members_and_roles_from_rest).and_return(members.transform_values(&:upcase)) + end + + it "uses live installation-specific organization membership and accepts owners" do + expect(service).not_to receive(:org_members) + expect(service.active_members).to eq(members) + end + + it "reads all catalog roles and paginates direct, indirect and mixed user assignments" do + roles = [organization_role, organization_role(11, nil, "custom-org-capabilities"), + organization_role(12, "read", "security_manager")] + stub_organization(roles: roles) + stub_request(:get, "https://api.github.com/orgs/example/organization-roles/10/users").with(query: { per_page: 100 }) + .to_return(status: 200, body: '[{"login":"ALICE","assignment":"direct"}]', + headers: { "Content-Type" => "application/json", "Link" => '; rel="next"' }) + stub_request(:get, "https://api.github.com/orgs/example/organization-roles/10/users").with(query: { per_page: 100, page: 2 }) + .to_return(status: 200, body: '[{"login":"bob","assignment":"indirect"},{"login":"carol","assignment":"mixed"}]', + headers: { "Content-Type" => "application/json" }) + [11, 12].each do |id| + stub_request(:get, "https://api.github.com/orgs/example/organization-roles/#{id}/users").with(query: { per_page: 100 }) + .to_return(status: 200, body: '[{"login":"alice","assignment":"indirect"}]', headers: { "Content-Type" => "application/json" }) + end + context = service.organization_access + expect(context.assignments["alice"].size).to eq(3) + expect(context.inherited_role("alice")).to eq("write") + expect(context.inherited_role("bob")).to eq("write") + expect(context.inherited_role("carol")).to eq("write") + expect(context.sources("alice")).to include('organization role "security_manager"', 'organization role "custom-org-capabilities"') + end + + it "fails closed when organization settings, role catalog or assignments are unavailable" do + stub_request(:get, "https://api.github.com/orgs/example").to_return(status: 200, body: "null", + headers: { "Content-Type" => "application/json" }) + expect { service.organization_access }.to raise_error(backend::Error, /Missing organization access/) + stub_organization(base_role: nil) + expect { service.organization_access }.to raise_error(backend::Error, /Malformed organization access/) + stub_organization + ["{}", '{"roles":[],"total_count":1}', '{"roles":null,"total_count":0}'].each do |body| + stub_request(:get, "https://api.github.com/orgs/example/organization-roles") + .to_return(status: 200, body: body, headers: { "Content-Type" => "application/json" }) + expect { service.organization_access }.to raise_error(backend::Error, /Incomplete/) + end + [403, 404].each do |status| + stub_request(:get, "https://api.github.com/orgs/example/organization-roles").to_return(status: status) + expect { service.organization_access }.to raise_error(backend::Error, /Reading organization access/) + end + end + + it "rejects malformed or unsupported roles and malformed or duplicate assignees" do + [organization_role(0), organization_role(10, "unknown"), organization_role.merge(permissions: [nil]), + organization_role.reject { |key, _| key == :base_role }].each do |role| + stub_organization(roles: [role]) + expect { service.organization_access }.to raise_error(backend::Error, /Malformed organization role/) + end + stub_organization(roles: [organization_role]) + ["{}", "[{}]", '[{"login":"alice","assignment":"unknown"}]', + '[{"login":"alice","assignment":"direct"},{"login":"ALICE","assignment":"indirect"}]'].each do |body| + stub_request(:get, "https://api.github.com/orgs/example/organization-roles/10/users").with(query: { per_page: 100 }) + .to_return(status: 200, body: body, headers: { "Content-Type" => "application/json" }) + expect { service.organization_access }.to raise_error(backend::Error) + end + end + + it "does not treat synthetic owner Repository grants as removable direct grants" do + sources = [{ "source" => { "__typename" => "Organization" }, "roleName" => nil }, + { "source" => { "__typename" => "Repository" }, "roleName" => "admin" }, + { "source" => { "__typename" => "Repository" }, "roleName" => "read" }] + stub_page(page([edge("owner", sources: sources)])) + expect(service.read_repository("app").roles).to be_empty + end + + it "rejects owner mutations even if an invalid instruction bypassed the planner" do + [:upsert, :remove].each do |action| + expect { service.apply("app", [{ action: action, login: "owner", permission: "pull" }]) } + .to raise_error(backend::Error, /owner.*deferred/) + end + expect(a_request(:put, /collaborators/)).not_to have_been_made + expect(a_request(:delete, /collaborators/)).not_to have_been_made + end + + it "preserves enterprise permission sources and reads only an accompanying explicit user grant" do + sources = [{ "source" => { "__typename" => "EnterpriseTeam" }, "roleName" => "admin" }, + { "source" => { "__typename" => "Repository" }, "roleName" => "read" }] + stub_page(page([edge("alice", sources: sources)])) + stub_teams([{ id: 9, slug: "enterprise", parent: nil, type: "enterprise", access_source: "enterprise" }]) + snapshot = service.read_repository("app") + expect(snapshot.roles).to eq("alice" => "read") + expect(snapshot.direct_teams).to be_empty + end + + it "rejects team lists without source metadata instead of guessing that grants are direct" do + stub_page(page([])) + stub_teams([{ id: 1, slug: "team", parent: nil, access_source: nil }]) + expect { service.read_repository("app") }.to raise_error(backend::Error, /access_source/) + stub_teams([{ id: 1, slug: "team", parent: nil, type: "enterprise" }]) + expect { service.read_repository("app") }.to raise_error(backend::Error, /access_source/) end - it "uses the organization membership cache and excludes owners" do - expect(service).to receive(:invalidate_org_members_predictive_cache) - expect(service.active_members).to eq(members.reject { |_, role| role == "admin" }) + it "refuses to delete a team whose source became organization-wide" do + stub_teams([{ id: 1, slug: "team", parent: nil, access_source: "organization" }]) + expect { service.apply("app", [{ action: :remove_team, team_id: 1, slug: "team" }]) } + .to raise_error(backend::Error, /access source changed/) + expect(a_request(:delete, /teams/)).not_to have_been_made end it "paginates, uses direct roles instead of effective permissions, and caches per repository" do @@ -391,8 +602,8 @@ def filtered?(person) request = stub_request(:post, "https://api.github.com/graphql") .with(headers: { "Authorization" => "bearer test-token" }) .to_return({ status: 200, body: JSON.generate(first) }, { status: 200, body: JSON.generate(second) }) - expect(service.read_repository("app").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write", "owner" => "write") - expect(service.read_repository("APP").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write", "owner" => "write") + expect(service.read_repository("app").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write") + expect(service.read_repository("APP").roles).to eq("alice" => "read", "carol" => "maintain", "outsider" => "write") expect(request).to have_been_requested.twice expect(a_request(:post, "https://api.github.com/graphql").with { |req| JSON.parse(req.body).fetch("query").include?('after: "a\\"b"') @@ -441,7 +652,7 @@ def filtered?(person) edge("alice", sources: [nil]), edge("alice", sources: [{ "source" => {} }]), edge("alice", sources: [{ "source" => { "__typename" => nil } }]), - edge("alice", sources: [{ "source" => { "__typename" => "EnterpriseTeam" } }]), + edge("alice", sources: []), edge("alice", sources: [edge("alice")["permissionSources"].first] * 2), edge("../alice") ].each do |invalid| @@ -567,6 +778,7 @@ def filtered?(person) it "uses GHES REST and GraphQL API paths" do enterprise = backend::Service.new(org: "example", token: "test-token", ou: base, addr: "https://github.test/api/v3/") allow(enterprise).to receive(:active_members).and_return(members) + allow(enterprise).to receive(:organization_access).and_return(organization_access) stub_teams([], endpoint: "https://github.test/api/v3/repos/example/app/teams") stub_page(page([edge("alice")]), endpoint: "https://github.test/api/graphql") expect(enterprise.read_repository("app").role_for("alice")).to eq("write") @@ -578,10 +790,10 @@ def filtered?(person) it "paginates repository teams including empty teams, independent of collaborators" do stub_page(page([])) stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100 }) - .to_return(status: 200, body: '[{"id":1,"slug":"empty","parent":null}]', + .to_return(status: 200, body: '[{"id":1,"slug":"empty","parent":null,"type":"organization","access_source":"direct"}]', headers: { "Content-Type" => "application/json", "Link" => '; rel="next"' }) stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100, page: 2 }) - .to_return(status: 200, body: '[{"id":2,"slug":"child","parent":{"id":1}}]', headers: { "Content-Type" => "application/json" }) + .to_return(status: 200, body: '[{"id":2,"slug":"child","parent":{"id":1},"type":"organization","access_source":"direct"}]', headers: { "Content-Type" => "application/json" }) snapshot = service.read_repository("app") expect(snapshot.roles).to be_empty expect(snapshot.ordered_teams).to eq([team(1, "empty"), team(2, "child", 1)]) @@ -589,7 +801,8 @@ def filtered?(person) it "rejects inaccessible or malformed repository team lists" do stub_page(page([])) - ["{}", "[{}]", '[{"id":1,"slug":"team","parent":{}}]', '[{"id":0,"slug":"team","parent":null}]'].each do |body| + ["{}", "[{}]", '[{"id":1,"slug":"team","parent":{}}]', + '[{"id":0,"slug":"team","parent":null,"type":"organization","access_source":"direct"}]'].each do |body| stub_request(:get, "https://api.github.com/repos/example/app/teams").with(query: { per_page: 100 }) .to_return(status: 200, body: body, headers: { "Content-Type" => "application/json" }) expect { service.read_repository("app") }.to raise_error(backend::Error, /Malformed/)