Skip to content

Commit cffa861

Browse files
Consider can_push? instead of ownership
Ownership is proving to be trickier to keep consistent than I had hoped it would be. I've been migrating to a permissions-based approach, which has similar concerns beneath the covers, but is more explicit in its implementation.
1 parent f9deac5 commit cffa861

File tree

7 files changed

+31
-12
lines changed

7 files changed

+31
-12
lines changed

app/models/oidc/trusted_publisher/github_action.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ def repository = "#{repository_owner}/#{repository_name}"
147147

148148
def workflow_slug = ".github/workflows/#{workflow_filename}"
149149

150-
def owns_gem?(rubygem) = rubygem_trusted_publishers.exists?(rubygem: rubygem)
150+
def can_push?(rubygem) = rubygem_trusted_publishers.exists?(rubygem: rubygem)
151151

152152
private
153153

app/models/pusher.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ def process
3232
end
3333

3434
def authorize
35-
(rubygem.pushable? && (api_key.user? || find_pending_trusted_publisher)) || owner.owns_gem?(rubygem) || notify_unauthorized
35+
(rubygem.pushable? && (api_key.user? || find_pending_trusted_publisher)) || owner.can_push?(rubygem) || notify_unauthorized
3636
end
3737

3838
def verify_gem_scope
@@ -260,7 +260,7 @@ def republish_notification(version)
260260
notify("It appears that #{version.full_name} did not finish pushing.\n" \
261261
"Please contact [email protected] for assistance if you pushed this gem more than a minute ago.", 409)
262262
else
263-
different_owner = "pushed by a previous owner of this gem " unless owner.owns_gem?(version.rubygem)
263+
different_owner = "pushed by a previous owner of this gem " unless owner.can_push?(version.rubygem)
264264
notify("A yanked version #{different_owner}already exists (#{version.full_name}).\n" \
265265
"Repushing of gem versions is not allowed. Please use a new version and retry", 409)
266266
end

app/models/user.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,8 +275,8 @@ def blocked?
275275
blocked_email.present?
276276
end
277277

278-
def owns_gem?(rubygem)
279-
rubygem.owned_by?(self)
278+
def can_push?(rubygem)
279+
GemPermissions.new(rubygem, self).can_push?
280280
end
281281

282282
def acknowledge_policies!

test/integration/push_test.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ class PushTest < ActionDispatch::IntegrationTest
189189

190190
rubygem = Rubygem.find_by!(name: "sandworm")
191191

192-
assert rubygem.owned_by?(@user)
192+
assert @user.can_push?(rubygem)
193193
assert rubygem.oidc_rubygem_trusted_publishers.exists?(trusted_publisher: pending_trusted_publisher.trusted_publisher)
194194
end
195195

@@ -213,7 +213,7 @@ class PushTest < ActionDispatch::IntegrationTest
213213

214214
rubygem = Rubygem.find_by!(name: "sandworm")
215215

216-
assert rubygem.owned_by?(@user)
216+
assert @user.can_push?(rubygem)
217217
assert rubygem.oidc_rubygem_trusted_publishers.exists?(trusted_publisher: pending_trusted_publisher.trusted_publisher)
218218
end
219219

test/models/oidc/trusted_publisher/github_action_test.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,15 +111,15 @@ class OIDC::TrustedPublisher::GitHubActionTest < ActiveSupport::TestCase
111111
assert_equal "GitHub Actions example/bar @ .github/workflows/push_gem.yml (test)", publisher.name
112112
end
113113

114-
test "#owns_gem?" do
114+
test "#can_push?" do
115115
rubygem1 = create(:rubygem)
116116
rubygem2 = create(:rubygem)
117117

118118
publisher = create(:oidc_trusted_publisher_github_action)
119119
create(:oidc_rubygem_trusted_publisher, trusted_publisher: publisher, rubygem: rubygem1)
120120

121-
assert publisher.owns_gem?(rubygem1)
122-
refute publisher.owns_gem?(rubygem2)
121+
assert publisher.can_push?(rubygem1)
122+
refute publisher.can_push?(rubygem2)
123123
end
124124

125125
test "#to_access_policy" do

test/models/pusher_test.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ def two_cert_chain(signing_key:, root_not_before: Time.current, cert_not_before:
504504
owner = stub("owner", to_gid: nil)
505505
@api_key.update_columns(owner_id: 0, owner_type: "stub")
506506
@cutter.stubs(:owner).returns owner
507-
owner.expects(:owns_gem?).with(@cutter.rubygem).returns(false)
507+
owner.expects(:can_push?).with(@cutter.rubygem).returns(false)
508508

509509
refute @cutter.authorize
510510
assert_equal "You are not allowed to push this gem.",
@@ -541,7 +541,7 @@ def two_cert_chain(signing_key:, root_not_before: Time.current, cert_not_before:
541541
owner = stub("owner", to_gid: nil)
542542
@api_key.update_columns(owner_id: 0, owner_type: "stub")
543543
@cutter.stubs(:owner).returns owner
544-
owner.expects(:owns_gem?).with(@rubygem).returns(false)
544+
owner.expects(:can_push?).with(@rubygem).returns(false)
545545

546546
refute @cutter.authorize
547547
assert_equal "You are not allowed to push this gem.",

test/models/user_test.rb

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,4 +1028,23 @@ class UserTest < ActiveSupport::TestCase
10281028
refute_predicate user, :policies_acknowledged?
10291029
end
10301030
end
1031+
1032+
context "#can_push?" do
1033+
should "delegate to GemPermissions#can_push?" do
1034+
user = build(:user)
1035+
rubygem = build(:rubygem)
1036+
1037+
gem_permissions = mock
1038+
gem_permissions.expects(:can_push?).returns(true)
1039+
GemPermissions.expects(:new).with(rubygem, user).returns(gem_permissions)
1040+
1041+
assert user.can_push?(rubygem)
1042+
1043+
gem_permissions = mock
1044+
gem_permissions.expects(:can_push?).returns(false)
1045+
GemPermissions.expects(:new).with(rubygem, user).returns(gem_permissions)
1046+
1047+
refute user.can_push?(rubygem)
1048+
end
1049+
end
10311050
end

0 commit comments

Comments
 (0)