From c10fe4936f97a071e346a7457d3253724590e6d6 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Mon, 10 Dec 2018 15:40:28 -0500 Subject: [PATCH] implement RFC072: only allow admins to remove cookbooks and cookbook versions Chef RFC072[1] was accepted as a plan for Supermarket to prevent cookbook artifacts from disappearing. These changes are a beginning: prevent a cookbook owner from removing any version of a cookbook or the entire cookbook itself. This change does not address the proposed hiding behavior in the RFC. For backward compatibility, a new configuration option has been added to enable the previous behavior to allow artifact removal by owners. The option is set via an environment variable managed by the omnibus install. The current default is to enable it with the intention to change the default to disabled in a future major release. [1] https://github.com/chef/chef-rfc/blob/f8250a4746d2df530b605ecfaa2dc5ae9b7dc7ff/rfc072-artifact-yanking.md Co-authored-by: Jon Morrow Co-authored-by: Marc A. Paradise Co-authored-by: tyler-ball Signed-off-by: Robb Kidd --- .../omnibus-supermarket/attributes/default.rb | 6 ++ .../app/authorizers/cookbook_authorizer.rb | 2 +- .../api/v1/cookbook_uploads_controller.rb | 8 +- src/supermarket/config/locales/en.yml | 2 + .../spec/api/cookbook_destroy_spec.rb | 50 +++++++---- .../spec/api/cookbook_version_destroy_spec.rb | 26 +++++- .../authorizers/cookbook_authorizer_spec.rb | 9 +- .../v1/cookbook_uploads_controller_spec.rb | 84 +++++++++++++++++-- 8 files changed, 159 insertions(+), 28 deletions(-) diff --git a/omnibus/cookbooks/omnibus-supermarket/attributes/default.rb b/omnibus/cookbooks/omnibus-supermarket/attributes/default.rb index fde5bd185..b262db007 100644 --- a/omnibus/cookbooks/omnibus-supermarket/attributes/default.rb +++ b/omnibus/cookbooks/omnibus-supermarket/attributes/default.rb @@ -339,6 +339,12 @@ default['supermarket']['api_item_limit'] = 100 default['supermarket']['rails_log_to_stdout'] = true +# Allow owners to remove their cookbooks, cookbook versions, or tools. +# Added as a step towards implementing RFC072 Artifact Yanking +# https://github.com/chef/chef-rfc/blob/f8250a4746d2df530b605ecfaa2dc5ae9b7dc7ff/rfc072-artifact-yanking.md +# recommend false; set to true as default for backward compatibility +default['supermarket']['owners_can_remove_artifacts'] = true + # ### Chef URL Settings # # URLs for various links used within Supermarket diff --git a/src/supermarket/app/authorizers/cookbook_authorizer.rb b/src/supermarket/app/authorizers/cookbook_authorizer.rb index 6211db53a..34eee093d 100644 --- a/src/supermarket/app/authorizers/cookbook_authorizer.rb +++ b/src/supermarket/app/authorizers/cookbook_authorizer.rb @@ -10,7 +10,7 @@ def create? # Owners of a cookbook can destroy a cookbook. # def destroy? - owner? + ENV['OWNERS_CAN_REMOVE_ARTIFACTS'] == 'true' ? owner_or_admin? : admin? end # diff --git a/src/supermarket/app/controllers/api/v1/cookbook_uploads_controller.rb b/src/supermarket/app/controllers/api/v1/cookbook_uploads_controller.rb index a2a3e70ef..106758e7a 100644 --- a/src/supermarket/app/controllers/api/v1/cookbook_uploads_controller.rb +++ b/src/supermarket/app/controllers/api/v1/cookbook_uploads_controller.rb @@ -85,7 +85,13 @@ def destroy begin authorize! @cookbook rescue Pundit::NotAuthorizedError - error({}, 403) + error( + { + error_code: t('api.error_codes.forbidden'), + error_messages: [t('api.error_messages.unauthorized_destroy_error')] + }, + 403 + ) else @latest_cookbook_version_url = api_v1_cookbook_version_url( @cookbook, @cookbook.latest_cookbook_version diff --git a/src/supermarket/config/locales/en.yml b/src/supermarket/config/locales/en.yml index 897079763..4d46cc30f 100644 --- a/src/supermarket/config/locales/en.yml +++ b/src/supermarket/config/locales/en.yml @@ -110,6 +110,7 @@ en: invalid_data: "INVALID_DATA" authentication_failed: "AUTHENTICATION_FAILED" unauthorized: "UNAUTHORIZED" + forbidden: "FORBIDDEN" conflict: "CONFLICT" error_messages: privacy_violation: 'Private cookbook upload not allowed' @@ -132,6 +133,7 @@ en: authentication_request_error: "Authentication failed due to an invalid signed request." authentication_key_error: "Authentication failed due to an invalid public/private key pair. If you have changed your keys recently try logging out and logging back in to Supermarket." unauthorized_upload_error: "You are not authorized to upload this cookbook." + unauthorized_destroy_error: "You are not authorized to destroy this cookbook. If you are the cookbook owner, this Supermarket has disabled artifact removal by owner per Chef RFC072. Contact the administrators if there are concerns." unauthorized_post_error: "You are not authorized to POST to this end-point" missing_public_key_error: "Before you can perform knife activities that require authentication, you must log into %{current_host} and grant it permission to access your Chef account information." negative_parameter: "Start and item parameters must be positive numbers but they are start: %{start} and items: %{items}." diff --git a/src/supermarket/spec/api/cookbook_destroy_spec.rb b/src/supermarket/spec/api/cookbook_destroy_spec.rb index c898b1e6b..8795af0d3 100644 --- a/src/supermarket/spec/api/cookbook_destroy_spec.rb +++ b/src/supermarket/spec/api/cookbook_destroy_spec.rb @@ -3,28 +3,44 @@ describe 'DELETE /api/v1/cookbooks/:cookbook' do let(:user) { create(:user) } - context 'the cookbook exists' do - let(:cookbook_metadata_signature) do - { - 'name' => 'redis-test', - 'maintainer' => user.username, - 'external_url' => nil, - 'source_url' => nil, - 'issues_url' => nil, - 'description' => 'Installs/Configures redis-test', - 'average_rating' => nil, - 'category' => 'Other', - 'latest_version' => 'http://www.example.com/api/v1/cookbooks/redis-test/versions/1.0.0', - 'up_for_adoption' => nil - } - end + let(:cookbook_metadata_signature) do + { + 'name' => 'redis-test', + 'maintainer' => user.username, + 'external_url' => nil, + 'source_url' => nil, + 'issues_url' => nil, + 'description' => 'Installs/Configures redis-test', + 'average_rating' => nil, + 'category' => 'Other', + 'latest_version' => 'http://www.example.com/api/v1/cookbooks/redis-test/versions/1.0.0', + 'up_for_adoption' => nil + } + end + context 'from the cookbook owner' do before do share_cookbook('redis-test', user) unshare_cookbook('redis-test', user) end - it 'returns a 200' do + it 'is not authorized' do + expect(response.status.to_i).to eql(403) + end + + it 'returns an error message' do + expect(json_body['error_messages']).to_not be_nil + end + end + + context 'from an admin' do + let(:admin) { create(:admin) } + before do + share_cookbook('redis-test', user) + unshare_cookbook('redis-test', admin) + end + + it 'is authorized' do expect(response.status.to_i).to eql(200) end @@ -33,7 +49,7 @@ end end - context "the cookbook doesn't exist" do + context "when the cookbook doesn't exist" do before { unshare_cookbook('mamimi', user) } it 'returns a 404' do diff --git a/src/supermarket/spec/api/cookbook_version_destroy_spec.rb b/src/supermarket/spec/api/cookbook_version_destroy_spec.rb index da2e06020..7eaa6cbe3 100644 --- a/src/supermarket/spec/api/cookbook_version_destroy_spec.rb +++ b/src/supermarket/spec/api/cookbook_version_destroy_spec.rb @@ -3,13 +3,37 @@ describe 'DELETE /api/v1/cookbooks/:cookbook/versions/:version' do let(:user) { create(:user) } - context 'the cookbook version exists' do + context 'from the cookbook owner' do before do share_cookbook('redis-test', user, custom_metadata: { version: '1.1.0' }) share_cookbook('redis-test', user, custom_metadata: { version: '1.2.0' }) unshare_cookbook_version('redis-test', '1.2.0', user) end + it 'returns a 403' do + expect(response.status.to_i).to eql(403) + end + + it 'does not destroy the cookbook version' do + get '/api/v1/cookbooks/redis-test/versions/1.2.0' + expect(response.status.to_i).to eql(200) + end + + it 'does not destroy other cookbook versions' do + get '/api/v1/cookbooks/redis-test/versions/1.1.0' + expect(json_body.to_s).to match(/\"1.1.0\"/) + end + end + + context 'from an admin' do + let(:admin) { create(:admin) } + + before do + share_cookbook('redis-test', user, custom_metadata: { version: '1.1.0' }) + share_cookbook('redis-test', user, custom_metadata: { version: '1.2.0' }) + unshare_cookbook_version('redis-test', '1.2.0', admin) + end + it 'returns a 200' do expect(response.status.to_i).to eql(200) end diff --git a/src/supermarket/spec/authorizers/cookbook_authorizer_spec.rb b/src/supermarket/spec/authorizers/cookbook_authorizer_spec.rb index 791c0dcad..d09ba9dc9 100644 --- a/src/supermarket/spec/authorizers/cookbook_authorizer_spec.rb +++ b/src/supermarket/spec/authorizers/cookbook_authorizer_spec.rb @@ -8,8 +8,13 @@ subject { described_class.new(user, record) } + context 'when configuration permits owner artifact removal' do + before { allow(ENV).to receive(:[]).with('OWNERS_CAN_REMOVE_ARTIFACTS').and_return('true') } + it { should permit_authorization(:destroy) } + end + it { should permit_authorization(:create) } - it { should permit_authorization(:destroy) } + it { should_not permit_authorization(:destroy) } it { should permit_authorization(:create_collaborator) } it { should permit_authorization(:manage_cookbook_urls) } it { should permit_authorization(:deprecate) } @@ -64,7 +69,7 @@ it { should permit_authorization(:toggle_featured) } it { should permit_authorization(:manage) } it { should_not permit_authorization(:create) } - it { should_not permit_authorization(:destroy) } + it { should permit_authorization(:destroy) } it { should permit_authorization(:create_collaborator) } it { should permit_authorization(:manage_cookbook_urls) } it { should permit_authorization(:manage_adoption) } diff --git a/src/supermarket/spec/controllers/api/v1/cookbook_uploads_controller_spec.rb b/src/supermarket/spec/controllers/api/v1/cookbook_uploads_controller_spec.rb index 0b9b32221..09462906a 100644 --- a/src/supermarket/spec/controllers/api/v1/cookbook_uploads_controller_spec.rb +++ b/src/supermarket/spec/controllers/api/v1/cookbook_uploads_controller_spec.rb @@ -124,16 +124,14 @@ end describe '#destroy' do + let(:user) { create(:user) } + before do allow(subject).to receive(:authenticate_user!) { true } - allow(subject).to receive(:current_user) { create(:user) } + allow(subject).to receive(:current_user) { user } end - context 'when a cookbook exists' do - let!(:cookbook) { create(:cookbook) } - let(:unshare) { delete :destroy, params: { cookbook: cookbook.name, format: :json } } - before { auto_authorize!(Cookbook, 'destroy') } - + shared_examples 'authorized to destroy cookbook' do it 'sends the cookbook to the view' do unshare expect(assigns[:cookbook]).to eql(cookbook) @@ -163,6 +161,80 @@ end end + shared_examples 'not authorized to destroy cookbook' do + it 'sends the cookbook to the view' do + unshare + expect(assigns[:cookbook]).to eql(cookbook) + end + + it 'responds with unauthorized' do + unshare + expect(response.status.to_i).to eql(403) + end + + it 'does not destroy a cookbook' do + expect { unshare }.not_to change(Cookbook, :count) + end + + it 'does not destroy all associated cookbook versions' do + expect { unshare }.not_to change(CookbookVersion, :count) + end + + it 'does not kick off a deletion process in a worker' do + expect(CookbookDeletionWorker).not_to receive(:perform_async) + unshare + end + + it 'does not regenerate the universe cache' do + expect(UniverseCache).not_to receive(:flush) + unshare + end + end + + context 'when a cookbook exists' do + context 'and the current user is the owner' do + let!(:cookbook) { create(:cookbook, owner: user) } + let(:unshare) { delete :destroy, params: { cookbook: cookbook.name, format: :json } } + context 'and owners are allowed to remove cookbooks' do + before do + allow(ENV).to receive(:[]).with('OWNERS_CAN_REMOVE_ARTIFACTS').and_return('true') + end + + it_behaves_like 'authorized to destroy cookbook' + end + + context 'and owners are not allowed to remove cookbooks' do + before do + allow(ENV).to receive(:[]).with('OWNERS_CAN_REMOVE_ARTIFACTS').and_return('false') + end + + it_behaves_like 'not authorized to destroy cookbook' + end + end + + context 'and the current user is an admin' do + let(:user) { create(:admin) } + let!(:cookbook) { create(:cookbook) } + let(:unshare) { delete :destroy, params: { cookbook: cookbook.name, format: :json } } + + context 'and owners are allowed to remove cookbooks' do + before do + allow(ENV).to receive(:[]).with('OWNERS_CAN_REMOVE_ARTIFACTS').and_return('true') + end + + it_behaves_like 'authorized to destroy cookbook' + end + + context 'and owners are not allowed to remove cookbooks' do + before do + allow(ENV).to receive(:[]).with('OWNERS_CAN_REMOVE_ARTIFACTS').and_return('false') + end + + it_behaves_like 'authorized to destroy cookbook' + end + end + end + context 'when the user is not authorized to destroy the cookbook' do let!(:cookbook) { create(:cookbook) } let(:unshare) { delete :destroy, params: { cookbook: cookbook.name, format: :json } }