Skip to content

Commit

Permalink
Add gem scope check to gem push
Browse files Browse the repository at this point in the history
  • Loading branch information
jenshenny committed Apr 8, 2022
1 parent a49a9d5 commit 77de212
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 4 deletions.
2 changes: 1 addition & 1 deletion app/controllers/api/v1/rubygems_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def show
def create
return render_api_key_forbidden unless @api_key.can_push_rubygem?

gemcutter = Pusher.new(@api_key.user, request.body, request.remote_ip)
gemcutter = Pusher.new(@api_key.user, request.body, request.remote_ip, @api_key.rubygem)
enqueue_web_hook_jobs(gemcutter.version) if gemcutter.process
render plain: gemcutter.message, status: gemcutter.code
rescue => e
Expand Down
11 changes: 9 additions & 2 deletions app/models/pusher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,29 @@
class Pusher
attr_reader :user, :spec, :message, :code, :rubygem, :body, :version, :version_id, :size

def initialize(user, body, remote_ip = "")
def initialize(user, body, remote_ip = "", scoped_rubygem = nil)
@user = user
@body = StringIO.new(body.read)
@size = @body.size
@indexer = Indexer.new
@remote_ip = remote_ip
@scoped_rubygem = scoped_rubygem
end

def process
pull_spec && find && authorize && verify_mfa_requirement && validate && save
pull_spec && find && authorize && verify_gem_scope && verify_mfa_requirement && validate && save
end

def authorize
rubygem.pushable? || rubygem.owned_by?(user) || notify_unauthorized
end

def verify_gem_scope
return true unless @scoped_rubygem && rubygem != @scoped_rubygem

notify("This API key cannot perform the specified action on this gem.", 403)
end

def verify_mfa_requirement
user.mfa_enabled? || !(version_mfa_required? || rubygem.mfa_required?) ||
notify("Rubygem requires owners to enable MFA. You must enable MFA before pushing new version.", 403)
Expand Down
45 changes: 45 additions & 0 deletions test/functional/api/v1/rubygems_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,51 @@ def self.should_respond_to(format)
end
end

context "push with api key with gem scoped" do
context "to a gem with ownership removed" do
setup do
ownership = create(:ownership, user: create(:user), rubygem: create(:rubygem, name: "test-gem123"))
@api_key = create(:api_key, key: "12343", user: ownership.user, ownership: ownership, push_rubygem: true)
ownership.destroy!
@request.env["HTTP_AUTHORIZATION"] = "12343"

post :create, body: gem_file("test-1.0.0.gem").read
end

should respond_with :forbidden
should "#render_soft_deleted_api_key and display an error" do
assert_equal "An invalid API key cannot be used. Please delete it and create a new one.", @response.body
end
end

context "to a different gem" do
setup do
ownership = create(:ownership, user: create(:user), rubygem: create(:rubygem, name: "test-gem"))
create(:api_key, key: "12343", user: ownership.user, ownership: ownership, push_rubygem: true)
@request.env["HTTP_AUTHORIZATION"] = "12343"

post :create, body: gem_file("test-1.0.0.gem").read
end

should respond_with :forbidden
should "say gem scope is invalid" do
assert_equal "This API key cannot perform the specified action on this gem.", @response.body
end
end

context "to the gem being pushed" do
setup do
ownership = create(:ownership, user: create(:user), rubygem: create(:rubygem, name: "test"))
create(:api_key, key: "12343", user: ownership.user, ownership: ownership, push_rubygem: true)
@request.env["HTTP_AUTHORIZATION"] = "12343"

post :create, body: gem_file("test-1.0.0.gem").read
end

should respond_with :ok
end
end

context "with incorrect api key" do
context "on GET to index with JSON for a list of gems without api key" do
setup do
Expand Down
40 changes: 39 additions & 1 deletion test/unit/pusher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_mfa_requirement).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:validate).returns true
@cutter.stubs(:save)

Expand All @@ -41,6 +42,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:pull_spec).returns false
@cutter.stubs(:find).never
@cutter.stubs(:authorize).never
@cutter.stubs(:verify_gem_scope).never
@cutter.stubs(:verify_mfa_requirement).never
@cutter.stubs(:save).never
@cutter.process
Expand All @@ -50,16 +52,30 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find)
@cutter.stubs(:authorize).never
@cutter.stubs(:verify_gem_scope).never
@cutter.stubs(:verify_mfa_requirement).never
@cutter.stubs(:save).never

@cutter.process
end

should "not attempt to check mfa requirement if not authorized" do
should "not attempt to check gem scope if not authorized" do
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns false
@cutter.stubs(:verify_gem_scope).never
@cutter.stubs(:verify_mfa_requirement).never
@cutter.stubs(:validate).never
@cutter.stubs(:save).never

@cutter.process
end

should "not attempt to check mfa requirement if scoped to another gem" do
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns false
@cutter.stubs(:verify_mfa_requirement).never
@cutter.stubs(:validate).never
@cutter.stubs(:save).never
Expand All @@ -71,6 +87,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_mfa_requirement).returns false
@cutter.stubs(:validate).never
@cutter.stubs(:save).never
Expand All @@ -82,6 +99,7 @@ class PusherTest < ActiveSupport::TestCase
@cutter.stubs(:pull_spec).returns true
@cutter.stubs(:find).returns true
@cutter.stubs(:authorize).returns true
@cutter.stubs(:verify_gem_scope).returns true
@cutter.stubs(:verify_mfa_requirement).returns true
@cutter.stubs(:validate).returns false
@cutter.stubs(:save).never
Expand Down Expand Up @@ -532,6 +550,26 @@ def two_cert_chain(signing_key:, root_not_before: Time.current, cert_not_before:
teardown { RubygemFs.mock! }
end

context "has a scoped gem" do
setup do
@rubygem = create(:rubygem)
end

should "pushes gem if scoped to the same gem" do
create(:version, rubygem: @rubygem, number: "0.1.1", indexed: false)
cutter = Pusher.new(@user, @gem, "", @rubygem)
cutter.stubs(:rubygem).returns @rubygem
assert cutter.verify_gem_scope
end

should "does not push gem if scoped to another gem" do
create(:version, rubygem: @rubygem, number: "0.1.1", indexed: false)
cutter = Pusher.new(@user, @gem, "", create(:rubygem))
cutter.stubs(:rubygem).returns @rubygem
refute cutter.verify_gem_scope
end
end

context "the gem has been signed and not tampered with" do
setup do
@gem = gem_file("valid_signature-0.0.0.gem")
Expand Down

0 comments on commit 77de212

Please sign in to comment.