Commit c5a7b76c authored by Valery Sizov's avatar Valery Sizov

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
parents f8453da5 961fe45c
v2.4.0
- Show error message when git access is rejected
v2.2.0 v2.2.0
- Support for custom hooks (Drew Blessing and Jose Kahan) - Support for custom hooks (Drew Blessing and Jose Kahan)
......
require_relative 'gitlab_init' require_relative 'gitlab_init'
require_relative 'gitlab_net' require_relative 'gitlab_net'
require_relative 'gitlab_access_status'
require_relative 'names_helper' require_relative 'names_helper'
require 'json' require 'json'
...@@ -17,13 +18,14 @@ class GitlabAccess ...@@ -17,13 +18,14 @@ class GitlabAccess
end end
def exec def exec
if api.allowed?('git-receive-pack', @repo_name, @actor, @changes) status = api.check_access('git-receive-pack', @repo_name, @actor, @changes)
return true if status.allowed?
true
else else
# reset GL_ID env since we stop git push here # reset GL_ID env since we stop git push here
ENV['GL_ID'] = nil ENV['GL_ID'] = nil
puts "GitLab: You are not allowed to access some of the refs!" puts "GitLab: #{status.message}"
return false false
end end
end end
......
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
...@@ -6,7 +6,7 @@ require_relative 'gitlab_config' ...@@ -6,7 +6,7 @@ require_relative 'gitlab_config'
require_relative 'gitlab_logger' require_relative 'gitlab_logger'
class GitlabNet class GitlabNet
def allowed?(cmd, repo, actor, changes) def check_access(cmd, repo, actor, changes)
project_name = repo.gsub("'", "") project_name = repo.gsub("'", "")
project_name = project_name.gsub(/\.git\Z/, "") project_name = project_name.gsub(/\.git\Z/, "")
project_name = project_name.gsub(/\A\//, "") project_name = project_name.gsub(/\A\//, "")
...@@ -26,7 +26,11 @@ class GitlabNet ...@@ -26,7 +26,11 @@ class GitlabNet
url = "#{host}/allowed" url = "#{host}/allowed"
resp = post(url, params) 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 end
def discover(key) def discover(key)
......
...@@ -60,7 +60,7 @@ class GitlabShell ...@@ -60,7 +60,7 @@ class GitlabShell
end end
def validate_access def validate_access
api.allowed?(@git_cmd, @repo_name, @key_id, '_any') api.check_access(@git_cmd, @repo_name, @key_id, '_any').allowed?
end end
# This method is not covered by Rspec because it ends the current Ruby process. # This method is not covered by Rspec because it ends the current Ruby process.
......
require_relative 'spec_helper' require_relative 'spec_helper'
require_relative '../lib/gitlab_net' require_relative '../lib/gitlab_net'
require_relative '../lib/gitlab_access_status'
describe GitlabNet, vcr: true do describe GitlabNet, vcr: true do
...@@ -43,26 +44,26 @@ describe GitlabNet, vcr: true do ...@@ -43,26 +44,26 @@ describe GitlabNet, vcr: true do
end end
end end
describe :allowed? do describe :check_access do
context 'ssh key with access to project' do context 'ssh key with access to project' do
it 'should allow pull access for dev.gitlab.org' do it 'should allow pull access for dev.gitlab.org' do
VCR.use_cassette("allowed-pull") do VCR.use_cassette("allowed-pull") do
access = gitlab_net.allowed?('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes) access = gitlab_net.check_access('git-receive-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
access.should be_true access.allowed?.should be_true
end end
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 VCR.use_cassette("allowed-pull") do
Net::HTTP::Post.any_instance.should_receive(:set_form_data).with(hash_including(secret_token: 'a123')) 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
end end
it 'should allow push access for dev.gitlab.org' do it 'should allow push access for dev.gitlab.org' do
VCR.use_cassette("allowed-push") do VCR.use_cassette("allowed-push") do
access = gitlab_net.allowed?('git-upload-pack', 'gitlab/gitlabhq.git', 'key-126', changes) access = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'key-126', changes)
access.should be_true access.allowed?.should be_true
end end
end end
end end
...@@ -70,22 +71,22 @@ describe GitlabNet, vcr: true do ...@@ -70,22 +71,22 @@ describe GitlabNet, vcr: true do
context 'ssh key without access to project' do context 'ssh key without access to project' do
it 'should deny pull access for dev.gitlab.org' do it 'should deny pull access for dev.gitlab.org' do
VCR.use_cassette("denied-pull") do VCR.use_cassette("denied-pull") do
access = gitlab_net.allowed?('git-receive-pack', 'gitlab/gitlabhq.git', 'key-2', changes) access = gitlab_net.check_access('git-receive-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
access.should be_false access.allowed?.should be_false
end end
end end
it 'should deny push access for dev.gitlab.org' do it 'should deny push access for dev.gitlab.org' do
VCR.use_cassette("denied-push") do VCR.use_cassette("denied-push") do
access = gitlab_net.allowed?('git-upload-pack', 'gitlab/gitlabhq.git', 'key-2', changes) access = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'key-2', changes)
access.should be_false access.allowed?.should be_false
end end
end end
it 'should deny push access for dev.gitlab.org (with user)' do it 'should deny push access for dev.gitlab.org (with user)' do
VCR.use_cassette("denied-push-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 = gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'user-1', changes)
access.should be_false access.allowed?.should be_false
end end
end end
end end
......
require_relative 'spec_helper' require_relative 'spec_helper'
require_relative '../lib/gitlab_shell' require_relative '../lib/gitlab_shell'
require_relative '../lib/gitlab_access_status'
describe GitlabShell do describe GitlabShell do
subject do subject do
...@@ -12,7 +13,7 @@ describe GitlabShell do ...@@ -12,7 +13,7 @@ describe GitlabShell do
let(:api) do let(:api) do
double(GitlabNet).tap do |api| double(GitlabNet).tap do |api|
api.stub(discover: { 'name' => 'John Doe' }) api.stub(discover: { 'name' => 'John Doe' })
api.stub(allowed?: true) api.stub(check_access: GitAccessStatus.new(true))
end end
end end
let(:key_id) { "key-#{rand(100) + 100}" } let(:key_id) { "key-#{rand(100) + 100}" }
...@@ -140,13 +141,13 @@ describe GitlabShell do ...@@ -140,13 +141,13 @@ describe GitlabShell do
before { ssh_cmd 'git-upload-pack gitlab-ci.git' } before { ssh_cmd 'git-upload-pack gitlab-ci.git' }
after { subject.exec } after { subject.exec }
it "should call api.allowed?" do it "should call api.check_access" do
api.should_receive(:allowed?). api.should_receive(:check_access).
with('git-upload-pack', 'gitlab-ci.git', key_id, '_any') with('git-upload-pack', 'gitlab-ci.git', key_id, '_any')
end end
it "should disallow access and log the attempt if allowed? returns false" do it "should disallow access and log the attempt if check_access returns false status" do
api.stub(allowed?: false) api.stub(check_access: GitAccessStatus.new(false))
message = "gitlab-shell: Access denied for git command <git-upload-pack gitlab-ci.git> " message = "gitlab-shell: Access denied for git command <git-upload-pack gitlab-ci.git> "
message << "by user with key #{key_id}." message << "by user with key #{key_id}."
$logger.should_receive(:warn).with(message) $logger.should_receive(:warn).with(message)
......
...@@ -42,7 +42,7 @@ http_interactions: ...@@ -42,7 +42,7 @@ http_interactions:
- '0.089741' - '0.089741'
body: body:
encoding: UTF-8 encoding: UTF-8
string: 'true' string: '{"status": "true"}'
http_version: http_version:
recorded_at: Wed, 03 Sep 2014 11:27:36 GMT recorded_at: Wed, 03 Sep 2014 11:27:36 GMT
recorded_with: VCR 2.4.0 recorded_with: VCR 2.4.0
...@@ -42,7 +42,7 @@ http_interactions: ...@@ -42,7 +42,7 @@ http_interactions:
- '0.833195' - '0.833195'
body: body:
encoding: UTF-8 encoding: UTF-8
string: 'true' string: '{"status": "true"}'
http_version: http_version:
recorded_at: Wed, 03 Sep 2014 11:27:37 GMT recorded_at: Wed, 03 Sep 2014 11:27:37 GMT
recorded_with: VCR 2.4.0 recorded_with: VCR 2.4.0
...@@ -40,7 +40,7 @@ http_interactions: ...@@ -40,7 +40,7 @@ http_interactions:
- '0.028027' - '0.028027'
body: body:
encoding: UTF-8 encoding: UTF-8
string: '{"message":"404 Not found"}' string: '{"status": false, "message":"404 Not found"}'
http_version: http_version:
recorded_at: Wed, 03 Sep 2014 11:27:38 GMT recorded_at: Wed, 03 Sep 2014 11:27:38 GMT
recorded_with: VCR 2.4.0 recorded_with: VCR 2.4.0
...@@ -42,7 +42,7 @@ http_interactions: ...@@ -42,7 +42,7 @@ http_interactions:
- '0.019640' - '0.019640'
body: body:
encoding: UTF-8 encoding: UTF-8
string: 'false' string: '{"status": false}'
http_version: http_version:
recorded_at: Wed, 03 Sep 2014 11:27:39 GMT recorded_at: Wed, 03 Sep 2014 11:27:39 GMT
recorded_with: VCR 2.4.0 recorded_with: VCR 2.4.0
...@@ -40,7 +40,7 @@ http_interactions: ...@@ -40,7 +40,7 @@ http_interactions:
- '0.015198' - '0.015198'
body: body:
encoding: UTF-8 encoding: UTF-8
string: '{"message":"404 Not found"}' string: '{"status": false, "message":"404 Not found"}'
http_version: http_version:
recorded_at: Wed, 03 Sep 2014 11:27:38 GMT recorded_at: Wed, 03 Sep 2014 11:27:38 GMT
recorded_with: VCR 2.4.0 recorded_with: VCR 2.4.0
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment