Merge pull request #1789 from chef/robb/disallow-unsharing

RFC072 Artifact Yanking: disallow cookbook removal by owner

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

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
 
   #

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

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}."

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

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

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) }

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 } }