Skip to content
Snippets Groups Projects
Commit 301e4272 authored by Mario Celi's avatar Mario Celi :palm_tree: Committed by GitLab Release Tools Bot
Browse files

Don't allow unauthorized users to close issues automatically

Merge branch 'security-508742-fix-auto-close-issues-17-5' into '17-5-stable-ee'

See merge request gitlab-org/security/gitlab!4676

Changelog: security
parent a8c14a95
No related branches found
No related tags found
No related merge requests found
...@@ -37,7 +37,18 @@ def perform(project_id, issue_id, issue_type, params = {}) ...@@ -37,7 +37,18 @@ def perform(project_id, issue_id, issue_type, params = {})
author = User.find_by_id(params["closed_by"]) author = User.find_by_id(params["closed_by"])
unless author unless author
logger.info(structured_payload(message: "User not found.", user_id: params["closed_by"])) logger.info(structured_payload(message: "Author not found.", user_id: params["closed_by"]))
return
end
user = User.find_by_id(params["user_id"])
# Only authorizing if user is present for backwards compatibility.
# TODO: Remove with https://gitlab.com/gitlab-org/gitlab/-/work_items/509422
if user && !issue.is_a?(ExternalIssue) && !user.can?(:update_issue, issue)
logger.info(
structured_payload(message: "User cannot update issue.", user_id: params["user_id"], issue_id: issue_id)
)
return return
end end
......
...@@ -57,7 +57,12 @@ def close_issues(project, user, author, commit, issues) ...@@ -57,7 +57,12 @@ def close_issues(project, user, author, commit, issues)
Issues::CloseWorker.bulk_perform_async_with_contexts( Issues::CloseWorker.bulk_perform_async_with_contexts(
issues, issues,
arguments_proc: ->(issue) { arguments_proc: ->(issue) {
[project.id, issue.id, issue.class.to_s, { closed_by: author.id, commit_hash: commit.to_hash }] [
project.id,
issue.id,
issue.class.to_s,
{ closed_by: author.id, user_id: user.id, commit_hash: commit.to_hash }
]
}, },
context_proc: ->(issue) { { project: project } } context_proc: ->(issue) { { project: project } }
) )
......
...@@ -4,48 +4,66 @@ ...@@ -4,48 +4,66 @@
RSpec.describe Issues::CloseWorker, feature_category: :team_planning do RSpec.describe Issues::CloseWorker, feature_category: :team_planning do
describe "#perform" do describe "#perform" do
let_it_be(:user) { create(:user) } let_it_be(:developer) { create(:user) }
let_it_be(:project) { create(:project, :public, :repository) } let_it_be(:author) { create(:user) }
let_it_be(:issue) { create(:issue, project: project, author: user) } let_it_be(:project) { create(:project, :public, :repository, developers: [developer]) }
let_it_be_with_reload(:issue) { create(:issue, project: project, author: author) }
let(:project_id) { project.id }
let(:issue_id) { issue.id }
let(:current_user_id) { developer&.id }
let(:current_author_id) { author&.id }
let(:commit) { project.commit } let(:commit) { project.commit }
let(:opts) do let(:opts) do
{ "closed_by" => user&.id, "commit_hash" => commit.to_hash } { "closed_by" => current_author_id, "user_id" => current_user_id, "commit_hash" => commit.to_hash }
end end
subject(:worker) { described_class.new } subject(:perform_job) { described_class.new.perform(project_id, issue_id, issue.class.to_s, opts) }
describe "#perform" do describe "#perform" do
context "when the user can update the issues" do context "when the user can update the issues" do
it "closes the issues" do it "closes the issues" do
worker.perform(project.id, issue.id, issue.class.to_s, opts) perform_job
issue.reload issue.reload
expect(issue.closed?).to eq(true) expect(issue).to be_closed
end end
it "closes external issues" do it "closes external issues" do
external_issue = ExternalIssue.new("foo", project) external_issue = ExternalIssue.new("foo", project)
closer = instance_double(Issues::CloseService, execute: true) closer = instance_double(Issues::CloseService, execute: true)
expect(Issues::CloseService).to receive(:new).with(container: project, current_user: user).and_return(closer) expect(Issues::CloseService).to receive(:new).with(container: project, current_user: author)
.and_return(closer)
expect(closer).to receive(:execute).with(external_issue, commit: commit) expect(closer).to receive(:execute).with(external_issue, commit: commit)
worker.perform(project.id, external_issue.id, external_issue.class.to_s, opts) described_class.new.perform(project.id, external_issue.id, external_issue.class.to_s, opts)
end end
end end
context "when the user can not update the issues" do context "when the user can not update the issues" do
it "does not close the issues" do let(:current_user_id) { create(:user).id }
other_user = create(:user)
opts = { "closed_by" => other_user.id, "commit_hash" => commit.to_hash }
worker.perform(project.id, issue.id, issue.class.to_s, opts) it 'does not close the issues' do
perform_job
issue.reload issue.reload
expect(issue.closed?).to eq(false) expect(issue).not_to be_closed
end
end
# TODO: Remove with https://gitlab.com/gitlab-org/gitlab/-/work_items/509422
context "when user is not provided to the worker (backwards compatibility)" do
let(:current_user_id) { nil }
it 'does closes the issue' do
perform_job
issue.reload
expect(issue).to be_closed
end end
end end
end end
...@@ -54,31 +72,24 @@ ...@@ -54,31 +72,24 @@
it "does not call the close issue service" do it "does not call the close issue service" do
expect(Issues::CloseService).not_to receive(:new) expect(Issues::CloseService).not_to receive(:new)
expect { worker.perform(project.id, issue.id, issue.class.to_s, opts) } expect { perform_job }.not_to raise_exception
.not_to raise_exception
end end
end end
context "when the project does not exist" do context "when the project does not exist" do
before do let(:project_id) { non_existing_record_id }
allow(Project).to receive(:find_by_id).with(project.id).and_return(nil)
end
it_behaves_like "when object does not exist" it_behaves_like "when object does not exist"
end end
context "when the user does not exist" do context "when the author does not exist" do
before do let(:current_author_id) { non_existing_record_id }
allow(User).to receive(:find_by_id).with(user.id).and_return(nil)
end
it_behaves_like "when object does not exist" it_behaves_like "when object does not exist"
end end
context "when the issue does not exist" do context "when the issue does not exist" do
before do let(:issue_id) { non_existing_record_id }
allow(Issue).to receive(:find_by_id).with(issue.id).and_return(nil)
end
it_behaves_like "when object does not exist" it_behaves_like "when object does not exist"
end end
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
RSpec.describe ProcessCommitWorker, feature_category: :source_code_management do RSpec.describe ProcessCommitWorker, feature_category: :source_code_management do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:author) { create(:user) }
let(:auto_close_issues) { true } let(:auto_close_issues) { true }
let(:project) { create(:project, :public, :repository, autoclose_referenced_issues: auto_close_issues) } let(:project) { create(:project, :public, :repository, autoclose_referenced_issues: auto_close_issues) }
...@@ -70,15 +71,30 @@ ...@@ -70,15 +71,30 @@
context 'when commit is not a merge request merge commit' do context 'when commit is not a merge request merge commit' do
context 'when commit has issue reference' do context 'when commit has issue reference' do
before do before do
allow(commit).to receive(:safe_message).and_return("Closes #{issue.to_reference}") allow(commit).to receive_messages(
safe_message: "Closes #{issue.to_reference}",
author: author
)
end end
it 'closes issues that should be closed per the commit message' do it 'closes issues that should be closed per the commit message' do
expect { perform }.to change { Issues::CloseWorker.jobs.size }.by(1) expect { perform }.to change { Issues::CloseWorker.jobs.size }.by(1)
end end
it 'passes both author and user_id to CloseWorker' do
expect { perform }.to change { Issues::CloseWorker.jobs.size }.by(1)
last_job = Issues::CloseWorker.jobs.last
expect(last_job['args']).to include(
project.id,
issue.id,
issue.class.to_s,
hash_including('closed_by' => commit.author.id, 'user_id' => user.id)
)
end
it 'creates cross references' do it 'creates cross references' do
expect(commit).to receive(:create_cross_references!).with(user, [issue]) expect(commit).to receive(:create_cross_references!).with(author, [issue])
perform perform
end end
...@@ -136,8 +152,12 @@ ...@@ -136,8 +152,12 @@
) )
end end
it 'closes issues that should be closed per the commit message' do it 'closes issues that should be closed per the commit message', :sidekiq_inline do
expect { perform }.to change { Issues::CloseWorker.jobs.size }.by(1) expect_next_instance_of(Issues::CloseService) do |close_service|
expect(close_service).to receive(:execute).with(issue, commit: commit)
end
perform
end end
end end
end end
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment