Make split deletion only accessible to owning app

Author: jmileham

app/controllers/api/v1/split_configs_controller.rb

@@ -9,7 +9,7 @@ def create
   end
 
   def destroy
-    split = Split.find_by!(name: params[:id])
+    split = current_app.splits.find_by!(name: params[:id])
     split.update!(finished_at: Time.zone.now) unless split.finished?
     head :no_content
   end

app/controllers/authenticated_api_controller.rb

@@ -5,7 +5,7 @@ class AuthenticatedApiController < UnauthenticatedApiController
   attr_reader :current_app
 
   def authenticate
-    authenticate_with_http_basic do |app_name, auth_secret|
+    authenticate_or_request_with_http_basic do |app_name, auth_secret|
       app = App.find_by(name: app_name)
       if app && ActiveSupport::SecurityUtils.secure_compare(app.auth_secret, auth_secret)
         @current_app = app

spec/controllers/api/v1/split_configs_controller_spec.rb

@@ -3,53 +3,73 @@
 RSpec.describe Api::V1::SplitConfigsController, type: :controller do
   let(:default_app) { FactoryGirl.create :app, name: "default_app", auth_secret: "6Sd6T7T6Q8hKcoo0t8CTzV0IdN1EEHqXB2Ig4raZsOU" }
 
-  before do
-    http_authenticate username: default_app.name, auth_secret: default_app.auth_secret
-  end
-
   describe '#create' do
-    it "creates a new split with desired weightings" do
+    it "doesn't create when unauthenticated" do
       post :create, name: 'foobar', weighting_registry: { foo: 10, bar: 90 }
 
-      expect(response).to have_http_status(:no_content)
-      split = Split.find_by(name: 'foobar', owner_app: default_app)
-      expect(split).to be_truthy
-      expect(split.registry).to eq 'foo' => 10, 'bar' => 90
+      expect(response).to have_http_status(:unauthorized)
+      expect(Split.where(name: 'foobar')).to be_empty
     end
+  end
 
-    it 'returns errors when invalid' do
-      post :create, name: 'fooBar', weighting_registry: { foo: 10, bar: 90 }
-      expect(response).to have_http_status(:unprocessable_entity)
-      expect(response_json['errors']['name'].first).to include("snake_case")
+  describe "while authenticated" do
+    before do
+      http_authenticate username: default_app.name, auth_secret: default_app.auth_secret
     end
-  end
 
-  describe '#destroy' do
-    it "sets the finished_at time on the split" do
-      split = FactoryGirl.create(:split, name: "old_split", owner_app: default_app, finished_at: nil)
+    describe '#create' do
+      it "creates a new split with desired weightings" do
+        post :create, name: 'foobar', weighting_registry: { foo: 10, bar: 90 }
 
-      delete :destroy, id: "old_split"
+        expect(response).to have_http_status(:no_content)
+        split = Split.find_by(name: 'foobar', owner_app: default_app)
+        expect(split).to be_truthy
+        expect(split.registry).to eq 'foo' => 10, 'bar' => 90
+      end
 
-      expect(response).to have_http_status(:no_content)
-      expect(split.reload).to be_finished
+      it 'returns errors when invalid' do
+        post :create, name: 'fooBar', weighting_registry: { foo: 10, bar: 90 }
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response_json['errors']['name'].first).to include("snake_case")
+      end
     end
 
-    it "is idempotent" do
-      split = FactoryGirl.create(:split, name: "old_split", owner_app: default_app, finished_at: nil)
+    describe '#destroy' do
+      it "sets the finished_at time on the split" do
+        split = FactoryGirl.create(:split, name: "old_split", owner_app: default_app, finished_at: nil)
 
-      Timecop.freeze(Time.zone.parse('2011-01-01')) do
         delete :destroy, id: "old_split"
+
+        expect(response).to have_http_status(:no_content)
+        expect(split.reload).to be_finished
       end
 
-      expect(response).to have_http_status(:no_content)
-      expect(split.reload.finished_at).to eq Time.zone.parse('2011-01-01')
+      it "can't delete another app's split" do
+        other_app = FactoryGirl.create :app, name: "other_app"
+        split = FactoryGirl.create(:split, name: "other_split", owner_app: other_app, finished_at: nil)
 
-      Timecop.freeze(Time.zone.parse('2011-01-02')) do
-        delete :destroy, id: "old_split"
+        expect { delete :destroy, id: "other_split" }.to raise_error(ActiveRecord::RecordNotFound)
+
+        expect(split.reload).not_to be_finished
       end
 
-      expect(response).to have_http_status(:no_content)
-      expect(split.reload.finished_at).to eq Time.zone.parse('2011-01-01')
+      it "is idempotent" do
+        split = FactoryGirl.create(:split, name: "old_split", owner_app: default_app, finished_at: nil)
+
+        Timecop.freeze(Time.zone.parse('2011-01-01')) do
+          delete :destroy, id: "old_split"
+        end
+
+        expect(response).to have_http_status(:no_content)
+        expect(split.reload.finished_at).to eq Time.zone.parse('2011-01-01')
+
+        Timecop.freeze(Time.zone.parse('2011-01-02')) do
+          delete :destroy, id: "old_split"
+        end
+
+        expect(response).to have_http_status(:no_content)
+        expect(split.reload.finished_at).to eq Time.zone.parse('2011-01-01')
+      end
     end
   end
 end