Skip to content

Commit a3c86ff

Browse files
Verify publishing environment reviewers after setup. (#31)
1 parent 6985916 commit a3c86ff

6 files changed

Lines changed: 130 additions & 11 deletions

File tree

‎context/getting-started.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,14 @@ reviewers:
6969
7070
Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership.
7171

72+
Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility.
73+
7274
GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval.
7375

7476
Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged.
7577

78+
After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply.
79+
7680
Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub.
7781

7882
The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free.

‎fixtures/bake/gem/github/project_client.rb‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@ def readlines(*arguments, **options)
3232
@writes << File.read(arguments[arguments.index("--body-file") + 1])
3333
["https://github.com/socketry/example/pull/42\n"]
3434
else
35-
[JSON.generate(@responses.fetch(arguments.fetch(2)))]
35+
response = @responses.fetch(arguments.fetch(2))
36+
response = response.call if response.respond_to?(:call)
37+
[JSON.generate(response)]
3638
end
3739
end
3840

‎guides/getting-started/readme.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,14 @@ reviewers:
6969
7070
Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership.
7171

72+
Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility.
73+
7274
GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval.
7375

7476
Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged.
7577

78+
After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply.
79+
7680
Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub.
7781

7882
The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free.

‎lib/bake/gem/github/project.rb‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ def inspect_commit(commit)
157157

158158
# Return a read-only comparison of managed settings and current repository settings.
159159
# @returns [Hash] Desired rules, existing rules, environments, optional environment changes, and expected Trusted Publisher settings. This does not verify RubyGems ownership or publisher configuration.
160+
# @raises [RuntimeError] If a reviewer team is secret or belongs to another organization.
160161
def doctor
161162
{
162163
desired_rules: Setup.rules(@config),
@@ -174,8 +175,9 @@ def doctor
174175

175176
# Apply the named rulesets and configured reviewers for an existing environment. Invoke after reviewing doctor output.
176177
# Preserves the environment's wait timer, self-review prevention, administrator bypass, and branch restrictions.
178+
# Reads back updated environment settings to verify GitHub retained them.
177179
# @returns [Hash] The desired ruleset payloads after successful application.
178-
# @raises [RuntimeError] If more than one existing ruleset has a managed name.
180+
# @raises [RuntimeError] If a reviewer team is unsuitable, managed rulesets are ambiguous, or environment verification fails. Earlier updates may already have completed.
179181
# @raises [Bake::Gem::CommandExecutionError] If an API operation fails; earlier updates may already have completed.
180182
def apply
181183
changes = environment_changes
@@ -193,6 +195,9 @@ def apply
193195

194196
if changes && changes.fetch(:current) != changes.fetch(:desired)
195197
write_api("repos/#{@repository}/#{environment_path}", changes.fetch(:desired), method: "PUT")
198+
unless environment_settings == changes.fetch(:desired)
199+
raise "GitHub did not retain the requested settings for environment #{changes.fetch(:name)}. Check its reviewers and protection rules in GitHub, then rerun gem:github:setup:plan."
200+
end
196201
end
197202

198203
return rules
@@ -209,19 +214,26 @@ def environment_changes
209214
return nil unless @config.key?("reviewers")
210215
Setup.validate_reviewers(@config["reviewers"])
211216

217+
current = environment_settings
218+
reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)}.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)}
219+
220+
return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)}
221+
end
222+
223+
# Read the environment settings, comparing reviewer identities independently of their order.
224+
def environment_settings
212225
environment = api(environment_path)
213226
protections = environment.fetch("protection_rules").to_h{|rule| [rule.fetch("type"), rule]}
214227
reviews = protections.fetch("required_reviewers", {})
215-
current = {
228+
reviewers = reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}}
229+
230+
return {
216231
wait_timer: protections.fetch("wait_timer", {}).fetch("wait_timer", 0),
217232
prevent_self_review: reviews.fetch("prevent_self_review", false),
218233
can_admins_bypass: environment.fetch("can_admins_bypass"),
219234
deployment_branch_policy: environment.fetch("deployment_branch_policy"),
220-
reviewers: reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}},
235+
reviewers: reviewers.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)},
221236
}
222-
reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)}
223-
224-
return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)}
225237
end
226238

227239
def resolve_reviewer(name)
@@ -235,6 +247,9 @@ def resolve_reviewer(name)
235247
type = "User"
236248
end
237249
response = JSON.parse(readlines("gh", "api", path, chdir: @root).join)
250+
if type == "Team" && response.fetch("privacy") == "secret"
251+
raise "Reviewer team #{name} is Secret. Change its visibility to Visible in GitHub team settings before running setup."
252+
end
238253

239254
return {type: type, id: response.fetch("id")}
240255
end

‎releases.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Releases
22

3+
## Unreleased
4+
5+
- Reject Secret environment reviewer teams before applying release settings, and verify that GitHub retained the requested reviewers and existing protections after updating the environment.
6+
37
## v0.3.1
48

59
- Publish validated release PRs from pushes to the default branch, retaining the exact merged commit and environment approval without requiring `pull_request_target`.

‎test/bake/gem/github/project/environment.rb‎

Lines changed: 94 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,26 @@
1818
}
1919
end
2020

21+
let(:saved_reviewers) {[{"type" => "Team", "reviewer" => {"id" => 123, "slug" => "managers"}}]}
22+
let(:saved_environment) do
23+
{
24+
"can_admins_bypass" => true,
25+
"deployment_branch_policy" => {"protected_branches" => false, "custom_branch_policies" => true},
26+
"protection_rules" => [
27+
{"type" => "branch_policy"},
28+
{"type" => "required_reviewers", "prevent_self_review" => false, "reviewers" => saved_reviewers},
29+
],
30+
}
31+
end
32+
2133
before do
2234
Bake::Gem::GitHub::Setup.new(root).generate(repository: "socketry/example", checks: ["Tests"], reviewers: ["socketry/managers"])
2335
project.responses["repos/socketry/example/rulesets?per_page=100"] = []
2436
project.responses["repos/socketry/example/environments"] = {"environments" => [environment]}
25-
project.responses["repos/socketry/example/environments/rubygems"] = environment
26-
project.responses["orgs/socketry/teams/managers"] = {"id" => 123}
37+
project.responses["repos/socketry/example/environments/rubygems"] = -> do
38+
project.writes.any?{|payload| payload.key?("reviewers")} ? saved_environment : environment
39+
end
40+
project.responses["orgs/socketry/teams/managers"] = {"id" => 123, "privacy" => "closed"}
2741
end
2842

2943
with "#doctor" do
@@ -53,6 +67,9 @@
5367
with "#apply" do
5468
it "updates reviewers without weakening existing environment protections" do
5569
environment["can_admins_bypass"] = false
70+
saved_environment["can_admins_bypass"] = false
71+
saved_environment["protection_rules"] << {"type" => "wait_timer", "wait_timer" => 30}
72+
saved_environment["protection_rules"].find{|rule| rule["type"] == "required_reviewers"}["prevent_self_review"] = true
5673
environment["protection_rules"] += [
5774
{"type" => "wait_timer", "wait_timer" => 30},
5875
{"type" => "required_reviewers", "prevent_self_review" => true, "reviewers" => [{"type" => "User", "reviewer" => {"id" => 456}}]},
@@ -66,12 +83,13 @@
6683
"deployment_branch_policy" => environment.fetch("deployment_branch_policy"),
6784
"reviewers" => [{"type" => "Team", "id" => 123}],
6885
}
69-
expect(project.requests.last[0, 5]).to be == ["gh", "api", "repos/socketry/example/environments/rubygems", "--method", "PUT"]
86+
expect(project.requests.last).to be == ["gh", "api", "repos/socketry/example/environments/rubygems"]
7087
expect(project.writes.size).to be == 5
7188
end
7289

7390
it "preserves an unrestricted branch policy and enabled administrator bypass" do
7491
environment["deployment_branch_policy"] = nil
92+
saved_environment["deployment_branch_policy"] = nil
7593
project.apply
7694

7795
expect(project.writes.last).to have_keys("deployment_branch_policy" => be_nil, "can_admins_bypass" => be == true)
@@ -85,6 +103,67 @@
85103
expect(project.writes.all?{|payload| payload.key?("rules")}).to be == true
86104
end
87105

106+
it "accepts reordered reviewers and ignores duplicate configured identities" do
107+
project.config["reviewers"] = ["ioquatix", "socketry/managers", "ioquatix"]
108+
project.responses["users/ioquatix"] = {"id" => 456}
109+
saved_reviewers.unshift({"type" => "User", "reviewer" => {"id" => 456}})
110+
111+
project.apply
112+
expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}, {"type" => "User", "id" => 456}]
113+
114+
project.apply
115+
expect(project.writes.count{|payload| payload.key?("reviewers")}).to be == 1
116+
end
117+
118+
it "rejects a successful write which silently drops every reviewer" do
119+
saved_reviewers.clear
120+
121+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain.*rubygems.*gem:github:setup:plan/)
122+
expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}]
123+
end
124+
125+
it "rejects a successful write which drops one of the requested reviewers" do
126+
project.config["reviewers"] << "ioquatix"
127+
project.responses["users/ioquatix"] = {"id" => 456}
128+
129+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
130+
end
131+
132+
it "rejects an unexpected reviewer who could approve publication" do
133+
saved_reviewers << {"type" => "User", "reviewer" => {"id" => 456}}
134+
135+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
136+
end
137+
138+
it "distinguishes user and team reviewers with the same numeric ID" do
139+
saved_reviewers.first["type"] = "User"
140+
141+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
142+
end
143+
144+
it "rejects a successful write which removes the reviewer protection rule" do
145+
saved_environment["protection_rules"].reject!{|rule| rule["type"] == "required_reviewers"}
146+
147+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
148+
end
149+
150+
it "rejects a successful write which changes an existing protection" do
151+
environment["can_admins_bypass"] = false
152+
153+
expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
154+
expect(project.writes.last.fetch("can_admins_bypass")).to be == false
155+
end
156+
157+
it "propagates failure to read back the applied environment" do
158+
project.responses["repos/socketry/example/environments/rubygems"] = -> do
159+
raise "GitHub readback failed" unless project.writes.empty?
160+
environment
161+
end
162+
163+
expect{project.apply}.to raise_exception(RuntimeError, message: be == "GitHub readback failed")
164+
expect(project.writes.size).to be == 5
165+
end
166+
88167
it "leaves environment settings unmanaged when reviewers are omitted" do
89168
project.config.delete("reviewers")
90169
project.apply
@@ -125,10 +204,21 @@
125204

126205
it "escapes the configured environment name in API requests" do
127206
project.config["environment"] = "release / gems"
128-
project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = environment
207+
project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = project.responses.fetch("repos/socketry/example/environments/rubygems")
129208
project.apply
130209

131210
expect(project.requests.last.fetch(2)).to be == "repos/socketry/example/environments/release%20%2F%20gems"
132211
end
133212
end
213+
214+
[:doctor, :apply].each do |operation|
215+
with "##{operation}" do
216+
it "rejects secret reviewer teams before changing any settings" do
217+
project.responses["orgs/socketry/teams/managers"]["privacy"] = "secret"
218+
219+
expect{project.public_send(operation)}.to raise_exception(RuntimeError, message: be =~ /socketry\/managers is Secret.*Visible/)
220+
expect(project.writes).to be == []
221+
end
222+
end
223+
end
134224
end

0 commit comments

Comments
 (0)