Merge branch 'git_messages' into 'master'

Better git hook messages

DZ already merged it but we had to revert it because of lack of time to deploy to dev.

See merge request !48

13 files changed, 62 insertions, 31 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4748fc2..2dee545 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,6 @@
+v2.4.0
+  - Show error message when git access is rejected
+
 v2.2.0
   - Support for custom hooks (Drew Blessing and Jose Kahan)
 
diff --git a/VERSION b/VERSION
index 2bf1c1c..197c4d5 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.3.1
+2.4.0
diff --git a/lib/gitlab_access.rb b/lib/gitlab_access.rb
index 78d353c..547b81d 100644
--- a/lib/gitlab_access.rb
+++ b/lib/gitlab_access.rb
@@ -1,5 +1,6 @@
 require_relative 'gitlab_init'
 require_relative 'gitlab_net'
+require_relative 'gitlab_access_status'
 require_relative 'names_helper'
 require 'json'
 
@@ -17,13 +18,14 @@ class GitlabAccess
   end
 
   def exec
-    if api.allowed?('git-receive-pack', @repo_name, @actor, @changes)
-      return true
+    status = api.check_access('git-receive-pack', @repo_name, @actor, @changes)
+    if status.allowed?
+      true
     else
       # reset GL_ID env since we stop git push here
       ENV['GL_ID'] = nil
-      puts "GitLab: You are not allowed to access some of the refs!"
-      return false
+      puts "GitLab: #{status.message}"
+      false
     end
   end
 
diff --git a/lib/gitlab_access_status.rb b/lib/gitlab_access_status.rb
new file mode 100644
index 0000000..597fcbb
--- /dev/null
+++ b/lib/gitlab_access_status.rb
@@ -0,0 +1,20 @@
+require 'json'
+
+class GitAccessStatus
+  attr_accessor :status, :message
+  alias_method :allowed?, :status
+
+  def initialize(status, message = '')
+    @status = status
+    @message = message
+  end
+
+  def self.create_from_json(json)
+    values = JSON.parse(json)
+    self.new(values["status"], values["message"])
+  end
+
+  def to_json
+    {status: @status, message: @message}.to_json
+  end
+end
\ No newline at end of file
diff --git a/lib/gitlab_net.rb b/lib/gitlab_net.rb
index e6478ef..1f27398 100644
--- a/lib/gitlab_net.rb
+++ b/lib/gitlab_net.rb
@@ -6,7 +6,7 @@ require_relative 'gitlab_config'
 require_relative 'gitlab_logger'
 
 class GitlabNet
-  def allowed?(cmd, repo, actor, changes)
+  def check_access(cmd, repo, actor, changes)
     project_name = repo.gsub("'", "")
     project_name = project_name.gsub(/\.git\Z/, "")
     project_name = project_name.gsub(/\A\//, "")
@@ -26,7 +26,11 @@ class GitlabNet
     url = "#{host}/allowed"
     resp = post(url, params)
 
-    !!(resp.code == '200' && resp.body == 'true')
+    if resp.code == '200'
+      GitAccessStatus.create_from_json(resp.body)
+    else
+      GitAccessStatus.new(false, "API is not accesible")
+    end
   end
 
   def discover(key)
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index e2cb2cc..95fad9e 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -60,7 +60,7 @@ class GitlabShell
   end
 
   def validate_access
-    api.allowed?(@git_cmd, @repo_name, @key_id, '_any')
+    api.check_access(@git_cmd, @repo_name, @key_id, '_any').allowed?
   end
 
   # This method is not covered by Rspec because it ends the current Ruby process.
diff --git a/spec/gitlab_net_spec.rb b/spec/gitlab_net_spec.rb
index 9ccabe0..d431ac7 100644
--- a/spec/gitlab_net_spec.rb
+++ b/spec/gitlab_net_spec.rb
@@ -1,5 +1,6 @@
 require_relative 'spec_helper'
 require_relative '../lib/gitlab_net'
+require_relative '../lib/gitlab_access_status'
 
 
 describe GitlabNet, vcr: true do
@@ -43,26 +44,26 @@ describe GitlabNet, vcr: true do
     end
   end
 
-  describe :allowed? do
+  describe :check_access do
     context 'ssh key with access to project' do
       it 'should allow pull access for dev.gitlab.org' do
         VCR.use_cassette("allowed-pull") do
-          access = gitlab_net.allowed?('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
-          access.should be_true
+          access = gitlab_net.check_access('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
+          access.allowed?.should be_true
         end
       end
 
-      it 'adds the secret_token theo request' do
+      it 'adds the secret_token to the request' do
         VCR.use_cassette("allowed-pull") do
           Net::HTTP::Post.any_instance.should_receive(:set_form_data).with(hash_including(secret_token: 'a123'))
-          gitlab_net.allowed?('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
+          gitlab_net.check_access('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
         end
       end
 
       it 'should allow push access for dev.gitlab.org' do
         VCR.use_cassette("allowed-push") do
-          access = gitlab_net.allowed?('git-upload-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
-          access.should be_true
+          access = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
+          access.allowed?.should be_true
         end
       end
     end
@@ -70,22 +71,22 @@ describe GitlabNet, vcr: true do
     context 'ssh key without access to project' do
       it 'should deny pull access for dev.gitlab.org' do
         VCR.use_cassette("denied-pull") do
-          access = gitlab_net.allowed?('git-receive-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
-          access.should be_false
+          access = gitlab_net.check_access('git-receive-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
+          access.allowed?.should be_false
         end
       end
 
       it 'should deny push access for dev.gitlab.org' do
         VCR.use_cassette("denied-push") do
-          access = gitlab_net.allowed?('git-upload-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
-          access.should be_false
+          access = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
+          access.allowed?.should be_false
         end
       end
 
       it 'should deny push access for dev.gitlab.org (with user)' do
         VCR.use_cassette("denied-push-with-user") do
-          access = gitlab_net.allowed?('git-upload-pack', 'gitlab/gitlabhq.git', 'user-1', changes)
-          access.should be_false
+          access = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'user-1', changes)
+          access.allowed?.should be_false
         end
       end
     end
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 4741303..5df2391 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -1,5 +1,6 @@
 require_relative 'spec_helper'
 require_relative '../lib/gitlab_shell'
+require_relative '../lib/gitlab_access_status'
 
 describe GitlabShell do
   subject do
@@ -12,7 +13,7 @@ describe GitlabShell do
   let(:api) do
     double(GitlabNet).tap do |api|
       api.stub(discover: { 'name' => 'John Doe' })
-      api.stub(allowed?: true)
+      api.stub(check_access: GitAccessStatus.new(true))
     end
   end
   let(:key_id) { "key-#{rand(100) + 100}" }
@@ -140,13 +141,13 @@ describe GitlabShell do
     before { ssh_cmd 'git-upload-pack gitlab-ci.git' }
     after { subject.exec }
 
-    it "should call api.allowed?" do
-      api.should_receive(:allowed?).
+    it "should call api.check_access" do
+      api.should_receive(:check_access).
         with('git-upload-pack', 'gitlab-ci.git', key_id, '_any')
     end
 
-    it "should disallow access and log the attempt if allowed? returns false" do
-      api.stub(allowed?: false)
+    it "should disallow access and log the attempt if check_access returns false status" do
+      api.stub(check_access: GitAccessStatus.new(false))
       message = "gitlab-shell: Access denied for git command <git-upload-pack gitlab-ci.git> "
       message << "by user with key #{key_id}."
       $logger.should_receive(:warn).with(message)
diff --git a/spec/vcr_cassettes/allowed-pull.yml b/spec/vcr_cassettes/allowed-pull.yml
index 337b00f..5a10ec9 100644
--- a/spec/vcr_cassettes/allowed-pull.yml
+++ b/spec/vcr_cassettes/allowed-pull.yml
@@ -42,7 +42,7 @@ http_interactions:
       - '0.089741'
     body:
       encoding: UTF-8
-      string: 'true'
+      string: '{"status": "true"}'
     http_version: 
   recorded_at: Wed, 03 Sep 2014 11:27:36 GMT
 recorded_with: VCR 2.4.0
diff --git a/spec/vcr_cassettes/allowed-push.yml b/spec/vcr_cassettes/allowed-push.yml
index cb757bf..a75c2db 100644
--- a/spec/vcr_cassettes/allowed-push.yml
+++ b/spec/vcr_cassettes/allowed-push.yml
@@ -42,7 +42,7 @@ http_interactions:
       - '0.833195'
     body:
       encoding: UTF-8
-      string: 'true'
+      string: '{"status": "true"}'
     http_version: 
   recorded_at: Wed, 03 Sep 2014 11:27:37 GMT
 recorded_with: VCR 2.4.0
diff --git a/spec/vcr_cassettes/denied-pull.yml b/spec/vcr_cassettes/denied-pull.yml
index 9941e70..8535b4e 100644
--- a/spec/vcr_cassettes/denied-pull.yml
+++ b/spec/vcr_cassettes/denied-pull.yml
@@ -40,7 +40,7 @@ http_interactions:
       - '0.028027'
     body:
       encoding: UTF-8
-      string: '{"message":"404 Not found"}'
+      string: '{"status": false, "message":"404 Not found"}'
     http_version: 
   recorded_at: Wed, 03 Sep 2014 11:27:38 GMT
 recorded_with: VCR 2.4.0
diff --git a/spec/vcr_cassettes/denied-push-with-user.yml b/spec/vcr_cassettes/denied-push-with-user.yml
index 4694797..101a868 100644
--- a/spec/vcr_cassettes/denied-push-with-user.yml
+++ b/spec/vcr_cassettes/denied-push-with-user.yml
@@ -42,7 +42,7 @@ http_interactions:
       - '0.019640'
     body:
       encoding: UTF-8
-      string: 'false'
+      string: '{"status": false}'
     http_version: 
   recorded_at: Wed, 03 Sep 2014 11:27:39 GMT
 recorded_with: VCR 2.4.0
diff --git a/spec/vcr_cassettes/denied-push.yml b/spec/vcr_cassettes/denied-push.yml
index fc0a309..53ccc57 100644
--- a/spec/vcr_cassettes/denied-push.yml
+++ b/spec/vcr_cassettes/denied-push.yml
@@ -40,7 +40,7 @@ http_interactions:
       - '0.015198'
     body:
       encoding: UTF-8
-      string: '{"message":"404 Not found"}'
+      string: '{"status": false, "message":"404 Not found"}'
     http_version: 
   recorded_at: Wed, 03 Sep 2014 11:27:38 GMT
 recorded_with: VCR 2.4.0