Cache normalized organization membership lookups

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
 

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

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.

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

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

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(

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