From 63f158843de58c3b6d7c6250c9a7829ac4a86c0e Mon Sep 17 00:00:00 2001 From: Karl Krukow Date: Tue, 22 Aug 2023 20:58:01 +0200 Subject: [PATCH 1/2] Support: role maintainer in team membership --- .../backend/github_team/service.rb | 62 ++++++++++++-- .../backend/github_team/service_spec.rb | 82 +++++++++++++++++++ 2 files changed, 137 insertions(+), 7 deletions(-) diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index 3b36f14..24c7a3e 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -4,6 +4,7 @@ require_relative "../../service/github" require "base64" +require "set" module Entitlements class Backend @@ -196,14 +197,56 @@ def sync_team(desired_state, current_state) end end - added_members = desired_state.member_strings.map { |u| u.downcase } - current_state.member_strings.map { |u| u.downcase } - removed_members = current_state.member_strings.map { |u| u.downcase } - desired_state.member_strings.map { |u| u.downcase } + desired_team_members = Set.new(desired_state.member_strings.map { |u| u.downcase }) + current_team_members = Set.new(current_state.member_strings.map { |u| u.downcase }) + + added_members = desired_team_members - current_team_members + removed_members = current_team_members - desired_team_members added_members.select! { |username| add_user_to_team(user: username, team: current_state) } removed_members.select! { |username| remove_user_from_team(user: username, team: current_state) } + added_maintainers = Set.new + removed_maintainers = Set.new + unless desired_metadata["team_maintainers"] == current_metadata["team_maintainers"] + if desired_metadata["team_maintainers"].nil? + # We will not delete ALL maintainers from a team and leave it without maintainers. + Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): IGNORING GitHub Team Maintainer DELETE" + else + desired_maintainers_str = desired_metadata["team_maintainers"] # not nil, we tested that above + desired_maintainers = Set.new(desired_maintainers_str.split(",").map { |u| u.strip.downcase }) + unless desired_maintainers.subset?(desired_team_members) + maintainer_not_member = desired_maintainers - desired_team_members + Entitlements.logger.warn "sync_team(#{current_state.team_name}=#{current_state.team_id}): Maintainers must be a subset of team members. Desired maintainers: #{maintainer_not_member.to_a} are not members. Ignoring." + desired_maintainers = desired_maintainers.intersection(desired_team_members) + end + + current_maintainers_str = current_metadata["team_maintainers"] + current_maintainers = Set.new( + current_maintainers_str.nil? ? [] : current_maintainers_str.split(",").map { |u| u.strip.downcase } + ) + # We ignore any current maintainer who is not a member of the team according to the team spec + # This avoids messing with teams who have been manually modified to add a maintainer + current_maintainers = current_maintainers.intersection(desired_team_members) + added_maintainers = desired_maintainers - current_maintainers + removed_maintainers = current_maintainers - desired_maintainers + if added_maintainers.empty? && removed_maintainers.empty? + Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Textual change but no semantic change in maintainers. It is remains: #{current_maintainers.to_a}." + else + Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Maintainer members change found - From #{current_maintainers.to_a} to #{desired_maintainers.to_a}" + added_maintainers.select! { |username| add_user_to_team(user: username, team: current_state, role: "maintainer") } + + ## We only touch previous maintainers who are actually still going to be members of the team + removed_maintainers = removed_maintainers.intersection(desired_team_members) + ## Downgrade membership to default (role: "member") + removed_maintainers.select! { |username| add_user_to_team(user: username, team: current_state, role: "member") } + end + end + end + + Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Added #{added_members.count}, removed #{removed_members.count}" - added_members.any? || removed_members.any? || changed_parent_team + added_members.any? || removed_members.any? || added_maintainers.any? || removed_maintainers.any? || changed_parent_team end # Create a team @@ -366,17 +409,22 @@ def validate_team_id_and_slug!(team_id, team_slug) # # user - String with the GitHub username # team - Entitlements::Backend::GitHubTeam::Models::Team object for the team. + # role - optional (default: "member") String with the role to assign to the user: either "member" or "maintainer" # - # Returns true if the user was added to the team, false if user was already on team. + # Returns true if the user was added to the team or role changed; false if user was already on team with same role Contract C::KeywordArgs[ user: String, team: Entitlements::Backend::GitHubTeam::Models::Team, + role: C::Optional[String] ] => C::Bool - def add_user_to_team(user:, team:) + def add_user_to_team(user:, team:, role: "member") return false unless org_members.include?(user.downcase) - Entitlements.logger.debug "#{identifier} add_user_to_team(user=#{user}, org=#{org}, team_id=#{team.team_id})" + unless role == "member" || role == "maintainer" + raise "add_user_to_team role mismatch: team_id=#{team.team_id} user=#{user} expected role=maintainer/member got=#{role}" + 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) + result = octokit.add_team_membership(team.team_id, user, role: role) result[:state] == "active" || result[:state] == "pending" end diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index a8722ee..1468997 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -340,6 +340,53 @@ ) end + let(:team_metadata_add_maintainer) do + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: 1001, + team_name: "russian-blues", + members: Set.new(%w[blackmanx ragamuffin MAINECOON]), + ou: "ou=kittensinc,dc=github,dc=com", + metadata: { + "parent_team_name" => "cuddly-kittens", + "team_maintainers" => "blackmanx,ragamuffin" + } + ) + end + let(:team_metadata_maintainer_old) do + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: 1001, + team_name: "russian-blues", + members: Set.new(%w[blackmanx ragamuffin MAINECOON]), + ou: "ou=kittensinc,dc=github,dc=com", + metadata: { + "parent_team_name" => "cuddly-kittens", + "team_maintainers" => "ragamuffin" + } + ) + end + let(:team_metadata_remove_all_maintainers) do + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: 1001, + team_name: "russian-blues", + members: Set.new(%w[blackmanx ragamuffin MAINECOON]), + ou: "ou=kittensinc,dc=github,dc=com", + metadata: { + "parent_team_name" => "cuddly-kittens", + } + ) + end + let(:team_metadata_add_non_member_maintainer) do + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: 1001, + team_name: "russian-blues", + members: Set.new(%w[blackmanx ragamuffin MAINECOON]), + ou: "ou=kittensinc,dc=github,dc=com", + metadata: { + "parent_team_name" => "cuddly-kittens", + "team_maintainers" => "krukow,ragamuffin" + } + ) + end let(:team_metadata_remove) do Entitlements::Backend::GitHubTeam::Models::Team.new( team_id: 1001, @@ -401,6 +448,41 @@ expect(result).to eq(true) end + it "returns true when there were metadata changes to add maintainer" do + allow(subject).to receive(:read_team).with(team_metadata_add_maintainer).and_return(team_metadata_maintainer_old) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Maintainer members change found - From \["ragamuffin"\] to \["blackmanx", \"ragamuffin\"\]/) + expect(subject).to receive(:add_user_to_team).with(user: "blackmanx", team: team_metadata_maintainer_old, role: "maintainer").and_return(true) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/) + result = subject.sync_team(team_metadata_add_maintainer, team_metadata_maintainer_old) + expect(result).to eq(true) + end + + it "returns false when there were metadata changes to remove ALL maintainers" do + allow(subject).to receive(:read_team).with(team_metadata_remove_all_maintainers).and_return(team_metadata_maintainer_old) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): IGNORING GitHub Team Maintainer DELETE/) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/) + result = subject.sync_team(team_metadata_remove_all_maintainers, team_metadata_maintainer_old) + expect(result).to eq(false) + end + + it "returns true when there were metadata changes to remove a maintainer" do + allow(subject).to receive(:read_team).with(team_metadata_maintainer_old).and_return(team_metadata_add_maintainer) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Maintainer members change found - From \["blackmanx", "ragamuffin"\] to \["ragamuffin"\]/) + expect(subject).to receive(:add_user_to_team).with(user: "blackmanx", team: team_metadata_maintainer_old, role: "member").and_return(true) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/) + result = subject.sync_team(team_metadata_maintainer_old, team_metadata_add_maintainer) + expect(result).to eq(true) + end + + it "returns false when there were metadata changes to add maintainer who is NOT in the team" do + allow(subject).to receive(:read_team).with(team_metadata_add_non_member_maintainer).and_return(team_metadata_maintainer_old) + expect(logger).to receive(:warn).with(/sync_team\(russian-blues=1001\): Maintainers must be a subset of team members. Desired maintainers: \["krukow"\] are not members. Ignoring./) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Textual change but no semantic change in maintainers. It is remains: \["ragamuffin\"\]./) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/) + result = subject.sync_team(team_metadata_add_non_member_maintainer, team_metadata_maintainer_old) + expect(result).to eq(false) + end + # TODO: I'm hard-coding a block for deletes, for now. I'm doing that by making sure we dont set the desired parent_team_id to nil for teams where it is already set it "returns false while deletes are prevented" do allow(subject).to receive(:read_team).with(team_metadata_add).and_return(team_metadata_remove) From f3956a34c284822f491f1652dcafd2738189794e Mon Sep 17 00:00:00 2001 From: Karl Krukow Date: Wed, 23 Aug 2023 08:59:02 +0200 Subject: [PATCH 2/2] Add: no cov for trivial block --- lib/entitlements/backend/github_team/service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index 24c7a3e..a174150 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -420,6 +420,7 @@ def validate_team_id_and_slug!(team_id, team_slug) def add_user_to_team(user:, team:, role: "member") return false unless org_members.include?(user.downcase) unless role == "member" || role == "maintainer" + # :nocov: raise "add_user_to_team role mismatch: team_id=#{team.team_id} user=#{user} expected role=maintainer/member got=#{role}" end Entitlements.logger.debug "#{identifier} add_user_to_team(user=#{user}, org=#{org}, team_id=#{team.team_id}, role=#{role})"