From 999b7e553b1e1e04c2be4289b94557813f140cfe Mon Sep 17 00:00:00 2001 From: David Turner Date: Thu, 21 Sep 2017 16:40:17 -0400 Subject: [PATCH 1/3] remove unused from_gitaly method --- lib/gitlab/git/user.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb index ea634d39668..e142992f324 100644 --- a/lib/gitlab/git/user.rb +++ b/lib/gitlab/git/user.rb @@ -7,10 +7,6 @@ module Gitlab new(gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user)) end - def self.from_gitaly(gitaly_user) - new(gitaly_user.name, gitaly_user.email, gitaly_user.gl_id) - end - def initialize(name, email, gl_id) @name = name @email = email From dbcf48af8b21c0f1e54b73ea421911028081e1c1 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 2 Aug 2017 14:14:50 -0400 Subject: [PATCH 2/3] Add username as GL_USERNAME in hooks (http) When calling pre-receive, post-receive, and update hooks, add the GitLab username as the GL_USERNAME environment variable. This patch only handles cases where pushes are over http, or via the web interface. Later, we will address the ssh case. --- changelogs/unreleased/remote_user.yml | 4 ++++ lib/gitlab/git/hook.rb | 18 ++++++++++++------ lib/gitlab/git/hooks_service.rb | 15 ++++++++------- lib/gitlab/git/user.rb | 9 +++++---- spec/lib/gitlab/git/hook_spec.rb | 8 +++++--- spec/lib/gitlab/git/hooks_service_spec.rb | 2 +- spec/lib/gitlab/git/user_spec.rb | 18 ++++++++++-------- spec/lib/gitlab/workhorse_spec.rb | 14 ++++++++++++-- spec/models/repository_spec.rb | 6 +++--- 9 files changed, 60 insertions(+), 34 deletions(-) create mode 100644 changelogs/unreleased/remote_user.yml diff --git a/changelogs/unreleased/remote_user.yml b/changelogs/unreleased/remote_user.yml new file mode 100644 index 00000000000..75a941fa95f --- /dev/null +++ b/changelogs/unreleased/remote_user.yml @@ -0,0 +1,4 @@ +--- +title: Add username as GL_USERNAME in hooks +merge_request: +author: diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index 208e4bbaf60..8ccb3e64636 100644 --- a/lib/gitlab/git/hook.rb +++ b/lib/gitlab/git/hook.rb @@ -22,22 +22,22 @@ module Gitlab File.exist?(path) end - def trigger(gl_id, oldrev, newrev, ref) + def trigger(gl_id, gl_username, oldrev, newrev, ref) return [true, nil] unless exists? Bundler.with_clean_env do case name when "pre-receive", "post-receive" - call_receive_hook(gl_id, oldrev, newrev, ref) + call_receive_hook(gl_id, gl_username, oldrev, newrev, ref) when "update" - call_update_hook(gl_id, oldrev, newrev, ref) + call_update_hook(gl_id, gl_username, oldrev, newrev, ref) end end end private - def call_receive_hook(gl_id, oldrev, newrev, ref) + def call_receive_hook(gl_id, gl_username, oldrev, newrev, ref) changes = [oldrev, newrev, ref].join(" ") exit_status = false @@ -45,6 +45,7 @@ module Gitlab vars = { 'GL_ID' => gl_id, + 'GL_USERNAME' => gl_username, 'PWD' => repo_path, 'GL_PROTOCOL' => GL_PROTOCOL, 'GL_REPOSITORY' => repository.gl_repository @@ -80,9 +81,14 @@ module Gitlab [exit_status, exit_message] end - def call_update_hook(gl_id, oldrev, newrev, ref) + def call_update_hook(gl_id, gl_username, oldrev, newrev, ref) Dir.chdir(repo_path) do - stdout, stderr, status = Open3.capture3({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev) + stdout, stderr, status = Open3.capture3( + { + 'GL_ID' => gl_id, + 'GL_USERNAME' => gl_username + }, + path, ref, oldrev, newrev) [status.success?, (stderr.presence || stdout).gsub(/\R/, "
").html_safe] end end diff --git a/lib/gitlab/git/hooks_service.rb b/lib/gitlab/git/hooks_service.rb index ea8a87a1290..c327e9b1616 100644 --- a/lib/gitlab/git/hooks_service.rb +++ b/lib/gitlab/git/hooks_service.rb @@ -5,12 +5,13 @@ module Gitlab attr_accessor :oldrev, :newrev, :ref - def execute(committer, repository, oldrev, newrev, ref) - @repository = repository - @gl_id = committer.gl_id - @oldrev = oldrev - @newrev = newrev - @ref = ref + def execute(pusher, repository, oldrev, newrev, ref) + @repository = repository + @gl_id = pusher.gl_id + @gl_username = pusher.name + @oldrev = oldrev + @newrev = newrev + @ref = ref %w(pre-receive update).each do |hook_name| status, message = run_hook(hook_name) @@ -29,7 +30,7 @@ module Gitlab def run_hook(name) hook = Gitlab::Git::Hook.new(name, @repository) - hook.trigger(@gl_id, oldrev, newrev, ref) + hook.trigger(@gl_id, @gl_username, oldrev, newrev, ref) end end end diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb index e142992f324..cb1af5f3b7c 100644 --- a/lib/gitlab/git/user.rb +++ b/lib/gitlab/git/user.rb @@ -1,20 +1,21 @@ module Gitlab module Git class User - attr_reader :name, :email, :gl_id + attr_reader :username, :name, :email, :gl_id def self.from_gitlab(gitlab_user) - new(gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user)) + new(gitlab_user.username, gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user)) end - def initialize(name, email, gl_id) + def initialize(username, name, email, gl_id) + @username = username @name = name @email = email @gl_id = gl_id end def ==(other) - [name, email, gl_id] == [other.name, other.email, other.gl_id] + [username, name, email, gl_id] == [other.username, other.name, other.email, other.gl_id] end end end diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb index 0ff4f3bd105..2fe1f5603ce 100644 --- a/spec/lib/gitlab/git/hook_spec.rb +++ b/spec/lib/gitlab/git/hook_spec.rb @@ -14,6 +14,7 @@ describe Gitlab::Git::Hook do let(:repo_path) { repository.path } let(:user) { create(:user) } let(:gl_id) { Gitlab::GlId.gl_id(user) } + let(:gl_username) { user.username } def create_hook(name) FileUtils.mkdir_p(File.join(repo_path, 'hooks')) @@ -42,6 +43,7 @@ describe Gitlab::Git::Hook do let(:env) do { 'GL_ID' => gl_id, + 'GL_USERNAME' => gl_username, 'PWD' => repo_path, 'GL_PROTOCOL' => 'web', 'GL_REPOSITORY' => gl_repository @@ -59,7 +61,7 @@ describe Gitlab::Git::Hook do .with(env, hook_path, chdir: repo_path).and_call_original end - status, errors = hook.trigger(gl_id, blank, blank, ref) + status, errors = hook.trigger(gl_id, gl_username, blank, blank, ref) expect(status).to be true expect(errors).to be_blank end @@ -72,7 +74,7 @@ describe Gitlab::Git::Hook do blank = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' - status, errors = hook.trigger(gl_id, blank, blank, ref) + status, errors = hook.trigger(gl_id, gl_username, blank, blank, ref) expect(status).to be false expect(errors).to eq("error message from the hook
error message from the hook line 2
") end @@ -86,7 +88,7 @@ describe Gitlab::Git::Hook do blank = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' - status, errors = hook.trigger(gl_id, blank, blank, ref) + status, errors = hook.trigger(gl_id, gl_username, blank, blank, ref) expect(status).to be true expect(errors).to be_nil end diff --git a/spec/lib/gitlab/git/hooks_service_spec.rb b/spec/lib/gitlab/git/hooks_service_spec.rb index d4d75b66659..51e4e3fdad1 100644 --- a/spec/lib/gitlab/git/hooks_service_spec.rb +++ b/spec/lib/gitlab/git/hooks_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::Git::HooksService, seed_helper: true do - let(:user) { Gitlab::Git::User.new('Jane Doe', 'janedoe@example.com', 'user-456') } + let(:user) { Gitlab::Git::User.new('janedoe', 'Jane Doe', 'janedoe@example.com', 'user-456') } let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, 'project-123') } let(:service) { described_class.new } diff --git a/spec/lib/gitlab/git/user_spec.rb b/spec/lib/gitlab/git/user_spec.rb index 0ebcecb26c0..ab64b041187 100644 --- a/spec/lib/gitlab/git/user_spec.rb +++ b/spec/lib/gitlab/git/user_spec.rb @@ -1,22 +1,24 @@ require 'spec_helper' describe Gitlab::Git::User do + let(:username) { 'janedo' } let(:name) { 'Jane Doe' } let(:email) { 'janedoe@example.com' } let(:gl_id) { 'user-123' } - subject { described_class.new(name, email, gl_id) } + subject { described_class.new(username, name, email, gl_id) } describe '#==' do - def eq_other(name, email, gl_id) - eq(described_class.new(name, email, gl_id)) + def eq_other(username, name, email, gl_id) + eq(described_class.new(username, name, email, gl_id)) end - it { expect(subject).to eq_other(name, email, gl_id) } + it { expect(subject).to eq_other(username, name, email, gl_id) } - it { expect(subject).not_to eq_other(nil, nil, nil) } - it { expect(subject).not_to eq_other(name + 'x', email, gl_id) } - it { expect(subject).not_to eq_other(name, email + 'x', gl_id) } - it { expect(subject).not_to eq_other(name, email, gl_id + 'x') } + it { expect(subject).not_to eq_other(nil, nil, nil, nil) } + it { expect(subject).not_to eq_other(username + 'x', name, email, gl_id) } + it { expect(subject).not_to eq_other(username, name + 'x', email, gl_id) } + it { expect(subject).not_to eq_other(username, name, email + 'x', gl_id) } + it { expect(subject).not_to eq_other(username, name, email, gl_id + 'x') } end end diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 5708aa6754f..e56e2df8376 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -182,7 +182,12 @@ describe Gitlab::Workhorse do let(:repo_path) { repository.path_to_repo } let(:action) { 'info_refs' } let(:params) do - { GL_ID: "user-#{user.id}", GL_REPOSITORY: "project-#{project.id}", RepoPath: repo_path } + { + GL_ID: "user-#{user.id}", + GL_USERNAME: user.username, + GL_REPOSITORY: "project-#{project.id}", + RepoPath: repo_path + } end subject { described_class.git_http_ok(repository, false, user, action) } @@ -191,7 +196,12 @@ describe Gitlab::Workhorse do context 'when is_wiki' do let(:params) do - { GL_ID: "user-#{user.id}", GL_REPOSITORY: "wiki-#{project.id}", RepoPath: repo_path } + { + GL_ID: "user-#{user.id}", + GL_USERNAME: user.username, + GL_REPOSITORY: "wiki-#{project.id}", + RepoPath: repo_path + } end subject { described_class.git_http_ok(repository, true, user, action) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index ab81d39691b..869cf51f6a2 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1679,11 +1679,11 @@ describe Repository do tag_sha = tag.target expect(pre_receive_hook).to have_received(:trigger) - .with(anything, anything, commit_sha, anything) + .with(anything, anything, anything, commit_sha, anything) expect(update_hook).to have_received(:trigger) - .with(anything, anything, commit_sha, anything) + .with(anything, anything, anything, commit_sha, anything) expect(post_receive_hook).to have_received(:trigger) - .with(anything, anything, tag_sha, anything) + .with(anything, anything, anything, tag_sha, anything) end end end From 01ce58bde4ddb9bdf3c54dbd2cc65f7a6b81661e Mon Sep 17 00:00:00 2001 From: David Turner Date: Thu, 3 Aug 2017 14:38:33 -0400 Subject: [PATCH 3/3] add username to authorized result, so that gitlab-shell can pass it to hooks --- GITLAB_SHELL_VERSION | 3 ++- lib/api/internal.rb | 7 +++++++ lib/gitlab/git/hook.rb | 11 +++++------ lib/gitlab/workhorse.rb | 2 +- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index da902181863..5508e17c23b 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1,2 @@ -5.9.2 +5.9.3 + diff --git a/lib/api/internal.rb b/lib/api/internal.rb index a0557a609ca..6e78ac2c903 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -31,6 +31,12 @@ module API protocol = params[:protocol] actor.update_last_used_at if actor.is_a?(Key) + user = + if actor.is_a?(Key) + actor.user + else + actor + end access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess access_checker = access_checker_klass @@ -47,6 +53,7 @@ module API { status: true, gl_repository: gl_repository, + gl_username: user&.username, repository_path: repository_path, gitaly: gitaly_payload(params[:action]) } diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index 8ccb3e64636..e29a1f7afa1 100644 --- a/lib/gitlab/git/hook.rb +++ b/lib/gitlab/git/hook.rb @@ -83,12 +83,11 @@ module Gitlab def call_update_hook(gl_id, gl_username, oldrev, newrev, ref) Dir.chdir(repo_path) do - stdout, stderr, status = Open3.capture3( - { - 'GL_ID' => gl_id, - 'GL_USERNAME' => gl_username - }, - path, ref, oldrev, newrev) + env = { + 'GL_ID' => gl_id, + 'GL_USERNAME' => gl_username + } + stdout, stderr, status = Open3.capture3(env, path, ref, oldrev, newrev) [status.success?, (stderr.presence || stdout).gsub(/\R/, "
").html_safe] end end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 17550cf9074..45f246242f1 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -22,9 +22,9 @@ module Gitlab params = { GL_ID: Gitlab::GlId.gl_id(user), GL_REPOSITORY: Gitlab::GlRepository.gl_repository(project, is_wiki), + GL_USERNAME: user&.username, RepoPath: repo_path } - server = { address: Gitlab::GitalyClient.address(project.repository_storage), token: Gitlab::GitalyClient.token(project.repository_storage)