Merge branch 'fix-internal-api' into 'master'

Fix internal api

Fixes #1787

See merge request !1280

4 files changed, 67 insertions, 17 deletions

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 1648834f034..180e50611cf 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -33,15 +33,20 @@ module API
           end
 
         project = Project.find_with_namespace(project_path)
-        return false unless project
+
+        unless project
+          return Gitlab::GitAccessStatus.new(false, 'No such project')
+        end
 
         actor = if params[:key_id]
-                  Key.find(params[:key_id])
+                  Key.find_by(id: params[:key_id])
                 elsif params[:user_id]
-                  User.find(params[:user_id])
+                  User.find_by(id: params[:user_id])
                 end
 
-        return false unless actor
+        unless actor
+          return Gitlab::GitAccessStatus.new(false, 'No such user or key')
+        end
 
         access.check(
           actor,
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 3452240dad8..5f8cb19efdf 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -8,15 +8,7 @@ module Gitlab
     def check(actor, cmd, project, changes = nil)
       case cmd
       when *DOWNLOAD_COMMANDS
-        if actor.is_a? User
-          download_access_check(actor, project)
-        elsif actor.is_a? DeployKey
-          actor.projects.include?(project)
-        elsif actor.is_a? Key
-          download_access_check(actor.user, project)
-        else
-          raise 'Wrong actor'
-        end
+        download_access_check(actor, project)
       when *PUSH_COMMANDS
         if actor.is_a? User
           push_access_check(actor, project, changes)
@@ -32,7 +24,23 @@ module Gitlab
       end
     end
 
-    def download_access_check(user, project)
+    def download_access_check(actor, project)
+      if actor.is_a?(User)
+        user_download_access_check(actor, project)
+      elsif actor.is_a?(DeployKey)
+        if actor.projects.include?(project)
+          build_status_object(true)
+        else
+          build_status_object(false, "Deploy key not allowed to access this project")
+        end
+      elsif actor.is_a? Key
+        user_download_access_check(actor.user, project)
+      else
+        raise 'Wrong actor'
+      end
+    end
+
+    def user_download_access_check(user, project)
       if user && user_allowed?(user) && user.can?(:download_code, project)
         build_status_object(true)
       else
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 1addba55787..66e87e57cbc 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -46,6 +46,25 @@ describe Gitlab::GitAccess do
         it { subject.allowed?.should be_false }
       end
     end
+
+    describe 'deploy key permissions' do
+      let(:key) { create(:deploy_key) }
+
+      context 'pull code' do
+        context 'allowed' do
+          before { key.projects << project }
+          subject { access.download_access_check(key, project) }
+
+          it { subject.allowed?.should be_true }
+        end
+
+        context 'denied' do
+          subject { access.download_access_check(key, project) }
+
+          it { subject.allowed?.should be_false }
+        end
+      end
+    end
   end
 
   describe 'push_access_check' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 53b7808d4c3..4faa1f9b964 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -26,7 +26,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /internal/allowed" do
+  describe "POST /internal/allowed" do
     context "access granted" do
       before do
         project.team << [user, :developer]
@@ -140,7 +140,7 @@ describe API::API, api: true  do
           archive(key, project)
 
           response.status.should == 200
-          response.body.should == 'true'
+          JSON.parse(response.body)["status"].should be_true
         end
       end
 
@@ -149,10 +149,28 @@ describe API::API, api: true  do
           archive(key, project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
     end
+
+    context 'project does not exist' do
+      it do
+        pull(key, OpenStruct.new(path_with_namespace: 'gitlab/notexists'))
+
+        response.status.should == 200
+        JSON.parse(response.body)["status"].should be_false
+      end
+    end
+
+    context 'user does not exist' do
+      it do
+        pull(OpenStruct.new(id: 0), project)
+
+        response.status.should == 200
+        JSON.parse(response.body)["status"].should be_false
+      end
+    end
   end
 
   def pull(key, project)