From 325de662ce79ea75348c303e05b2f0045835193e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 29 Aug 2016 18:17:11 -0300 Subject: [PATCH 01/10] Don't create groups for unallowed users when importing projects --- app/controllers/import/base_controller.rb | 6 ++- .../import/bitbucket_controller.rb | 13 ++---- app/controllers/import/github_controller.rb | 7 +--- app/controllers/import/gitlab_controller.rb | 9 +--- app/helpers/import_helper.rb | 5 +++ app/views/import/bitbucket/status.html.haml | 2 +- app/views/import/github/status.html.haml | 2 +- app/views/import/gitlab/status.html.haml | 2 +- .../import/bitbucket_controller_spec.rb | 41 ++++++++++++++----- .../import/github_controller_spec.rb | 41 ++++++++++++++----- .../import/gitlab_controller_spec.rb | 41 ++++++++++++++----- spec/helpers/import_helper_spec.rb | 24 +++++++++++ 12 files changed, 137 insertions(+), 56 deletions(-) diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 7e8597a5eb3..1ca33bc5d22 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -1,12 +1,16 @@ class Import::BaseController < ApplicationController private - def get_or_create_namespace + def find_or_create_namespace(name, owner) begin + @target_namespace = params[:new_namespace].presence || name + @target_namespace = current_user.namespace_path if name == owner || !current_user.can_create_group? + namespace = Group.create!(name: @target_namespace, path: @target_namespace, owner: current_user) namespace.add_owner(current_user) rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid namespace = Namespace.find_by_path_or_name(@target_namespace) + unless current_user.can?(:create_projects, namespace) @already_been_taken = true return false diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 944c73d139a..94e213b8743 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -35,15 +35,10 @@ class Import::BitbucketController < Import::BaseController end def create - @repo_id = params[:repo_id] || "" - repo = client.project(@repo_id.gsub("___", "/")) - @project_name = repo["slug"] - - repo_owner = repo["owner"] - repo_owner = current_user.username if repo_owner == client.user["user"]["username"] - @target_namespace = params[:new_namespace].presence || repo_owner - - namespace = get_or_create_namespace || (render and return) + @repo_id = params[:repo_id].to_s + repo = client.project(@repo_id.gsub('___', '/')) + @project_name = repo['slug'] + namespace = find_or_create_namespace(repo['owner'], client.user['user']['username']) || (render and return) unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user, access_params).execute @access_denied = true diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 9c1b0eb20f4..4047e62efa2 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -41,12 +41,7 @@ class Import::GithubController < Import::BaseController @repo_id = params[:repo_id].to_i repo = client.repo(@repo_id) @project_name = repo.name - - repo_owner = repo.owner.login - repo_owner = current_user.username if repo_owner == client.user.login - @target_namespace = params[:new_namespace].presence || repo_owner - - namespace = get_or_create_namespace || (render and return) + namespace = find_or_create_namespace(repo.owner.login, client.user.login) || (render and return) @project = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute end diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb index 08130ee8176..bc967e55ab1 100644 --- a/app/controllers/import/gitlab_controller.rb +++ b/app/controllers/import/gitlab_controller.rb @@ -26,13 +26,8 @@ class Import::GitlabController < Import::BaseController def create @repo_id = params[:repo_id].to_i repo = client.project(@repo_id) - @project_name = repo["name"] - - repo_owner = repo["namespace"]["path"] - repo_owner = current_user.username if repo_owner == client.user["username"] - @target_namespace = params[:new_namespace].presence || repo_owner - - namespace = get_or_create_namespace || (render and return) + @project_name = repo['name'] + namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username']) || (render and return) @project = Gitlab::GitlabImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute end diff --git a/app/helpers/import_helper.rb b/app/helpers/import_helper.rb index 109bc1a02d1..021d2b14718 100644 --- a/app/helpers/import_helper.rb +++ b/app/helpers/import_helper.rb @@ -1,4 +1,9 @@ module ImportHelper + def import_project_target(owner, name) + namespace = current_user.can_create_group? ? owner : current_user.namespace_path + "#{namespace}/#{name}" + end + def github_project_link(path_with_namespace) link_to path_with_namespace, github_project_url(path_with_namespace), target: '_blank' end diff --git a/app/views/import/bitbucket/status.html.haml b/app/views/import/bitbucket/status.html.haml index 15dd98077c8..f8b4b107513 100644 --- a/app/views/import/bitbucket/status.html.haml +++ b/app/views/import/bitbucket/status.html.haml @@ -51,7 +51,7 @@ %td = link_to "#{repo["owner"]}/#{repo["slug"]}", "https://bitbucket.org/#{repo["owner"]}/#{repo["slug"]}", target: "_blank" %td.import-target - = "#{repo["owner"]}/#{repo["slug"]}" + = import_project_target(repo['owner'], repo['slug']) %td.import-actions.job-status = button_tag class: "btn btn-import js-add-to-import" do Import diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml index 54ff1d27c67..bd3be20c4f8 100644 --- a/app/views/import/github/status.html.haml +++ b/app/views/import/github/status.html.haml @@ -45,7 +45,7 @@ %td = github_project_link(repo.full_name) %td.import-target - = repo.full_name + = import_project_target(repo.owner.login, repo.name) %td.import-actions.job-status = button_tag class: "btn btn-import js-add-to-import" do Import diff --git a/app/views/import/gitlab/status.html.haml b/app/views/import/gitlab/status.html.haml index fcfc6fd37f4..d31fc2e6adb 100644 --- a/app/views/import/gitlab/status.html.haml +++ b/app/views/import/gitlab/status.html.haml @@ -45,7 +45,7 @@ %td = link_to repo["path_with_namespace"], "https://gitlab.com/#{repo["path_with_namespace"]}", target: "_blank" %td.import-target - = repo["path_with_namespace"] + = import_project_target(repo['namespace']['path'], repo['name']) %td.import-actions.job-status = button_tag class: "btn btn-import js-add-to-import" do Import diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index 07bf8d2d1c3..1d3c9fbbe2f 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -146,21 +146,42 @@ describe Import::BitbucketController do end context "when a namespace with the Bitbucket user's username doesn't exist" do - it "creates the namespace" do - expect(Gitlab::BitbucketImport::ProjectCreator). - to receive(:new).and_return(double(execute: true)) + context "when current user can create namespaces" do + it "creates the namespace" do + expect(Gitlab::BitbucketImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) - post :create, format: :js + expect { post :create, format: :js }.to change(Namespace, :count).by(1) + end - expect(Namespace.where(name: other_username).first).not_to be_nil + it "takes the new namespace" do + expect(Gitlab::BitbucketImport::ProjectCreator). + to receive(:new).with(bitbucket_repo, an_instance_of(Group), user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end - it "takes the new namespace" do - expect(Gitlab::BitbucketImport::ProjectCreator). - to receive(:new).with(bitbucket_repo, an_instance_of(Group), user, access_params). - and_return(double(execute: true)) + context "when current user can't create namespaces" do + before do + user.update_attribute(:can_create_group, false) + end - post :create, format: :js + it "doesn't create the namespace" do + expect(Gitlab::BitbucketImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) + + expect { post :create, format: :js }.not_to change(Namespace, :count) + end + + it "takes the current user's namespace" do + expect(Gitlab::BitbucketImport::ProjectCreator). + to receive(:new).with(bitbucket_repo, user.namespace, user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end end end diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index 51d59526854..ebfbf54182b 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -181,21 +181,42 @@ describe Import::GithubController do end context "when a namespace with the GitHub user's username doesn't exist" do - it "creates the namespace" do - expect(Gitlab::GithubImport::ProjectCreator). - to receive(:new).and_return(double(execute: true)) + context "when current user can create namespaces" do + it "creates the namespace" do + expect(Gitlab::GithubImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) - post :create, format: :js + expect { post :create, format: :js }.to change(Namespace, :count).by(1) + end - expect(Namespace.where(name: other_username).first).not_to be_nil + it "takes the new namespace" do + expect(Gitlab::GithubImport::ProjectCreator). + to receive(:new).with(github_repo, an_instance_of(Group), user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end - it "takes the new namespace" do - expect(Gitlab::GithubImport::ProjectCreator). - to receive(:new).with(github_repo, an_instance_of(Group), user, access_params). - and_return(double(execute: true)) + context "when current user can't create namespaces" do + before do + user.update_attribute(:can_create_group, false) + end - post :create, format: :js + it "doesn't create the namespace" do + expect(Gitlab::GithubImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) + + expect { post :create, format: :js }.not_to change(Namespace, :count) + end + + it "takes the current user's namespace" do + expect(Gitlab::GithubImport::ProjectCreator). + to receive(:new).with(github_repo, user.namespace, user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end end end diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb index e8cf6aa7767..6f75ebb16c8 100644 --- a/spec/controllers/import/gitlab_controller_spec.rb +++ b/spec/controllers/import/gitlab_controller_spec.rb @@ -136,21 +136,42 @@ describe Import::GitlabController do end context "when a namespace with the GitLab.com user's username doesn't exist" do - it "creates the namespace" do - expect(Gitlab::GitlabImport::ProjectCreator). - to receive(:new).and_return(double(execute: true)) + context "when current user can create namespaces" do + it "creates the namespace" do + expect(Gitlab::GitlabImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) - post :create, format: :js + expect { post :create, format: :js }.to change(Namespace, :count).by(1) + end - expect(Namespace.where(name: other_username).first).not_to be_nil + it "takes the new namespace" do + expect(Gitlab::GitlabImport::ProjectCreator). + to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end - it "takes the new namespace" do - expect(Gitlab::GitlabImport::ProjectCreator). - to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params). - and_return(double(execute: true)) + context "when current user can't create namespaces" do + before do + user.update_attribute(:can_create_group, false) + end - post :create, format: :js + it "doesn't create the namespace" do + expect(Gitlab::GitlabImport::ProjectCreator). + to receive(:new).and_return(double(execute: true)) + + expect { post :create, format: :js }.not_to change(Namespace, :count) + end + + it "takes the current user's namespace" do + expect(Gitlab::GitlabImport::ProjectCreator). + to receive(:new).with(gitlab_repo, user.namespace, user, access_params). + and_return(double(execute: true)) + + post :create, format: :js + end end end end diff --git a/spec/helpers/import_helper_spec.rb b/spec/helpers/import_helper_spec.rb index 3391234e9f5..187b891b927 100644 --- a/spec/helpers/import_helper_spec.rb +++ b/spec/helpers/import_helper_spec.rb @@ -1,6 +1,30 @@ require 'rails_helper' describe ImportHelper do + describe '#import_project_target' do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'when current user can create namespaces' do + it 'returns project namespace' do + user.update_attribute(:can_create_group, true) + + expect(helper.import_project_target('asd', 'vim')).to eq 'asd/vim' + end + end + + context 'when current user can not create namespaces' do + it "takes the current user's namespace" do + user.update_attribute(:can_create_group, false) + + expect(helper.import_project_target('asd', 'vim')).to eq "#{user.namespace_path}/vim" + end + end + end + describe '#github_project_link' do context 'when provider does not specify a custom URL' do it 'uses default GitHub URL' do From e293ffd48fb16c8ad15c066cfbbe1dcead7c52e0 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 14:34:37 -0300 Subject: [PATCH 02/10] Refactoring Import::BaseController#find_or_create_namespace --- app/assets/javascripts/importer_status.js | 15 +++++++------ app/controllers/import/base_controller.rb | 19 +++++++---------- .../import/bitbucket_controller.rb | 12 ++++++----- app/controllers/import/github_controller.rb | 8 +++++-- app/controllers/import/gitlab_controller.rb | 8 +++++-- app/views/import/base/create.js.haml | 21 +------------------ app/views/import/base/unauthorized.js.haml | 14 +++++++++++++ app/views/import/bitbucket/deploy_key.js.haml | 3 +++ 8 files changed, 53 insertions(+), 47 deletions(-) create mode 100644 app/views/import/base/unauthorized.js.haml create mode 100644 app/views/import/bitbucket/deploy_key.js.haml diff --git a/app/assets/javascripts/importer_status.js b/app/assets/javascripts/importer_status.js index 0f840821f53..9efad1ce943 100644 --- a/app/assets/javascripts/importer_status.js +++ b/app/assets/javascripts/importer_status.js @@ -10,21 +10,24 @@ ImporterStatus.prototype.initStatusPage = function() { $('.js-add-to-import').off('click').on('click', (function(_this) { return function(e) { - var $btn, $namespace_input, $target_field, $tr, id, new_namespace; + var $btn, $namespace_input, $target_field, $tr, id, target_namespace; $btn = $(e.currentTarget); $tr = $btn.closest('tr'); $target_field = $tr.find('.import-target'); $namespace_input = $target_field.find('input'); id = $tr.attr('id').replace('repo_', ''); - new_namespace = null; + target_namespace = null; + if ($namespace_input.length > 0) { - new_namespace = $namespace_input.prop('value'); - $target_field.empty().append(new_namespace + "/" + ($target_field.data('project_name'))); + target_namespace = $namespace_input.prop('value'); + $target_field.empty().append(target_namespace + "/" + ($target_field.data('project_name'))); } + $btn.disable().addClass('is-loading'); + return $.post(_this.import_url, { repo_id: id, - new_namespace: new_namespace + target_namespace: target_namespace }, { dataType: 'script' }); @@ -70,7 +73,7 @@ if ($('.js-importer-status').length) { var jobsImportPath = $('.js-importer-status').data('jobs-import-path'); var importPath = $('.js-importer-status').data('import-path'); - + new ImporterStatus(jobsImportPath, importPath); } }); diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 1ca33bc5d22..256c41e6145 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -2,21 +2,16 @@ class Import::BaseController < ApplicationController private def find_or_create_namespace(name, owner) + return current_user.namespace if name == owner + return current_user.namespace unless current_user.can_create_group? + begin - @target_namespace = params[:new_namespace].presence || name - @target_namespace = current_user.namespace_path if name == owner || !current_user.can_create_group? - - namespace = Group.create!(name: @target_namespace, path: @target_namespace, owner: current_user) + name = params[:target_namespace].presence || name + namespace = Group.create!(name: name, path: name, owner: current_user) namespace.add_owner(current_user) + namespace rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid - namespace = Namespace.find_by_path_or_name(@target_namespace) - - unless current_user.can?(:create_projects, namespace) - @already_been_taken = true - return false - end + Namespace.find_by_path_or_name(name) end - - namespace end end diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 94e213b8743..6ea54744da8 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -38,15 +38,17 @@ class Import::BitbucketController < Import::BaseController @repo_id = params[:repo_id].to_s repo = client.project(@repo_id.gsub('___', '/')) @project_name = repo['slug'] - namespace = find_or_create_namespace(repo['owner'], client.user['user']['username']) || (render and return) + @target_namespace = find_or_create_namespace(repo['owner'], client.user['user']['username']) unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user, access_params).execute - @access_denied = true - render - return + render 'deploy_key' and return end - @project = Gitlab::BitbucketImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute + if current_user.can?(:create_projects, @target_namespace) + @project = Gitlab::BitbucketImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute + else + render 'unauthorized' + end end private diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 4047e62efa2..8c6bdd16383 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -41,9 +41,13 @@ class Import::GithubController < Import::BaseController @repo_id = params[:repo_id].to_i repo = client.repo(@repo_id) @project_name = repo.name - namespace = find_or_create_namespace(repo.owner.login, client.user.login) || (render and return) + @target_namespace = find_or_create_namespace(repo.owner.login, client.user.login) - @project = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute + if current_user.can?(:create_projects, @target_namespace) + @project = Gitlab::GithubImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute + else + render 'unauthorized' + end end private diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb index bc967e55ab1..73837ffbe67 100644 --- a/app/controllers/import/gitlab_controller.rb +++ b/app/controllers/import/gitlab_controller.rb @@ -27,9 +27,13 @@ class Import::GitlabController < Import::BaseController @repo_id = params[:repo_id].to_i repo = client.project(@repo_id) @project_name = repo['name'] - namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username']) || (render and return) + @target_namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username']) - @project = Gitlab::GitlabImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute + if current_user.can?(:create_projects, @target_namespace) + @project = Gitlab::GitlabImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute + else + render 'unauthorized' + end end private diff --git a/app/views/import/base/create.js.haml b/app/views/import/base/create.js.haml index 804ad88468f..8e929538351 100644 --- a/app/views/import/base/create.js.haml +++ b/app/views/import/base/create.js.haml @@ -1,23 +1,4 @@ -- if @already_been_taken - :plain - tr = $("tr#repo_#{@repo_id}") - target_field = tr.find(".import-target") - import_button = tr.find(".btn-import") - origin_target = target_field.text() - project_name = "#{@project_name}" - origin_namespace = "#{@target_namespace}" - target_field.empty() - target_field.append("

This namespace already been taken! Please choose another one

") - target_field.append("") - target_field.append("/" + project_name) - target_field.data("project_name", project_name) - target_field.find('input').prop("value", origin_namespace) - import_button.enable().removeClass('is-loading') -- elsif @access_denied - :plain - job = $("tr#repo_#{@repo_id}") - job.find(".import-actions").html("

Access denied! Please verify you can add deploy keys to this repository.

") -- elsif @project.persisted? +- if @project.persisted? :plain job = $("tr#repo_#{@repo_id}") job.attr("id", "project_#{@project.id}") diff --git a/app/views/import/base/unauthorized.js.haml b/app/views/import/base/unauthorized.js.haml new file mode 100644 index 00000000000..36f8069c1f7 --- /dev/null +++ b/app/views/import/base/unauthorized.js.haml @@ -0,0 +1,14 @@ +:plain + tr = $("tr#repo_#{@repo_id}") + target_field = tr.find(".import-target") + import_button = tr.find(".btn-import") + origin_target = target_field.text() + project_name = "#{@project_name}" + origin_namespace = "#{@target_namespace.path}" + target_field.empty() + target_field.append("

This namespace has already been taken! Please choose another one.

") + target_field.append("") + target_field.append("/" + project_name) + target_field.data("project_name", project_name) + target_field.find('input').prop("value", origin_namespace) + import_button.enable().removeClass('is-loading') diff --git a/app/views/import/bitbucket/deploy_key.js.haml b/app/views/import/bitbucket/deploy_key.js.haml new file mode 100644 index 00000000000..81b34ab5c9d --- /dev/null +++ b/app/views/import/bitbucket/deploy_key.js.haml @@ -0,0 +1,3 @@ +:plain + job = $("tr#repo_#{@repo_id}") + job.find(".import-actions").html("

Access denied! Please verify you can add deploy keys to this repository.

") From 03fe9de99ce62fe8328a749040b3910a55c69184 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 19:30:48 -0300 Subject: [PATCH 03/10] Update CHANGELOG --- CHANGELOG | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 427b40121ea..95d1504e982 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -55,11 +55,9 @@ v 8.11.4 (unreleased) - Creating an issue through our API now emails label subscribers !5720 - Fix resolving conflicts on forks - Fix diff commenting on merge requests created prior to 8.10 - -v 8.11.4 (unreleased) + - Don't create groups for unallowed users when importing projects - Fix issue boards leak private label names and descriptions -v 8.11.3 (unreleased) v 8.11.3 - Do not enforce using hash with hidden key in CI configuration. !6079 - Allow system info page to handle case where info is unavailable From 44340fede26278a7f9d70e0fb063df9673cfc551 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 12 Aug 2016 16:05:10 -0300 Subject: [PATCH 04/10] Fix confidential issues should not be passed to Webhooks --- app/services/issues/base_service.rb | 2 + spec/services/issues/close_service_spec.rb | 17 ++++++-- spec/services/issues/create_service_spec.rb | 9 +++++ spec/services/issues/reopen_service_spec.rb | 26 ++++++++---- spec/services/issues/update_service_spec.rb | 45 +++++++++++++++------ 5 files changed, 77 insertions(+), 22 deletions(-) diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 089b0f527e2..241efc44d36 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -14,6 +14,8 @@ module Issues end def execute_hooks(issue, action = 'open') + return if issue.confidential? + issue_data = hook_data(issue, action) issue.project.execute_hooks(issue_data, :issue_hooks) issue.project.execute_services(issue_data, :issue_hooks) diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index aff022a573e..229c8eb5b5b 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -18,7 +18,7 @@ describe Issues::CloseService, services: true do context "valid params" do before do perform_enqueued_jobs do - @issue = described_class.new(project, user, {}).execute(issue) + @issue = described_class.new(project, user).execute(issue) end end @@ -53,10 +53,21 @@ describe Issues::CloseService, services: true do end end - context "external issue tracker" do + context 'when issue is confidential' do + it 'does not execute hooks' do + issue = create(:issue, :confidential, project: project) + + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_services) + + described_class.new(project, user).execute(issue) + end + end + + context 'external issue tracker' do before do allow(project).to receive(:default_issues_tracker?).and_return(false) - @issue = described_class.new(project, user, {}).execute(issue) + @issue = described_class.new(project, user).execute(issue) end it { expect(@issue).to be_valid } diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index fcc3c0a00bd..81beca47738 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -72,6 +72,15 @@ describe Issues::CreateService, services: true do expect(issue.milestone).not_to eq milestone end end + + it 'does not execute hooks when issue is confidential' do + opts = { title: 'Title', description: 'Description', confidential: true } + + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_services) + + described_class.new(project, user, opts).execute + end end it_behaves_like 'new issuable record that supports slash commands' diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 34a89fcd4e1..83c72e64c87 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -1,17 +1,15 @@ require 'spec_helper' describe Issues::ReopenService, services: true do - let(:guest) { create(:user) } - let(:issue) { create(:issue, :closed) } - let(:project) { issue.project } - - before do - project.team << [guest, :guest] - end + let(:project) { create(:empty_project) } + let(:issue) { create(:issue, :closed, project: project) } describe '#execute' do context 'current user is not authorized to reopen issue' do before do + guest = create(:user) + project.team << [guest, :guest] + perform_enqueued_jobs do @issue = described_class.new(project, guest).execute(issue) end @@ -21,5 +19,19 @@ describe Issues::ReopenService, services: true do expect(@issue).to be_closed end end + + context 'when issue is confidential' do + it 'does not execute hooks' do + user = create(:user) + project.team << [user, :master] + + issue = create(:issue, :confidential, :closed, project: project) + + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_services) + + described_class.new(project, user).execute(issue) + end + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 0313f424463..a4bd19d317b 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -28,6 +28,11 @@ describe Issues::UpdateService, services: true do end end + def update_issue(opts) + @issue = described_class.new(project, user, opts).execute(issue) + @issue.reload + end + context "valid params" do before do opts = { @@ -35,12 +40,11 @@ describe Issues::UpdateService, services: true do description: 'Also please fix', assignee_id: user2.id, state_event: 'close', - label_ids: [label.id], - confidential: true + label_ids: [label.id] } perform_enqueued_jobs do - @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + @issue = described_class.new(project, user, opts).execute(issue) end @issue.reload @@ -81,18 +85,35 @@ describe Issues::UpdateService, services: true do expect(note).not_to be_nil expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**' end + end + + context 'when issue turns confidential' do + let(:opts) do + { + title: 'New title', + description: 'Also please fix', + assignee_id: user2.id, + state_event: 'close', + label_ids: [label.id], + confidential: true + } + end it 'creates system note about confidentiality change' do + update_issue({ confidential: true }) + note = find_note('Made the issue confidential') expect(note).not_to be_nil expect(note.note).to eq 'Made the issue confidential' end - end - def update_issue(opts) - @issue = Issues::UpdateService.new(project, user, opts).execute(issue) - @issue.reload + it 'does not execute hooks' do + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_services) + + update_issue({ confidential: true }) + end end context 'todos' do @@ -176,7 +197,7 @@ describe Issues::UpdateService, services: true do opts = { label_ids: [label.id] } perform_enqueued_jobs do - @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + @issue = described_class.new(project, user, opts).execute(issue) end should_email(subscriber) @@ -190,7 +211,7 @@ describe Issues::UpdateService, services: true do opts = { label_ids: [label.id, label2.id] } perform_enqueued_jobs do - @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + @issue = described_class.new(project, user, opts).execute(issue) end should_not_email(subscriber) @@ -201,7 +222,7 @@ describe Issues::UpdateService, services: true do opts = { label_ids: [label2.id] } perform_enqueued_jobs do - @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + @issue = described_class.new(project, user, opts).execute(issue) end should_not_email(subscriber) @@ -210,7 +231,7 @@ describe Issues::UpdateService, services: true do end end - context 'when Issue has tasks' do + context 'when issue has tasks' do before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } it { expect(@issue.tasks?).to eq(true) } @@ -277,7 +298,7 @@ describe Issues::UpdateService, services: true do context 'updating labels' do let(:label3) { create(:label, project: project) } - let(:result) { Issues::UpdateService.new(project, user, params).execute(issue).reload } + let(:result) { described_class.new(project, user, params).execute(issue).reload } context 'when add_label_ids and label_ids are passed' do let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } } From debb65b5c81094765b2aa515a20e964cd8a14b6f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 29 Aug 2016 16:08:40 -0300 Subject: [PATCH 05/10] Update service specs to avoid instance variables --- spec/services/issues/close_service_spec.rb | 18 +++---- spec/services/issues/reopen_service_spec.rb | 4 +- spec/services/issues/update_service_spec.rb | 58 ++++++++++----------- 3 files changed, 40 insertions(+), 40 deletions(-) diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 229c8eb5b5b..4df99e41987 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -18,12 +18,12 @@ describe Issues::CloseService, services: true do context "valid params" do before do perform_enqueued_jobs do - @issue = described_class.new(project, user).execute(issue) + described_class.new(project, user).execute(issue) end end - it { expect(@issue).to be_valid } - it { expect(@issue).to be_closed } + it { expect(issue).to be_valid } + it { expect(issue).to be_closed } it 'sends email to user2 about assign of new issue' do email = ActionMailer::Base.deliveries.last @@ -32,7 +32,7 @@ describe Issues::CloseService, services: true do end it 'creates system note about issue reassign' do - note = @issue.notes.last + note = issue.notes.last expect(note.note).to include "Status changed to closed" end @@ -44,12 +44,12 @@ describe Issues::CloseService, services: true do context 'current user is not authorized to close issue' do before do perform_enqueued_jobs do - @issue = described_class.new(project, guest).execute(issue) + described_class.new(project, guest).execute(issue) end end it 'does not close the issue' do - expect(@issue).to be_open + expect(issue).to be_open end end @@ -67,11 +67,11 @@ describe Issues::CloseService, services: true do context 'external issue tracker' do before do allow(project).to receive(:default_issues_tracker?).and_return(false) - @issue = described_class.new(project, user).execute(issue) + described_class.new(project, user).execute(issue) end - it { expect(@issue).to be_valid } - it { expect(@issue).to be_opened } + it { expect(issue).to be_valid } + it { expect(issue).to be_opened } it { expect(todo.reload).to be_pending } end end diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 83c72e64c87..4549e2b395b 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -11,12 +11,12 @@ describe Issues::ReopenService, services: true do project.team << [guest, :guest] perform_enqueued_jobs do - @issue = described_class.new(project, guest).execute(issue) + described_class.new(project, guest).execute(issue) end end it 'does not reopen the issue' do - expect(@issue).to be_closed + expect(issue).to be_closed end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index a4bd19d317b..3c3a861b3af 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -23,14 +23,13 @@ describe Issues::UpdateService, services: true do describe 'execute' do def find_note(starting_with) - @issue.notes.find do |note| + issue.notes.find do |note| note && note.note.start_with?(starting_with) end end def update_issue(opts) - @issue = described_class.new(project, user, opts).execute(issue) - @issue.reload + described_class.new(project, user, opts).execute(issue) end context "valid params" do @@ -44,18 +43,16 @@ describe Issues::UpdateService, services: true do } perform_enqueued_jobs do - @issue = described_class.new(project, user, opts).execute(issue) + update_issue(opts) end - - @issue.reload end - it { expect(@issue).to be_valid } - it { expect(@issue.title).to eq('New title') } - it { expect(@issue.assignee).to eq(user2) } - it { expect(@issue).to be_closed } - it { expect(@issue.labels.count).to eq(1) } - it { expect(@issue.labels.first.title).to eq(label.name) } + it { expect(issue).to be_valid } + it { expect(issue.title).to eq('New title') } + it { expect(issue.assignee).to eq(user2) } + it { expect(issue).to be_closed } + it { expect(issue.labels.count).to eq(1) } + it { expect(issue.labels.first.title).to eq(label.name) } it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries @@ -100,7 +97,7 @@ describe Issues::UpdateService, services: true do end it 'creates system note about confidentiality change' do - update_issue({ confidential: true }) + update_issue(confidential: true) note = find_note('Made the issue confidential') @@ -112,7 +109,7 @@ describe Issues::UpdateService, services: true do expect(project).not_to receive(:execute_hooks) expect(project).not_to receive(:execute_services) - update_issue({ confidential: true }) + update_issue(confidential: true) end end @@ -121,7 +118,7 @@ describe Issues::UpdateService, services: true do context 'when the title change' do before do - update_issue({ title: 'New title' }) + update_issue(title: 'New title') end it 'marks pending todos as done' do @@ -131,7 +128,7 @@ describe Issues::UpdateService, services: true do context 'when the description change' do before do - update_issue({ description: 'Also please fix' }) + update_issue(description: 'Also please fix') end it 'marks todos as done' do @@ -141,7 +138,7 @@ describe Issues::UpdateService, services: true do context 'when is reassigned' do before do - update_issue({ assignee: user2 }) + update_issue(assignee: user2) end it 'marks previous assignee todos as done' do @@ -165,7 +162,7 @@ describe Issues::UpdateService, services: true do context 'when the milestone change' do before do - update_issue({ milestone: create(:milestone) }) + update_issue(milestone: create(:milestone)) end it 'marks todos as done' do @@ -175,7 +172,7 @@ describe Issues::UpdateService, services: true do context 'when the labels change' do before do - update_issue({ label_ids: [label.id] }) + update_issue(label_ids: [label.id]) end it 'marks todos as done' do @@ -186,6 +183,7 @@ describe Issues::UpdateService, services: true do context 'when the issue is relabeled' do let!(:non_subscriber) { create(:user) } + let!(:subscriber) do create(:user).tap do |u| label.toggle_subscription(u) @@ -232,12 +230,14 @@ describe Issues::UpdateService, services: true do end context 'when issue has tasks' do - before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } + before do + update_issue(description: "- [ ] Task 1\n- [ ] Task 2") + end - it { expect(@issue.tasks?).to eq(true) } + it { expect(issue.tasks?).to eq(true) } context 'when tasks are marked as completed' do - before { update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) } + before { update_issue(description: "- [x] Task 1\n- [X] Task 2") } it 'creates system note about task status change' do note1 = find_note('Marked the task **Task 1** as completed') @@ -250,8 +250,8 @@ describe Issues::UpdateService, services: true do context 'when tasks are marked as incomplete' do before do - update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) - update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) + update_issue(description: "- [x] Task 1\n- [X] Task 2") + update_issue(description: "- [ ] Task 1\n- [ ] Task 2") end it 'creates system note about task status change' do @@ -265,8 +265,8 @@ describe Issues::UpdateService, services: true do context 'when tasks position has been modified' do before do - update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) - update_issue({ description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2" }) + update_issue(description: "- [x] Task 1\n- [X] Task 2") + update_issue(description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2") end it 'does not create a system note' do @@ -278,8 +278,8 @@ describe Issues::UpdateService, services: true do context 'when a Task list with a completed item is totally replaced' do before do - update_issue({ description: "- [ ] Task 1\n- [X] Task 2" }) - update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" }) + update_issue(description: "- [ ] Task 1\n- [X] Task 2") + update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three") end it 'does not create a system note referencing the position the old item' do @@ -290,7 +290,7 @@ describe Issues::UpdateService, services: true do it 'does not generate a new note at all' do expect do - update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" }) + update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three") end.not_to change { Note.count } end end From a103a5d9cc2f496854d75492fe3f759fad0a913f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 17:56:26 -0300 Subject: [PATCH 06/10] Add option to confidential issues events to trigger Webhooks --- app/controllers/projects/hooks_controller.rb | 1 + app/models/hooks/web_hook.rb | 1 + .../projects/hooks/_project_hook.html.haml | 2 +- app/views/shared/web_hooks/_form.html.haml | 7 ++++++ ...confidential_issues_events_to_web_hooks.rb | 15 ++++++++++++ db/schema.rb | 23 ++++++++++--------- 6 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 db/migrate/20160830203109_add_confidential_issues_events_to_web_hooks.rb diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb index b5624046387..0ae8ff98009 100644 --- a/app/controllers/projects/hooks_controller.rb +++ b/app/controllers/projects/hooks_controller.rb @@ -59,6 +59,7 @@ class Projects::HooksController < Projects::ApplicationController :pipeline_events, :enable_ssl_verification, :issues_events, + :confidential_issues_events, :merge_requests_events, :note_events, :push_events, diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index f365dee3141..595602e80fe 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base default_value_for :push_events, true default_value_for :issues_events, false + default_value_for :confidential_issues_events, false default_value_for :note_events, false default_value_for :merge_requests_events, false default_value_for :tag_push_events, false diff --git a/app/views/projects/hooks/_project_hook.html.haml b/app/views/projects/hooks/_project_hook.html.haml index 3fcf1692e09..ceabe2eab3d 100644 --- a/app/views/projects/hooks/_project_hook.html.haml +++ b/app/views/projects/hooks/_project_hook.html.haml @@ -3,7 +3,7 @@ .col-md-8.col-lg-7 %strong.light-header= hook.url %div - - %w(push_events tag_push_events issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger| + - %w(push_events tag_push_events issues_events confidential_issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger| - if hook.send(trigger) %span.label.label-gray.deploy-project-label= trigger.titleize .col-md-4.col-lg-5.text-right-lg.prepend-top-5 diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml index d2ec6c3ddef..5d659eb83a9 100644 --- a/app/views/shared/web_hooks/_form.html.haml +++ b/app/views/shared/web_hooks/_form.html.haml @@ -51,6 +51,13 @@ %strong Issues events %p.light This URL will be triggered when an issue is created/updated/merged + %li + = f.check_box :confidential_issues_events, class: 'pull-left' + .prepend-left-20 + = f.label :confidential_issues_events, class: 'list-label' do + %strong Confidential Issues events + %p.light + This URL will be triggered when a confidential issue is created/updated/merged %li = f.check_box :merge_requests_events, class: 'pull-left' .prepend-left-20 diff --git a/db/migrate/20160830203109_add_confidential_issues_events_to_web_hooks.rb b/db/migrate/20160830203109_add_confidential_issues_events_to_web_hooks.rb new file mode 100644 index 00000000000..a27947212f6 --- /dev/null +++ b/db/migrate/20160830203109_add_confidential_issues_events_to_web_hooks.rb @@ -0,0 +1,15 @@ +class AddConfidentialIssuesEventsToWebHooks < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :web_hooks, :confidential_issues_events, :boolean, default: false, allow_null: false + end + + def down + remove_column :web_hooks, :confidential_issues_events + end +end diff --git a/db/schema.rb b/db/schema.rb index 963d528d170..a972f89a3a9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1113,22 +1113,23 @@ ActiveRecord::Schema.define(version: 20160830232601) do add_index "users_star_projects", ["user_id"], name: "index_users_star_projects_on_user_id", using: :btree create_table "web_hooks", force: :cascade do |t| - t.string "url", limit: 2000 + t.string "url", limit: 2000 t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.string "type", default: "ProjectHook" + t.string "type", default: "ProjectHook" t.integer "service_id" - t.boolean "push_events", default: true, null: false - t.boolean "issues_events", default: false, null: false - t.boolean "merge_requests_events", default: false, null: false - t.boolean "tag_push_events", default: false - t.boolean "note_events", default: false, null: false - t.boolean "enable_ssl_verification", default: true - t.boolean "build_events", default: false, null: false - t.boolean "wiki_page_events", default: false, null: false + t.boolean "push_events", default: true, null: false + t.boolean "issues_events", default: false, null: false + t.boolean "merge_requests_events", default: false, null: false + t.boolean "tag_push_events", default: false + t.boolean "note_events", default: false, null: false + t.boolean "enable_ssl_verification", default: true + t.boolean "build_events", default: false, null: false + t.boolean "wiki_page_events", default: false, null: false t.string "token" - t.boolean "pipeline_events", default: false, null: false + t.boolean "pipeline_events", default: false, null: false + t.boolean "confidential_issues_events", default: false, null: false end add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree From dd64f8aaf867516043dbbf559595a0ed9671ab3b Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 18:39:25 -0300 Subject: [PATCH 07/10] Add option to confidential issues events to trigger services --- app/controllers/concerns/service_params.rb | 2 +- app/helpers/services_helper.rb | 6 +++-- .../project_services/hipchat_service.rb | 2 +- app/models/project_services/slack_service.rb | 2 +- app/models/service.rb | 3 ++- ..._confidential_issues_events_to_services.rb | 15 +++++++++++ db/schema.rb | 25 ++++++++++--------- 7 files changed, 37 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20160830211132_add_confidential_issues_events_to_services.rb diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index a69877edfd4..4cb3be41064 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -13,7 +13,7 @@ module ServiceParams # `issue_events` and `merge_request_events` (singular!) # See app/helpers/services_helper.rb for how we # make those event names plural as special case. - :issues_events, :merge_requests_events, + :issues_events, :confidential_issues_events, :merge_requests_events, :notify_only_broken_builds, :notify_only_broken_pipelines, :add_pusher, :send_from_committer_email, :disable_diffs, :external_wiki_url, :notify, :color, diff --git a/app/helpers/services_helper.rb b/app/helpers/services_helper.rb index 2dd0bf5d71e..3d4abf76419 100644 --- a/app/helpers/services_helper.rb +++ b/app/helpers/services_helper.rb @@ -8,7 +8,9 @@ module ServicesHelper when "note" "Event will be triggered when someone adds a comment" when "issue" - "Event will be triggered when an issue is created/updated/merged" + "Event will be triggered when an issue is created/updated/closed" + when "confidential_issue" + "Event will be triggered when a confidential issue is created/updated/closed" when "merge_request" "Event will be triggered when a merge request is created/updated/merged" when "build" @@ -19,7 +21,7 @@ module ServicesHelper end def service_event_field_name(event) - event = event.pluralize if %w[merge_request issue].include?(event) + event = event.pluralize if %w[merge_request issue confidential_issue].include?(event) "#{event}_events" end end diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index d7c986c1a91..afebd3b6a12 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -39,7 +39,7 @@ class HipchatService < Service end def supported_events - %w(push issue merge_request note tag_push build) + %w(push issue confidential_issue merge_request note tag_push build) end def execute(data) diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index abbc780dc1a..e6c943db2bf 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -44,7 +44,7 @@ class SlackService < Service end def supported_events - %w(push issue merge_request note tag_push build wiki_page) + %w(push issue confidential_issue merge_request note tag_push build wiki_page) end def execute(data) diff --git a/app/models/service.rb b/app/models/service.rb index 09b4717a523..7333f8d381b 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -7,6 +7,7 @@ class Service < ActiveRecord::Base default_value_for :active, false default_value_for :push_events, true default_value_for :issues_events, true + default_value_for :confidential_issues_events, true default_value_for :merge_requests_events, true default_value_for :tag_push_events, true default_value_for :note_events, true @@ -100,7 +101,7 @@ class Service < ActiveRecord::Base end def supported_events - %w(push tag_push issue merge_request wiki_page) + %w(push tag_push issue confidential_issue merge_request wiki_page) end def execute(data) diff --git a/db/migrate/20160830211132_add_confidential_issues_events_to_services.rb b/db/migrate/20160830211132_add_confidential_issues_events_to_services.rb new file mode 100644 index 00000000000..030e7c39350 --- /dev/null +++ b/db/migrate/20160830211132_add_confidential_issues_events_to_services.rb @@ -0,0 +1,15 @@ +class AddConfidentialIssuesEventsToServices < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :services, :confidential_issues_events, :boolean, default: true, allow_null: false + end + + def down + remove_column :services, :confidential_issues_events + end +end diff --git a/db/schema.rb b/db/schema.rb index a972f89a3a9..c9560abeb92 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -901,19 +901,20 @@ ActiveRecord::Schema.define(version: 20160830232601) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.boolean "active", default: false, null: false + t.boolean "active", default: false, null: false t.text "properties" - t.boolean "template", default: false - t.boolean "push_events", default: true - t.boolean "issues_events", default: true - t.boolean "merge_requests_events", default: true - t.boolean "tag_push_events", default: true - t.boolean "note_events", default: true, null: false - t.boolean "build_events", default: false, null: false - t.string "category", default: "common", null: false - t.boolean "default", default: false - t.boolean "wiki_page_events", default: true - t.boolean "pipeline_events", default: false, null: false + t.boolean "template", default: false + t.boolean "push_events", default: true + t.boolean "issues_events", default: true + t.boolean "merge_requests_events", default: true + t.boolean "tag_push_events", default: true + t.boolean "note_events", default: true, null: false + t.boolean "build_events", default: false, null: false + t.string "category", default: "common", null: false + t.boolean "default", default: false + t.boolean "wiki_page_events", default: true + t.boolean "pipeline_events", default: false, null: false + t.boolean "confidential_issues_events", default: true, null: false end add_index "services", ["project_id"], name: "index_services_on_project_id", using: :btree From 21f10af0956c69b6a5bad6b36b7d6e12e60e7867 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 19:11:46 -0300 Subject: [PATCH 08/10] Scope hooks thal will run for confidential issues --- app/models/hooks/project_hook.rb | 1 + app/models/service.rb | 1 + app/services/issues/base_service.rb | 9 +++---- spec/services/issues/close_service_spec.rb | 15 ++++++++--- spec/services/issues/create_service_spec.rb | 15 ++++++++--- spec/services/issues/reopen_service_spec.rb | 30 +++++++++++++++------ spec/services/issues/update_service_spec.rb | 6 ++--- 7 files changed, 55 insertions(+), 22 deletions(-) diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 836a75b0608..c631e7a7df5 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -2,6 +2,7 @@ class ProjectHook < WebHook belongs_to :project scope :issue_hooks, -> { where(issues_events: true) } + scope :confidential_issue_hooks, -> { where(confidential_issues_events: true) } scope :note_hooks, -> { where(note_events: true) } scope :merge_request_hooks, -> { where(merge_requests_events: true) } scope :build_hooks, -> { where(build_events: true) } diff --git a/app/models/service.rb b/app/models/service.rb index 7333f8d381b..198e7247838 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -34,6 +34,7 @@ class Service < ActiveRecord::Base scope :push_hooks, -> { where(push_events: true, active: true) } scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) } scope :issue_hooks, -> { where(issues_events: true, active: true) } + scope :confidential_issue_hooks, -> { where(confidential_issues_events: true, active: true) } scope :merge_request_hooks, -> { where(merge_requests_events: true, active: true) } scope :note_hooks, -> { where(note_events: true, active: true) } scope :build_hooks, -> { where(build_events: true, active: true) } diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 241efc44d36..9ea3ce084ba 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -14,11 +14,10 @@ module Issues end def execute_hooks(issue, action = 'open') - return if issue.confidential? - - issue_data = hook_data(issue, action) - issue.project.execute_hooks(issue_data, :issue_hooks) - issue.project.execute_services(issue_data, :issue_hooks) + issue_data = hook_data(issue, action) + hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks + issue.project.execute_hooks(issue_data, hooks_scope) + issue.project.execute_services(issue_data, hooks_scope) end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 4df99e41987..5dfb33f4b28 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -53,12 +53,21 @@ describe Issues::CloseService, services: true do end end + context 'when issue is not confidential' do + it 'executes issue hooks' do + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) + + described_class.new(project, user).execute(issue) + end + end + context 'when issue is confidential' do - it 'does not execute hooks' do + it 'executes confidential issue hooks' do issue = create(:issue, :confidential, project: project) - expect(project).not_to receive(:execute_hooks) - expect(project).not_to receive(:execute_services) + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) described_class.new(project, user).execute(issue) end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 81beca47738..58569ba96c3 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -73,11 +73,20 @@ describe Issues::CreateService, services: true do end end - it 'does not execute hooks when issue is confidential' do + it 'executes issue hooks when issue is not confidential' do + opts = { title: 'Title', description: 'Description', confidential: false } + + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) + + described_class.new(project, user, opts).execute + end + + it 'executes confidential issue hooks when issue is confidential' do opts = { title: 'Title', description: 'Description', confidential: true } - expect(project).not_to receive(:execute_hooks) - expect(project).not_to receive(:execute_services) + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) described_class.new(project, user, opts).execute end diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 4549e2b395b..93a8270fd16 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -5,7 +5,7 @@ describe Issues::ReopenService, services: true do let(:issue) { create(:issue, :closed, project: project) } describe '#execute' do - context 'current user is not authorized to reopen issue' do + context 'when user is not authorized to reopen issue' do before do guest = create(:user) project.team << [guest, :guest] @@ -20,17 +20,31 @@ describe Issues::ReopenService, services: true do end end - context 'when issue is confidential' do - it 'does not execute hooks' do - user = create(:user) + context 'when user is authrized to reopen issue' do + let(:user) { create(:user) } + + before do project.team << [user, :master] + end - issue = create(:issue, :confidential, :closed, project: project) + context 'when issue is not confidential' do + it 'executes issue hooks' do + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) - expect(project).not_to receive(:execute_hooks) - expect(project).not_to receive(:execute_services) + described_class.new(project, user).execute(issue) + end + end - described_class.new(project, user).execute(issue) + context 'when issue is confidential' do + it 'executes confidential issue hooks' do + issue = create(:issue, :confidential, :closed, project: project) + + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) + + described_class.new(project, user).execute(issue) + end end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 3c3a861b3af..4f5375a3583 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -105,9 +105,9 @@ describe Issues::UpdateService, services: true do expect(note.note).to eq 'Made the issue confidential' end - it 'does not execute hooks' do - expect(project).not_to receive(:execute_hooks) - expect(project).not_to receive(:execute_services) + it 'executes confidential issue hooks' do + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) update_issue(confidential: true) end From 443573ad936f792737fbd5f1902aa4fb87134231 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 31 Aug 2016 19:24:21 -0300 Subject: [PATCH 09/10] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 95d1504e982..f4b81bb4907 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -56,6 +56,7 @@ v 8.11.4 (unreleased) - Fix resolving conflicts on forks - Fix diff commenting on merge requests created prior to 8.10 - Don't create groups for unallowed users when importing projects + - Scope webhooks/services that will run for confidential issues - Fix issue boards leak private label names and descriptions v 8.11.3 From 9a5c9b46922aeb91e5027b0511e6526c8dd34827 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 1 Sep 2016 11:23:15 -0300 Subject: [PATCH 10/10] Add migration to set confidential issues events on web hooks --- ..._set_confidential_issues_events_on_webhooks.rb | 15 +++++++++++++++ db/schema.rb | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb diff --git a/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb b/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb new file mode 100644 index 00000000000..f1a1f001cb3 --- /dev/null +++ b/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb @@ -0,0 +1,15 @@ +class SetConfidentialIssuesEventsOnWebhooks < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + update_column_in_batches(:web_hooks, :confidential_issues_events, true) do |table, query| + query.where(table[:issues_events].eq(true)) + end + end + + def down + # noop + end +end diff --git a/db/schema.rb b/db/schema.rb index c9560abeb92..f5abdbfeb9c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160830232601) do +ActiveRecord::Schema.define(version: 20160901141443) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql"