Address feedback from last code review.

- Added new compound unique key for lfs_file_locks
- Improved uniqueness validation for creation of locks.
- Added authorization checks for Lock and Unlock services
- Improve description in controler specs

app/models/lfs_file_lock.rb

@@ -3,7 +3,6 @@ class LfsFileLock < ActiveRecord::Base
   belongs_to :user
 
   validates :project_id, :user_id, :path, presence: true
-  validates :path, uniqueness: { scope: [:project_id] }
 
   def can_be_unlocked_by?(current_user, forced = false)
     return true if current_user.id == user_id

app/services/lfs/lock_file_service.rb

@@ -3,11 +3,15 @@ module Lfs
     prepend EE::Lfs::LockFileService
 
     def execute
-      if current_lock
-        error('already created lock', 409, current_lock)
-      else
-        create_lock!
+      unless can?(current_user, :push_code, project)
+        raise Gitlab::GitAccess::UnauthorizedError, 'You have no permissions'
       end
+
+      create_lock!
+    rescue ActiveRecord::RecordNotUnique
+      error('already created lock', 409, current_lock)
+    rescue Gitlab::GitAccess::UnauthorizedError => ex
+      error(ex.message, 403)
     rescue => ex
       error(ex.message, 500)
     end
@@ -15,7 +19,7 @@ module Lfs
     private
 
     def current_lock
-      @current_lock ||= project.lfs_file_locks.find_by(path: params[:path])
+      project.lfs_file_locks.find_by(path: params[:path])
     end
 
     def create_lock!

app/services/lfs/unlock_file_service.rb

@@ -3,11 +3,15 @@ module Lfs
     prepend EE::Lfs::UnlockFileService
 
     def execute
-      @lock = project.lfs_file_locks.find_by(id: params[:id])
-
-      return error('Lock not found', 404) unless @lock
+      unless can?(current_user, :push_code, project)
+        raise Gitlab::GitAccess::UnauthorizedError, 'You have no permissions'
+      end
 
       unlock_file
+    rescue Gitlab::GitAccess::UnauthorizedError => ex
+      error(ex.message, 403)
+    rescue ActiveRecord::RecordNotFound
+      error('Lock not found', 404)
     rescue => ex
       error(ex.message, 500)
     end
@@ -16,15 +20,16 @@ module Lfs
 
     def unlock_file
       forced = params[:force] == true
+      lock = project.lfs_file_locks.find(params[:id])
 
-      if @lock.can_be_unlocked_by?(current_user, forced)
-        @lock.destroy!
+      if lock.can_be_unlocked_by?(current_user, forced)
+        lock.destroy!
 
-        success(lock: @lock, http_status: :ok)
+        success(lock: lock, http_status: :ok)
       elsif forced
         error('You must have master access to force delete a lock', 403)
       else
-        error("#{@lock.path} is locked by GitLab User #{@lock.user_id}", 403)
+        error("#{lock.path} is locked by GitLab User #{lock.user_id}", 403)
       end
     end
   end

db/migrate/20180116193854_create_lfs_file_locks.rb

@@ -8,5 +8,7 @@ class CreateLfsFileLocks < ActiveRecord::Migration
       t.string :path
       t.datetime :created_at, null: false
     end
+
+    add_index :lfs_file_locks, [:path, :project_id], unique: true
   end
 end

db/schema.rb

@@ -1309,6 +1309,7 @@ ActiveRecord::Schema.define(version: 20180204200836) do
     t.datetime "created_at", null: false
   end
 
+  add_index "lfs_file_locks", ["path", "project_id"], name: "index_lfs_file_locks_on_path_and_project_id", unique: true, using: :btree
   add_index "lfs_file_locks", ["project_id"], name: "index_lfs_file_locks_on_project_id", using: :btree
   add_index "lfs_file_locks", ["user_id"], name: "index_lfs_file_locks_on_user_id", using: :btree
 

spec/models/lfs_file_lock_spec.rb

@@ -10,7 +10,6 @@ describe LfsFileLock do
   it { is_expected.to validate_presence_of(:project_id) }
   it { is_expected.to validate_presence_of(:user_id) }
   it { is_expected.to validate_presence_of(:path) }
-  it { is_expected.to validate_uniqueness_of(:path).scoped_to(:project_id) }
 
   describe '#can_be_unlocked_by?' do
     let(:developer) { create(:user) }

spec/requests/lfs_locks_api_spec.rb

@@ -46,7 +46,7 @@ describe 'Git LFS File Locking API' do
         lock_file('README.md', developer)
       end
 
-      it 'responds with 409' do
+      it 'return an error message' do
         post_lfs_json url, body, headers
 
         expect(response).to have_gitlab_http_status(409)
@@ -54,10 +54,16 @@ describe 'Git LFS File Locking API' do
         expect(json_response.keys).to match_array(%w(lock message documentation_url))
         expect(json_response['message']).to match(/already created lock/)
       end
+
+      it 'returns the existen lock' do
+        post_lfs_json url, body, headers
+
+        expect(json_response['lock']['path']).to eq('README.md')
+      end
     end
 
     context 'without an existent lock' do
-      it 'responds with 201' do
+      it 'creates the lock' do
         post_lfs_json url, body, headers
 
         expect(response).to have_gitlab_http_status(201)
@@ -73,7 +79,7 @@ describe 'Git LFS File Locking API' do
 
     include_examples 'unauthorized request'
 
-    it 'responds with 200' do
+    it 'returns the list of locked files' do
       lock_file('README.md', developer)
       lock_file('README', developer)
 
@@ -92,7 +98,7 @@ describe 'Git LFS File Locking API' do
 
     include_examples 'unauthorized request'
 
-    it 'responds with 200' do
+    it 'returns the list of locked files grouped by owner' do
       lock_file('README.md', master)
       lock_file('README', developer)
 
@@ -115,10 +121,14 @@ describe 'Git LFS File Locking API' do
     include_examples 'unauthorized request'
 
     context 'with an existent lock' do
-      it 'responds with 200' do
+      it 'deletes the lock' do
         post_lfs_json url, nil, headers
 
         expect(response).to have_gitlab_http_status(200)
+      end
+
+      it 'returns the deleted lock' do
+        post_lfs_json url, nil, headers
 
         expect(json_response['lock'].keys).to match_array(%w(id path locked_at owner))
       end

spec/services/lfs/lock_file_service_spec.rb

 require 'spec_helper'
 
 describe Lfs::LockFileService do
-  let(:project) { create(:project) }
-  let(:user)    { create(:user) }
+  let(:project)      { create(:project) }
+  let(:current_user) { create(:user) }
 
-  subject { described_class.new(project, user, params) }
+  subject { described_class.new(project, current_user, params) }
 
   describe '#execute' do
     let(:params) { { path: 'README.md' } }
 
-    context 'with an existent lock' do
-      let!(:lock) { create(:lfs_file_lock, project: project) }
-
+    context 'when not authorized' do
       it "doesn't succeed" do
-        expect(subject.execute[:status]).to eq(:error)
-      end
+        result = subject.execute
 
-      it "doesn't create the Lock" do
-        expect do
-          subject.execute
-        end.not_to change { LfsFileLock.count }
+        expect(result[:status]).to eq(:error)
+        expect(result[:http_status]).to eq(403)
+        expect(result[:message]).to eq('You have no permissions')
       end
     end
 
-    context 'without an existent lock' do
-      it "succeeds" do
-        expect(subject.execute[:status]).to eq(:success)
+    context 'when authorized' do
+      before do
+        project.add_developer(current_user)
       end
 
-      it "creates the Lock" do
-        expect do
-          subject.execute
-        end.to change { LfsFileLock.count }.by(1)
+      context 'with an existent lock' do
+        let!(:lock) { create(:lfs_file_lock, project: project) }
+
+        it "doesn't succeed" do
+          expect(subject.execute[:status]).to eq(:error)
+        end
+
+        it "doesn't create the Lock" do
+          expect do
+            subject.execute
+          end.not_to change { LfsFileLock.count }
+        end
       end
-    end
 
-    context 'when an error is raised' do
-      it "doesn't succeed" do
-        allow_any_instance_of(described_class).to receive(:create_lock!).and_raise(StandardError)
+      context 'without an existent lock' do
+        it "succeeds" do
+          expect(subject.execute[:status]).to eq(:success)
+        end
+
+        it "creates the Lock" do
+          expect do
+            subject.execute
+          end.to change { LfsFileLock.count }.by(1)
+        end
+      end
+
+      context 'when an error is raised' do
+        it "doesn't succeed" do
+          allow_any_instance_of(described_class).to receive(:create_lock!).and_raise(StandardError)
 
-        expect(subject.execute[:status]).to eq(:error)
+          expect(subject.execute[:status]).to eq(:error)
+        end
       end
     end
   end

spec/services/lfs/unlock_file_service_spec.rb

@@ -3,85 +3,101 @@ require 'spec_helper'
 describe Lfs::UnlockFileService do
   let(:project)      { create(:project) }
   let(:current_user) { create(:user) }
-  let(:lock_author) { create(:user) }
+  let(:lock_author)  { create(:user) }
   let!(:lock)        { create(:lfs_file_lock, user: lock_author, project: project) }
   let(:params)       { {} }
 
   subject { described_class.new(project, current_user, params) }
 
   describe '#execute' do
-    context 'when lock does not exists' do
-      let(:params) { { id: 123 } }
+    context 'when not authorized' do
       it "doesn't succeed" do
         result = subject.execute
 
         expect(result[:status]).to eq(:error)
-        expect(result[:http_status]).to eq(404)
+        expect(result[:http_status]).to eq(403)
+        expect(result[:message]).to eq('You have no permissions')
       end
     end
 
-    context 'when unlocked by the author' do
-      let(:current_user) { lock_author }
-      let(:params) { { id: lock.id } }
-
-      it "succeeds" do
-        result = subject.execute
-
-        expect(result[:status]).to eq(:success)
-        expect(result[:lock]).to be_present
+    context 'when authorized' do
+      before do
+        project.add_developer(current_user)
       end
-    end
-
-    context 'when unlocked by a different user' do
-      let(:current_user) { create(:user) }
-      let(:params) { { id: lock.id } }
 
-      it "doesn't succeed" do
-        result = subject.execute
+      context 'when lock does not exists' do
+        let(:params) { { id: 123 } }
+        it "doesn't succeed" do
+          result = subject.execute
 
-        expect(result[:status]).to eq(:error)
-        expect(result[:message]).to match(/is locked by GitLab User #{lock_author.id}/)
-        expect(result[:http_status]).to eq(403)
+          expect(result[:status]).to eq(:error)
+          expect(result[:http_status]).to eq(404)
+        end
       end
-    end
 
-    context 'when forced' do
-      let(:developer) { create(:user) }
-      let(:master)    { create(:user) }
+      context 'when unlocked by the author' do
+        let(:current_user) { lock_author }
+        let(:params) { { id: lock.id } }
 
-      before do
-        project.add_developer(developer)
-        project.add_master(master)
-      end
+        it "succeeds" do
+          result = subject.execute
 
-      context 'by a regular user' do
-        let(:current_user) { developer }
-        let(:params) do
-          { id: lock.id,
-            force: true }
+          expect(result[:status]).to eq(:success)
+          expect(result[:lock]).to be_present
         end
+      end
+
+      context 'when unlocked by a different user' do
+        let(:current_user) { create(:user) }
+        let(:params) { { id: lock.id } }
 
         it "doesn't succeed" do
           result = subject.execute
 
           expect(result[:status]).to eq(:error)
-          expect(result[:message]).to match(/You must have master access/)
+          expect(result[:message]).to match(/is locked by GitLab User #{lock_author.id}/)
           expect(result[:http_status]).to eq(403)
         end
       end
 
-      context 'by a master user' do
-        let(:current_user) { master }
-        let(:params) do
-          { id: lock.id,
-            force: true }
+      context 'when forced' do
+        let(:developer) { create(:user) }
+        let(:master)    { create(:user) }
+
+        before do
+          project.add_developer(developer)
+          project.add_master(master)
         end
 
-        it "succeeds" do
-          result = subject.execute
+        context 'by a regular user' do
+          let(:current_user) { developer }
+          let(:params) do
+            { id: lock.id,
+              force: true }
+          end
 
-          expect(result[:status]).to eq(:success)
-          expect(result[:lock]).to be_present
+          it "doesn't succeed" do
+            result = subject.execute
+
+            expect(result[:status]).to eq(:error)
+            expect(result[:message]).to match(/You must have master access/)
+            expect(result[:http_status]).to eq(403)
+          end
+        end
+
+        context 'by a master user' do
+          let(:current_user) { master }
+          let(:params) do
+            { id: lock.id,
+              force: true }
+          end
+
+          it "succeeds" do
+            result = subject.execute
+
+            expect(result[:status]).to eq(:success)
+            expect(result[:lock]).to be_present
+          end
         end
       end
     end