From 4e2a79da6bce7d42181847b156a03a2d8c1113b3 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Tue, 8 Sep 2026 16:29:24 -0400 Subject: [PATCH] Cache normalized organization membership lookups Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bff1e95b-3949-4ae8-b3a5-737fd278a977 --- .../backend/github_org/service.rb | 2 + .../backend/github_team/provider.rb | 7 +- lib/entitlements/service/github.rb | 33 +++++++++ .../backend/github_org/service_spec.rb | 45 ++++++++++++ .../backend/github_team/controller_spec.rb | 10 ++- .../backend/github_team/provider_spec.rb | 24 +++++++ spec/unit/entitlements/service/github_spec.rb | 69 +++++++++++++++++++ 7 files changed, 184 insertions(+), 6 deletions(-) diff --git a/lib/entitlements/backend/github_org/service.rb b/lib/entitlements/backend/github_org/service.rb index 17062e0..ddd61bd 100644 --- a/lib/entitlements/backend/github_org/service.rb +++ b/lib/entitlements/backend/github_org/service.rb @@ -66,6 +66,7 @@ def add_user_to_organization(user, role) return true elsif new_membership[:state] == "active" org_members[user] = role + add_org_member_to_normalized_lookup(user) return true end end @@ -92,6 +93,7 @@ def remove_user_from_organization(user) # operations in this organization will ignore this user. if result org_members.delete(user) + remove_org_member_from_normalized_lookup(user) pending_members.delete(user) end diff --git a/lib/entitlements/backend/github_team/provider.rb b/lib/entitlements/backend/github_team/provider.rb index f21841c..e1abb12 100644 --- a/lib/entitlements/backend/github_team/provider.rb +++ b/lib/entitlements/backend/github_team/provider.rb @@ -144,9 +144,10 @@ def commit(entitlement_group) # Returns a set of strings with usernames meeting the criteria. Contract Entitlements::Models::Group => C::SetOf[String] def auto_generate_ignored_users(entitlement_group) - org_members = github.org_members.keys.map(&:downcase) - group_members = entitlement_group.member_strings.map(&:downcase) - Set.new(group_members - org_members) + entitlement_group.member_strings.each_with_object(Set.new) do |username, ignored_users| + normalized_username = username.downcase + ignored_users.add(normalized_username) unless github.org_member?(normalized_username) + end end private diff --git a/lib/entitlements/service/github.rb b/lib/entitlements/service/github.rb index 0a977a9..612e267 100644 --- a/lib/entitlements/service/github.rb +++ b/lib/entitlements/service/github.rb @@ -4,6 +4,7 @@ require "net/http" require "octokit" +require "set" require "uri" module Entitlements @@ -96,6 +97,20 @@ def org_members Entitlements.cache[:github_org_members][org_signature][:value] end + # Determine whether a username is an active member of the organization. + # + # username - GitHub username to look up. + # + # Returns true if the username is an active organization member. + Contract String => C::Bool + def org_member?(username) + org_members + entry = Entitlements.cache[:github_org_members].fetch(org_signature) + entry[:normalized_members] ||= Set.new(entry[:value].keys) + normalized_username = /[A-Z]/.match?(username) ? username.downcase : username + entry[:normalized_members].include?(normalized_username) + end + # Returns true if the github instance is an enterprise server instance Contract C::None => C::Bool def enterprise? @@ -160,6 +175,24 @@ def invalidate_org_members_predictive_cache private + # Keep an already-materialized membership lookup synchronized after an addition or + # role change without forcing it to be built. + Contract String => nil + def add_org_member_to_normalized_lookup(username) + entry = Entitlements.cache[:github_org_members][org_signature] + entry[:normalized_members].add(username.downcase) if entry&.key?(:normalized_members) + nil + end + + # Keep an already-materialized membership lookup synchronized after a removal without + # forcing it to be built. + Contract String => nil + def remove_org_member_from_normalized_lookup(username) + entry = Entitlements.cache[:github_org_members][org_signature] + entry[:normalized_members].delete(username.downcase) if entry&.key?(:normalized_members) + nil + end + # The octokit object is initialized the first time it's called. # # Takes no arguments. diff --git a/spec/unit/entitlements/backend/github_org/service_spec.rb b/spec/unit/entitlements/backend/github_org/service_spec.rb index 05c8a99..b8340aa 100644 --- a/spec/unit/entitlements/backend/github_org/service_spec.rb +++ b/spec/unit/entitlements/backend/github_org/service_spec.rb @@ -129,6 +129,39 @@ expect(subject.pending_members).to eq(Set.new) expect(subject.org_members).to eq("bob" => "admin") end + + it "adds active members and role changes to a materialized normalized lookup" do + allow(subject).to receive(:members_and_roles_from_rest).and_return("bob" => "MEMBER") + expect(subject.org_member?("bob")).to eq(true) + + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/alice").to_return( + status: 200, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate( + "url" => "https://github.fake/api/v3/orgs/kittensinc/memberships/alice", + "state" => "active", + "role" => "admin" + ) + ) + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return( + status: 200, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate( + "url" => "https://github.fake/api/v3/orgs/kittensinc/memberships/bob", + "state" => "active", + "role" => "admin" + ) + ) + + expect(subject.send(:add_user_to_organization, "alice", "admin")).to eq(true) + expect(subject.org_member?("alice")).to eq(true) + expect(subject.send(:add_user_to_organization, "bob", "admin")).to eq(true) + expect(subject.org_member?("bob")).to eq(true) + end end context "sad path" do @@ -244,5 +277,17 @@ expect(subject.pending_members).to eq(Set.new) expect(subject.org_members).to eq({}) end + + it "removes members from a materialized normalized lookup" do + allow(subject).to receive(:members_and_roles_from_rest).and_return("bob" => "ADMIN") + allow(subject).to receive(:enterprise?).and_return(false) + allow(subject).to receive(:pending_members_from_graphql).and_return(Set.new) + expect(subject.org_member?("bob")).to eq(true) + + stub_request(:delete, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return(status: 204) + + expect(subject.send(:remove_user_from_organization, "bob")).to eq(true) + expect(subject.org_member?("bob")).to eq(false) + 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 44a4869..cffb484 100644 --- a/spec/unit/entitlements/backend/github_team/controller_spec.rb +++ b/spec/unit/entitlements/backend/github_team/controller_spec.rb @@ -27,6 +27,7 @@ "RUSSIANBLue" => "member" } end + let(:org_member_set) { Set.new(org_member_hash.keys.map(&:downcase)) } describe "#calculate" do let(:russian_blue_team) do @@ -123,7 +124,8 @@ allow(dotcom_obj).to receive(:org).and_return("kittensinc") allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(russian_blue_team) allow(dotcom_obj).to receive(:read_team).with(snowshoe_group).and_return(snowshoe_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=russian-blues,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1001) with 2 member(s)") @@ -169,7 +171,8 @@ allow(dotcom_obj).to receive(:identifier).and_return("github.com") allow(dotcom_obj).to receive(:org).and_return("kittensinc") allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(russian_blue_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=russian-blues,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1001) with 2 member(s)") @@ -218,7 +221,8 @@ allow(dotcom_obj).to receive(:ou).and_return("GitHub") allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(nil) allow(dotcom_obj).to receive(:read_team).with(snowshoe_group).and_return(snowshoe_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=snowshoes,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1002) with 2 member(s)") diff --git a/spec/unit/entitlements/backend/github_team/provider_spec.rb b/spec/unit/entitlements/backend/github_team/provider_spec.rb index d3df3dc..4293507 100644 --- a/spec/unit/entitlements/backend/github_team/provider_spec.rb +++ b/spec/unit/entitlements/backend/github_team/provider_spec.rb @@ -454,6 +454,30 @@ end end + describe "#auto_generate_ignored_users" do + it "returns lowercase non-members and does not enumerate organization membership" do + entitlement_group = instance_double( + Entitlements::Models::Group, + member_strings: Set.new(%w[SnowShoe RUSSIAN_BLUE PendingCat]) + ) + allow(subject).to receive(:github).and_return(github) + expect(github).not_to receive(:org_members) + expect(github).to receive(:org_member?).with("snowshoe").and_return(true) + expect(github).to receive(:org_member?).with("russian_blue").and_return(false) + expect(github).to receive(:org_member?).with("pendingcat").and_return(false) + + expect(subject.auto_generate_ignored_users(entitlement_group)).to eq(Set.new(%w[russian_blue pendingcat])) + end + + it "returns an empty set for an empty team" do + entitlement_group = instance_double(Entitlements::Models::Group, member_strings: Set.new) + allow(subject).to receive(:github).and_return(github) + expect(github).not_to receive(:org_member?) + + expect(subject.auto_generate_ignored_users(entitlement_group)).to eq(Set.new) + end + end + describe "#create_github_team_group" do it "returns a new empty team" do entitlement_group = Entitlements::Models::Group.new( diff --git a/spec/unit/entitlements/service/github_spec.rb b/spec/unit/entitlements/service/github_spec.rb index eea708e..8e8c1e0 100644 --- a/spec/unit/entitlements/service/github_spec.rb +++ b/spec/unit/entitlements/service/github_spec.rb @@ -47,6 +47,71 @@ end end + describe "#org_member?" do + let(:members_and_roles) do + { + "alice" => "MEMBER", + "bob" => "ADMIN" + } + end + + before do + allow(subject).to receive(:members_and_roles_from_rest).and_return(members_and_roles) + end + + it "performs case-insensitive lookups" do + expect(subject.org_member?("ALIce")).to eq(true) + expect(subject.org_member?("charles")).to eq(false) + end + + it "reuses the normalized membership set" do + expect(subject.org_member?("alice")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] + expect(subject.org_member?("bob")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).to equal(normalized_members) + end + + it "shares the normalized membership set between services for the same organization signature" do + other = described_class.new( + addr: "https://github.fake/api/v3", + org: "kittensinc", + token: "DifferentToken", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false + ) + + expect(subject.org_member?("alice")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] + expect(other.org_member?("bob")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).to equal(normalized_members) + end + + it "isolates normalized membership sets by organization and GitHub instance" do + other_org = described_class.new( + addr: "https://github.fake/api/v3", + org: "puppiesinc", + token: "GoPackGo", + ou: "ou=puppiesinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false + ) + other_instance = described_class.new( + addr: "https://github.example/api/v3", + org: "kittensinc", + token: "GoPackGo", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=example", + ignore_not_found: false + ) + allow(other_org).to receive(:members_and_roles_from_rest).and_return("charles" => "MEMBER") + allow(other_instance).to receive(:members_and_roles_from_rest).and_return("david" => "MEMBER") + + expect(subject.org_member?("alice")).to eq(true) + expect(other_org.org_member?("alice")).to eq(false) + expect(other_org.org_member?("charles")).to eq(true) + expect(other_instance.org_member?("alice")).to eq(false) + expect(other_instance.org_member?("david")).to eq(true) + end + end + describe "#enterprise?" do it "returns false if an instance is not enterprise" do stub_request(:get, "https://github.fake/api/v3/meta"). @@ -122,6 +187,8 @@ # First load should read from the cache. expect(subject.org_members).to eq(answer.map { |k, v| [k, v.downcase] }.to_h) + expect(subject.org_member?("monalisa")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] # Invalidating cache should force a re-read. answer_2 = answer.dup @@ -137,6 +204,8 @@ # Check that the re-read has occurred and the correct result is achieved. expect(subject).not_to receive(:members_and_roles_from_graphql) # Should already be in object's cache expect(subject.org_members).to eq(answer_2.map { |k, v| [k, v.downcase] }.to_h) + expect(subject.org_member?("ragamuffin")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).not_to equal(normalized_members) end end