Skip to content

Commit

Permalink
Support ignoring 404s when user is not found in GitHub instance
Browse files Browse the repository at this point in the history
  • Loading branch information
jasonmacgowan committed Dec 14, 2023
1 parent 62f8827 commit 7f4ede5
Show file tree
Hide file tree
Showing 14 changed files with 125 additions and 51 deletions.
13 changes: 7 additions & 6 deletions lib/entitlements/backend/github_org/controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion lib/entitlements/backend/github_org/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion lib/entitlements/backend/github_org/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/entitlements/backend/github_team/controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion lib/entitlements/backend/github_team/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand Down
5 changes: 3 additions & 2 deletions lib/entitlements/backend/github_team/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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] ||= {}
Expand Down
8 changes: 5 additions & 3 deletions lib/entitlements/service/github.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#
Expand All @@ -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.
Expand Down
52 changes: 26 additions & 26 deletions spec/unit/entitlements/backend/github_org/controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion spec/unit/entitlements/backend/github_org/provider_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading

0 comments on commit 7f4ede5

Please sign in to comment.