Merge pull request #28 from github/jasonmacgowan/ignore_not_found

Support ignoring 404s when user is not found in GitHub instance

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)

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"
 ```
 

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)

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

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

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)

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 = {}

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] ||= {}
@@ -436,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.

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.

lib/version.rb

@@ -2,6 +2,6 @@
 
 module Entitlements
   module Version
-    VERSION = "0.4.4"
+    VERSION = "0.5.0"
   end
 end

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)

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