From 7f4ede52d613777258ece1c6c587e6eb16cdf30a Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 11:41:28 -0500 Subject: [PATCH 1/7] Support ignoring 404s when user is not found in GitHub instance --- .../backend/github_org/controller.rb | 13 +++-- .../backend/github_org/provider.rb | 3 +- .../backend/github_org/service.rb | 10 +++- .../backend/github_team/controller.rb | 3 +- .../backend/github_team/provider.rb | 3 +- .../backend/github_team/service.rb | 5 +- lib/entitlements/service/github.rb | 8 ++- .../backend/github_org/controller_spec.rb | 52 +++++++++--------- .../backend/github_org/provider_spec.rb | 3 +- .../backend/github_org/service_spec.rb | 55 ++++++++++++++++++- .../backend/github_team/controller_spec.rb | 9 +-- .../backend/github_team/provider_spec.rb | 3 +- .../backend/github_team/service_spec.rb | 3 +- spec/unit/entitlements/service/github_spec.rb | 6 +- 14 files changed, 125 insertions(+), 51 deletions(-) diff --git a/lib/entitlements/backend/github_org/controller.rb b/lib/entitlements/backend/github_org/controller.rb index f4c5be0..3f599b1 100644 --- a/lib/entitlements/backend/github_org/controller.rb +++ b/lib/entitlements/backend/github_org/controller.rb @@ -120,12 +120,13 @@ def apply(action) Contract String, C::HashOf[String => C::Any] => nil def validate_config!(key, data) spec = COMMON_GROUP_CONFIG.merge({ - "base" => { required: true, type: String }, - "addr" => { required: false, type: String }, - "org" => { required: true, type: String }, - "token" => { required: true, type: String }, - "features" => { required: false, type: Array }, - "ignore" => { required: false, type: Array } + "base" => { required: true, type: String }, + "addr" => { required: false, type: String }, + "org" => { required: true, type: String }, + "token" => { required: true, type: String }, + "features" => { required: false, type: Array }, + "ignore" => { required: false, type: Array }, + "ignore_not_found" => { required: false, type: [FalseClass, TrueClass] }, }) text = "GitHub organization group #{key.inspect}" Entitlements::Util::Util.validate_attr!(spec, data, text) diff --git a/lib/entitlements/backend/github_org/provider.rb b/lib/entitlements/backend/github_org/provider.rb index c25a0b9..7f5de3b 100644 --- a/lib/entitlements/backend/github_org/provider.rb +++ b/lib/entitlements/backend/github_org/provider.rb @@ -25,7 +25,8 @@ def initialize(config:) org: config.fetch("org"), addr: config.fetch("addr", nil), token: config.fetch("token"), - ou: config.fetch("base") + ou: config.fetch("base"), + ignore_not_found: config.fetch("ignore_not_found", false) ) @role_cache = {} end diff --git a/lib/entitlements/backend/github_org/service.rb b/lib/entitlements/backend/github_org/service.rb index 2c57482..0e7e760 100644 --- a/lib/entitlements/backend/github_org/service.rb +++ b/lib/entitlements/backend/github_org/service.rb @@ -44,7 +44,15 @@ def sync(implementation, role) Contract String, String => C::Bool def add_user_to_organization(user, role) Entitlements.logger.debug "#{identifier} add_user_to_organization(user=#{user}, org=#{org}, role=#{role})" - new_membership = octokit.update_organization_membership(org, user:, role:) + + begin + new_membership = octokit.update_organization_membership(org, user:, role:) + rescue Octokit::NotFound => e + raise e unless ignore_not_found + + Entitlements.logger.warn "User #{user} not found in GitHub instance #{identifier}, ignoring." + return false + end # Happy path if new_membership[:role] == role diff --git a/lib/entitlements/backend/github_team/controller.rb b/lib/entitlements/backend/github_team/controller.rb index d5df8ec..c8402e4 100644 --- a/lib/entitlements/backend/github_team/controller.rb +++ b/lib/entitlements/backend/github_team/controller.rb @@ -110,7 +110,8 @@ def validate_config!(key, data) "base" => { required: true, type: String }, "addr" => { required: false, type: String }, "org" => { required: true, type: String }, - "token" => { required: true, type: String } + "token" => { required: true, type: String }, + "ignore_not_found" => { required: false, type: [FalseClass, TrueClass] }, }) text = "GitHub group #{key.inspect}" Entitlements::Util::Util.validate_attr!(spec, data, text) diff --git a/lib/entitlements/backend/github_team/provider.rb b/lib/entitlements/backend/github_team/provider.rb index 532b329..a30f856 100644 --- a/lib/entitlements/backend/github_team/provider.rb +++ b/lib/entitlements/backend/github_team/provider.rb @@ -23,7 +23,8 @@ def initialize(config:) org: config.fetch("org"), addr: config.fetch("addr", nil), token: config.fetch("token"), - ou: config.fetch("base") + ou: config.fetch("base"), + ignore_not_found: config.fetch("ignore_not_found", false) ) @github_team_cache = {} diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index 05dbc29..8fbeee3 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -28,9 +28,10 @@ class TeamNotFound < RuntimeError; end addr: C::Maybe[String], org: String, token: String, - ou: String + ou: String, + ignore_not_found: C::Bool, ] => C::Any - def initialize(addr: nil, org:, token:, ou:) + def initialize(addr: nil, org:, token:, ou:, ignore_not_found: false) super Entitlements.cache[:github_team_members] ||= {} Entitlements.cache[:github_team_members][org] ||= {} diff --git a/lib/entitlements/service/github.rb b/lib/entitlements/service/github.rb index 33f7266..1b02c9c 100644 --- a/lib/entitlements/service/github.rb +++ b/lib/entitlements/service/github.rb @@ -17,7 +17,7 @@ class GitHub MAX_GRAPHQL_RETRIES = 3 WAIT_BETWEEN_GRAPHQL_RETRIES = 1 - attr_reader :addr, :org, :token, :ou + attr_reader :addr, :org, :token, :ou, :ignore_not_found # Constructor. # @@ -31,14 +31,16 @@ class GitHub addr: C::Maybe[String], org: String, token: String, - ou: String + ou: String, + ignore_not_found: C::Bool, ] => C::Any - def initialize(addr: nil, org:, token:, ou:) + def initialize(addr: nil, org:, token:, ou:, ignore_not_found: false) # Save some parameters for the connection but don't actually connect yet. @addr = addr @org = org @token = token @ou = ou + @ignore_not_found = ignore_not_found # This is a global cache across all invocations of this object. GitHub membership # need to be obtained only one time per organization, but might be used multiple times. diff --git a/spec/unit/entitlements/backend/github_org/controller_spec.rb b/spec/unit/entitlements/backend/github_org/controller_spec.rb index 0cbb69e..69c0828 100644 --- a/spec/unit/entitlements/backend/github_org/controller_spec.rb +++ b/spec/unit/entitlements/backend/github_org/controller_spec.rb @@ -9,10 +9,11 @@ let(:backend_config) { base_backend_config } let(:base_backend_config) do { - "org" => "kittensinc", - "token" => "CuteAndCuddlyKittens", - "type" => "github_org", - "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com" + "org" => "kittensinc", + "token" => "CuteAndCuddlyKittens", + "type" => "github_org", + "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com", + "ignore_not_found" => false } end let(:group_name) { "foo-githuborg" } @@ -98,9 +99,10 @@ it "logs expected output and returns expected actions" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) .with("foo-githuborg", { - "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com", - "org" => "kittensinc", - "token" => "CuteAndCuddlyKittens" + "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com", + "org" => "kittensinc", + "token" => "CuteAndCuddlyKittens", + "ignore_not_found" => false }).and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -179,7 +181,8 @@ end it "logs expected output and returns expected actions" do - allow(Entitlements::Data::Groups::Calculated).to receive(:read_all).with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -263,7 +266,8 @@ end it "logs expected output and returns expected actions" do - allow(Entitlements::Data::Groups::Calculated).to receive(:read_all).with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -328,7 +332,7 @@ it "does not run actions" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -374,7 +378,7 @@ it "handles removals and role changes but does not invite" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[remove], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[remove], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -437,7 +441,7 @@ it "reports as a no-op" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[remove], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[remove], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -486,7 +490,7 @@ it "handles removals and role changes but does not invite" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[invite], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[invite], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -555,7 +559,7 @@ it "reports as a no-op" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[invite], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>%w[invite], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -591,9 +595,8 @@ cache[:predictive_state] = { by_dn: { org1_admin_dn => { members: admins, metadata: nil }, org1_member_dn => { members:, metadata: nil } }, invalid: Set.new } allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", { - "base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens" - }).and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) + .with("foo-githuborg", { "base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) + .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -663,7 +666,7 @@ it "handles removals and role changes but does not invite" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>[], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>[], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -726,7 +729,7 @@ it "reports as a no-op" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>[], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "features"=>[], "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_admin_dn).and_return(org1_admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(org1_member_dn).and_return(org1_member_group) @@ -837,7 +840,7 @@ describe "#validate_github_org_ous!" do it "raises if an admin or member group is missing" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) github_double = instance_double(Entitlements::Backend::GitHubOrg::Provider) @@ -857,7 +860,7 @@ dns = %w[admin member kittens cats].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" } allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(dns)) allow(Entitlements::Backend::GitHubOrg::Service).to receive(:new).and_return(service) @@ -897,11 +900,8 @@ it "raises due to duplicate users" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githuborg", { - "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com", - "org" => "kittensinc", - "token" => "CuteAndCuddlyKittens" - }).and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) + .with("foo-githuborg", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) + .and_return(Set.new(%w[admin member].map { |cn| "cn=#{cn},ou=kittensinc,ou=GitHub,dc=github,dc=com" })) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(admin_dn).and_return(admin_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(member_dn).and_return(member_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with(member_dn).and_return(member_group) diff --git a/spec/unit/entitlements/backend/github_org/provider_spec.rb b/spec/unit/entitlements/backend/github_org/provider_spec.rb index 7a02ab1..b0e91f3 100644 --- a/spec/unit/entitlements/backend/github_org/provider_spec.rb +++ b/spec/unit/entitlements/backend/github_org/provider_spec.rb @@ -7,7 +7,8 @@ addr: "https://github.fake/api/v3", org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false } end diff --git a/spec/unit/entitlements/backend/github_org/service_spec.rb b/spec/unit/entitlements/backend/github_org/service_spec.rb index 791f4c8..ca136fc 100644 --- a/spec/unit/entitlements/backend/github_org/service_spec.rb +++ b/spec/unit/entitlements/backend/github_org/service_spec.rb @@ -11,7 +11,8 @@ addr: "https://github.fake/api/v3", org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false ) end @@ -148,6 +149,58 @@ result = subject.send(:add_user_to_organization, "bob", "admin") expect(result).to eq(false) end + + context "ignore_not_found is false" do + it "raises when user is not found" do + expect(logger).to receive(:debug).with("github.fake add_user_to_organization(user=bob, org=kittensinc, role=admin)") + expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") + + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return( + status: 404, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate({ + "message" => "Not Found", + "documentation_url" => "https://docs.github.com/rest" + }) + ) + + expect { subject.send(:add_user_to_organization, "bob", "admin") }.to raise_error(Octokit::NotFound) + end + end + + context "ignore_not_found is true" do + let(:subject) do + described_class.new( + addr: "https://github.fake/api/v3", + org: "kittensinc", + token: "GoPackGo", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: true + ) + end + + it "ignores 404s" do + expect(logger).to receive(:debug).with("github.fake add_user_to_organization(user=bob, org=kittensinc, role=admin)") + expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") + expect(logger).to receive(:warn).with("User bob not found in GitHub instance github.fake, ignoring.") + + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return( + status: 404, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate({ + "message" => "Not Found", + "documentation_url" => "https://docs.github.com/rest" + }) + ) + + result = subject.send(:add_user_to_organization, "bob", "admin") + expect(result).to eq(false) + end + end end end diff --git a/spec/unit/entitlements/backend/github_team/controller_spec.rb b/spec/unit/entitlements/backend/github_team/controller_spec.rb index 0bbd7a9..b0a0ea4 100644 --- a/spec/unit/entitlements/backend/github_team/controller_spec.rb +++ b/spec/unit/entitlements/backend/github_team/controller_spec.rb @@ -11,7 +11,8 @@ "org" => "kittensinc", "token" => "CuteAndCuddlyKittens", "type" => "github_team", - "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com" + "base" => "ou=kittensinc,ou=GitHub,dc=github,dc=com", + "ignore_not_found" => false } end let(:group_name) { "foo-githubteam" } @@ -110,7 +111,7 @@ it "logs expected output and returns expected actions" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[snowshoes russian-blues])) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with("snowshoes").and_return(snowshoe_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with("russian-blues").and_return(russian_blue_group) @@ -158,7 +159,7 @@ it "does not run actions if there are no diffs detected" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[russian-blues])) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with("russian-blues").and_return(russian_blue_group) allow(Entitlements::Util::Util).to receive(:dns_for_ou).with("foo-githubteam", anything).and_return([russian_blue_group.dn]) @@ -204,7 +205,7 @@ it "logs expected output and returns expected actions" do allow(Entitlements::Data::Groups::Calculated).to receive(:read_all) - .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens"}) + .with("foo-githubteam", {"base"=>"ou=kittensinc,ou=GitHub,dc=github,dc=com", "org"=>"kittensinc", "token"=>"CuteAndCuddlyKittens", "ignore_not_found"=>false}) .and_return(Set.new(%w[snowshoes russian-blues])) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with("snowshoes").and_return(snowshoe_group) allow(Entitlements::Data::Groups::Calculated).to receive(:read).with("russian-blues").and_return(russian_blue_group) diff --git a/spec/unit/entitlements/backend/github_team/provider_spec.rb b/spec/unit/entitlements/backend/github_team/provider_spec.rb index 63d5251..d8d7f14 100644 --- a/spec/unit/entitlements/backend/github_team/provider_spec.rb +++ b/spec/unit/entitlements/backend/github_team/provider_spec.rb @@ -7,7 +7,8 @@ addr: "https://github.fake/api/v3", org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false } end diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index a74df40..f8c9470 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -11,7 +11,8 @@ addr: "https://github.fake/api/v3", org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false ) end diff --git a/spec/unit/entitlements/service/github_spec.rb b/spec/unit/entitlements/service/github_spec.rb index 50b1bf3..32fe174 100644 --- a/spec/unit/entitlements/service/github_spec.rb +++ b/spec/unit/entitlements/service/github_spec.rb @@ -8,7 +8,8 @@ addr: "https://github.fake/api/v3", org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false ) end @@ -17,7 +18,8 @@ subject = described_class.new( org: "kittensinc", token: "GoPackGo", - ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake" + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false ) expect(subject.identifier).to eq("github.com") end From 6f5348f1e68b607b723139dc81aa28bf588a14ec Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 11:51:05 -0500 Subject: [PATCH 2/7] Make the linter happy --- spec/unit/entitlements/backend/github_org/service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/entitlements/backend/github_org/service_spec.rb b/spec/unit/entitlements/backend/github_org/service_spec.rb index ca136fc..6aebce7 100644 --- a/spec/unit/entitlements/backend/github_org/service_spec.rb +++ b/spec/unit/entitlements/backend/github_org/service_spec.rb @@ -185,7 +185,7 @@ expect(logger).to receive(:debug).with("github.fake add_user_to_organization(user=bob, org=kittensinc, role=admin)") expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") expect(logger).to receive(:warn).with("User bob not found in GitHub instance github.fake, ignoring.") - + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return( status: 404, headers: { @@ -196,7 +196,7 @@ "documentation_url" => "https://docs.github.com/rest" }) ) - + result = subject.send(:add_user_to_organization, "bob", "admin") expect(result).to eq(false) end From ab12625f00ed8a41d08ce10f5e099e177f3a66c2 Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:03:51 -0500 Subject: [PATCH 3/7] Support ignoring 404s when user is not found in GitHub instance --- .../backend/github_team/service.rb | 12 +++- .../backend/github_team/service_spec.rb | 65 +++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index 8fbeee3..fb531ab 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -437,8 +437,16 @@ def add_user_to_team(user:, team:, role: "member") end Entitlements.logger.debug "#{identifier} add_user_to_team(user=#{user}, org=#{org}, team_id=#{team.team_id}, role=#{role})" validate_team_id_and_slug!(team.team_id, team.team_name) - result = octokit.add_team_membership(team.team_id, user, role:) - result[:state] == "active" || result[:state] == "pending" + + begin + result = octokit.add_team_membership(team.team_id, user, role:) + result[:state] == "active" || result[:state] == "pending" + rescue Octokit::NotFound => e + raise e unless ignore_not_found + + Entitlements.logger.warn "User #{user} not found in GitHub instance #{identifier}, ignoring." + false + end end # Remove user from team. diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index f8c9470..8ac9699 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -580,6 +580,71 @@ result = subject.send(:add_user_to_team, user: "blackmanx", team:) expect(result).to eq(false) end + + context "ignore_not_found is false" do + it "raises when user is not found" do + expect(subject).to receive(:validate_team_id_and_slug!).with(1001, "russian-blues").and_return(true) + expect(subject).to receive(:org_members).and_return(Set.new(%w[blackmanx])) + + add_membership_response = { + "url" => "https://github.fake/api/v3/teams/1001/memberships/blackmanx", + "role" => "member", + "state" => "active" + } + + stub_request(:put, "https://github.fake/api/v3/teams/1001/memberships/blackmanx") + .to_return( + status: 404, + headers: { + "Content-Type" => "application/json" + }, + body: JSON.generate({ + "message" => "Not Found", + "documentation_url" => "https://docs.github.com/rest" + }) + ) + + expect { subject.send(:add_user_to_team, user: "blackmanx", team:) }.to raise_error(Octokit::NotFound) + end + end + + context "ignore_not_found is true" do + let(:subject) do + described_class.new( + addr: "https://github.fake/api/v3", + org: "kittensinc", + token: "GoPackGo", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: true + ) + end + + it "ignores 404s" do + expect(subject).to receive(:validate_team_id_and_slug!).with(1001, "russian-blues").and_return(true) + expect(subject).to receive(:org_members).and_return(Set.new(%w[blackmanx])) + + add_membership_response = { + "url" => "https://github.fake/api/v3/teams/1001/memberships/blackmanx", + "role" => "member", + "state" => "active" + } + + stub_request(:put, "https://github.fake/api/v3/teams/1001/memberships/blackmanx") + .to_return( + status: 404, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate({ + "message" => "Not Found", + "documentation_url" => "https://docs.github.com/rest" + }) + ) + + result = subject.send(:add_user_to_team, user: "blackmanx", team:) + expect(result).to eq(false) + end + end end describe "#remove_user_from_team" do From 89687bf01dbdbf0c35261da04729d696cecd3e2b Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:05:47 -0500 Subject: [PATCH 4/7] Make the linter happy v2 --- .../unit/entitlements/backend/github_team/service_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index 8ac9699..110d4e3 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -585,13 +585,13 @@ it "raises when user is not found" do expect(subject).to receive(:validate_team_id_and_slug!).with(1001, "russian-blues").and_return(true) expect(subject).to receive(:org_members).and_return(Set.new(%w[blackmanx])) - + add_membership_response = { "url" => "https://github.fake/api/v3/teams/1001/memberships/blackmanx", "role" => "member", "state" => "active" } - + stub_request(:put, "https://github.fake/api/v3/teams/1001/memberships/blackmanx") .to_return( status: 404, @@ -622,13 +622,13 @@ it "ignores 404s" do expect(subject).to receive(:validate_team_id_and_slug!).with(1001, "russian-blues").and_return(true) expect(subject).to receive(:org_members).and_return(Set.new(%w[blackmanx])) - + add_membership_response = { "url" => "https://github.fake/api/v3/teams/1001/memberships/blackmanx", "role" => "member", "state" => "active" } - + stub_request(:put, "https://github.fake/api/v3/teams/1001/memberships/blackmanx") .to_return( status: 404, From ff045239cc70be37c6d840a4c2d9e54a8a504d72 Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:12:04 -0500 Subject: [PATCH 5/7] Document ignore_not_found configuration option --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 6613793..dffde10 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ Any plugins defined in `lib/entitlements-and-plugins` will be loaded and used at dir: github.com/github/org org: github token: <%= ENV["GITHUB_ORG_TOKEN"] %> + ignore_not_found: false # optional argument to ignore users who are not found in the GitHub instance type: "github_org" ``` @@ -72,6 +73,7 @@ Any plugins defined in `lib/entitlements-and-plugins` will be loaded and used at dir: github.com/github/teams org: github token: <%= ENV["GITHUB_ORG_TOKEN"] %> + ignore_not_found: false # optional argument to ignore users who are not found in the GitHub instance type: "github_team" ``` From 202c07e635f29f5d64e305c0649d3490f1236589 Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 19:12:02 -0500 Subject: [PATCH 6/7] Bump minor release --- lib/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/version.rb b/lib/version.rb index 1578d44..4ea7c91 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -2,6 +2,6 @@ module Entitlements module Version - VERSION = "0.4.4" + VERSION = "0.5.0" end end From 7f71f6087afb615470b5316e618eff131484dd87 Mon Sep 17 00:00:00 2001 From: Jason Macgowan <1389531+jasonmacgowan@users.noreply.github.com> Date: Thu, 14 Dec 2023 19:12:39 -0500 Subject: [PATCH 7/7] Bump minor release --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 2576a9a..4f7921f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - entitlements-github-plugin (0.4.4) + entitlements-github-plugin (0.5.0) contracts (~> 0.17.0) faraday (~> 2.0) faraday-retry (~> 2.0)