From 9f7811e474a3f3f54f4624d19ec982239518ed67 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 13 Sep 2017 17:17:34 +0200 Subject: execute system hooks from project --- app/models/hooks/system_hook.rb | 4 +++- app/models/project.rb | 4 ++++ spec/models/hooks/system_hook_spec.rb | 3 ++- spec/models/project_spec.rb | 23 +++++++++++++++++++++++ 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb index 180c479c41b..78b053f36c3 100644 --- a/app/models/hooks/system_hook.rb +++ b/app/models/hooks/system_hook.rb @@ -2,7 +2,8 @@ class SystemHook < WebHook TRIGGERS = { repository_update_hooks: :repository_update_events, push_hooks: :push_events, - tag_push_hooks: :tag_push_events + tag_push_hooks: :tag_push_events, + merge_request_hooks: :merge_requests_events }.freeze TRIGGERS.each do |trigger, event| @@ -11,4 +12,5 @@ class SystemHook < WebHook default_value_for :push_events, false default_value_for :repository_update_events, true + default_value_for :merge_requests_events, false end diff --git a/app/models/project.rb b/app/models/project.rb index 9c0bbf697e2..a196c8b5c09 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -974,6 +974,10 @@ class Project < ActiveRecord::Base hook.async_execute(data, hooks_scope.to_s) end end + + SystemHook.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend + hook.async_execute(data, hooks_scope.to_s) + end end def execute_services(data, hooks_scope = :push_hooks) diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index 0e965f541d8..8bc45715dcd 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -7,7 +7,8 @@ describe SystemHook do it 'sets defined default parameters' do attrs = { push_events: false, - repository_update_events: true + repository_update_events: true, + merge_requests_events: false } expect(system_hook).to have_attributes(attrs) end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cea22bbd184..339186e25ae 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3177,4 +3177,27 @@ describe Project do expect { project.write_repository_config }.not_to raise_error end end + + describe '#execute_hooks' do + it 'executes the projects hooks with the specified scope' do + hook1 = create(:project_hook, merge_requests_events: true, tag_push_events: false) + hook2 = create(:project_hook, merge_requests_events: false, tag_push_events: true) + project = create(:project, hooks: [hook1, hook2]) + + expect_any_instance_of(ProjectHook).to receive(:async_execute).once + + project.execute_hooks({}, :tag_push_hooks) + end + + it 'executes the system hooks with the specified scope' do + create :system_hook, merge_requests_events: true, tag_push_events: false + create :system_hook, merge_requests_events: false, tag_push_events: true + allow_any_instance_of(SystemHook).to receive(:async_execute).once + project = create :project + + expect_any_instance_of(SystemHook).to receive(:async_execute).once + + project.execute_hooks({}, :tag_push_hooks) + end + end end -- cgit v1.2.1 From b22ae0c00eaa6bad6e664953167022d17c196d8d Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 14 Sep 2017 14:22:57 +0200 Subject: invoke SystemHooksService instead of direct model --- app/models/project.rb | 4 +--- spec/models/project_spec.rb | 10 +++------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index a196c8b5c09..a8c634200ce 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -975,9 +975,7 @@ class Project < ActiveRecord::Base end end - SystemHook.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend - hook.async_execute(data, hooks_scope.to_s) - end + SystemHooksService.new.execute_hooks(data, hooks_scope) end def execute_services(data, hooks_scope = :push_hooks) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 339186e25ae..e3899cc9bff 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3190,14 +3190,10 @@ describe Project do end it 'executes the system hooks with the specified scope' do - create :system_hook, merge_requests_events: true, tag_push_events: false - create :system_hook, merge_requests_events: false, tag_push_events: true - allow_any_instance_of(SystemHook).to receive(:async_execute).once - project = create :project + expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with({ data: 'data' }, :merge_request_hooks) - expect_any_instance_of(SystemHook).to receive(:async_execute).once - - project.execute_hooks({}, :tag_push_hooks) + project = build(:project) + project.execute_hooks({ data: 'data' }, :merge_request_hooks) end end end -- cgit v1.2.1 From d71d8ad7f8363d7ffa55d52e1dcbc6bfadaf070e Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 20 Sep 2017 10:41:43 +0200 Subject: test hook for system merge request hook --- app/services/test_hooks/system_service.rb | 7 +++++++ spec/services/test_hooks/system_service_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/services/test_hooks/system_service.rb b/app/services/test_hooks/system_service.rb index 67552edefc9..9016c77b7f0 100644 --- a/app/services/test_hooks/system_service.rb +++ b/app/services/test_hooks/system_service.rb @@ -13,5 +13,12 @@ module TestHooks def repository_update_events_data Gitlab::DataBuilder::Repository.sample_data end + + def merge_requests_events_data + merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first + throw(:validation_error, 'Ensure one of your projects has merge requests.') unless merge_request.present? + + merge_request.to_hook_data(current_user) + end end end diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index ff8b9595538..74d7715e50f 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -60,5 +60,25 @@ describe TestHooks::SystemService do expect(service.execute).to include(success_result) end end + + context 'merge_requests_events' do + let(:trigger) { 'merge_requests_events' } + + it 'returns error message if the user does not have any repository with a merge request' do + expect(hook).not_to receive(:execute) + expect(service.execute).to include({ status: :error, message: 'Ensure one of your projects has merge requests.' }) + end + + it 'executes hook' do + trigger_key = :merge_request_hooks + sample_data = { data: 'sample' } + create(:project_member, user: current_user, project: project) + create(:merge_request, source_project: project) + allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data) + + expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(service.execute).to include(success_result) + end + end end end -- cgit v1.2.1 From 630c3380510d4ada5d33b3dee0966e6dfb163dcf Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 20 Sep 2017 11:44:39 +0200 Subject: merge requests on system hook admin page --- app/views/admin/hooks/_form.html.haml | 7 ++++ spec/features/admin/admin_hooks_spec.rb | 71 +++++++++++++++++++++++++++++---- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/app/views/admin/hooks/_form.html.haml b/app/views/admin/hooks/_form.html.haml index 645005c6deb..1b6de917f5a 100644 --- a/app/views/admin/hooks/_form.html.haml +++ b/app/views/admin/hooks/_form.html.haml @@ -38,6 +38,13 @@ %strong Tag push events %p.light This URL will be triggered when a new tag is pushed to the repository + %div + = form.check_box :merge_requests_events, class: 'pull-left' + .prepend-left-20 + = form.label :merge_requests_events, class: 'list-label' do + %strong Merge Request events + %p.light + This URL will be triggered when a merge request is created/updated/merged .form-group = form.label :enable_ssl_verification, 'SSL verification', class: 'control-label checkbox' .col-sm-10 diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb index eec44549a03..d7f254ec89b 100644 --- a/spec/features/admin/admin_hooks_spec.rb +++ b/spec/features/admin/admin_hooks_spec.rb @@ -1,11 +1,10 @@ require 'spec_helper' -describe 'Admin::Hooks', :js do - before do - @project = create(:project) - sign_in(create(:admin)) +describe 'Admin::Hooks' do + let(:user) { create(:admin) } - @system_hook = create(:system_hook) + before do + sign_in(user) end describe 'GET /admin/hooks' do @@ -13,15 +12,17 @@ describe 'Admin::Hooks', :js do visit admin_root_path page.within '.nav-sidebar' do - click_on 'Hooks' + click_on 'System Hooks', match: :first end expect(current_path).to eq(admin_hooks_path) end it 'has hooks list' do + system_hook = create(:system_hook) + visit admin_hooks_path - expect(page).to have_content(@system_hook.url) + expect(page).to have_content(system_hook.url) end end @@ -43,6 +44,10 @@ describe 'Admin::Hooks', :js do describe 'Update existing hook' do let(:new_url) { generate(:url) } + before do + create(:system_hook) + end + it 'updates existing hook' do visit admin_hooks_path @@ -58,6 +63,10 @@ describe 'Admin::Hooks', :js do end describe 'Remove existing hook' do + before do + create(:system_hook) + end + context 'removes existing hook' do it 'from hooks list page' do visit admin_hooks_path @@ -76,7 +85,8 @@ describe 'Admin::Hooks', :js do describe 'Test', :js do before do - WebMock.stub_request(:post, @system_hook.url) + system_hook = create(:system_hook) + WebMock.stub_request(:post, system_hook.url) visit admin_hooks_path find('.hook-test-button.dropdown').click @@ -85,4 +95,49 @@ describe 'Admin::Hooks', :js do it { expect(current_path).to eq(admin_hooks_path) } end + + context 'Merge request hook' do + describe 'New Hook' do + let(:url) { generate(:url) } + + it 'adds new hook' do + visit admin_hooks_path + + fill_in 'hook_url', with: url + uncheck 'Repository update events' + check 'Merge Request events' + + expect { click_button 'Add system hook' }.to change(SystemHook, :count).by(1) + expect(current_path).to eq(admin_hooks_path) + expect(page).to have_content(url) + end + end + + describe 'Test', :js do + before do + system_hook = create(:system_hook) + WebMock.stub_request(:post, system_hook.url) + end + + it 'fails if the user does not have any repository with a merge request' do + visit admin_hooks_path + find('.hook-test-button.dropdown').click + click_link 'Merge requests events' + + expect(page).to have_content 'Ensure one of your projects has merge requests.' + end + + it 'succeeds if the user has a repository with a merge request' do + project = create(:project, :repository) + create(:project_member, user: user, project: project) + create(:merge_request, source_project: project) + + visit admin_hooks_path + find('.hook-test-button.dropdown').click + click_link 'Merge requests events' + + expect(page).to have_content 'Hook executed successfully' + end + end + end end -- cgit v1.2.1 From a2655e3fbda186ea598f6649dee8bb5565d6b980 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 13:49:35 +0100 Subject: use sentence case for "Merge request events" --- app/views/admin/hooks/_form.html.haml | 2 +- app/views/shared/web_hooks/_form.html.haml | 2 +- doc/workflow/notifications.md | 2 +- spec/features/admin/admin_hooks_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/admin/hooks/_form.html.haml b/app/views/admin/hooks/_form.html.haml index 1b6de917f5a..d8f96ed5b0d 100644 --- a/app/views/admin/hooks/_form.html.haml +++ b/app/views/admin/hooks/_form.html.haml @@ -42,7 +42,7 @@ = form.check_box :merge_requests_events, class: 'pull-left' .prepend-left-20 = form.label :merge_requests_events, class: 'list-label' do - %strong Merge Request events + %strong Merge request events %p.light This URL will be triggered when a merge request is created/updated/merged .form-group diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml index 1f0e7629fb4..ad4d39b4aa1 100644 --- a/app/views/shared/web_hooks/_form.html.haml +++ b/app/views/shared/web_hooks/_form.html.haml @@ -50,7 +50,7 @@ = form.check_box :merge_requests_events, class: 'pull-left' .prepend-left-20 = form.label :merge_requests_events, class: 'list-label' do - %strong Merge Request events + %strong Merge request events %p.light This URL will be triggered when a merge request is created/updated/merged %li diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 3e2e7d0f7b6..0203757e0e1 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -65,7 +65,7 @@ Below is the table of events users can be notified of: | Group access level changed | User | Sent when user group access level is changed | | Project moved | Project members [1] | [1] not disabled | -### Issue / Merge Request events +### Issue / Merge request events In all of the below cases, the notification will be sent to: - Participants: diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb index d7f254ec89b..197e6cd07c4 100644 --- a/spec/features/admin/admin_hooks_spec.rb +++ b/spec/features/admin/admin_hooks_spec.rb @@ -105,7 +105,7 @@ describe 'Admin::Hooks' do fill_in 'hook_url', with: url uncheck 'Repository update events' - check 'Merge Request events' + check 'Merge request events' expect { click_button 'Add system hook' }.to change(SystemHook, :count).by(1) expect(current_path).to eq(admin_hooks_path) -- cgit v1.2.1 From 57e2488085b623e4b48a6b8518e844fa842b6f4a Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 13:51:15 +0100 Subject: we only need a test for the happy path the failure case is already covered by the test in spec/services/test_hooks/system_service_spec.rb --- spec/features/admin/admin_hooks_spec.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb index 197e6cd07c4..725e603d1e3 100644 --- a/spec/features/admin/admin_hooks_spec.rb +++ b/spec/features/admin/admin_hooks_spec.rb @@ -119,14 +119,6 @@ describe 'Admin::Hooks' do WebMock.stub_request(:post, system_hook.url) end - it 'fails if the user does not have any repository with a merge request' do - visit admin_hooks_path - find('.hook-test-button.dropdown').click - click_link 'Merge requests events' - - expect(page).to have_content 'Ensure one of your projects has merge requests.' - end - it 'succeeds if the user has a repository with a merge request' do project = create(:project, :repository) create(:project_member, user: user, project: project) -- cgit v1.2.1 From 9a5bfcfdc53bf9b6217121b8639b43cc603f91c1 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 13:53:16 +0100 Subject: add changelog --- changelogs/unreleased/feature-merge-request-system-hook.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-merge-request-system-hook.yml diff --git a/changelogs/unreleased/feature-merge-request-system-hook.yml b/changelogs/unreleased/feature-merge-request-system-hook.yml new file mode 100644 index 00000000000..cfc4c4235d6 --- /dev/null +++ b/changelogs/unreleased/feature-merge-request-system-hook.yml @@ -0,0 +1,5 @@ +--- +title: System hooks for Merge Requests +merge_request: 14387 +author: Alexis Reigel +type: added -- cgit v1.2.1 From 2a0a7b426e63dac685654cafb0904e7668ad1fde Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 16:45:00 +0100 Subject: api support for merge_requests_events system hook --- doc/api/system_hooks.md | 1 + lib/api/entities.rb | 4 ++-- lib/api/system_hooks.rb | 1 + spec/requests/api/system_hooks_spec.rb | 20 +++++++++++++++++++- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/doc/api/system_hooks.md b/doc/api/system_hooks.md index 9750475f0a6..482f5083bb2 100644 --- a/doc/api/system_hooks.md +++ b/doc/api/system_hooks.md @@ -33,6 +33,7 @@ Example response: "created_at":"2016-10-31T12:32:15.192Z", "push_events":true, "tag_push_events":false, + "merge_requests_events": true, "enable_ssl_verification":true } ] diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 4ad4a1f7867..d09402b1c1c 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -65,12 +65,12 @@ module API end class Hook < Grape::Entity - expose :id, :url, :created_at, :push_events, :tag_push_events, :repository_update_events + expose :id, :url, :created_at, :push_events, :tag_push_events, :merge_requests_events, :repository_update_events expose :enable_ssl_verification end class ProjectHook < Hook - expose :project_id, :issues_events, :merge_requests_events + expose :project_id, :issues_events expose :note_events, :pipeline_events, :wiki_page_events expose :job_events end diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index 6b6a03e3300..c7a460df46a 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -26,6 +26,7 @@ module API optional :token, type: String, desc: 'The token used to validate payloads' optional :push_events, type: Boolean, desc: "Trigger hook on push events" optional :tag_push_events, type: Boolean, desc: "Trigger hook on tag push events" + optional :merge_requests_events, type: Boolean, desc: "Trigger hook on tag push events" optional :enable_ssl_verification, type: Boolean, desc: "Do SSL verification when triggering the hook" end post do diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index c7a009e999e..6c57d443cbf 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/spec/requests/api/system_hooks_spec.rb @@ -36,6 +36,7 @@ describe API::SystemHooks do expect(json_response.first['url']).to eq(hook.url) expect(json_response.first['push_events']).to be false expect(json_response.first['tag_push_events']).to be false + expect(json_response.first['merge_requests_events']).to be false expect(json_response.first['repository_update_events']).to be true end end @@ -67,11 +68,28 @@ describe API::SystemHooks do end it 'sets default values for events' do - post api('/hooks', admin), url: 'http://mep.mep', enable_ssl_verification: true + post api('/hooks', admin), url: 'http://mep.mep' expect(response).to have_gitlab_http_status(201) expect(json_response['enable_ssl_verification']).to be true + expect(json_response['push_events']).to be false expect(json_response['tag_push_events']).to be false + expect(json_response['merge_requests_events']).to be false + end + + it 'sets explicit values for events' do + post api('/hooks', admin), + url: 'http://mep.mep', + enable_ssl_verification: false, + push_events: true, + tag_push_events: true, + merge_requests_events: true + + expect(response).to have_http_status(201) + expect(json_response['enable_ssl_verification']).to be false + expect(json_response['push_events']).to be true + expect(json_response['tag_push_events']).to be true + expect(json_response['merge_requests_events']).to be true end end -- cgit v1.2.1 From 7c669eee49dba533ed95b559ba56bc3fdb7e0c29 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 21:36:58 +0100 Subject: doc for system hook merge_requests_events api --- doc/api/system_hooks.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/api/system_hooks.md b/doc/api/system_hooks.md index 482f5083bb2..dd424470b67 100644 --- a/doc/api/system_hooks.md +++ b/doc/api/system_hooks.md @@ -55,6 +55,7 @@ POST /hooks | `token` | string | no | Secret token to validate received payloads; this will not be returned in the response | | `push_events` | boolean | no | When true, the hook will fire on push events | | `tag_push_events` | boolean | no | When true, the hook will fire on new tags being pushed | +| `merge_requests_events` | boolean | no | Trigger hook on merge requests events | | `enable_ssl_verification` | boolean | no | Do SSL verification when triggering the hook | Example request: @@ -73,6 +74,7 @@ Example response: "created_at":"2016-10-31T12:32:15.192Z", "push_events":true, "tag_push_events":false, + "merge_requests_events": true, "enable_ssl_verification":true } ] -- cgit v1.2.1 From ecf6f1f8729119f7a7c4b9847a6a5c4eea331cab Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 4 Jan 2018 09:13:26 +0000 Subject: Revert "Merge branch 'pre-commit-prettier' into 'master'" This reverts merge request !16061 --- package.json | 1 - scripts/add-code-formatters | 18 ------------------ scripts/pre-commit | 18 ------------------ yarn.lock | 4 ---- 4 files changed, 41 deletions(-) delete mode 100755 scripts/add-code-formatters delete mode 100644 scripts/pre-commit diff --git a/package.json b/package.json index 8c3932dccfd..d80e25e1ac6 100644 --- a/package.json +++ b/package.json @@ -61,7 +61,6 @@ "mousetrap": "^1.4.6", "name-all-modules-plugin": "^1.0.1", "pikaday": "^1.6.1", - "prettier": "^1.9.2", "prismjs": "^1.6.0", "raphael": "^2.2.7", "raven-js": "^3.14.0", diff --git a/scripts/add-code-formatters b/scripts/add-code-formatters deleted file mode 100755 index 56bb8754d80..00000000000 --- a/scripts/add-code-formatters +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh - -# Check if file exists with -f. Check if in in the gdk rook directory. -if [ ! -f ../GDK_ROOT ]; then - echo "Please run script from gitlab (e.g. gitlab-development-kit/gitlab) root directory." - exit 1 -fi - -PRECOMMIT=$(git rev-parse --git-dir)/hooks/pre-commit - -# Check if symlink exists with -L. Check if script was already installed. -if [ -L $PRECOMMIT ]; then - echo "Pre-commit script already installed." - exit 1 -fi - -ln -s ./pre-commit $PRECOMMIT -echo "Pre-commit script installed successfully" diff --git a/scripts/pre-commit b/scripts/pre-commit deleted file mode 100644 index 48935e90a87..00000000000 --- a/scripts/pre-commit +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh - -# Check if file exists with -f. Check if in in the gdk rook directory. -if [ ! -f ../GDK_ROOT ]; then - echo "Please run pre-commit from gitlab (e.g. gitlab-development-kit/gitlab) root directory." - exit 1 -fi - -jsfiles=$(git diff --cached --name-only --diff-filter=ACM "*.js" | tr '\n' ' ') -[ -z "$jsfiles" ] && exit 0 - -# Prettify all staged .js files -echo "$jsfiles" | xargs ./node_modules/.bin/prettier --write - -# Add back the modified/prettified files to staging -echo "$jsfiles" | xargs git add - -exit 0 diff --git a/yarn.lock b/yarn.lock index 381b1a243f8..358a1baec42 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5150,10 +5150,6 @@ prettier@^1.7.0: version "1.8.2" resolved "https://registry.yarnpkg.com/prettier/-/prettier-1.8.2.tgz#bff83e7fd573933c607875e5ba3abbdffb96aeb8" -prettier@^1.9.2: - version "1.9.2" - resolved "https://registry.yarnpkg.com/prettier/-/prettier-1.9.2.tgz#96bc2132f7a32338e6078aeb29727178c6335827" - prismjs@^1.6.0: version "1.6.0" resolved "https://registry.yarnpkg.com/prismjs/-/prismjs-1.6.0.tgz#118d95fb7a66dba2272e343b345f5236659db365" -- cgit v1.2.1 From eaab545903749f88371bcf0b2bf4293189fc8a6e Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Sun, 7 Jan 2018 12:28:17 +0900 Subject: Remove unused CSS selectors for Cycle Analytics --- app/assets/stylesheets/pages/cycle_analytics.scss | 41 ---------------------- .../41743-unused-selectors-for-cycle-analytics.yml | 5 +++ 2 files changed, 5 insertions(+), 41 deletions(-) create mode 100644 changelogs/unreleased/41743-unused-selectors-for-cycle-analytics.yml diff --git a/app/assets/stylesheets/pages/cycle_analytics.scss b/app/assets/stylesheets/pages/cycle_analytics.scss index 3b35beb7695..cfef6476d4d 100644 --- a/app/assets/stylesheets/pages/cycle_analytics.scss +++ b/app/assets/stylesheets/pages/cycle_analytics.scss @@ -117,47 +117,6 @@ top: $gl-padding-top; } - .content-list { - li { - padding: 18px $gl-padding $gl-padding; - - .container-fluid { - padding: 0; - } - } - - .title-col { - p { - margin: 0; - - &.title { - line-height: 19px; - font-size: 14px; - font-weight: $gl-font-weight-bold; - color: $gl-text-color; - } - - &.text { - color: $layout-link-gray; - - &.value-col { - color: $gl-text-color; - } - } - } - } - - .value-col { - text-align: right; - - span { - position: relative; - vertical-align: middle; - top: 3px; - } - } - } - .fa-spinner { font-size: 28px; position: relative; diff --git a/changelogs/unreleased/41743-unused-selectors-for-cycle-analytics.yml b/changelogs/unreleased/41743-unused-selectors-for-cycle-analytics.yml new file mode 100644 index 00000000000..03060c357fe --- /dev/null +++ b/changelogs/unreleased/41743-unused-selectors-for-cycle-analytics.yml @@ -0,0 +1,5 @@ +--- +title: Remove unused CSS selectors for Cycle Analytics +merge_request: 16270 +author: Takuya Noguchi +type: other -- cgit v1.2.1 From 8b6162eeefcf15b2d52a9cb6f0049de38b65ec23 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 11 Jan 2018 19:02:30 +0000 Subject: Remove remaining word from artifacts date --- app/assets/javascripts/job.js | 12 +----------- app/views/projects/jobs/_sidebar.html.haml | 2 +- spec/javascripts/job_spec.js | 5 ----- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/job.js b/app/assets/javascripts/job.js index 8f32dcc94e2..7927e97ea47 100644 --- a/app/assets/javascripts/job.js +++ b/app/assets/javascripts/job.js @@ -71,7 +71,6 @@ export default class Job { .off('resize.build') .on('resize.build', _.throttle(this.sidebarOnResize.bind(this), 100)); - this.updateArtifactRemoveDate(); this.initAffixTopArea(); this.getBuildTrace(); @@ -261,16 +260,7 @@ export default class Job { sidebarOnClick() { if (this.shouldHideSidebarForViewport()) this.toggleSidebar(); } - // eslint-disable-next-line class-methods-use-this, consistent-return - updateArtifactRemoveDate() { - const $date = $('.js-artifacts-remove'); - if ($date.length) { - const date = $date.text(); - return $date.text( - timeFor(new Date(date.replace(/([0-9]+)-([0-9]+)-([0-9]+)/g, '$1/$2/$3'))), - ); - } - } + // eslint-disable-next-line class-methods-use-this populateJobs(stage) { $('.build-job').hide(); diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml index a71333497e6..e779473c239 100644 --- a/app/views/projects/jobs/_sidebar.html.haml +++ b/app/views/projects/jobs/_sidebar.html.haml @@ -24,7 +24,7 @@ - elsif @build.has_expiring_artifacts? %p.build-detail-row The artifacts will be removed in - %span.js-artifacts-remove= @build.artifacts_expire_at + %span= time_ago_in_words @build.artifacts_expire_at - if @build.artifacts? .btn-group.btn-group-justified{ role: :group } diff --git a/spec/javascripts/job_spec.js b/spec/javascripts/job_spec.js index b740c9ed893..feb341d22e6 100644 --- a/spec/javascripts/job_spec.js +++ b/spec/javascripts/job_spec.js @@ -52,11 +52,6 @@ describe('Job', () => { expect($('.build-job[data-stage="test"]').is(':visible')).toBe(false); expect($('.build-job[data-stage="deploy"]').is(':visible')).toBe(false); }); - - it('displays the remove date correctly', () => { - const removeDateElement = document.querySelector('.js-artifacts-remove'); - expect(removeDateElement.innerText.trim()).toBe('1 year remaining'); - }); }); describe('running build', () => { -- cgit v1.2.1 From 3f1235a17cf420ed2a1c830c4030b1a062039c51 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 11 Jan 2018 19:50:41 +0000 Subject: Fix bad import in job.js --- app/assets/javascripts/job.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/job.js b/app/assets/javascripts/job.js index 7927e97ea47..9b5092c5e3f 100644 --- a/app/assets/javascripts/job.js +++ b/app/assets/javascripts/job.js @@ -3,7 +3,6 @@ import { visitUrl } from './lib/utils/url_utility'; import bp from './breakpoints'; import { numberToHumanSize } from './lib/utils/number_utils'; import { setCiStatusFavicon } from './lib/utils/common_utils'; -import { timeFor } from './lib/utils/datetime_utility'; export default class Job { constructor(options) { -- cgit v1.2.1 From dcc1ab979a9acdb94aa33a46c262ca21cc8055a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Kadlecova=CC=81?= Date: Wed, 10 Jan 2018 08:29:58 +0100 Subject: Keep subscribers when promoting labels to group labels --- app/services/labels/promote_service.rb | 12 +++++++++++- changelogs/unreleased/37199-labels-fix.yml | 5 +++++ spec/services/labels/promote_service_spec.rb | 13 +++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/37199-labels-fix.yml diff --git a/app/services/labels/promote_service.rb b/app/services/labels/promote_service.rb index 997d247be46..74a85e5c9f0 100644 --- a/app/services/labels/promote_service.rb +++ b/app/services/labels/promote_service.rb @@ -13,6 +13,7 @@ module Labels update_issuables(new_label, batched_ids) update_issue_board_lists(new_label, batched_ids) update_priorities(new_label, batched_ids) + subscribe_users(new_label, batched_ids) # Order is important, project labels need to be last update_project_labels(batched_ids) end @@ -26,6 +27,15 @@ module Labels private + def subscribe_users(new_label, label_ids) + # users can be subscribed to multiple labels that will be merged into the group one + # we want to keep only one subscription / user + ids_to_update = Subscription.where(subscribable_id: label_ids, subscribable_type: 'Label') + .group(:user_id) + .pluck('MAX(id)') + Subscription.where(id: ids_to_update).update_all(subscribable_id: new_label.id) + end + def label_ids_for_merge(new_label) LabelsFinder .new(current_user, title: new_label.title, group_id: project.group.id) @@ -53,7 +63,7 @@ module Labels end def update_project_labels(label_ids) - Label.where(id: label_ids).delete_all + Label.where(id: label_ids).destroy_all end def clone_label_to_group_label(label) diff --git a/changelogs/unreleased/37199-labels-fix.yml b/changelogs/unreleased/37199-labels-fix.yml new file mode 100644 index 00000000000..bd70babb73d --- /dev/null +++ b/changelogs/unreleased/37199-labels-fix.yml @@ -0,0 +1,5 @@ +--- +title: Keep subscribers when promoting labels to group labels +merge_request: +author: +type: fixed diff --git a/spec/services/labels/promote_service_spec.rb b/spec/services/labels/promote_service_spec.rb index 8809b282127..aa9aba6bdff 100644 --- a/spec/services/labels/promote_service_spec.rb +++ b/spec/services/labels/promote_service_spec.rb @@ -85,6 +85,19 @@ describe Labels::PromoteService do change(project_3.labels, :count).by(-1) end + it 'keeps users\' subscriptions' do + user2 = create(:user) + project_label_1_1.subscriptions.create(user: user, subscribed: true) + project_label_2_1.subscriptions.create(user: user, subscribed: true) + project_label_3_2.subscriptions.create(user: user, subscribed: true) + project_label_2_1.subscriptions.create(user: user2, subscribed: true) + + expect { service.execute(project_label_1_1) }.to change { Subscription.count }.from(4).to(3) + + expect(new_label.subscribed?(user)).to be_truthy + expect(new_label.subscribed?(user2)).to be_truthy + end + it 'recreates priorities' do service.execute(project_label_1_1) -- cgit v1.2.1 From af79397f37f8f9e1f21017d42b7ec98d6ff79263 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 15 Jan 2018 10:20:55 +0000 Subject: Remove IDE option from user preferences --- app/views/profiles/preferences/show.html.haml | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/app/views/profiles/preferences/show.html.haml b/app/views/profiles/preferences/show.html.haml index aeae7455a1c..66d1d1e8d44 100644 --- a/app/views/profiles/preferences/show.html.haml +++ b/app/views/profiles/preferences/show.html.haml @@ -3,23 +3,6 @@ = render 'profiles/head' = form_for @user, url: profile_preferences_path, remote: true, method: :put, html: { class: 'row prepend-top-default js-preferences-form' } do |f| - .col-lg-4 - %h4.prepend-top-0 - Web IDE (Beta) - %p Enable the new web IDE on this device to make it possible to open and edit multiple files with a single commit - .col-lg-8.multi-file-editor-options - = label_tag do - .preview.append-bottom-10= image_tag "multi-editor-off.png" - = f.radio_button :multi_file, "off", checked: true - Off - = label_tag do - .preview.append-bottom-10= image_tag "multi-editor-on.png" - = f.radio_button :multi_file, "on", checked: false - On - - .col-sm-12 - %hr - .col-lg-4.application-theme %h4.prepend-top-0 GitLab navigation theme -- cgit v1.2.1 From 18abb3034209525a7e9a9a98d92ae5a168a97251 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 15 Jan 2018 12:41:00 +0000 Subject: remove specs --- .../profiles/user_visits_profile_preferences_page_spec.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/spec/features/profiles/user_visits_profile_preferences_page_spec.rb b/spec/features/profiles/user_visits_profile_preferences_page_spec.rb index 266af8f4e3d..90d6841af0e 100644 --- a/spec/features/profiles/user_visits_profile_preferences_page_spec.rb +++ b/spec/features/profiles/user_visits_profile_preferences_page_spec.rb @@ -32,18 +32,6 @@ describe 'User visits the profile preferences page' do end end - describe 'User changes their multi file editor preferences', :js do - it 'set the new_repo cookie when the option is ON' do - choose 'user_multi_file_on' - expect(get_cookie('new_repo')).not_to be_nil - end - - it 'deletes the new_repo cookie when the option is OFF' do - choose 'user_multi_file_off' - expect(get_cookie('new_repo')).to be_nil - end - end - describe 'User changes their default dashboard', :js do it 'creates a flash message' do select 'Starred Projects', from: 'user_dashboard' -- cgit v1.2.1 From e268575b08ccaf30a844fdb37898d680d0cbb98d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 15 Jan 2018 22:38:15 +0800 Subject: Add view elements to deploy keys pages --- qa/qa/page/project/settings/common.rb | 4 ++-- qa/qa/page/project/settings/deploy_keys.rb | 18 ++++++++++++------ qa/qa/page/project/settings/repository.rb | 11 ++++------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/qa/qa/page/project/settings/common.rb b/qa/qa/page/project/settings/common.rb index 5d1d5120929..b4ef07e1540 100644 --- a/qa/qa/page/project/settings/common.rb +++ b/qa/qa/page/project/settings/common.rb @@ -3,9 +3,9 @@ module QA module Project module Settings module Common - def expand(selector) + def expand(element_name) page.within('#content-body') do - find(selector).click + click_element(element_name) yield end diff --git a/qa/qa/page/project/settings/deploy_keys.rb b/qa/qa/page/project/settings/deploy_keys.rb index a8d6f09777c..c6a07b59493 100644 --- a/qa/qa/page/project/settings/deploy_keys.rb +++ b/qa/qa/page/project/settings/deploy_keys.rb @@ -3,12 +3,18 @@ module QA module Project module Settings class DeployKeys < Page::Base - ## - # TODO, define all selectors required by this page object - # - # See gitlab-org/gitlab-qa#154 - # - view 'app/views/projects/deploy_keys/edit.html.haml' + view 'app/views/projects/deploy_keys/_form.html.haml' do + element :deploy_key_title, 'text_field :title' + element :deploy_key_key, 'text_area :key' + end + + view 'app/assets/javascripts/deploy_keys/components/app.vue' do + element :deploy_keys_section, 'deploy-keys' + end + + view 'app/assets/javascripts/deploy_keys/components/key.vue' do + element :key_title, 'class="title"' + end def fill_key_title(title) fill_in 'deploy_key_title', with: title diff --git a/qa/qa/page/project/settings/repository.rb b/qa/qa/page/project/settings/repository.rb index 524d87c6be9..6cc68358c8c 100644 --- a/qa/qa/page/project/settings/repository.rb +++ b/qa/qa/page/project/settings/repository.rb @@ -5,15 +5,12 @@ module QA class Repository < Page::Base include Common - ## - # TODO, define all selectors required by this page object - # - # See gitlab-org/gitlab-qa#154 - # - view 'app/views/projects/settings/repository/show.html.haml' + view 'app/views/projects/deploy_keys/_index.html.haml' do + element :expand_deploy_keys + end def expand_deploy_keys(&block) - expand('.qa-expand-deploy-keys') do + expand(:expand_deploy_keys) do DeployKeys.perform(&block) end end -- cgit v1.2.1 From 225c1c1239c85583dd745b6e8965857673d2e114 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 16 Jan 2018 10:45:04 +0000 Subject: Replace pipeline's action icons with svg --- app/assets/javascripts/pipelines/components/async_button.vue | 12 +++++------- .../javascripts/pipelines/components/pipelines_actions.vue | 8 +++++--- .../javascripts/pipelines/components/pipelines_table_row.vue | 2 +- app/assets/stylesheets/pages/pipelines.scss | 7 ------- spec/javascripts/pipelines/async_button_spec.js | 6 +++--- 5 files changed, 14 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/pipelines/components/async_button.vue b/app/assets/javascripts/pipelines/components/async_button.vue index 4ad3f66ee8c..77553ca67cc 100644 --- a/app/assets/javascripts/pipelines/components/async_button.vue +++ b/app/assets/javascripts/pipelines/components/async_button.vue @@ -3,6 +3,7 @@ import eventHub from '../event_hub'; import loadingIcon from '../../vue_shared/components/loading_icon.vue'; + import icon from '../../vue_shared/components/icon.vue'; import tooltip from '../../vue_shared/directives/tooltip'; export default { @@ -11,6 +12,7 @@ }, components: { loadingIcon, + icon, }, props: { endpoint: { @@ -41,9 +43,6 @@ }; }, computed: { - iconClass() { - return `fa fa-${this.icon}`; - }, buttonClass() { return `btn ${this.cssClass}`; }, @@ -76,10 +75,9 @@ data-container="body" data-placement="top" :disabled="isLoading"> - + diff --git a/app/assets/javascripts/pipelines/components/pipelines_actions.vue b/app/assets/javascripts/pipelines/components/pipelines_actions.vue index efda36c12d6..4d4b6b1776d 100644 --- a/app/assets/javascripts/pipelines/components/pipelines_actions.vue +++ b/app/assets/javascripts/pipelines/components/pipelines_actions.vue @@ -1,7 +1,7 @@ + + diff --git a/app/assets/javascripts/pages/projects/shared/permissions/components/project_setting_row.vue b/app/assets/javascripts/pages/projects/shared/permissions/components/project_setting_row.vue new file mode 100644 index 00000000000..25a88f846eb --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/permissions/components/project_setting_row.vue @@ -0,0 +1,51 @@ + + + diff --git a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue new file mode 100644 index 00000000000..380c6c3ea7d --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue @@ -0,0 +1,328 @@ + + + diff --git a/app/assets/javascripts/pages/projects/shared/permissions/constants.js b/app/assets/javascripts/pages/projects/shared/permissions/constants.js new file mode 100644 index 00000000000..ce47562f259 --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/permissions/constants.js @@ -0,0 +1,11 @@ +export const visibilityOptions = { + PRIVATE: 0, + INTERNAL: 10, + PUBLIC: 20, +}; + +export const visibilityLevelDescriptions = { + [visibilityOptions.PRIVATE]: 'The project is accessible only by members of the project. Access must be granted explicitly to each user.', + [visibilityOptions.INTERNAL]: 'The project can be accessed by any user who is logged in.', + [visibilityOptions.PUBLIC]: 'The project can be accessed by anyone, regardless of authentication.', +}; diff --git a/app/assets/javascripts/pages/projects/shared/permissions/external.js b/app/assets/javascripts/pages/projects/shared/permissions/external.js new file mode 100644 index 00000000000..460af4a2111 --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/permissions/external.js @@ -0,0 +1,18 @@ +const selectorCache = []; + +// workaround since we don't have a polyfill for classList.toggle 2nd parameter +export function toggleHiddenClass(element, hidden) { + if (hidden) { + element.classList.add('hidden'); + } else { + element.classList.remove('hidden'); + } +} + +// hide external feature-specific settings when a given feature is disabled +export function toggleHiddenClassBySelector(selector, hidden) { + if (!selectorCache[selector]) { + selectorCache[selector] = document.querySelectorAll(selector); + } + selectorCache[selector].forEach(elm => toggleHiddenClass(elm, hidden)); +} diff --git a/app/assets/javascripts/pages/projects/shared/permissions/index.js b/app/assets/javascripts/pages/projects/shared/permissions/index.js new file mode 100644 index 00000000000..dbde8dda634 --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/permissions/index.js @@ -0,0 +1,13 @@ +import Vue from 'vue'; +import settingsPanel from './components/settings_panel.vue'; + +export default function initProjectPermissionsSettings() { + const mountPoint = document.querySelector('.js-project-permissions-form'); + const componentPropsEl = document.querySelector('.js-project-permissions-form-data'); + const componentProps = JSON.parse(componentPropsEl.innerHTML); + + return new Vue({ + el: mountPoint, + render: createElement => createElement(settingsPanel, { props: { ...componentProps } }), + }); +} diff --git a/app/assets/javascripts/pages/projects/shared/project_avatar.js b/app/assets/javascripts/pages/projects/shared/project_avatar.js new file mode 100644 index 00000000000..56627aa155c --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/project_avatar.js @@ -0,0 +1,13 @@ +export default function projectAvatar() { + $('.js-choose-project-avatar-button').bind('click', function onClickAvatar() { + const form = $(this).closest('form'); + return form.find('.js-project-avatar-input').click(); + }); + + $('.js-project-avatar-input').bind('change', function onClickAvatarInput() { + const form = $(this).closest('form'); + // eslint-disable-next-line no-useless-escape + const filename = $(this).val().replace(/^.*[\\\/]/, ''); + return form.find('.js-avatar-filename').text(filename); + }); +} diff --git a/app/assets/javascripts/pages/projects/shared/project_new.js b/app/assets/javascripts/pages/projects/shared/project_new.js new file mode 100644 index 00000000000..86faba0b910 --- /dev/null +++ b/app/assets/javascripts/pages/projects/shared/project_new.js @@ -0,0 +1,151 @@ +/* eslint-disable func-names, no-var, no-underscore-dangle, prefer-template, prefer-arrow-callback*/ + +import VisibilitySelect from '../../../visibility_select'; + +function highlightChanges($elm) { + $elm.addClass('highlight-changes'); + setTimeout(() => $elm.removeClass('highlight-changes'), 10); +} + +export default class ProjectNew { + constructor() { + this.toggleSettings = this.toggleSettings.bind(this); + this.$selects = $('.features select'); + this.$repoSelects = this.$selects.filter('.js-repo-select'); + this.$projectSelects = this.$selects.not('.js-repo-select'); + + $('.project-edit-container').on('ajax:before', () => { + $('.project-edit-container').hide(); + return $('.save-project-loader').show(); + }); + + this.initVisibilitySelect(); + + this.toggleSettings(); + this.toggleSettingsOnclick(); + this.toggleRepoVisibility(); + } + + initVisibilitySelect() { + const visibilityContainer = document.querySelector('.js-visibility-select'); + if (!visibilityContainer) return; + const visibilitySelect = new VisibilitySelect(visibilityContainer); + visibilitySelect.init(); + + const $visibilitySelect = $(visibilityContainer).find('select'); + let projectVisibility = $visibilitySelect.val(); + const PROJECT_VISIBILITY_PRIVATE = '0'; + + $visibilitySelect.on('change', () => { + const newProjectVisibility = $visibilitySelect.val(); + + if (projectVisibility !== newProjectVisibility) { + this.$projectSelects.each((idx, select) => { + const $select = $(select); + const $options = $select.find('option'); + const values = $.map($options, e => e.value); + + // if switched to "private", limit visibility options + if (newProjectVisibility === PROJECT_VISIBILITY_PRIVATE) { + if ($select.val() !== values[0] && $select.val() !== values[1]) { + $select.val(values[1]).trigger('change'); + highlightChanges($select); + } + $options.slice(2).disable(); + } + + // if switched from "private", increase visibility for non-disabled options + if (projectVisibility === PROJECT_VISIBILITY_PRIVATE) { + $options.enable(); + if ($select.val() !== values[0] && $select.val() !== values[values.length - 1]) { + $select.val(values[values.length - 1]).trigger('change'); + highlightChanges($select); + } + } + }); + + projectVisibility = newProjectVisibility; + } + }); + } + + toggleSettings() { + this.$selects.each(function () { + var $select = $(this); + var className = $select.data('field') + .replace(/_/g, '-') + .replace('access-level', 'feature'); + ProjectNew._showOrHide($select, '.' + className); + }); + } + + toggleSettingsOnclick() { + this.$selects.on('change', this.toggleSettings); + } + + static _showOrHide(checkElement, container) { + const $container = $(container); + + if ($(checkElement).val() !== '0') { + return $container.show(); + } + return $container.hide(); + } + + toggleRepoVisibility() { + var $repoAccessLevel = $('.js-repo-access-level select'); + var $lfsEnabledOption = $('.js-lfs-enabled select'); + var containerRegistry = document.querySelectorAll('.js-container-registry')[0]; + var containerRegistryCheckbox = document.getElementById('project_container_registry_enabled'); + var prevSelectedVal = parseInt($repoAccessLevel.val(), 10); + + this.$repoSelects.find("option[value='" + $repoAccessLevel.val() + "']") + .nextAll() + .hide(); + + $repoAccessLevel + .off('change') + .on('change', function () { + var selectedVal = parseInt($repoAccessLevel.val(), 10); + + this.$repoSelects.each(function () { + var $this = $(this); + var repoSelectVal = parseInt($this.val(), 10); + + $this.find('option').enable(); + + if (selectedVal < repoSelectVal || repoSelectVal === prevSelectedVal) { + $this.val(selectedVal).trigger('change'); + highlightChanges($this); + } + + $this.find("option[value='" + selectedVal + "']").nextAll().disable(); + }); + + if (selectedVal) { + this.$repoSelects.removeClass('disabled'); + + if ($lfsEnabledOption.length) { + $lfsEnabledOption.removeClass('disabled'); + highlightChanges($lfsEnabledOption); + } + if (containerRegistry) { + containerRegistry.style.display = ''; + } + } else { + this.$repoSelects.addClass('disabled'); + + if ($lfsEnabledOption.length) { + $lfsEnabledOption.val('false').addClass('disabled'); + highlightChanges($lfsEnabledOption); + } + if (containerRegistry) { + containerRegistry.style.display = 'none'; + containerRegistryCheckbox.checked = false; + } + } + + prevSelectedVal = selectedVal; + }.bind(this)); + } +} diff --git a/app/assets/javascripts/pages/projects/wikis/index.js b/app/assets/javascripts/pages/projects/wikis/index.js new file mode 100644 index 00000000000..eb14c7a0e78 --- /dev/null +++ b/app/assets/javascripts/pages/projects/wikis/index.js @@ -0,0 +1,11 @@ +import Wikis from './wikis'; +import ShortcutsWiki from '../../../shortcuts_wiki'; +import ZenMode from '../../../zen_mode'; +import GLForm from '../../../gl_form'; + +export default () => { + new Wikis(); // eslint-disable-line no-new + new ShortcutsWiki(); // eslint-disable-line no-new + new ZenMode(); // eslint-disable-line no-new + new GLForm($('.wiki-form'), true); // eslint-disable-line no-new +}; diff --git a/app/assets/javascripts/pages/projects/wikis/wikis.js b/app/assets/javascripts/pages/projects/wikis/wikis.js new file mode 100644 index 00000000000..34a12ef76a1 --- /dev/null +++ b/app/assets/javascripts/pages/projects/wikis/wikis.js @@ -0,0 +1,60 @@ +import bp from '../../../breakpoints'; +import { slugify } from '../../../lib/utils/text_utility'; + +export default class Wikis { + constructor() { + this.sidebarEl = document.querySelector('.js-wiki-sidebar'); + this.sidebarExpanded = false; + + const sidebarToggles = document.querySelectorAll('.js-sidebar-wiki-toggle'); + for (let i = 0; i < sidebarToggles.length; i += 1) { + sidebarToggles[i].addEventListener('click', e => this.handleToggleSidebar(e)); + } + + this.newWikiForm = document.querySelector('form.new-wiki-page'); + if (this.newWikiForm) { + this.newWikiForm.addEventListener('submit', e => this.handleNewWikiSubmit(e)); + } + + window.addEventListener('resize', () => this.renderSidebar()); + this.renderSidebar(); + } + + handleNewWikiSubmit(e) { + if (!this.newWikiForm) return; + + const slugInput = this.newWikiForm.querySelector('#new_wiki_path'); + const slug = slugify(slugInput.value); + + if (slug.length > 0) { + const wikisPath = slugInput.getAttribute('data-wikis-path'); + window.location.href = `${wikisPath}/${slug}`; + e.preventDefault(); + } + } + + handleToggleSidebar(e) { + e.preventDefault(); + this.sidebarExpanded = !this.sidebarExpanded; + this.renderSidebar(); + } + + static sidebarCanCollapse() { + const bootstrapBreakpoint = bp.getBreakpointSize(); + return bootstrapBreakpoint === 'xs' || bootstrapBreakpoint === 'sm'; + } + + renderSidebar() { + if (!this.sidebarEl) return; + const { classList } = this.sidebarEl; + if (this.sidebarExpanded || !Wikis.sidebarCanCollapse()) { + if (!classList.contains('right-sidebar-expanded')) { + classList.remove('right-sidebar-collapsed'); + classList.add('right-sidebar-expanded'); + } + } else if (classList.contains('right-sidebar-expanded')) { + classList.add('right-sidebar-collapsed'); + classList.remove('right-sidebar-expanded'); + } + } +} diff --git a/app/assets/javascripts/project.js b/app/assets/javascripts/project.js deleted file mode 100644 index d4f26b81f30..00000000000 --- a/app/assets/javascripts/project.js +++ /dev/null @@ -1,133 +0,0 @@ -/* eslint-disable func-names, space-before-function-paren, no-var, consistent-return, no-new, prefer-arrow-callback, no-return-assign, one-var, one-var-declaration-per-line, object-shorthand, no-else-return, newline-per-chained-call, no-shadow, vars-on-top, prefer-template, max-len */ - -import Cookies from 'js-cookie'; -import { visitUrl } from './lib/utils/url_utility'; -import projectSelect from './project_select'; - -export default class Project { - constructor() { - const $cloneOptions = $('ul.clone-options-dropdown'); - const $projectCloneField = $('#project_clone'); - const $cloneBtnText = $('a.clone-dropdown-btn span'); - - const selectedCloneOption = $cloneBtnText.text().trim(); - if (selectedCloneOption.length > 0) { - $(`a:contains('${selectedCloneOption}')`, $cloneOptions).addClass('is-active'); - } - - $('a', $cloneOptions).on('click', (e) => { - const $this = $(e.currentTarget); - const url = $this.attr('href'); - const activeText = $this.find('.dropdown-menu-inner-title').text(); - - e.preventDefault(); - - $('.is-active', $cloneOptions).not($this).removeClass('is-active'); - $this.toggleClass('is-active'); - $projectCloneField.val(url); - $cloneBtnText.text(activeText); - - return $('.clone').text(url); - }); - // Ref switcher - Project.initRefSwitcher(); - $('.project-refs-select').on('change', function() { - return $(this).parents('form').submit(); - }); - $('.hide-no-ssh-message').on('click', function(e) { - Cookies.set('hide_no_ssh_message', 'false'); - $(this).parents('.no-ssh-key-message').remove(); - return e.preventDefault(); - }); - $('.hide-no-password-message').on('click', function(e) { - Cookies.set('hide_no_password_message', 'false'); - $(this).parents('.no-password-message').remove(); - return e.preventDefault(); - }); - Project.projectSelectDropdown(); - } - - static projectSelectDropdown () { - projectSelect(); - $('.project-item-select').on('click', e => Project.changeProject($(e.currentTarget).val())); - } - - static changeProject(url) { - return window.location = url; - } - - static initRefSwitcher() { - var refListItem = document.createElement('li'); - var refLink = document.createElement('a'); - - refLink.href = '#'; - - return $('.js-project-refs-dropdown').each(function() { - var $dropdown, selected; - $dropdown = $(this); - selected = $dropdown.data('selected'); - return $dropdown.glDropdown({ - data: function(term, callback) { - return $.ajax({ - url: $dropdown.data('refs-url'), - data: { - ref: $dropdown.data('ref'), - search: term, - }, - dataType: 'json', - }).done(function(refs) { - return callback(refs); - }); - }, - selectable: true, - filterable: true, - filterRemote: true, - filterByText: true, - inputFieldName: $dropdown.data('input-field-name'), - fieldName: $dropdown.data('field-name'), - renderRow: function(ref) { - var li = refListItem.cloneNode(false); - - if (ref.header != null) { - li.className = 'dropdown-header'; - li.textContent = ref.header; - } else { - var link = refLink.cloneNode(false); - - if (ref === selected) { - link.className = 'is-active'; - } - - link.textContent = ref; - link.dataset.ref = ref; - - li.appendChild(link); - } - - return li; - }, - id: function(obj, $el) { - return $el.attr('data-ref'); - }, - toggleLabel: function(obj, $el) { - return $el.text().trim(); - }, - clicked: function(options) { - const { e } = options; - e.preventDefault(); - if ($('input[name="ref"]').length) { - var $form = $dropdown.closest('form'); - - var $visit = $dropdown.data('visit'); - var shouldVisit = $visit ? true : $visit; - var action = $form.attr('action'); - var divider = action.indexOf('?') === -1 ? '?' : '&'; - if (shouldVisit) { - visitUrl(`${action}${divider}${$form.serialize()}`); - } - } - }, - }); - }); - } -} diff --git a/app/assets/javascripts/project_avatar.js b/app/assets/javascripts/project_avatar.js deleted file mode 100644 index 56627aa155c..00000000000 --- a/app/assets/javascripts/project_avatar.js +++ /dev/null @@ -1,13 +0,0 @@ -export default function projectAvatar() { - $('.js-choose-project-avatar-button').bind('click', function onClickAvatar() { - const form = $(this).closest('form'); - return form.find('.js-project-avatar-input').click(); - }); - - $('.js-project-avatar-input').bind('change', function onClickAvatarInput() { - const form = $(this).closest('form'); - // eslint-disable-next-line no-useless-escape - const filename = $(this).val().replace(/^.*[\\\/]/, ''); - return form.find('.js-avatar-filename').text(filename); - }); -} diff --git a/app/assets/javascripts/project_new.js b/app/assets/javascripts/project_new.js deleted file mode 100644 index ca548d011b6..00000000000 --- a/app/assets/javascripts/project_new.js +++ /dev/null @@ -1,151 +0,0 @@ -/* eslint-disable func-names, no-var, no-underscore-dangle, prefer-template, prefer-arrow-callback*/ - -import VisibilitySelect from './visibility_select'; - -function highlightChanges($elm) { - $elm.addClass('highlight-changes'); - setTimeout(() => $elm.removeClass('highlight-changes'), 10); -} - -export default class ProjectNew { - constructor() { - this.toggleSettings = this.toggleSettings.bind(this); - this.$selects = $('.features select'); - this.$repoSelects = this.$selects.filter('.js-repo-select'); - this.$projectSelects = this.$selects.not('.js-repo-select'); - - $('.project-edit-container').on('ajax:before', () => { - $('.project-edit-container').hide(); - return $('.save-project-loader').show(); - }); - - this.initVisibilitySelect(); - - this.toggleSettings(); - this.toggleSettingsOnclick(); - this.toggleRepoVisibility(); - } - - initVisibilitySelect() { - const visibilityContainer = document.querySelector('.js-visibility-select'); - if (!visibilityContainer) return; - const visibilitySelect = new VisibilitySelect(visibilityContainer); - visibilitySelect.init(); - - const $visibilitySelect = $(visibilityContainer).find('select'); - let projectVisibility = $visibilitySelect.val(); - const PROJECT_VISIBILITY_PRIVATE = '0'; - - $visibilitySelect.on('change', () => { - const newProjectVisibility = $visibilitySelect.val(); - - if (projectVisibility !== newProjectVisibility) { - this.$projectSelects.each((idx, select) => { - const $select = $(select); - const $options = $select.find('option'); - const values = $.map($options, e => e.value); - - // if switched to "private", limit visibility options - if (newProjectVisibility === PROJECT_VISIBILITY_PRIVATE) { - if ($select.val() !== values[0] && $select.val() !== values[1]) { - $select.val(values[1]).trigger('change'); - highlightChanges($select); - } - $options.slice(2).disable(); - } - - // if switched from "private", increase visibility for non-disabled options - if (projectVisibility === PROJECT_VISIBILITY_PRIVATE) { - $options.enable(); - if ($select.val() !== values[0] && $select.val() !== values[values.length - 1]) { - $select.val(values[values.length - 1]).trigger('change'); - highlightChanges($select); - } - } - }); - - projectVisibility = newProjectVisibility; - } - }); - } - - toggleSettings() { - this.$selects.each(function () { - var $select = $(this); - var className = $select.data('field') - .replace(/_/g, '-') - .replace('access-level', 'feature'); - ProjectNew._showOrHide($select, '.' + className); - }); - } - - toggleSettingsOnclick() { - this.$selects.on('change', this.toggleSettings); - } - - static _showOrHide(checkElement, container) { - const $container = $(container); - - if ($(checkElement).val() !== '0') { - return $container.show(); - } - return $container.hide(); - } - - toggleRepoVisibility() { - var $repoAccessLevel = $('.js-repo-access-level select'); - var $lfsEnabledOption = $('.js-lfs-enabled select'); - var containerRegistry = document.querySelectorAll('.js-container-registry')[0]; - var containerRegistryCheckbox = document.getElementById('project_container_registry_enabled'); - var prevSelectedVal = parseInt($repoAccessLevel.val(), 10); - - this.$repoSelects.find("option[value='" + $repoAccessLevel.val() + "']") - .nextAll() - .hide(); - - $repoAccessLevel - .off('change') - .on('change', function () { - var selectedVal = parseInt($repoAccessLevel.val(), 10); - - this.$repoSelects.each(function () { - var $this = $(this); - var repoSelectVal = parseInt($this.val(), 10); - - $this.find('option').enable(); - - if (selectedVal < repoSelectVal || repoSelectVal === prevSelectedVal) { - $this.val(selectedVal).trigger('change'); - highlightChanges($this); - } - - $this.find("option[value='" + selectedVal + "']").nextAll().disable(); - }); - - if (selectedVal) { - this.$repoSelects.removeClass('disabled'); - - if ($lfsEnabledOption.length) { - $lfsEnabledOption.removeClass('disabled'); - highlightChanges($lfsEnabledOption); - } - if (containerRegistry) { - containerRegistry.style.display = ''; - } - } else { - this.$repoSelects.addClass('disabled'); - - if ($lfsEnabledOption.length) { - $lfsEnabledOption.val('false').addClass('disabled'); - highlightChanges($lfsEnabledOption); - } - if (containerRegistry) { - containerRegistry.style.display = 'none'; - containerRegistryCheckbox.checked = false; - } - } - - prevSelectedVal = selectedVal; - }.bind(this)); - } -} diff --git a/app/assets/javascripts/projects/permissions/components/project_feature_setting.vue b/app/assets/javascripts/projects/permissions/components/project_feature_setting.vue deleted file mode 100644 index 3ebfe82597a..00000000000 --- a/app/assets/javascripts/projects/permissions/components/project_feature_setting.vue +++ /dev/null @@ -1,111 +0,0 @@ - - - diff --git a/app/assets/javascripts/projects/permissions/components/project_setting_row.vue b/app/assets/javascripts/projects/permissions/components/project_setting_row.vue deleted file mode 100644 index 25a88f846eb..00000000000 --- a/app/assets/javascripts/projects/permissions/components/project_setting_row.vue +++ /dev/null @@ -1,51 +0,0 @@ - - - diff --git a/app/assets/javascripts/projects/permissions/components/settings_panel.vue b/app/assets/javascripts/projects/permissions/components/settings_panel.vue deleted file mode 100644 index c96ce12d9fb..00000000000 --- a/app/assets/javascripts/projects/permissions/components/settings_panel.vue +++ /dev/null @@ -1,328 +0,0 @@ - - - diff --git a/app/assets/javascripts/projects/permissions/constants.js b/app/assets/javascripts/projects/permissions/constants.js deleted file mode 100644 index ce47562f259..00000000000 --- a/app/assets/javascripts/projects/permissions/constants.js +++ /dev/null @@ -1,11 +0,0 @@ -export const visibilityOptions = { - PRIVATE: 0, - INTERNAL: 10, - PUBLIC: 20, -}; - -export const visibilityLevelDescriptions = { - [visibilityOptions.PRIVATE]: 'The project is accessible only by members of the project. Access must be granted explicitly to each user.', - [visibilityOptions.INTERNAL]: 'The project can be accessed by any user who is logged in.', - [visibilityOptions.PUBLIC]: 'The project can be accessed by anyone, regardless of authentication.', -}; diff --git a/app/assets/javascripts/projects/permissions/external.js b/app/assets/javascripts/projects/permissions/external.js deleted file mode 100644 index 460af4a2111..00000000000 --- a/app/assets/javascripts/projects/permissions/external.js +++ /dev/null @@ -1,18 +0,0 @@ -const selectorCache = []; - -// workaround since we don't have a polyfill for classList.toggle 2nd parameter -export function toggleHiddenClass(element, hidden) { - if (hidden) { - element.classList.add('hidden'); - } else { - element.classList.remove('hidden'); - } -} - -// hide external feature-specific settings when a given feature is disabled -export function toggleHiddenClassBySelector(selector, hidden) { - if (!selectorCache[selector]) { - selectorCache[selector] = document.querySelectorAll(selector); - } - selectorCache[selector].forEach(elm => toggleHiddenClass(elm, hidden)); -} diff --git a/app/assets/javascripts/projects/permissions/index.js b/app/assets/javascripts/projects/permissions/index.js deleted file mode 100644 index dbde8dda634..00000000000 --- a/app/assets/javascripts/projects/permissions/index.js +++ /dev/null @@ -1,13 +0,0 @@ -import Vue from 'vue'; -import settingsPanel from './components/settings_panel.vue'; - -export default function initProjectPermissionsSettings() { - const mountPoint = document.querySelector('.js-project-permissions-form'); - const componentPropsEl = document.querySelector('.js-project-permissions-form-data'); - const componentProps = JSON.parse(componentPropsEl.innerHTML); - - return new Vue({ - el: mountPoint, - render: createElement => createElement(settingsPanel, { props: { ...componentProps } }), - }); -} diff --git a/app/assets/javascripts/projects/project_new.js b/app/assets/javascripts/projects/project_new.js deleted file mode 100644 index 4710e70d619..00000000000 --- a/app/assets/javascripts/projects/project_new.js +++ /dev/null @@ -1,108 +0,0 @@ -let hasUserDefinedProjectPath = false; - -const deriveProjectPathFromUrl = ($projectImportUrl) => { - const $currentProjectPath = $projectImportUrl.parents('.toggle-import-form').find('#project_path'); - if (hasUserDefinedProjectPath) { - return; - } - - let importUrl = $projectImportUrl.val().trim(); - if (importUrl.length === 0) { - return; - } - - /* - \/?: remove trailing slash - (\.git\/?)?: remove trailing .git (with optional trailing slash) - (\?.*)?: remove query string - (#.*)?: remove fragment identifier - */ - importUrl = importUrl.replace(/\/?(\.git\/?)?(\?.*)?(#.*)?$/, ''); - - // extract everything after the last slash - const pathMatch = /\/([^/]+)$/.exec(importUrl); - if (pathMatch) { - $currentProjectPath.val(pathMatch[1]); - } -}; - -const bindEvents = () => { - const $newProjectForm = $('#new_project'); - const $projectImportUrl = $('#project_import_url'); - const $projectPath = $('#project_path'); - const $useTemplateBtn = $('.template-button > input'); - const $projectFieldsForm = $('.project-fields-form'); - const $selectedTemplateText = $('.selected-template'); - const $changeTemplateBtn = $('.change-template'); - const $selectedIcon = $('.selected-icon svg'); - const $templateProjectNameInput = $('#template-project-name #project_path'); - - if ($newProjectForm.length !== 1) { - return; - } - - $('.how_to_import_link').on('click', (e) => { - e.preventDefault(); - $(e.currentTarget).next('.modal').show(); - }); - - $('.modal-header .close').on('click', () => { - $('.modal').hide(); - }); - - $('.btn_import_gitlab_project').on('click', () => { - const importHref = $('a.btn_import_gitlab_project').attr('href'); - $('.btn_import_gitlab_project').attr('href', `${importHref}?namespace_id=${$('#project_namespace_id').val()}&path=${$projectPath.val()}`); - }); - - function chooseTemplate() { - $('.template-option').hide(); - $projectFieldsForm.addClass('selected'); - $selectedIcon.removeClass('active'); - const value = $(this).val(); - const templates = { - rails: { - text: 'Ruby on Rails', - icon: '.selected-icon .icon-rails', - }, - express: { - text: 'NodeJS Express', - icon: '.selected-icon .icon-node-express', - }, - spring: { - text: 'Spring', - icon: '.selected-icon .icon-java-spring', - }, - }; - - const selectedTemplate = templates[value]; - $selectedTemplateText.text(selectedTemplate.text); - $(selectedTemplate.icon).addClass('active'); - $templateProjectNameInput.focus(); - } - - $useTemplateBtn.on('change', chooseTemplate); - - $changeTemplateBtn.on('click', () => { - $('.template-option').show(); - $projectFieldsForm.removeClass('selected'); - $useTemplateBtn.prop('checked', false); - }); - - $newProjectForm.on('submit', () => { - $projectPath.val($projectPath.val().trim()); - }); - - $projectPath.on('keyup', () => { - hasUserDefinedProjectPath = $projectPath.val().trim().length > 0; - }); - - $projectImportUrl.keyup(() => deriveProjectPathFromUrl($projectImportUrl)); -}; - -document.addEventListener('DOMContentLoaded', bindEvents); - -export default { - bindEvents, - deriveProjectPathFromUrl, -}; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 2075f8e4fec..98d33f9efaa 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -1,4 +1,4 @@ -import Project from '~/project'; +import Project from '~/pages/projects/project'; import SmartInterval from '~/smart_interval'; import Flash from '../flash'; import { diff --git a/app/assets/javascripts/wikis.js b/app/assets/javascripts/wikis.js deleted file mode 100644 index 7a865587444..00000000000 --- a/app/assets/javascripts/wikis.js +++ /dev/null @@ -1,60 +0,0 @@ -import bp from './breakpoints'; -import { slugify } from './lib/utils/text_utility'; - -export default class Wikis { - constructor() { - this.sidebarEl = document.querySelector('.js-wiki-sidebar'); - this.sidebarExpanded = false; - - const sidebarToggles = document.querySelectorAll('.js-sidebar-wiki-toggle'); - for (let i = 0; i < sidebarToggles.length; i += 1) { - sidebarToggles[i].addEventListener('click', e => this.handleToggleSidebar(e)); - } - - this.newWikiForm = document.querySelector('form.new-wiki-page'); - if (this.newWikiForm) { - this.newWikiForm.addEventListener('submit', e => this.handleNewWikiSubmit(e)); - } - - window.addEventListener('resize', () => this.renderSidebar()); - this.renderSidebar(); - } - - handleNewWikiSubmit(e) { - if (!this.newWikiForm) return; - - const slugInput = this.newWikiForm.querySelector('#new_wiki_path'); - const slug = slugify(slugInput.value); - - if (slug.length > 0) { - const wikisPath = slugInput.getAttribute('data-wikis-path'); - window.location.href = `${wikisPath}/${slug}`; - e.preventDefault(); - } - } - - handleToggleSidebar(e) { - e.preventDefault(); - this.sidebarExpanded = !this.sidebarExpanded; - this.renderSidebar(); - } - - static sidebarCanCollapse() { - const bootstrapBreakpoint = bp.getBreakpointSize(); - return bootstrapBreakpoint === 'xs' || bootstrapBreakpoint === 'sm'; - } - - renderSidebar() { - if (!this.sidebarEl) return; - const { classList } = this.sidebarEl; - if (this.sidebarExpanded || !Wikis.sidebarCanCollapse()) { - if (!classList.contains('right-sidebar-expanded')) { - classList.remove('right-sidebar-collapsed'); - classList.add('right-sidebar-expanded'); - } - } else if (classList.contains('right-sidebar-expanded')) { - classList.add('right-sidebar-collapsed'); - classList.remove('right-sidebar-expanded'); - } - } -} diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index 2f56630c22e..61ae0ebbce6 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -4,8 +4,6 @@ - page_title 'New Project' - header_title "Projects", dashboard_projects_path - visibility_level = params.dig(:project, :visibility_level) || default_project_visibility -- content_for :page_specific_javascripts do - = webpack_bundle_tag 'project_new' .project-edit-container .project-edit-errors diff --git a/config/webpack.config.js b/config/webpack.config.js index 95fa79990e2..23784dd2a29 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -66,7 +66,6 @@ var config = { pipelines_times: './pipelines/pipelines_times.js', profile: './profile/profile_bundle.js', project_import_gl: './projects/project_import_gitlab_project.js', - project_new: './projects/project_new.js', prometheus_metrics: './prometheus_metrics', protected_branches: './protected_branches', protected_tags: './protected_tags', diff --git a/spec/javascripts/projects/project_new_spec.js b/spec/javascripts/projects/project_new_spec.js index c314ca8ab72..a95fe83e60e 100644 --- a/spec/javascripts/projects/project_new_spec.js +++ b/spec/javascripts/projects/project_new_spec.js @@ -1,4 +1,4 @@ -import projectNew from '~/projects/project_new'; +import projectNew from '~/pages/projects/shared/project_new'; describe('New Project', () => { let $projectImportUrl; -- cgit v1.2.1 From 4ee29e897d38b1d8b31fce8d197270265a0a6f72 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 12 Jan 2018 13:18:53 +0000 Subject: fixed shortcuts not working on some pages --- app/assets/javascripts/dispatcher.js | 2 +- app/assets/javascripts/shortcuts.js | 6 ++---- app/assets/javascripts/shortcuts_issuable.js | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 5609553a51c..1e8d693093f 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -302,7 +302,7 @@ import Activities from './activities'; window.mergeRequest = new MergeRequest({ action: mrShowNode.dataset.mrAction, }); - + console.log('asd'); shortcut_handler = new ShortcutsIssuable(true); break; case 'dashboard:activity': diff --git a/app/assets/javascripts/shortcuts.js b/app/assets/javascripts/shortcuts.js index d2f0d7410da..35a3f05c980 100644 --- a/app/assets/javascripts/shortcuts.js +++ b/app/assets/javascripts/shortcuts.js @@ -13,12 +13,10 @@ Mousetrap.stopCallback = (e, element, combo) => { }; export default class Shortcuts { - constructor(skipResetBindings) { + constructor() { this.onToggleHelp = this.onToggleHelp.bind(this); this.enabledHelp = []; - if (!skipResetBindings) { - Mousetrap.reset(); - } + Mousetrap.bind('?', this.onToggleHelp); Mousetrap.bind('s', Shortcuts.focusSearch); Mousetrap.bind('f', this.focusFilter.bind(this)); diff --git a/app/assets/javascripts/shortcuts_issuable.js b/app/assets/javascripts/shortcuts_issuable.js index 6aeae84cdc5..689befc742e 100644 --- a/app/assets/javascripts/shortcuts_issuable.js +++ b/app/assets/javascripts/shortcuts_issuable.js @@ -1,10 +1,10 @@ import Mousetrap from 'mousetrap'; import _ from 'underscore'; import Sidebar from './right_sidebar'; -import ShortcutsNavigation from './shortcuts_navigation'; +import Shortcuts from './shortcuts'; import { CopyAsGFM } from './behaviors/copy_as_gfm'; -export default class ShortcutsIssuable extends ShortcutsNavigation { +export default class ShortcutsIssuable extends Shortcuts { constructor(isMergeRequest) { super(); -- cgit v1.2.1 From f818518785a76b52afd81b8949146ce872771277 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 15 Jan 2018 09:17:00 +0000 Subject: removed console.log --- app/assets/javascripts/dispatcher.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 1e8d693093f..aa6eeab9827 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -302,7 +302,6 @@ import Activities from './activities'; window.mergeRequest = new MergeRequest({ action: mrShowNode.dataset.mrAction, }); - console.log('asd'); shortcut_handler = new ShortcutsIssuable(true); break; case 'dashboard:activity': -- cgit v1.2.1 From 8e629a454545a32dd6737cc7ccbd5d2d4bf3fa02 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 16 Jan 2018 16:20:59 +0000 Subject: fixed specs --- app/assets/javascripts/dispatcher.js | 2 +- app/assets/javascripts/projects/project_new.js | 108 +++++++++++++++++++++++++ spec/javascripts/projects/project_new_spec.js | 4 +- 3 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/projects/project_new.js diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index aa6eeab9827..b372d038eac 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -64,7 +64,7 @@ import Activities from './activities'; return false; } - const fail = (e) => { throw e; Flash('Error loading dynamic module'); } + const fail = () => Flash('Error loading dynamic module'); const callDefault = m => m.default(); path = page.split(':'); diff --git a/app/assets/javascripts/projects/project_new.js b/app/assets/javascripts/projects/project_new.js new file mode 100644 index 00000000000..4710e70d619 --- /dev/null +++ b/app/assets/javascripts/projects/project_new.js @@ -0,0 +1,108 @@ +let hasUserDefinedProjectPath = false; + +const deriveProjectPathFromUrl = ($projectImportUrl) => { + const $currentProjectPath = $projectImportUrl.parents('.toggle-import-form').find('#project_path'); + if (hasUserDefinedProjectPath) { + return; + } + + let importUrl = $projectImportUrl.val().trim(); + if (importUrl.length === 0) { + return; + } + + /* + \/?: remove trailing slash + (\.git\/?)?: remove trailing .git (with optional trailing slash) + (\?.*)?: remove query string + (#.*)?: remove fragment identifier + */ + importUrl = importUrl.replace(/\/?(\.git\/?)?(\?.*)?(#.*)?$/, ''); + + // extract everything after the last slash + const pathMatch = /\/([^/]+)$/.exec(importUrl); + if (pathMatch) { + $currentProjectPath.val(pathMatch[1]); + } +}; + +const bindEvents = () => { + const $newProjectForm = $('#new_project'); + const $projectImportUrl = $('#project_import_url'); + const $projectPath = $('#project_path'); + const $useTemplateBtn = $('.template-button > input'); + const $projectFieldsForm = $('.project-fields-form'); + const $selectedTemplateText = $('.selected-template'); + const $changeTemplateBtn = $('.change-template'); + const $selectedIcon = $('.selected-icon svg'); + const $templateProjectNameInput = $('#template-project-name #project_path'); + + if ($newProjectForm.length !== 1) { + return; + } + + $('.how_to_import_link').on('click', (e) => { + e.preventDefault(); + $(e.currentTarget).next('.modal').show(); + }); + + $('.modal-header .close').on('click', () => { + $('.modal').hide(); + }); + + $('.btn_import_gitlab_project').on('click', () => { + const importHref = $('a.btn_import_gitlab_project').attr('href'); + $('.btn_import_gitlab_project').attr('href', `${importHref}?namespace_id=${$('#project_namespace_id').val()}&path=${$projectPath.val()}`); + }); + + function chooseTemplate() { + $('.template-option').hide(); + $projectFieldsForm.addClass('selected'); + $selectedIcon.removeClass('active'); + const value = $(this).val(); + const templates = { + rails: { + text: 'Ruby on Rails', + icon: '.selected-icon .icon-rails', + }, + express: { + text: 'NodeJS Express', + icon: '.selected-icon .icon-node-express', + }, + spring: { + text: 'Spring', + icon: '.selected-icon .icon-java-spring', + }, + }; + + const selectedTemplate = templates[value]; + $selectedTemplateText.text(selectedTemplate.text); + $(selectedTemplate.icon).addClass('active'); + $templateProjectNameInput.focus(); + } + + $useTemplateBtn.on('change', chooseTemplate); + + $changeTemplateBtn.on('click', () => { + $('.template-option').show(); + $projectFieldsForm.removeClass('selected'); + $useTemplateBtn.prop('checked', false); + }); + + $newProjectForm.on('submit', () => { + $projectPath.val($projectPath.val().trim()); + }); + + $projectPath.on('keyup', () => { + hasUserDefinedProjectPath = $projectPath.val().trim().length > 0; + }); + + $projectImportUrl.keyup(() => deriveProjectPathFromUrl($projectImportUrl)); +}; + +document.addEventListener('DOMContentLoaded', bindEvents); + +export default { + bindEvents, + deriveProjectPathFromUrl, +}; diff --git a/spec/javascripts/projects/project_new_spec.js b/spec/javascripts/projects/project_new_spec.js index a95fe83e60e..2dd28f873f6 100644 --- a/spec/javascripts/projects/project_new_spec.js +++ b/spec/javascripts/projects/project_new_spec.js @@ -1,6 +1,6 @@ -import projectNew from '~/pages/projects/shared/project_new'; +import projectNew from '~/projects/project_new'; -describe('New Project', () => { +fdescribe('New Project', () => { let $projectImportUrl; let $projectPath; -- cgit v1.2.1 From e4239eb2a2eb92e738ac4dbbe541e1ba8191603f Mon Sep 17 00:00:00 2001 From: Danny Date: Tue, 16 Jan 2018 18:25:06 +0000 Subject: fix readability xterm colors --- app/assets/stylesheets/pages/xterm.scss | 29 +++++++++++----------- ...adability-of-colored-text-in-job-output-log.yml | 5 ++++ 2 files changed, 19 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/37898-increase-readability-of-colored-text-in-job-output-log.yml diff --git a/app/assets/stylesheets/pages/xterm.scss b/app/assets/stylesheets/pages/xterm.scss index c7297a34ad8..7d40c61da26 100644 --- a/app/assets/stylesheets/pages/xterm.scss +++ b/app/assets/stylesheets/pages/xterm.scss @@ -3,22 +3,21 @@ // see also: https://gist.github.com/jasonm23/2868981 $black: #000; - $red: #cd0000; - $green: #00cd00; - $yellow: #cdcd00; - $blue: #00e; // according to wikipedia, this is the xterm standard - //$blue: #1e90ff; // this is used by all the terminals I tried (when configured with the xterm color profile) - $magenta: #cd00cd; - $cyan: #00cdcd; - $white: #e5e5e5; + $red: #ea1010; + $green: #009900; + $yellow: #999900; + $blue: #0073e6; + $magenta: #d411d4; + $cyan: #009999; + $white: #ccc; $l-black: #373b41; - $l-red: #c66; - $l-green: #b5bd68; - $l-yellow: #f0c674; - $l-blue: #81a2be; - $l-magenta: #b294bb; - $l-cyan: #8abeb7; - $l-white: $gray-darkest; + $l-red: #ff6161; + $l-green: #00d600; + $l-yellow: #bdbd00; + $l-blue: #5797ff; + $l-magenta: #d96dd9; + $l-cyan: #00bdbd; + $l-white: #fff; /* * xterm colors diff --git a/changelogs/unreleased/37898-increase-readability-of-colored-text-in-job-output-log.yml b/changelogs/unreleased/37898-increase-readability-of-colored-text-in-job-output-log.yml new file mode 100644 index 00000000000..813b9ab81fa --- /dev/null +++ b/changelogs/unreleased/37898-increase-readability-of-colored-text-in-job-output-log.yml @@ -0,0 +1,5 @@ +--- +title: increase-readability-of-colored-text-in-job-output-log +merge_request: +author: +type: other -- cgit v1.2.1 From 5139a19e1eb7896dbfd1f1c54a6899ee63eca161 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Tue, 16 Jan 2018 12:31:17 -0700 Subject: Update styling of all disabled buttons --- app/assets/stylesheets/framework/awards.scss | 15 ++++++++------- app/assets/stylesheets/framework/buttons.scss | 17 +++++++++++++++++ app/assets/stylesheets/framework/dropdowns.scss | 5 ----- app/assets/stylesheets/framework/variables.scss | 1 + app/assets/stylesheets/pages/note_form.scss | 8 +++++++- 5 files changed, 33 insertions(+), 13 deletions(-) diff --git a/app/assets/stylesheets/framework/awards.scss b/app/assets/stylesheets/framework/awards.scss index e0d2ed80de5..0f45e499162 100644 --- a/app/assets/stylesheets/framework/awards.scss +++ b/app/assets/stylesheets/framework/awards.scss @@ -173,13 +173,14 @@ } &.user-authored { - cursor: default; - opacity: 0.65; - - &:hover, - &:active { - background-color: $white-light; - border-color: $border-color; + cursor: not-allowed; + background-color: $gray-light; + border-color: $theme-gray-200; + color: $gl-text-color-disabled; + + gl-emoji { + opacity: 0.4; + filter: grayscale(100%); } } diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index fcc420923f9..5cb34a0771d 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -450,3 +450,20 @@ .btn-svg svg { @include btn-svg; } + +// All disabled buttons, regardless of color +.btn.disabled, +.btn[disabled], +fieldset[disabled] .btn, +.dropdown-toggle[disabled], +[disabled].dropdown-menu-toggle { + background-color: $gray-light; + border-color: $theme-gray-200; + color: $gl-text-color-disabled; + opacity: 1; + cursor: not-allowed; + + i { + color: $gl-text-color-disabled; + } +} diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 1d2303a3a2b..4109da4d382 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -63,11 +63,6 @@ border-radius: $border-radius-base; white-space: nowrap; - &[disabled] { - opacity: .65; - cursor: not-allowed; - } - &.no-outline { outline: 0; } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index da18ddf78d3..4ae65e25393 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -164,6 +164,7 @@ $gl-text-color-tertiary: #949494; $gl-text-color-quaternary: #d6d6d6; $gl-text-color-inverted: rgba(255, 255, 255, 1); $gl-text-color-secondary-inverted: rgba(255, 255, 255, .85); +$gl-text-color-disabled: #919191; $gl-text-green: $green-600; $gl-text-green-hover: $green-700; $gl-text-red: $red-500; diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 6d4ccd53e12..bf8eb4c1f06 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -391,11 +391,17 @@ .dropdown-toggle { float: right; - .toggle-icon { + i { color: $white-light; padding-right: 2px; margin-top: 2px; } + + &[disabled] { + i { + color: $gl-text-color-disabled; + } + } } .dropdown-menu { -- cgit v1.2.1 From 70ac55bce189a5d6e860c4921d349267319f5960 Mon Sep 17 00:00:00 2001 From: dosire Date: Mon, 15 Jan 2018 08:23:48 -0800 Subject: Use the fast_blank gem See https://github.com/rails/rails/pull/24658#issuecomment-212651965 --- Gemfile | 3 +++ Gemfile.lock | 2 ++ changelogs/unreleased/16468-add-fast-blank.yml | 5 +++++ 3 files changed, 10 insertions(+) create mode 100644 changelogs/unreleased/16468-add-fast-blank.yml diff --git a/Gemfile b/Gemfile index 9e1e0b3c0c6..5f4911ff628 100644 --- a/Gemfile +++ b/Gemfile @@ -233,6 +233,9 @@ gem 'charlock_holmes', '~> 0.7.5' # Faster JSON gem 'oj', '~> 2.17.4' +# Faster blank +gem 'fast_blank' + # Parse time & duration gem 'chronic', '~> 0.10.2' gem 'chronic_duration', '~> 0.10.6' diff --git a/Gemfile.lock b/Gemfile.lock index 10e2585a0e8..b251b8c481a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -207,6 +207,7 @@ GEM faraday_middleware-multi_json (0.0.6) faraday_middleware multi_json + fast_blank (1.0.0) fast_gettext (1.4.0) ffaker (2.4.0) ffi (1.9.18) @@ -1034,6 +1035,7 @@ DEPENDENCIES email_spec (~> 1.6.0) factory_bot_rails (~> 4.8.2) faraday (~> 0.12) + fast_blank ffaker (~> 2.4) flay (~> 2.8.0) flipper (~> 0.11.0) diff --git a/changelogs/unreleased/16468-add-fast-blank.yml b/changelogs/unreleased/16468-add-fast-blank.yml new file mode 100644 index 00000000000..ef68888ae33 --- /dev/null +++ b/changelogs/unreleased/16468-add-fast-blank.yml @@ -0,0 +1,5 @@ +--- +title: "Add fast-blank" +merge_request: 16468 +author: +type: performance -- cgit v1.2.1 From a13eae6afaaac070da61b72392f1b830fad33546 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 16 Jan 2018 14:52:37 -0600 Subject: Further simplify app/views/projects/buttons/_dropdown.html.haml --- app/views/projects/buttons/_dropdown.html.haml | 35 +++++++++++--------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/app/views/projects/buttons/_dropdown.html.haml b/app/views/projects/buttons/_dropdown.html.haml index a94d9c14722..dab94d10bb1 100644 --- a/app/views/projects/buttons/_dropdown.html.haml +++ b/app/views/projects/buttons/_dropdown.html.haml @@ -9,34 +9,27 @@ - can_create_snippet = can?(current_user, :create_snippet, @project) - if can_create_issue - %li - = link_to _('New issue'), new_project_issue_path(@project) + %li= link_to _('New issue'), new_project_issue_path(@project) + - if merge_project - %li - = link_to _('New merge request'), project_new_merge_request_path(merge_project) + %li= link_to _('New merge request'), project_new_merge_request_path(merge_project) + - if can_create_snippet - %li - = link_to _('New snippet'), new_project_snippet_path(@project) + %li= link_to _('New snippet'), new_project_snippet_path(@project) - if can_create_issue || merge_project || can_create_snippet %li.divider - if can?(current_user, :push_code, @project) - %li - = link_to _('New file'), project_new_blob_path(@project, @project.default_branch || 'master') + %li= link_to _('New file'), project_new_blob_path(@project, @project.default_branch || 'master') - unless @project.empty_repo? - %li - = link_to _('New branch'), new_project_branch_path(@project) - %li - = link_to _('New tag'), new_project_tag_path(@project) + %li= link_to _('New branch'), new_project_branch_path(@project) + %li= link_to _('New tag'), new_project_tag_path(@project) - elsif current_user && current_user.already_forked?(@project) - %li - = link_to _('New file'), project_new_blob_path(@project, @project.default_branch || 'master') + %li= link_to _('New file'), project_new_blob_path(@project, @project.default_branch || 'master') - elsif can?(current_user, :fork_project, @project) - %li - - continue_params = { to: project_new_blob_path(@project, @project.default_branch || 'master'), - notice: edit_in_new_fork_notice, - notice_now: edit_in_new_fork_notice_now } - - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, - continue: continue_params) - = link_to _('New file'), fork_path, method: :post + - continue_params = { to: project_new_blob_path(@project, @project.default_branch || 'master'), + notice: edit_in_new_fork_notice, + notice_now: edit_in_new_fork_notice_now } + - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, continue: continue_params) + %li= link_to _('New file'), fork_path, method: :post -- cgit v1.2.1 From 5750ddf6dc3aa8e053383aa69ac313ffac45067b Mon Sep 17 00:00:00 2001 From: Serdar Dogruyol Date: Wed, 17 Jan 2018 00:01:07 +0300 Subject: Fix dereferenced_target naming in Gitlab::Git::Ref initialize --- lib/gitlab/git/ref.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git/ref.rb b/lib/gitlab/git/ref.rb index 372ce005b94..a3ba9475ad0 100644 --- a/lib/gitlab/git/ref.rb +++ b/lib/gitlab/git/ref.rb @@ -33,9 +33,9 @@ module Gitlab object end - def initialize(repository, name, target, derefenced_target) + def initialize(repository, name, target, dereferenced_target) @name = Gitlab::Git.ref_name(name) - @dereferenced_target = derefenced_target + @dereferenced_target = dereferenced_target @target = if target.respond_to?(:oid) target.oid elsif target.respond_to?(:name) -- cgit v1.2.1 From 435f0a134a1072999bd0f4d52378609cef76ee1d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 16 Jan 2018 15:47:19 -0600 Subject: Fix JS bundle not running on the Cluster update/destroy pages Fix https://gitlab.com/gitlab-org/gitlab-ee/issues/4378 Conflicts: app/assets/javascripts/dispatcher.js --- app/assets/javascripts/dispatcher.js | 2 ++ .../unreleased-ee/4378-fix-cluster-js-not-running-on-update-page.yml | 5 +++++ 2 files changed, 7 insertions(+) create mode 100644 changelogs/unreleased-ee/4378-fix-cluster-js-not-running-on-update-page.yml diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 1c6336073e9..e582a7ba83b 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -559,6 +559,8 @@ import Activities from './activities'; .catch(fail); break; case 'projects:clusters:show': + case 'projects:clusters:update': + case 'projects:clusters:destroy': import('./pages/projects/clusters/show') .then(callDefault) .catch(fail); diff --git a/changelogs/unreleased-ee/4378-fix-cluster-js-not-running-on-update-page.yml b/changelogs/unreleased-ee/4378-fix-cluster-js-not-running-on-update-page.yml new file mode 100644 index 00000000000..bbb6cbd05be --- /dev/null +++ b/changelogs/unreleased-ee/4378-fix-cluster-js-not-running-on-update-page.yml @@ -0,0 +1,5 @@ +--- +title: Fix JavaScript bundle running on Cluster update/destroy pages +merge_request: +author: +type: fixed -- cgit v1.2.1 From cf95fef198e72fad13297e614a96d9a74bd30b77 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Tue, 16 Jan 2018 15:07:42 -0700 Subject: Restore custom height for projects dropdown --- app/assets/stylesheets/framework/dropdowns.scss | 2 +- app/assets/stylesheets/framework/header.scss | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 1d2303a3a2b..4c6b32630e1 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -30,7 +30,7 @@ @include set-visible; min-height: $dropdown-min-height; max-height: $dropdown-max-height; - overflow: auto; + overflow-y: auto; @media (max-width: $screen-xs-max) { width: 100%; diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 3b7256f3000..634593aefd0 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -303,6 +303,8 @@ .projects-dropdown-menu { padding: 0; + overflow-y: initial; + max-height: initial; } .dropdown-chevron { -- cgit v1.2.1 From 0424801ec8854167d17c76b68e6ae8c5b5a6a52a Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 6 Jan 2018 06:18:13 +0000 Subject: Merge branch 'security-10-3-do-not-expose-passwords-or-tokens-in-service-integrations-api' into 'security-10-3' Filter out sensitive fields from the project services API See merge request gitlab/gitlabhq!2281 (cherry picked from commit 476f2576444632f2a9a61b4cead9c1077f2c81d7) 2bcbbda0 Filter out sensitive fields from the project services API --- app/models/service.rb | 5 ++++ changelogs/unreleased/api-no-service-pw-output.yml | 5 ++++ lib/api/entities.rb | 5 +--- lib/api/services.rb | 2 +- lib/api/v3/entities.rb | 5 +--- lib/api/v3/services.rb | 2 +- spec/models/service_spec.rb | 34 ++++++++++++++++++++++ spec/requests/api/services_spec.rb | 4 +-- spec/support/services_shared_context.rb | 8 ++--- 9 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/api-no-service-pw-output.yml diff --git a/app/models/service.rb b/app/models/service.rb index 7f260f7a96b..96a064697f0 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -118,6 +118,11 @@ class Service < ActiveRecord::Base nil end + def api_field_names + fields.map { |field| field[:name] } + .reject { |field_name| field_name =~ /(password|token|key)/ } + end + def global_fields fields end diff --git a/changelogs/unreleased/api-no-service-pw-output.yml b/changelogs/unreleased/api-no-service-pw-output.yml new file mode 100644 index 00000000000..f0d0adaad1c --- /dev/null +++ b/changelogs/unreleased/api-no-service-pw-output.yml @@ -0,0 +1,5 @@ +--- +title: Filter out sensitive fields from the project services API +merge_request: +author: Robert Schilling +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index c4ef2c74658..9618d6625bb 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -714,10 +714,7 @@ module API expose :job_events # Expose serialized properties expose :properties do |service, options| - field_names = service.fields - .select { |field| options[:include_passwords] || field[:type] != 'password' } - .map { |field| field[:name] } - service.properties.slice(*field_names) + service.properties.slice(*service.api_field_names) end end diff --git a/lib/api/services.rb b/lib/api/services.rb index a7f44e2869c..51e33e2c686 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -785,7 +785,7 @@ module API service_params = declared_params(include_missing: false).merge(active: true) if service.update_attributes(service_params) - present service, with: Entities::ProjectService, include_passwords: current_user.admin? + present service, with: Entities::ProjectService else render_api_error!('400 Bad Request', 400) end diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb index 64758dae7d3..2ccbb9da1c5 100644 --- a/lib/api/v3/entities.rb +++ b/lib/api/v3/entities.rb @@ -257,10 +257,7 @@ module API expose :job_events, as: :build_events # Expose serialized properties expose :properties do |service, options| - field_names = service.fields - .select { |field| options[:include_passwords] || field[:type] != 'password' } - .map { |field| field[:name] } - service.properties.slice(*field_names) + service.properties.slice(*service.api_field_names) end end diff --git a/lib/api/v3/services.rb b/lib/api/v3/services.rb index 44ed94d2869..20ca1021c71 100644 --- a/lib/api/v3/services.rb +++ b/lib/api/v3/services.rb @@ -622,7 +622,7 @@ module API end get ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - present service, with: Entities::ProjectService, include_passwords: current_user.admin? + present service, with: Entities::ProjectService end end diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index ab6678cab38..15c1e57c9e4 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -255,6 +255,7 @@ describe Service do end end +<<<<<<< HEAD describe "#deprecated?" do let(:project) { create(:project, :repository) } @@ -278,6 +279,39 @@ describe Service do it 'returns service template' do expect(KubernetesService.find_by_template).to eq(kubernetes_service) +======= + describe '#api_field_names' do + let(:fake_service) do + Class.new(Service) do + def fields + [ + { name: 'token' }, + { name: 'api_token' }, + { name: 'key' }, + { name: 'api_key' }, + { name: 'password' }, + { name: 'password_field' }, + { name: 'safe_field' } + ] + end + end + end + + let(:service) do + fake_service.new(properties: [ + { token: 'token-value' }, + { api_token: 'api_token-value' }, + { key: 'key-value' }, + { api_key: 'api_key-value' }, + { password: 'password-value' }, + { password_field: 'password_field-value' }, + { safe_field: 'safe_field-value' } + ]) + end + + it 'filters out sensitive fields' do + expect(service.api_field_names).to eq(['safe_field']) +>>>>>>> Merge branch 'security-10-3-do-not-expose-passwords-or-tokens-in-service-integrations-api' into 'security-10-3' end end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 26d56c04862..236f8d7faf5 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -83,14 +83,14 @@ describe API::Services do get api("/projects/#{project.id}/services/#{dashed_service}", admin) expect(response).to have_gitlab_http_status(200) - expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list.map) + expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end it "returns properties of service #{service} other than passwords when authenticated as project owner" do get api("/projects/#{project.id}/services/#{dashed_service}", user) expect(response).to have_gitlab_http_status(200) - expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list_without_passwords) + expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end it "returns error when authenticated but not a project owner" do diff --git a/spec/support/services_shared_context.rb b/spec/support/services_shared_context.rb index 3f1fd169b72..23f9b46ae0c 100644 --- a/spec/support/services_shared_context.rb +++ b/spec/support/services_shared_context.rb @@ -3,13 +3,9 @@ Service.available_services_names.each do |service| let(:dashed_service) { service.dasherize } let(:service_method) { "#{service}_service".to_sym } let(:service_klass) { "#{service}_service".classify.constantize } - let(:service_fields) { service_klass.new.fields } + let(:service_instance) { service_klass.new } + let(:service_fields) { service_instance.fields } let(:service_attrs_list) { service_fields.inject([]) {|arr, hash| arr << hash[:name].to_sym } } - let(:service_attrs_list_without_passwords) do - service_fields - .select { |field| field[:type] != 'password' } - .map { |field| field[:name].to_sym} - end let(:service_attrs) do service_attrs_list.inject({}) do |hash, k| if k =~ /^(token*|.*_token|.*_key)/ -- cgit v1.2.1 From 72a57525a87b694799cd6406e8e8f117a902a890 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 21 Dec 2017 18:34:34 +0000 Subject: Merge branch 'ac/41346-xss-ci-job-output' into 'security-10-3' [10.3] Fix XSS vulnerability in Pipeline job trace See merge request gitlab/gitlabhq!2258 (cherry picked from commit 44caa80ed9a2514a74a5eeab10ff51849d64851b) 5f86f3ff Fix XSS vulnerability in Pipeline job trace --- lib/gitlab/regex.rb | 2 +- spec/lib/gitlab/ci/ansi2html_spec.rb | 55 +++++++++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 0002c7da8f1..7ab85e1c35c 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -67,7 +67,7 @@ module Gitlab end def build_trace_section_regex - @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([^\r]+)\r\033\[0K/.freeze + @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([a-zA-Z0-9_.-]+)\r\033\[0K/.freeze end end end diff --git a/spec/lib/gitlab/ci/ansi2html_spec.rb b/spec/lib/gitlab/ci/ansi2html_spec.rb index 05e2d94cbd6..7549e9941b6 100644 --- a/spec/lib/gitlab/ci/ansi2html_spec.rb +++ b/spec/lib/gitlab/ci/ansi2html_spec.rb @@ -217,11 +217,58 @@ describe Gitlab::Ci::Ansi2html do "#{section_end[0...-5]}" end - it "prints light red" do - text = "#{section_start}\e[91mHello\e[0m\n#{section_end}" - html = %{#{section_start_html}Hello
#{section_end_html}} + shared_examples 'forbidden char in section_name' do + it 'ignores sections' do + text = "#{section_start}Some text#{section_end}" + html = text.gsub("\033[0K", '').gsub('<', '<') - expect(convert_html(text)).to eq(html) + expect(convert_html(text)).to eq(html) + end + end + + shared_examples 'a legit section' do + let(:text) { "#{section_start}Some text#{section_end}" } + + it 'prints light red' do + text = "#{section_start}\e[91mHello\e[0m\n#{section_end}" + html = %{#{section_start_html}Hello
#{section_end_html}} + + expect(convert_html(text)).to eq(html) + end + + it 'begins with a section_start html marker' do + expect(convert_html(text)).to start_with(section_start_html) + end + + it 'ends with a section_end html marker' do + expect(convert_html(text)).to end_with(section_end_html) + end + end + + it_behaves_like 'a legit section' + + context 'section name includes $' do + let(:section_name) { 'my_$ection'} + + it_behaves_like 'forbidden char in section_name' + end + + context 'section name includes <' do + let(:section_name) { ''} + + it_behaves_like 'forbidden char in section_name' + end + + context 'section name contains .-_' do + let(:section_name) { 'a.Legit-SeCtIoN_namE' } + + it_behaves_like 'a legit section' + end + + it 'do not allow XSS injections' do + text = "#{section_start}section_end:1:2#{section_end}" + + expect(convert_html(text)).not_to include('' + end + end end context 'editing issue labels', :js do -- cgit v1.2.1 From 8f4b06137577f868ffaa41d10c27aa1e763bc825 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 5 Jan 2018 17:53:31 +0000 Subject: Merge branch 'milestones-finder-order-fix' into 'security-10-3' Remove order param from the MilestoneFinder See merge request gitlab/gitlabhq!2259 (cherry picked from commit 14408042e78f2ebc2644f956621b461dbfa3d36d) 155881e7 Remove order param from the MilestoneFinder --- app/controllers/groups/milestones_controller.rb | 6 ++-- app/controllers/projects/milestones_controller.rb | 14 ++++---- app/finders/milestones_finder.rb | 8 ++--- app/models/global_milestone.rb | 8 ++--- .../unreleased/milestones-finder-order-fix.yml | 5 +++ spec/finders/milestones_finder_spec.rb | 41 +++++++--------------- .../services/projects/autocomplete_service_spec.rb | 15 ++++---- 7 files changed, 43 insertions(+), 54 deletions(-) create mode 100644 changelogs/unreleased/milestones-finder-order-fix.yml diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index f013d21275e..acf6aaf57f4 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController end def milestones - search_params = params.merge(group_ids: group.id) - milestones = MilestonesFinder.new(search_params).execute legacy_milestones = GroupMilestone.build_collection(group, group_projects, params) @@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController render_404 unless @milestone end + + def search_params + params.permit(:state).merge(group_ids: group.id) + end end diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 980bbf699b6..0f70efbce40 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController def milestones @milestones ||= begin - if @project.group && can?(current_user, :read_group, @project.group) - group = @project.group - end - - search_params = params.merge(project_ids: @project.id, group_ids: group&.id) - MilestonesFinder.new(search_params).execute end end @@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController def milestone_params params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event) end + + def search_params + if @project.group && can?(current_user, :read_group, @project.group) + group = @project.group + end + + params.permit(:state).merge(project_ids: @project.id, group_ids: group&.id) + end end diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb index 0a5a0ea2f35..b4605fca193 100644 --- a/app/finders/milestones_finder.rb +++ b/app/finders/milestones_finder.rb @@ -46,11 +46,7 @@ class MilestonesFinder end def order(items) - if params.has_key?(:order) - items.reorder(params[:order]) - else - order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC') - items.reorder(order_statement) - end + order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC') + items.reorder(order_statement).order('title ASC') end end diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb index c0864769314..dc2f6817190 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -44,10 +44,10 @@ class GlobalMilestone def self.group_milestones_states_count(group) return STATE_COUNT_HASH unless group - params = { group_ids: [group.id], state: 'all', order: nil } + params = { group_ids: [group.id], state: 'all' } relation = MilestonesFinder.new(params).execute - grouped_by_state = relation.group(:state).count + grouped_by_state = relation.reorder(nil).group(:state).count { opened: grouped_by_state['active'] || 0, @@ -60,10 +60,10 @@ class GlobalMilestone def self.legacy_group_milestone_states_count(projects) return STATE_COUNT_HASH unless projects - params = { project_ids: projects.map(&:id), state: 'all', order: nil } + params = { project_ids: projects.map(&:id), state: 'all' } relation = MilestonesFinder.new(params).execute - project_milestones_by_state_and_title = relation.group(:state, :title).count + project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count opened = count_by_state(project_milestones_by_state_and_title, 'active') closed = count_by_state(project_milestones_by_state_and_title, 'closed') diff --git a/changelogs/unreleased/milestones-finder-order-fix.yml b/changelogs/unreleased/milestones-finder-order-fix.yml new file mode 100644 index 00000000000..983c301a82d --- /dev/null +++ b/changelogs/unreleased/milestones-finder-order-fix.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a SQL injection in the MilestonesFinder +merge_request: +author: +type: security diff --git a/spec/finders/milestones_finder_spec.rb b/spec/finders/milestones_finder_spec.rb index 8ae08656e01..0b3cf7ece5f 100644 --- a/spec/finders/milestones_finder_spec.rb +++ b/spec/finders/milestones_finder_spec.rb @@ -21,10 +21,19 @@ describe MilestonesFinder do expect(result).to contain_exactly(milestone_1, milestone_2) end - it 'returns milestones for groups and projects' do - result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute + context 'milestones for groups and project' do + let(:result) do + described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute + end + + it 'returns milestones for groups and projects' do + expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) + end - expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) + it 'orders milestones by due date' do + expect(result.first).to eq(milestone_1) + expect(result.second).to eq(milestone_3) + end end context 'with filters' do @@ -61,30 +70,4 @@ describe MilestonesFinder do expect(result.to_a).to contain_exactly(milestone_1) end end - - context 'with order' do - let(:params) do - { - project_ids: [project_1.id, project_2.id], - group_ids: group.id, - state: 'all' - } - end - - it "default orders by due date" do - result = described_class.new(params).execute - - expect(result.first).to eq(milestone_1) - expect(result.second).to eq(milestone_3) - end - - it "orders by parameter" do - result = described_class.new(params.merge(order: 'id DESC')).execute - - expect(result.first).to eq(milestone_4) - expect(result.second).to eq(milestone_3) - expect(result.third).to eq(milestone_2) - expect(result.fourth).to eq(milestone_1) - end - end end diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 7a8c54673f7..f7ff8b80bd7 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -93,26 +93,27 @@ describe Projects::AutocompleteService do let(:user) { create(:user) } let(:group) { create(:group) } let(:project) { create(:project, group: group) } - let!(:group_milestone) { create(:milestone, group: group) } - let!(:project_milestone) { create(:milestone, project: project) } + let!(:group_milestone1) { create(:milestone, group: group, due_date: '2017-01-01', title: 'Second Title') } + let!(:group_milestone2) { create(:milestone, group: group, due_date: '2017-01-01', title: 'First Title') } + let!(:project_milestone) { create(:milestone, project: project, due_date: '2016-01-01') } let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) } - it 'includes project and group milestones' do - expect(milestone_titles).to eq([group_milestone.title, project_milestone.title]) + it 'includes project and group milestones and sorts them correctly' do + expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title, group_milestone1.title]) end it 'does not include closed milestones' do - group_milestone.close + group_milestone1.close - expect(milestone_titles).to eq([project_milestone.title]) + expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title]) end it 'does not include milestones from other projects in the group' do other_project = create(:project, group: group) project_milestone.update!(project: other_project) - expect(milestone_titles).to eq([group_milestone.title]) + expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title]) end end end -- cgit v1.2.1 From 1f96512ba189d1eceb01353ca41c1cb6216d32c1 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 4 Jan 2018 05:42:52 +0000 Subject: Merge branch 'sh-validate-path-project-import-10-3' into 'security-10-3' Validate project path in Gitlab import - 10.3 port See merge request gitlab/gitlabhq!2268 (cherry picked from commit 94c82376d66fc80d46dd2d5eeb5bade408ec6a7e) 2b94a7c2 Validate project path in Gitlab import --- .../projects/gitlab_projects_import_service.rb | 2 +- .../import/gitlab_projects_controller_spec.rb | 38 ++++++++++++++++++++++ .../projects/import_export/import_file_spec.rb | 2 +- .../gitlab_projects_import_service_spec.rb | 31 ++++++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 spec/controllers/import/gitlab_projects_controller_spec.rb create mode 100644 spec/services/projects/gitlab_projects_import_service_spec.rb diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index 4ca6414b73b..a3d7f5cbed5 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -26,7 +26,7 @@ module Projects end def tmp_filename - "#{SecureRandom.hex}_#{params[:path]}" + SecureRandom.hex end def file diff --git a/spec/controllers/import/gitlab_projects_controller_spec.rb b/spec/controllers/import/gitlab_projects_controller_spec.rb new file mode 100644 index 00000000000..8759d3c0b97 --- /dev/null +++ b/spec/controllers/import/gitlab_projects_controller_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe Import::GitlabProjectsController do + set(:namespace) { create(:namespace) } + set(:user) { namespace.owner } + let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } + + before do + sign_in(user) + end + + describe 'POST create' do + context 'with an invalid path' do + it 'redirects with an error' do + post :create, namespace_id: namespace.id, path: '/test', file: file + + expect(flash[:alert]).to start_with('Project could not be imported') + expect(response).to have_gitlab_http_status(302) + end + + it 'redirects with an error when a relative path is used' do + post :create, namespace_id: namespace.id, path: '../test', file: file + + expect(flash[:alert]).to start_with('Project could not be imported') + expect(response).to have_gitlab_http_status(302) + end + end + + context 'with a valid path' do + it 'redirects to the new project path' do + post :create, namespace_id: namespace.id, path: 'test', file: file + + expect(flash[:notice]).to include('is being imported') + expect(response).to have_gitlab_http_status(302) + end + end + end +end diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index af125e1b9d3..e8bb9c6a86c 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -32,7 +32,7 @@ feature 'Import/Export - project import integration test', :js do expect(page).to have_content('Import an exported GitLab project') expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") - expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path\h*\z/).and_call_original + expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}\z/).and_call_original attach_file('file', file) click_on 'Import project' diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb new file mode 100644 index 00000000000..bb0e274c93e --- /dev/null +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Projects::GitlabProjectsImportService do + set(:namespace) { build(:namespace) } + let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } + subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) } + + describe '#execute' do + context 'with an invalid path' do + let(:path) { '/invalid-path/' } + + it 'returns an invalid project' do + project = subject.execute + + expect(project).not_to be_persisted + expect(project).not_to be_valid + end + end + + context 'with a valid path' do + let(:path) { 'test-path' } + + it 'creates a project' do + project = subject.execute + + expect(project).to be_persisted + expect(project).to be_valid + end + end + end +end -- cgit v1.2.1 From 954a44574fd7a0be232a194d503032e16b8f3094 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 3 Jan 2018 18:00:36 +0000 Subject: Merge branch 'ac/fix-path-traversal' into 'security-10-3' [10.3] Fix path traversal in gitlab-ci.yml cache:key See merge request gitlab/gitlabhq!2270 (cherry picked from commit c32d0c6807dfd41d7838a35742e6d0986871b389) df29094a Fix path traversal in gitlab-ci.yml cache:key --- changelogs/unreleased/security-10-3.yml | 5 +++ doc/ci/yaml/README.md | 2 +- lib/gitlab/ci/config/entry/validators.rb | 16 +++++++- spec/lib/gitlab/ci/config/entry/key_spec.rb | 62 +++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-10-3.yml diff --git a/changelogs/unreleased/security-10-3.yml b/changelogs/unreleased/security-10-3.yml new file mode 100644 index 00000000000..5c718d976cd --- /dev/null +++ b/changelogs/unreleased/security-10-3.yml @@ -0,0 +1,5 @@ +--- +title: Fix path traversal in gitlab-ci.yml cache:key +merge_request: +author: +type: security diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index ae0b5c0a2ba..82052cc0376 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -258,7 +258,7 @@ The `cache:key` variable can use any of the [predefined variables](../variables/ The default key is **default** across the project, therefore everything is shared between each pipelines and jobs by default, starting from GitLab 9.0. ->**Note:** The `cache:key` variable cannot contain the `/` character. +>**Note:** The `cache:key` variable cannot contain the `/` character, or the equivalent URI encoded `%2F`; a value made only of dots (`.`, `%2E`) is also forbidden. --- diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb index eb606b57667..55658900628 100644 --- a/lib/gitlab/ci/config/entry/validators.rb +++ b/lib/gitlab/ci/config/entry/validators.rb @@ -64,10 +64,24 @@ module Gitlab include LegacyValidationHelpers def validate_each(record, attribute, value) - unless validate_string(value) + if validate_string(value) + validate_path(record, attribute, value) + else record.errors.add(attribute, 'should be a string or symbol') end end + + private + + def validate_path(record, attribute, value) + path = CGI.unescape(value.to_s) + + if path.include?('/') + record.errors.add(attribute, 'cannot contain the "/" character') + elsif path == '.' || path == '..' + record.errors.add(attribute, 'cannot be "." or ".."') + end + end end class RegexpValidator < ActiveModel::EachValidator diff --git a/spec/lib/gitlab/ci/config/entry/key_spec.rb b/spec/lib/gitlab/ci/config/entry/key_spec.rb index 5d4de60bc8a..846f5f44470 100644 --- a/spec/lib/gitlab/ci/config/entry/key_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/key_spec.rb @@ -4,6 +4,26 @@ describe Gitlab::Ci::Config::Entry::Key do let(:entry) { described_class.new(config) } describe 'validations' do + shared_examples 'key with slash' do + it 'is invalid' do + expect(entry).not_to be_valid + end + + it 'reports errors with config value' do + expect(entry.errors).to include 'key config cannot contain the "/" character' + end + end + + shared_examples 'key with only dots' do + it 'is invalid' do + expect(entry).not_to be_valid + end + + it 'reports errors with config value' do + expect(entry.errors).to include 'key config cannot be "." or ".."' + end + end + context 'when entry config value is correct' do let(:config) { 'test' } @@ -30,6 +50,48 @@ describe Gitlab::Ci::Config::Entry::Key do end end end + + context 'when entry value contains slash' do + let(:config) { 'key/with/some/slashes' } + + it_behaves_like 'key with slash' + end + + context 'when entry value contains URI encoded slash (%2F)' do + let(:config) { 'key%2Fwith%2Fsome%2Fslashes' } + + it_behaves_like 'key with slash' + end + + context 'when entry value is a dot' do + let(:config) { '.' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is two dots' do + let(:config) { '..' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is a URI encoded dot (%2E)' do + let(:config) { '%2e' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is two URI encoded dots (%2E)' do + let(:config) { '%2E%2e' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is one dot and one URI encoded dot' do + let(:config) { '.%2e' } + + it_behaves_like 'key with only dots' + end end describe '.default' do -- cgit v1.2.1 From 3fc0564ae09a9edf87a71a8c85ff9bf8ad35121d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 5 Jan 2018 17:55:37 +0000 Subject: Merge branch '41567-projectfix' into 'security-10-3' check project access on MR create See merge request gitlab/gitlabhq!2273 (cherry picked from commit 1fe2325d6ef2bced4c5e97b57691c894f38b2834) 43e85f49 check project access on MR create --- app/services/merge_requests/create_service.rb | 28 +++++++--- changelogs/unreleased/projectfix.yml | 6 +++ spec/features/cycle_analytics_spec.rb | 1 + .../microsoft_teams_service_spec.rb | 4 ++ spec/requests/api/merge_requests_spec.rb | 26 ++++++--- spec/requests/api/v3/merge_requests_spec.rb | 26 ++++++--- .../services/merge_requests/create_service_spec.rb | 61 ++++++++++++++++++++++ ...ack_mattermost_notifications_shared_examples.rb | 1 + 8 files changed, 133 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/projectfix.yml diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 49cf534dc0d..634bf3bd690 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -1,15 +1,11 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute - # @project is used to determine whether the user can set the merge request's - # assignee, milestone and labels. Whether they can depends on their - # permissions on the target project. - source_project = @project - @project = Project.find(params[:target_project_id]) if params[:target_project_id] + set_projects! merge_request = MergeRequest.new merge_request.target_project = @project - merge_request.source_project = source_project + merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) @@ -58,5 +54,25 @@ module MergeRequests pipelines.order(id: :desc).first end + + def set_projects! + # @project is used to determine whether the user can set the merge request's + # assignee, milestone and labels. Whether they can depends on their + # permissions on the target project. + @source_project = @project + @project = Project.find(params[:target_project_id]) if params[:target_project_id] + + # make sure that source/target project ids are not in + # params so it can't be overridden later when updating attributes + # from params when applying quick actions + params.delete(:source_project_id) + params.delete(:target_project_id) + + unless can?(current_user, :read_project, @source_project) && + can?(current_user, :read_project, @project) + + raise Gitlab::Access::AccessDeniedError + end + end end end diff --git a/changelogs/unreleased/projectfix.yml b/changelogs/unreleased/projectfix.yml new file mode 100644 index 00000000000..4d8ebe6194a --- /dev/null +++ b/changelogs/unreleased/projectfix.yml @@ -0,0 +1,6 @@ +--- +title: Check user authorization for source and target projects when creating a merge + request. +merge_request: +author: +type: security diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index d36954954b6..510677ecf56 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do context "as a guest" do before do + project.add_developer(user) project.add_guest(guest) allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) diff --git a/spec/models/project_services/microsoft_teams_service_spec.rb b/spec/models/project_services/microsoft_teams_service_spec.rb index 6a5d0decfec..733086e258f 100644 --- a/spec/models/project_services/microsoft_teams_service_spec.rb +++ b/spec/models/project_services/microsoft_teams_service_spec.rb @@ -92,6 +92,10 @@ describe MicrosoftTeamsService do service.hook_data(merge_request, 'open') end + before do + project.add_developer(user) + end + it "calls Microsoft Teams API" do chat_service.execute(merge_sample_data) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4eae3e50602..8e2982f1a5d 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -754,16 +754,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index b8b7d9d1c40..6b748369f0d 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -371,16 +371,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post v3_api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index dd8c803a2f7..5d226f34d2d 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -263,5 +263,66 @@ describe MergeRequests::CreateService do expect(issue_ids).to match_array([first_issue.id, second_issue.id]) end end + + context 'when source and target projects are different' do + let(:target_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + target_project_id: target_project.id + } + end + + context 'when user can not access source project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when user can not access target project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + end + + context 'when user sets source project id' do + let(:another_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + source_project_id: another_project.id + } + end + + before do + project.add_developer(assignee) + project.add_master(user) + end + + it 'ignores source_project_id' do + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.source_project_id).to eq(project.id) + end + end end end diff --git a/spec/support/slack_mattermost_notifications_shared_examples.rb b/spec/support/slack_mattermost_notifications_shared_examples.rb index 17f3a861ba8..e827a8da0b7 100644 --- a/spec/support/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/slack_mattermost_notifications_shared_examples.rb @@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do @issue = issue_service.execute @issues_sample_data = issue_service.hook_data(@issue, 'open') + project.add_developer(user) opts = { title: 'Awesome merge_request', description: 'please fix', -- cgit v1.2.1 From 536a47b4b70df0f2a8438ed0ada7654593fa5cd0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 5 Jan 2018 15:23:44 +0000 Subject: Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3' [10.3] Migrate `can_push` column from `keys` to `deploy_keys_project` See merge request gitlab/gitlabhq!2276 (cherry picked from commit f6ca52d31bac350a23938e0aebf717c767b4710c) 1f2bd3c0 Backport to 10.3 --- .../javascripts/deploy_keys/components/key.vue | 38 ++++++++++--- app/controllers/admin/deploy_keys_controller.rb | 4 +- app/controllers/projects/deploy_keys_controller.rb | 9 ++- app/models/deploy_key.rb | 20 ++++++- app/models/deploy_keys_project.rb | 10 +++- .../projects/settings/deploy_keys_presenter.rb | 2 +- app/serializers/deploy_key_entity.rb | 9 +-- app/serializers/deploy_keys_project_entity.rb | 4 ++ app/views/admin/deploy_keys/index.html.haml | 8 +-- app/views/projects/deploy_keys/_form.html.haml | 18 +++--- app/views/shared/deploy_keys/_form.html.haml | 19 ++++--- ...sh-migrate-can-push-to-deploy-keys-projects.yml | 5 ++ ...1145425_add_can_push_to_deploy_keys_projects.rb | 15 +++++ ..._populate_can_push_from_deploy_keys_projects.rb | 44 +++++++++++++++ ..._populate_can_push_from_deploy_keys_projects.rb | 43 +++++++++++++++ .../20171215121259_remove_can_push_from_keys.rb | 17 ++++++ db/schema.rb | 6 +- doc/api/deploy_keys.md | 48 ++++++++++++---- lib/api/deploy_keys.rb | 64 +++++++++++++++------- lib/api/entities.rb | 7 ++- lib/api/v3/deploy_keys.rb | 46 +++++++++++----- spec/factories/deploy_keys_projects.rb | 4 ++ spec/factories/keys.rb | 4 -- spec/features/admin/admin_deploy_keys_spec.rb | 14 +++-- .../projects/settings/repository_settings_spec.rb | 6 +- .../javascripts/deploy_keys/components/key_spec.js | 20 ++++--- spec/lib/gitlab/auth_spec.rb | 6 +- spec/lib/gitlab/git_access_spec.rb | 14 ++--- spec/models/deploy_keys_project_spec.rb | 2 +- spec/requests/api/deploy_keys_spec.rb | 12 +--- spec/requests/api/v3/deploy_keys_spec.rb | 2 +- spec/requests/lfs_http_spec.rb | 4 +- spec/serializers/deploy_key_entity_spec.rb | 15 +++-- 33 files changed, 400 insertions(+), 139 deletions(-) create mode 100644 app/serializers/deploy_keys_project_entity.rb create mode 100644 changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml create mode 100644 db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb create mode 100644 db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb create mode 100644 db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb create mode 100644 db/post_migrate/20171215121259_remove_can_push_from_keys.rb diff --git a/app/assets/javascripts/deploy_keys/components/key.vue b/app/assets/javascripts/deploy_keys/components/key.vue index a9e819b8a3c..8211ba280c5 100644 --- a/app/assets/javascripts/deploy_keys/components/key.vue +++ b/app/assets/javascripts/deploy_keys/components/key.vue @@ -1,6 +1,10 @@ @@ -52,21 +68,29 @@
{{ deployKey.fingerprint }}
-
- Write access allowed -
>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3' > - {{ project.full_name }} + {{ deployKeysProject.project.full_name }} +
diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb index a7ab481519d..b0c4c31cffc 100644 --- a/app/controllers/admin/deploy_keys_controller.rb +++ b/app/controllers/admin/deploy_keys_controller.rb @@ -50,10 +50,10 @@ class Admin::DeployKeysController < Admin::ApplicationController end def create_params - params.require(:deploy_key).permit(:key, :title, :can_push) + params.require(:deploy_key).permit(:key, :title) end def update_params - params.require(:deploy_key).permit(:title, :can_push) + params.require(:deploy_key).permit(:title) end end diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index e06dda1baa4..f43ef2e5f2f 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -24,7 +24,7 @@ class Projects::DeployKeysController < Projects::ApplicationController def create @key = DeployKeys::CreateService.new(current_user, create_params).execute - unless @key.valid? && @project.deploy_keys << @key + unless @key.valid? flash[:alert] = @key.errors.full_messages.join(', ').html_safe end @@ -71,11 +71,14 @@ class Projects::DeployKeysController < Projects::ApplicationController end def create_params - params.require(:deploy_key).permit(:key, :title, :can_push) + create_params = params.require(:deploy_key) + .permit(:key, :title, deploy_keys_projects_attributes: [:can_push]) + create_params.dig(:deploy_keys_projects_attributes, '0')&.merge!(project_id: @project.id) + create_params end def update_params - params.require(:deploy_key).permit(:title, :can_push) + params.require(:deploy_key).permit(:title, deploy_keys_projects_attributes: [:id, :can_push]) end def authorize_update_deploy_key! diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index eae5eee4fee..c2e0a5fa126 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -1,10 +1,16 @@ class DeployKey < Key - has_many :deploy_keys_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + include IgnorableColumn + + has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :deploy_keys_projects scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) } scope :are_public, -> { where(public: true) } + ignore_column :can_push + + accepts_nested_attributes_for :deploy_keys_projects + def private? !public? end @@ -22,10 +28,18 @@ class DeployKey < Key end def has_access_to?(project) - projects.include?(project) + deploy_keys_project_for(project).present? end def can_push_to?(project) - can_push? && has_access_to?(project) + !!deploy_keys_project_for(project)&.can_push? + end + + def deploy_keys_project_for(project) + deploy_keys_projects.find_by(project: project) + end + + def projects_with_write_access + Project.preload(:route).where(id: deploy_keys_projects.with_write_access.select(:project_id)) end end diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index b37b9bfbdac..6eef12c4373 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -1,8 +1,14 @@ class DeployKeysProject < ActiveRecord::Base belongs_to :project - belongs_to :deploy_key + belongs_to :deploy_key, inverse_of: :deploy_keys_projects - validates :deploy_key_id, presence: true + scope :without_project_deleted, -> { joins(:project).where(projects: { pending_delete: false }) } + scope :in_project, ->(project) { where(project: project) } + scope :with_write_access, -> { where(can_push: true) } + + accepts_nested_attributes_for :deploy_key + + validates :deploy_key, presence: true validates :deploy_key_id, uniqueness: { scope: [:project_id], message: "already exists in project" } validates :project_id, presence: true diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 229311eb6ee..c226586fba5 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/app/presenters/projects/settings/deploy_keys_presenter.rb @@ -7,7 +7,7 @@ module Projects delegate :size, to: :available_public_keys, prefix: true def new_key - @key ||= DeployKey.new + @key ||= DeployKey.new.tap { |dk| dk.deploy_keys_projects.build } end def enabled_keys diff --git a/app/serializers/deploy_key_entity.rb b/app/serializers/deploy_key_entity.rb index c75431a79ae..2678f99510c 100644 --- a/app/serializers/deploy_key_entity.rb +++ b/app/serializers/deploy_key_entity.rb @@ -3,19 +3,20 @@ class DeployKeyEntity < Grape::Entity expose :user_id expose :title expose :fingerprint - expose :can_push expose :destroyed_when_orphaned?, as: :destroyed_when_orphaned expose :almost_orphaned?, as: :almost_orphaned expose :created_at expose :updated_at - expose :projects, using: ProjectEntity do |deploy_key| - deploy_key.projects.without_deleted.select { |project| options[:user].can?(:read_project, project) } + expose :deploy_keys_projects, using: DeployKeysProjectEntity do |deploy_key| + deploy_key.deploy_keys_projects + .without_project_deleted + .select { |deploy_key_project| Ability.allowed?(options[:user], :read_project, deploy_key_project.project) } end expose :can_edit private def can_edit - options[:user].can?(:update_deploy_key, object) + Ability.allowed?(options[:user], :update_deploy_key, object) end end diff --git a/app/serializers/deploy_keys_project_entity.rb b/app/serializers/deploy_keys_project_entity.rb new file mode 100644 index 00000000000..568ef5ab75e --- /dev/null +++ b/app/serializers/deploy_keys_project_entity.rb @@ -0,0 +1,4 @@ +class DeployKeysProjectEntity < Grape::Entity + expose :can_push + expose :project, using: ProjectEntity +end diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml index 92370034baa..1420163fd5a 100644 --- a/app/views/admin/deploy_keys/index.html.haml +++ b/app/views/admin/deploy_keys/index.html.haml @@ -12,7 +12,7 @@ %tr %th.col-sm-2 Title %th.col-sm-4 Fingerprint - %th.col-sm-2 Write access allowed + %th.col-sm-2 Projects with write access %th.col-sm-2 Added at %th.col-sm-2 %tbody @@ -23,10 +23,8 @@ %td %code.key-fingerprint= deploy_key.fingerprint %td - - if deploy_key.can_push? - Yes - - else - No + - deploy_key.projects_with_write_access.each do |project| + = link_to project.full_name, admin_project_path(project), class: 'label deploy-project-label' %td %span.cgray added #{time_ago_with_tooltip(deploy_key.created_at)} diff --git a/app/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml index edaa3a1119e..c363180d0db 100644 --- a/app/views/projects/deploy_keys/_form.html.haml +++ b/app/views/projects/deploy_keys/_form.html.haml @@ -10,13 +10,15 @@ %p.light.append-bottom-0 Paste a machine public key here. Read more about how to generate it = link_to "here", help_page_path("ssh/README") - .form-group - .checkbox - = f.label :can_push do - = f.check_box :can_push - %strong Write access allowed - .form-group - %p.light.append-bottom-0 - Allow this key to push to repository as well? (Default only allows pull access.) + + = f.fields_for :deploy_keys_projects do |deploy_keys_project_form| + .form-group + .checkbox + = deploy_keys_project_form.label :can_push do + = deploy_keys_project_form.check_box :can_push + %strong Write access allowed + .form-group + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) = f.submit "Add key", class: "btn-create btn" diff --git a/app/views/shared/deploy_keys/_form.html.haml b/app/views/shared/deploy_keys/_form.html.haml index e6075c3ae3a..87c2965bb21 100644 --- a/app/views/shared/deploy_keys/_form.html.haml +++ b/app/views/shared/deploy_keys/_form.html.haml @@ -1,5 +1,6 @@ - form = local_assigns.fetch(:form) - deploy_key = local_assigns.fetch(:deploy_key) +- deploy_keys_project = deploy_key.deploy_keys_project_for(@project) = form_errors(deploy_key) @@ -20,11 +21,13 @@ .col-sm-10 = form.text_field :fingerprint, class: 'form-control', readonly: 'readonly' -.form-group - .control-label - .col-sm-10 - = form.label :can_push do - = form.check_box :can_push - %strong Write access allowed - %p.light.append-bottom-0 - Allow this key to push to repository as well? (Default only allows pull access.) +- if deploy_keys_project.present? + = form.fields_for :deploy_keys_projects, deploy_keys_project do |deploy_keys_project_form| + .form-group + .control-label + .col-sm-10 + = deploy_keys_project_form.label :can_push do + = deploy_keys_project_form.check_box :can_push + %strong Write access allowed + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) diff --git a/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml new file mode 100644 index 00000000000..cb53a94e465 --- /dev/null +++ b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix writable shared deploy keys +merge_request: +author: +type: security diff --git a/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb new file mode 100644 index 00000000000..5dc723db9f9 --- /dev/null +++ b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb @@ -0,0 +1,15 @@ +class AddCanPushToDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column_with_default :deploy_keys_projects, :can_push, :boolean, default: false, allow_null: false + end + + def down + remove_column :deploy_keys_projects, :can_push + end +end diff --git a/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..ec0427a8e5a --- /dev/null +++ b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,44 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + disable_ddl_transaction! + + class DeploysKeyProject < ActiveRecord::Base + include EachBatch + + self.table_name = 'deploy_keys_projects' + end + + def up + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE deploy_keys_projects + SET can_push = keys.can_push + FROM keys + WHERE deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end + + def down + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE keys + SET can_push = deploy_keys_projects.can_push + FROM deploy_keys_projects + WHERE deploy_keys_projects.deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end +end diff --git a/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..05d236f8e96 --- /dev/null +++ b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,43 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PostPopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + class DeploysKeyProject < ActiveRecord::Base + include EachBatch + + self.table_name = 'deploy_keys_projects' + end + + def up + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE deploy_keys_projects + SET can_push = keys.can_push + FROM keys + WHERE deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end + + def down + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE keys + SET can_push = deploy_keys_projects.can_push + FROM deploy_keys_projects + WHERE deploy_keys_projects.deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end +end diff --git a/db/post_migrate/20171215121259_remove_can_push_from_keys.rb b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb new file mode 100644 index 00000000000..0599811d986 --- /dev/null +++ b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveCanPushFromKeys < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + remove_column :keys, :can_push + end + + def down + add_column_with_default :keys, :can_push, :boolean, default: false, allow_null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index a32d20b8f28..25583170851 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,11 @@ # # It's strongly recommended that you check this file into your version control system. +<<<<<<< HEAD ActiveRecord::Schema.define(version: 20180105212544) do +======= +ActiveRecord::Schema.define(version: 20171215121259) do +>>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3' # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -626,6 +630,7 @@ ActiveRecord::Schema.define(version: 20180105212544) do t.integer "project_id", null: false t.datetime "created_at" t.datetime "updated_at" + t.boolean "can_push", default: false, null: false end add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree @@ -896,7 +901,6 @@ ActiveRecord::Schema.define(version: 20180105212544) do t.string "type" t.string "fingerprint" t.boolean "public", default: false, null: false - t.boolean "can_push", default: false, null: false t.datetime "last_used_at" end diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md index 273d5a56b6f..698fa22a438 100644 --- a/doc/api/deploy_keys.md +++ b/doc/api/deploy_keys.md @@ -19,15 +19,13 @@ Example response: { "id": 1, "title": "Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "created_at": "2013-10-02T10:12:29Z" }, { "id": 3, "title": "Another Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": true, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "created_at": "2013-10-02T11:12:29Z" } ] @@ -57,15 +55,15 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T10:12:29Z" + "created_at": "2013-10-02T10:12:29Z", + "can_push": false }, { "id": 3, "title": "Another Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T11:12:29Z" + "created_at": "2013-10-02T11:12:29Z", + "can_push": false } ] ``` @@ -96,8 +94,8 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T10:12:29Z" + "created_at": "2013-10-02T10:12:29Z", + "can_push": false } ``` @@ -135,6 +133,36 @@ Example response: } ``` +## Update deploy key + +Updates a deploy key for a project. + +``` +PUT /projects/:id/deploy_keys/:key_id +``` + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `title` | string | no | New deploy key's title | +| `can_push` | boolean | no | Can deploy key push to the project's repository | + +```bash +curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data '{"title": "New deploy key", "can_push": true}' "https://gitlab.example.com/api/v4/projects/5/deploy_keys/11" +``` + +Example response: + +```json +{ + "id": 11, + "title": "New deploy key", + "key": "ssh-rsa AAAA...", + "created_at": "2015-08-29T12:44:31.550Z", + "can_push": true +} +``` + ## Delete deploy key Removes a deploy key from the project. If the deploy key is used only for this project, it will be deleted from the system. diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 281269b1190..a92f17937b8 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -4,6 +4,16 @@ module API before { authenticate! } + helpers do + def add_deploy_keys_project(project, attrs = {}) + project.deploy_keys_projects.create(attrs) + end + + def find_by_deploy_key(project, key_id) + project.deploy_keys_projects.find_by!(deploy_key: key_id) + end + end + desc 'Return all deploy keys' params do use :pagination @@ -21,28 +31,31 @@ module API before { authorize_admin_project } desc "Get a specific project's deploy keys" do - success Entities::SSHKey + success Entities::DeployKeysProject end params do use :pagination end get ":id/deploy_keys" do - present paginate(user_project.deploy_keys), with: Entities::SSHKey + keys = user_project.deploy_keys_projects.preload(:deploy_key) + + present paginate(keys), with: Entities::DeployKeysProject end desc 'Get single deploy key' do - success Entities::SSHKey + success Entities::DeployKeysProject end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end get ":id/deploy_keys/:key_id" do - key = user_project.deploy_keys.find params[:key_id] - present key, with: Entities::SSHKey + key = find_by_deploy_key(user_project, params[:key_id]) + + present key, with: Entities::DeployKeysProject end desc 'Add new deploy key to currently authenticated user' do - success Entities::SSHKey + success Entities::DeployKeysProject end params do requires :key, type: String, desc: 'The new deploy key' @@ -53,24 +66,31 @@ module API params[:key].strip! # Check for an existing key joined to this project - key = user_project.deploy_keys.find_by(key: params[:key]) + key = user_project.deploy_keys_projects + .joins(:deploy_key) + .find_by(keys: { key: params[:key] }) + if key - present key, with: Entities::SSHKey + present key, with: Entities::DeployKeysProject break end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: params[:key]) if key - user_project.deploy_keys << key - present key, with: Entities::SSHKey + added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) + + present added_key, with: Entities::DeployKeysProject break end # Create a new deploy key - key = DeployKey.new(declared_params(include_missing: false)) - if key.valid? && user_project.deploy_keys << key - present key, with: Entities::SSHKey + key_attributes = { can_push: !!params[:can_push], + deploy_key_attributes: declared_params.except(:can_push) } + key = add_deploy_keys_project(user_project, key_attributes) + + if key.valid? + present key, with: Entities::DeployKeysProject else render_validation_error!(key) end @@ -86,14 +106,20 @@ module API at_least_one_of :title, :can_push end put ":id/deploy_keys/:key_id" do - key = DeployKey.find(params.delete(:key_id)) + deploy_keys_project = find_by_deploy_key(user_project, params[:key_id]) - authorize!(:update_deploy_key, key) + authorize!(:update_deploy_key, deploy_keys_project.deploy_key) - if key.update_attributes(declared_params(include_missing: false)) - present key, with: Entities::SSHKey + can_push = params[:can_push].nil? ? deploy_keys_project.can_push : params[:can_push] + title = params[:title] || deploy_keys_project.deploy_key.title + + result = deploy_keys_project.update_attributes(can_push: can_push, + deploy_key_attributes: { id: params[:key_id], + title: title }) + if result + present deploy_keys_project, with: Entities::DeployKeysProject else - render_validation_error!(key) + render_validation_error!(deploy_keys_project) end end @@ -122,7 +148,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the deploy key' end delete ":id/deploy_keys/:key_id" do - key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) + key = user_project.deploy_keys.find(params[:key_id]) not_found!('Deploy Key') unless key destroy_conditionally!(key) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9618d6625bb..971cb83a2d9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -554,13 +554,18 @@ module API end class SSHKey < Grape::Entity - expose :id, :title, :key, :created_at, :can_push + expose :id, :title, :key, :created_at end class SSHKeyWithUser < SSHKey expose :user, using: Entities::UserPublic end + class DeployKeysProject < Grape::Entity + expose :deploy_key, merge: true, using: Entities::SSHKey + expose :can_push + end + class GPGKey < Grape::Entity expose :id, :key, :created_at end diff --git a/lib/api/v3/deploy_keys.rb b/lib/api/v3/deploy_keys.rb index b90e2061da3..47e54ca85a5 100644 --- a/lib/api/v3/deploy_keys.rb +++ b/lib/api/v3/deploy_keys.rb @@ -3,6 +3,16 @@ module API class DeployKeys < Grape::API before { authenticate! } + helpers do + def add_deploy_keys_project(project, attrs = {}) + project.deploy_keys_projects.create(attrs) + end + + def find_by_deploy_key(project, key_id) + project.deploy_keys_projects.find_by!(deploy_key: key_id) + end + end + get "deploy_keys" do authenticated_as_admin! @@ -18,25 +28,28 @@ module API %w(keys deploy_keys).each do |path| desc "Get a specific project's deploy keys" do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end get ":id/#{path}" do - present user_project.deploy_keys, with: ::API::Entities::SSHKey + keys = user_project.deploy_keys_projects.preload(:deploy_key) + + present keys, with: ::API::Entities::DeployKeysProject end desc 'Get single deploy key' do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end get ":id/#{path}/:key_id" do - key = user_project.deploy_keys.find params[:key_id] - present key, with: ::API::Entities::SSHKey + key = find_by_deploy_key(user_project, params[:key_id]) + + present key, with: ::API::Entities::DeployKeysProject end desc 'Add new deploy key to currently authenticated user' do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end params do requires :key, type: String, desc: 'The new deploy key' @@ -47,24 +60,31 @@ module API params[:key].strip! # Check for an existing key joined to this project - key = user_project.deploy_keys.find_by(key: params[:key]) + key = user_project.deploy_keys_projects + .joins(:deploy_key) + .find_by(keys: { key: params[:key] }) + if key - present key, with: ::API::Entities::SSHKey + present key, with: ::API::Entities::DeployKeysProject break end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: params[:key]) if key - user_project.deploy_keys << key - present key, with: ::API::Entities::SSHKey + added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) + + present added_key, with: ::API::Entities::DeployKeysProject break end # Create a new deploy key - key = DeployKey.new(declared_params(include_missing: false)) - if key.valid? && user_project.deploy_keys << key - present key, with: ::API::Entities::SSHKey + key_attributes = { can_push: !!params[:can_push], + deploy_key_attributes: declared_params.except(:can_push) } + key = add_deploy_keys_project(user_project, key_attributes) + + if key.valid? + present key, with: ::API::Entities::DeployKeysProject else render_validation_error!(key) end diff --git a/spec/factories/deploy_keys_projects.rb b/spec/factories/deploy_keys_projects.rb index 30a6d468ed3..4350652fb79 100644 --- a/spec/factories/deploy_keys_projects.rb +++ b/spec/factories/deploy_keys_projects.rb @@ -2,5 +2,9 @@ FactoryBot.define do factory :deploy_keys_project do deploy_key project + + trait :write_access do + can_push true + end end end diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index 552b4b7e06e..f0c43f3d6f5 100644 --- a/spec/factories/keys.rb +++ b/spec/factories/keys.rb @@ -15,10 +15,6 @@ FactoryBot.define do factory :another_deploy_key, class: 'DeployKey' end - factory :write_access_key, class: 'DeployKey' do - can_push true - end - factory :rsa_key_2048 do key do <<~KEY.delete("\n") diff --git a/spec/features/admin/admin_deploy_keys_spec.rb b/spec/features/admin/admin_deploy_keys_spec.rb index 241c7cbc34e..cb96830cb7c 100644 --- a/spec/features/admin/admin_deploy_keys_spec.rb +++ b/spec/features/admin/admin_deploy_keys_spec.rb @@ -17,6 +17,16 @@ RSpec.describe 'admin deploy keys' do end end + it 'shows all the projects the deploy key has write access' do + write_key = create(:deploy_keys_project, :write_access, deploy_key: deploy_key) + + visit admin_deploy_keys_path + + page.within(find('.deploy-keys-list', match: :first)) do + expect(page).to have_content(write_key.project.full_name) + end + end + describe 'create a new deploy key' do let(:new_ssh_key) { attributes_for(:key)[:key] } @@ -28,14 +38,12 @@ RSpec.describe 'admin deploy keys' do it 'creates a new deploy key' do fill_in 'deploy_key_title', with: 'laptop' fill_in 'deploy_key_key', with: new_ssh_key - check 'deploy_key_can_push' click_button 'Create' expect(current_path).to eq admin_deploy_keys_path page.within(find('.deploy-keys-list', match: :first)) do expect(page).to have_content('laptop') - expect(page).to have_content('Yes') end end end @@ -48,14 +56,12 @@ RSpec.describe 'admin deploy keys' do it 'updates an existing deploy key' do fill_in 'deploy_key_title', with: 'new-title' - check 'deploy_key_can_push' click_button 'Save changes' expect(current_path).to eq admin_deploy_keys_path page.within(find('.deploy-keys-list', match: :first)) do expect(page).to have_content('new-title') - expect(page).to have_content('Yes') end end end diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 81b282502fc..14670e91006 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -43,7 +43,7 @@ feature 'Repository settings' do fill_in 'deploy_key_title', with: 'new_deploy_key' fill_in 'deploy_key_key', with: new_ssh_key - check 'deploy_key_can_push' + check 'deploy_key_deploy_keys_projects_attributes_0_can_push' click_button 'Add key' expect(page).to have_content('new_deploy_key') @@ -57,7 +57,7 @@ feature 'Repository settings' do find('li', text: private_deploy_key.title).click_link('Edit') fill_in 'deploy_key_title', with: 'updated_deploy_key' - check 'deploy_key_can_push' + check 'deploy_key_deploy_keys_projects_attributes_0_can_push' click_button 'Save changes' expect(page).to have_content('updated_deploy_key') @@ -74,11 +74,9 @@ feature 'Repository settings' do find('li', text: private_deploy_key.title).click_link('Edit') fill_in 'deploy_key_title', with: 'updated_deploy_key' - check 'deploy_key_can_push' click_button 'Save changes' expect(page).to have_content('updated_deploy_key') - expect(page).to have_content('Write access allowed') end scenario 'remove an existing deploy key' do diff --git a/spec/javascripts/deploy_keys/components/key_spec.js b/spec/javascripts/deploy_keys/components/key_spec.js index 2f28c5bbf01..b7aadf604a4 100644 --- a/spec/javascripts/deploy_keys/components/key_spec.js +++ b/spec/javascripts/deploy_keys/components/key_spec.js @@ -53,18 +53,24 @@ describe('Deploy keys key', () => { ).toBe('Remove'); }); - it('shows write access text when key has write access', (done) => { - vm.deployKey.can_push = true; + it('shows write access title when key has write access', (done) => { + vm.deployKey.deploy_keys_projects[0].can_push = true; Vue.nextTick(() => { expect( - vm.$el.querySelector('.write-access-allowed'), - ).not.toBeNull(); - - expect( - vm.$el.querySelector('.write-access-allowed').textContent.trim(), + vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'), ).toBe('Write access allowed'); + done(); + }); + }); + + it('does not show write access title when key has write access', (done) => { + vm.deployKey.deploy_keys_projects[0].can_push = false; + Vue.nextTick(() => { + expect( + vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'), + ).toBe('Read access only'); done(); }); }); diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index a6fbec295b5..cc202ce8bca 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -136,8 +136,8 @@ describe Gitlab::Auth do it 'grants deploy key write permissions' do project = create(:project) - key = create(:deploy_key, can_push: true) - create(:deploy_keys_project, deploy_key: key, project: project) + key = create(:deploy_key) + create(:deploy_keys_project, :write_access, deploy_key: key, project: project) token = Gitlab::LfsToken.new(key).token expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}") @@ -146,7 +146,7 @@ describe Gitlab::Auth do it 'does not grant deploy key write permissions' do project = create(:project) - key = create(:deploy_key, can_push: true) + key = create(:deploy_key) token = Gitlab::LfsToken.new(key).token expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}") diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 4290fbb0087..2009a8ac48c 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -51,12 +51,12 @@ describe Gitlab::GitAccess do context 'when the project exists' do context 'when actor exists' do context 'when actor is a DeployKey' do - let(:deploy_key) { create(:deploy_key, user: user, can_push: true) } + let(:deploy_key) { create(:deploy_key, user: user) } let(:actor) { deploy_key } context 'when the DeployKey has access to the project' do before do - deploy_key.projects << project + deploy_key.deploy_keys_projects.create(project: project, can_push: true) end it 'allows push and pull access' do @@ -696,15 +696,13 @@ describe Gitlab::GitAccess do end describe 'deploy key permissions' do - let(:key) { create(:deploy_key, user: user, can_push: can_push) } + let(:key) { create(:deploy_key, user: user) } let(:actor) { key } context 'when deploy_key can push' do - let(:can_push) { true } - context 'when project is authorized' do before do - key.projects << project + key.deploy_keys_projects.create(project: project, can_push: true) end it { expect { push_access_check }.not_to raise_error } @@ -732,11 +730,9 @@ describe Gitlab::GitAccess do end context 'when deploy_key cannot push' do - let(:can_push) { false } - context 'when project is authorized' do before do - key.projects << project + key.deploy_keys_projects.create(project: project, can_push: false) end it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) } diff --git a/spec/models/deploy_keys_project_spec.rb b/spec/models/deploy_keys_project_spec.rb index 0345fefb254..fca3090ff4a 100644 --- a/spec/models/deploy_keys_project_spec.rb +++ b/spec/models/deploy_keys_project_spec.rb @@ -8,7 +8,7 @@ describe DeployKeysProject do describe "Validation" do it { is_expected.to validate_presence_of(:project_id) } - it { is_expected.to validate_presence_of(:deploy_key_id) } + it { is_expected.to validate_presence_of(:deploy_key) } end describe "Destroying" do diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 1f1e6ea17e4..0772b3f2e64 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -110,7 +110,7 @@ describe API::DeployKeys do end it 'accepts can_push parameter' do - key_attrs = attributes_for :write_access_key + key_attrs = attributes_for(:another_key).merge(can_push: true) post api("/projects/#{project.id}/deploy_keys", admin), key_attrs @@ -160,16 +160,6 @@ describe API::DeployKeys do expect(json_response['title']).to eq('new title') expect(json_response['can_push']).to eq(true) end - - it 'updates a private ssh key from projects user has access with correct attributes' do - create(:deploy_keys_project, project: project2, deploy_key: private_deploy_key) - - put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), { title: 'new title', can_push: true } - - expect(json_response['id']).to eq(private_deploy_key.id) - expect(json_response['title']).to eq('new title') - expect(json_response['can_push']).to eq(true) - end end describe 'DELETE /projects/:id/deploy_keys/:key_id' do diff --git a/spec/requests/api/v3/deploy_keys_spec.rb b/spec/requests/api/v3/deploy_keys_spec.rb index 785bc1eb4ba..501af587ad4 100644 --- a/spec/requests/api/v3/deploy_keys_spec.rb +++ b/spec/requests/api/v3/deploy_keys_spec.rb @@ -107,7 +107,7 @@ describe API::V3::DeployKeys do end it 'accepts can_push parameter' do - key_attrs = attributes_for :write_access_key + key_attrs = attributes_for(:another_key).merge(can_push: true) post v3_api("/projects/#{project.id}/#{path}", admin), key_attrs diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 5e59bb0d585..bee918a20aa 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -781,11 +781,11 @@ describe 'Git LFS API and storage' do end context 'when deploy key has project push access' do - let(:key) { create(:deploy_key, can_push: true) } + let(:key) { create(:deploy_key) } let(:authorization) { authorize_deploy_key } let(:update_user_permissions) do - project.deploy_keys << key + project.deploy_keys_projects.create(deploy_key: key, can_push: true) end it_behaves_like 'pushes new LFS objects' diff --git a/spec/serializers/deploy_key_entity_spec.rb b/spec/serializers/deploy_key_entity_spec.rb index d3aefa2c9eb..2bd8162d1b7 100644 --- a/spec/serializers/deploy_key_entity_spec.rb +++ b/spec/serializers/deploy_key_entity_spec.rb @@ -21,18 +21,21 @@ describe DeployKeyEntity do user_id: deploy_key.user_id, title: deploy_key.title, fingerprint: deploy_key.fingerprint, - can_push: deploy_key.can_push, destroyed_when_orphaned: true, almost_orphaned: false, created_at: deploy_key.created_at, updated_at: deploy_key.updated_at, can_edit: false, - projects: [ + deploy_keys_projects: [ { - id: project.id, - name: project.name, - full_path: project_path(project), - full_name: project.full_name + can_push: false, + project: + { + id: project.id, + name: project.name, + full_path: project_path(project), + full_name: project.full_name + } } ] } -- cgit v1.2.1 From 791ca43f3f8f12451ee1e70efc90f5d82347af93 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 5 Jan 2018 21:36:18 +0000 Subject: Merge branch '41293-fix-command-injection-vulnerability-on-system_hook_push-queue-through-web-hook' into 'security-10-3' Don't allow line breaks on HTTP headers See merge request gitlab/gitlabhq!2277 (cherry picked from commit 7fc0a6fc096768a5604d6dd24d7d952e53300c82) 073b8f9c Don't allow line breaks on HTTP headers --- app/models/hooks/web_hook.rb | 1 + app/services/web_hook_service.rb | 2 +- lib/gitlab/utils.rb | 4 ++++ spec/lib/gitlab/utils_spec.rb | 16 ++++++++++++++++ spec/models/hooks/web_hook_spec.rb | 6 ++++++ 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 5a70e114f56..27729deeac9 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent validates :url, presence: true, url: true + validates :token, format: { without: /\n/ } def execute(data, hook_name) WebHookService.new(self, data, hook_name).execute diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 6ebc7c89500..36e589d5aa8 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -113,7 +113,7 @@ class WebHookService 'Content-Type' => 'application/json', 'X-Gitlab-Event' => hook_name.singularize.titleize }.tap do |hash| - hash['X-Gitlab-Token'] = hook.token if hook.token.present? + hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present? end end end diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index b3baaf036d8..fa22f0e37b2 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -27,6 +27,10 @@ module Gitlab .gsub(/(\A-+|-+\z)/, '') end + def remove_line_breaks(str) + str.gsub(/\r?\n/, '') + end + def to_boolean(value) return value if [true, false].include?(value) return true if value =~ /^(true|t|yes|y|1|on)$/i diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index e872a5290c5..bda239b7871 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -17,6 +17,22 @@ describe Gitlab::Utils do end end + describe '.remove_line_breaks' do + using RSpec::Parameterized::TableSyntax + + where(:original, :expected) do + "foo\nbar\nbaz" | "foobarbaz" + "foo\r\nbar\r\nbaz" | "foobarbaz" + "foobar" | "foobar" + end + + with_them do + it "replace line breaks with an empty string" do + expect(described_class.remove_line_breaks(original)).to eq(expected) + end + end + end + describe '.to_boolean' do it 'accepts booleans' do expect(to_boolean(true)).to be(true) diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 388120160ab..ea6d6e53ef5 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -29,6 +29,12 @@ describe WebHook do expect(hook.url).to eq('https://example.com') end end + + describe 'token' do + it { is_expected.to allow_value("foobar").for(:token) } + + it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) } + end end describe 'execute' do -- cgit v1.2.1 From 532a0b60184800b0442723498d5257c20d20a8aa Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 8 Jan 2018 15:42:41 +0000 Subject: Merge branch 'fix/import-rce-10-3' into 'security-10-3' [10.3] Fix RCE via project import mechanism See merge request gitlab/gitlabhq!2294 (cherry picked from commit dcfec507d6f9ee119d65a832393e7c593af1d3b2) 86d75812 Fix RCE via project import mechanism --- changelogs/unreleased/fix-import-rce.yml | 5 ++ lib/gitlab/import_export/file_importer.rb | 6 ++- lib/gitlab/import_export/saver.rb | 2 +- lib/gitlab/import_export/shared.rb | 14 +++++- .../lib/gitlab/import_export/file_importer_spec.rb | 57 +++++++++++++++++----- 5 files changed, 68 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/fix-import-rce.yml diff --git a/changelogs/unreleased/fix-import-rce.yml b/changelogs/unreleased/fix-import-rce.yml new file mode 100644 index 00000000000..c47e45fac31 --- /dev/null +++ b/changelogs/unreleased/fix-import-rce.yml @@ -0,0 +1,5 @@ +--- +title: Fix RCE via project import mechanism +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/file_importer.rb b/lib/gitlab/import_export/file_importer.rb index 989342389bc..5c971564a73 100644 --- a/lib/gitlab/import_export/file_importer.rb +++ b/lib/gitlab/import_export/file_importer.rb @@ -17,12 +17,16 @@ module Gitlab def import mkdir_p(@shared.export_path) + remove_symlinks! + wait_for_archived_file do decompress_archive end rescue => e @shared.error(e) false + ensure + remove_symlinks! end private @@ -43,7 +47,7 @@ module Gitlab raise Projects::ImportService::Error.new("Unable to decompress #{@archive_file} into #{@shared.export_path}") unless result - remove_symlinks! + result end def remove_symlinks! diff --git a/lib/gitlab/import_export/saver.rb b/lib/gitlab/import_export/saver.rb index 6130c124dd1..2daeba90a51 100644 --- a/lib/gitlab/import_export/saver.rb +++ b/lib/gitlab/import_export/saver.rb @@ -37,7 +37,7 @@ module Gitlab end def archive_file - @archive_file ||= File.join(@shared.export_path, '..', Gitlab::ImportExport.export_filename(project: @project)) + @archive_file ||= File.join(@shared.archive_path, Gitlab::ImportExport.export_filename(project: @project)) end end end diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 9fd0b709ef2..d03cbc880fd 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -9,7 +9,11 @@ module Gitlab end def export_path - @export_path ||= Gitlab::ImportExport.export_path(relative_path: opts[:relative_path]) + @export_path ||= Gitlab::ImportExport.export_path(relative_path: relative_path) + end + + def archive_path + @archive_path ||= Gitlab::ImportExport.export_path(relative_path: relative_archive_path) end def error(error) @@ -21,6 +25,14 @@ module Gitlab private + def relative_path + File.join(opts[:relative_path], SecureRandom.hex) + end + + def relative_archive_path + File.join(opts[:relative_path], '..') + end + def error_out(message, caller) Rails.logger.error("Import/Export error raised on #{caller}: #{message}") end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index 162b776e107..5cdc5138fda 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -12,30 +12,61 @@ describe Gitlab::ImportExport::FileImporter do stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) allow_any_instance_of(Gitlab::ImportExport::CommandLineUtil).to receive(:untar_zxf).and_return(true) - + allow(SecureRandom).to receive(:hex).and_return('abcd') setup_files - - described_class.import(archive_file: '', shared: shared) end after do FileUtils.rm_rf(export_path) end - it 'removes symlinks in root folder' do - expect(File.exist?(symlink_file)).to be false - end + context 'normal run' do + before do + described_class.import(archive_file: '', shared: shared) + end - it 'removes hidden symlinks in root folder' do - expect(File.exist?(hidden_symlink_file)).to be false - end + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end - it 'removes symlinks in subfolders' do - expect(File.exist?(subfolder_symlink_file)).to be false + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end + + it 'creates the file in the right subfolder' do + expect(shared.export_path).to include('test/abcd') + end end - it 'does not remove a valid file' do - expect(File.exist?(valid_file)).to be true + context 'error' do + before do + allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError) + described_class.import(archive_file: '', shared: shared) + end + + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end + + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end end def setup_files -- cgit v1.2.1 From 54636e1d4293a8465a772020a54b6193d7df9878 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 9 Jan 2018 08:39:22 +0000 Subject: Merge branch 'fl-ipythin-10-3' into 'security-10-3' Port of [10.2] Sanitizes IPython notebook output See merge request gitlab/gitlabhq!2285 (cherry picked from commit 1c46e031c70706450a8e0ae730f4c323b72f9e4c) aac035fe Port of [10.2] Sanitizes IPython notebook output --- app/assets/javascripts/notebook/cells/markdown.vue | 8 ++- .../javascripts/notebook/cells/output/html.vue | 27 ++++++++- package.json | 1 + spec/javascripts/notebook/cells/markdown_spec.js | 12 ++++ .../notebook/cells/output/html_sanitize_tests.js | 66 ++++++++++++++++++++++ .../javascripts/notebook/cells/output/html_spec.js | 29 ++++++++++ yarn.lock | 49 +++++++++++++++- 7 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 spec/javascripts/notebook/cells/output/html_sanitize_tests.js create mode 100644 spec/javascripts/notebook/cells/output/html_spec.js diff --git a/app/assets/javascripts/notebook/cells/markdown.vue b/app/assets/javascripts/notebook/cells/markdown.vue index d0ec70f1fcf..3d09d24b6ab 100644 --- a/app/assets/javascripts/notebook/cells/markdown.vue +++ b/app/assets/javascripts/notebook/cells/markdown.vue @@ -1,6 +1,7 @@ diff --git a/package.json b/package.json index 35c145aaebe..c9778865e93 100644 --- a/package.json +++ b/package.json @@ -64,6 +64,7 @@ "raven-js": "^3.14.0", "raw-loader": "^0.5.1", "react-dev-utils": "^0.5.2", + "sanitize-html": "^1.16.1", "select2": "3.5.2-browserify", "sql.js": "^0.4.0", "svg4everybody": "2.1.9", diff --git a/spec/javascripts/notebook/cells/markdown_spec.js b/spec/javascripts/notebook/cells/markdown_spec.js index a88e9ed3d99..db2a16b0b68 100644 --- a/spec/javascripts/notebook/cells/markdown_spec.js +++ b/spec/javascripts/notebook/cells/markdown_spec.js @@ -42,6 +42,18 @@ describe('Markdown component', () => { expect(vm.$el.querySelector('.markdown h1')).not.toBeNull(); }); + it('sanitizes output', (done) => { + Object.assign(cell, { + source: ['[XSS](data:text/html;base64,PHNjcmlwdD5hbGVydChkb2N1bWVudC5kb21haW4pPC9zY3JpcHQ+Cg==)\n'], + }); + + Vue.nextTick(() => { + expect(vm.$el.querySelector('a').getAttribute('href')).toBeNull(); + + done(); + }); + }); + describe('katex', () => { beforeEach(() => { json = getJSONFixture('blob/notebook/math.json'); diff --git a/spec/javascripts/notebook/cells/output/html_sanitize_tests.js b/spec/javascripts/notebook/cells/output/html_sanitize_tests.js new file mode 100644 index 00000000000..d587573fc9e --- /dev/null +++ b/spec/javascripts/notebook/cells/output/html_sanitize_tests.js @@ -0,0 +1,66 @@ +export default { + 'protocol-based JS injection: simple, no spaces': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces before': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces after': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces before and after': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: preceding colon': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: UTF-8 encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long UTF-8 encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long UTF-8 encoding without semicolons': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: hex encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long hex encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: hex encoding without semicolons': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: null char': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: invalid URL char': { + input: '', // eslint-disable-line no-useless-escape + output: '', + }, + 'protocol-based JS injection: Unicode': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: spaces and entities': { + input: 'foo', + output: 'foo', + }, + 'img on error': { + input: '', + output: '', + }, +}; diff --git a/spec/javascripts/notebook/cells/output/html_spec.js b/spec/javascripts/notebook/cells/output/html_spec.js new file mode 100644 index 00000000000..9c5385f2922 --- /dev/null +++ b/spec/javascripts/notebook/cells/output/html_spec.js @@ -0,0 +1,29 @@ +import Vue from 'vue'; +import htmlOutput from '~/notebook/cells/output/html.vue'; +import sanitizeTests from './html_sanitize_tests'; + +describe('html output cell', () => { + function createComponent(rawCode) { + const Component = Vue.extend(htmlOutput); + + return new Component({ + propsData: { + rawCode, + }, + }).$mount(); + } + + describe('sanitizes output', () => { + Object.keys(sanitizeTests).forEach((key) => { + it(key, () => { + const test = sanitizeTests[key]; + const vm = createComponent(test.input); + const outputEl = [...vm.$el.querySelectorAll('div')].pop(); + + expect(outputEl.innerHTML).toEqual(test.output); + + vm.$destroy(); + }); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index 64b66ee446a..693f7123e8c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -237,7 +237,7 @@ array-union@^1.0.1: dependencies: array-uniq "^1.0.1" -array-uniq@^1.0.1: +array-uniq@^1.0.1, array-uniq@^1.0.2: version "1.0.3" resolved "https://registry.yarnpkg.com/array-uniq/-/array-uniq-1.0.3.tgz#af6ac877a25cc7f74e058894753858dfdb24fdb6" @@ -3198,7 +3198,7 @@ html-entities@1.2.0, html-entities@^1.2.0: version "1.2.0" resolved "https://registry.yarnpkg.com/html-entities/-/html-entities-1.2.0.tgz#41948caf85ce82fed36e4e6a0ed371a6664379e2" -htmlparser2@^3.8.2: +htmlparser2@^3.8.2, htmlparser2@^3.9.0: version "3.9.2" resolved "https://registry.yarnpkg.com/htmlparser2/-/htmlparser2-3.9.2.tgz#1bdf87acca0f3f9e53fa4fcceb0f4b4cbb00b338" dependencies: @@ -4054,6 +4054,10 @@ lodash.capitalize@^4.0.0: version "4.2.1" resolved "https://registry.yarnpkg.com/lodash.capitalize/-/lodash.capitalize-4.2.1.tgz#f826c9b4e2a8511d84e3aca29db05e1a4f3b72a9" +lodash.clonedeep@^4.5.0: + version "4.5.0" + resolved "https://registry.yarnpkg.com/lodash.clonedeep/-/lodash.clonedeep-4.5.0.tgz#e23f3f9c4f8fbdde872529c1071857a086e5ccef" + lodash.cond@^4.3.0: version "4.5.2" resolved "https://registry.yarnpkg.com/lodash.cond/-/lodash.cond-4.5.2.tgz#f471a1da486be60f6ab955d17115523dd1d255d5" @@ -4069,6 +4073,10 @@ lodash.defaults@^3.1.2: lodash.assign "^3.0.0" lodash.restparam "^3.0.0" +lodash.escaperegexp@^4.1.2: + version "4.1.2" + resolved "https://registry.yarnpkg.com/lodash.escaperegexp/-/lodash.escaperegexp-4.1.2.tgz#64762c48618082518ac3df4ccf5d5886dae20347" + lodash.get@^3.7.0: version "3.7.0" resolved "https://registry.yarnpkg.com/lodash.get/-/lodash.get-3.7.0.tgz#3ce68ae2c91683b281cc5394128303cbf75e691f" @@ -4103,6 +4111,10 @@ lodash.memoize@^4.1.2: version "4.1.2" resolved "https://registry.yarnpkg.com/lodash.memoize/-/lodash.memoize-4.1.2.tgz#bcc6c49a42a2840ed997f323eada5ecd182e0bfe" +lodash.mergewith@^4.6.0: + version "4.6.0" + resolved "https://registry.yarnpkg.com/lodash.mergewith/-/lodash.mergewith-4.6.0.tgz#150cf0a16791f5903b8891eab154609274bdea55" + lodash.restparam@^3.0.0: version "3.6.1" resolved "https://registry.yarnpkg.com/lodash.restparam/-/lodash.restparam-3.6.1.tgz#936a4e309ef330a7645ed4145986c85ae5b20805" @@ -5155,6 +5167,14 @@ postcss@^5.0.10, postcss@^5.0.11, postcss@^5.0.12, postcss@^5.0.13, postcss@^5.0 source-map "^0.5.6" supports-color "^3.2.3" +postcss@^6.0.14: + version "6.0.15" + resolved "https://registry.yarnpkg.com/postcss/-/postcss-6.0.15.tgz#f460cd6269fede0d1bf6defff0b934a9845d974d" + dependencies: + chalk "^2.3.0" + source-map "^0.6.1" + supports-color "^5.1.0" + postcss@^6.0.8: version "6.0.14" resolved "https://registry.yarnpkg.com/postcss/-/postcss-6.0.14.tgz#5534c72114739e75d0afcf017db853099f562885" @@ -5669,6 +5689,18 @@ safe-buffer@^5.0.1, safe-buffer@~5.0.1: version "5.0.1" resolved "https://registry.yarnpkg.com/safe-buffer/-/safe-buffer-5.0.1.tgz#d263ca54696cd8a306b5ca6551e92de57918fbe7" +sanitize-html@^1.16.1: + version "1.16.3" + resolved "https://registry.yarnpkg.com/sanitize-html/-/sanitize-html-1.16.3.tgz#96c1b44a36ff7312e1c22a14b05274370ac8bd56" + dependencies: + htmlparser2 "^3.9.0" + lodash.clonedeep "^4.5.0" + lodash.escaperegexp "^4.1.2" + lodash.mergewith "^4.6.0" + postcss "^6.0.14" + srcset "^1.0.0" + xtend "^4.0.0" + sax@~1.2.1: version "1.2.2" resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.2.tgz#fd8631a23bc7826bef5d871bdb87378c95647828" @@ -5982,6 +6014,13 @@ sql.js@^0.4.0: version "0.4.0" resolved "https://registry.yarnpkg.com/sql.js/-/sql.js-0.4.0.tgz#23be9635520eb0ff43a741e7e830397266e88445" +srcset@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/srcset/-/srcset-1.0.0.tgz#a5669de12b42f3b1d5e83ed03c71046fc48f41ef" + dependencies: + array-uniq "^1.0.2" + number-is-nan "^1.0.0" + sshpk@^1.7.0: version "1.13.1" resolved "https://registry.yarnpkg.com/sshpk/-/sshpk-1.13.1.tgz#512df6da6287144316dc4c18fe1cf1d940739be3" @@ -6126,6 +6165,12 @@ supports-color@^4.2.1: dependencies: has-flag "^2.0.0" +supports-color@^5.1.0: + version "5.1.0" + resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-5.1.0.tgz#058a021d1b619f7ddf3980d712ea3590ce7de3d5" + dependencies: + has-flag "^2.0.0" + svg4everybody@2.1.9: version "2.1.9" resolved "https://registry.yarnpkg.com/svg4everybody/-/svg4everybody-2.1.9.tgz#5bd9f6defc133859a044646d4743fabc28db7e2d" -- cgit v1.2.1 From 4493ec08806813fec9ccc3a27a5a6f59af9780fd Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 9 Jan 2018 16:47:31 +0000 Subject: Merge branch 'jej/fix-disabled-oauth-access-10-3' into 'security-10-3' [10.3] Prevent login with disabled OAuth providers See merge request gitlab/gitlabhq!2296 (cherry picked from commit 4936650427ffc88e6ee927aedbb2c724d24b094c) a0f9d222 Prevents login with disabled OAuth providers --- app/controllers/omniauth_callbacks_controller.rb | 9 +++ .../unreleased/jej-fix-disabled-oauth-access.yml | 5 ++ lib/gitlab/o_auth.rb | 6 ++ lib/gitlab/o_auth/user.rb | 11 ++-- .../omniauth_callbacks_controller_spec.rb | 75 ++++++++++++++++++++++ spec/features/oauth_login_spec.rb | 3 +- spec/support/devise_helpers.rb | 15 +++-- spec/support/login_helpers.rb | 7 ++ 8 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/jej-fix-disabled-oauth-access.yml create mode 100644 lib/gitlab/o_auth.rb create mode 100644 spec/controllers/omniauth_callbacks_controller_spec.rb diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 689d2e3db22..d631d09f1b8 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -112,6 +112,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController continue_login_process end + rescue Gitlab::OAuth::SigninDisabledForProviderError + handle_disabled_provider rescue Gitlab::OAuth::SignupDisabledError handle_signup_error end @@ -168,6 +170,13 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to new_user_session_path end + def handle_disabled_provider + label = Gitlab::OAuth::Provider.label_for(oauth['provider']) + flash[:alert] = "Signing in using #{label} has been disabled" + + redirect_to new_user_session_path + end + def log_audit_event(user, options = {}) AuditEventService.new(user, user, options) .for_authentication.security_event diff --git a/changelogs/unreleased/jej-fix-disabled-oauth-access.yml b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml new file mode 100644 index 00000000000..3a92c432dc6 --- /dev/null +++ b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml @@ -0,0 +1,5 @@ +--- +title: Prevent OAuth login POST requests when a provider has been disabled +merge_request: +author: +type: security diff --git a/lib/gitlab/o_auth.rb b/lib/gitlab/o_auth.rb new file mode 100644 index 00000000000..5ad8d83bd6e --- /dev/null +++ b/lib/gitlab/o_auth.rb @@ -0,0 +1,6 @@ +module Gitlab + module OAuth + SignupDisabledError = Class.new(StandardError) + SigninDisabledForProviderError = Class.new(StandardError) + end +end diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index d33f33d192f..fff9360ea27 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -5,8 +5,6 @@ # module Gitlab module OAuth - SignupDisabledError = Class.new(StandardError) - class User attr_accessor :auth_hash, :gl_user @@ -29,7 +27,8 @@ module Gitlab end def save(provider = 'OAuth') - unauthorized_to_create unless gl_user + raise SigninDisabledForProviderError if oauth_provider_disabled? + raise SignupDisabledError unless gl_user block_after_save = needs_blocking? @@ -226,8 +225,10 @@ module Gitlab Gitlab::AppLogger end - def unauthorized_to_create - raise SignupDisabledError + def oauth_provider_disabled? + Gitlab::CurrentSettings.current_application_settings + .disabled_oauth_sign_in_sources + .include?(auth_hash.provider) end end end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb new file mode 100644 index 00000000000..c639ad32ec6 --- /dev/null +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe OmniauthCallbacksController do + include LoginHelpers + + let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) } + let(:provider) { :github } + + before do + mock_auth_hash(provider.to_s, 'my-uid', user.email) + stub_omniauth_provider(provider, context: request) + end + + it 'allows sign in' do + post provider + + expect(request.env['warden']).to be_authenticated + end + + shared_context 'sign_up' do + let(:user) { double(email: 'new@example.com') } + + before do + stub_omniauth_setting(block_auto_created_users: false) + end + end + + context 'sign up' do + include_context 'sign_up' + + it 'is allowed' do + post provider + + expect(request.env['warden']).to be_authenticated + end + end + + context 'when OAuth is disabled' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + settings = Gitlab::CurrentSettings.current_application_settings + settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) + end + + it 'prevents login via POST' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + + it 'shows warning when attempting login' do + post provider + + expect(response).to redirect_to new_user_session_path + expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + end + + it 'allows linking the disabled provider' do + user.identities.destroy_all + sign_in(user) + + expect { post provider }.to change { user.reload.identities.count }.by(1) + end + + context 'sign up' do + include_context 'sign_up' + + it 'is prevented' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + end + end +end diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 49d8e52f861..a5e325ee2e3 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -10,8 +10,7 @@ feature 'OAuth Login', :js, :allow_forgery_protection do def stub_omniauth_config(provider) OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new(provider: provider.to_s, uid: "12345")) - set_devise_mapping(context: Rails.application) - Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider] + stub_omniauth_provider(provider) end providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, diff --git a/spec/support/devise_helpers.rb b/spec/support/devise_helpers.rb index 890a2d9d287..66874e10f38 100644 --- a/spec/support/devise_helpers.rb +++ b/spec/support/devise_helpers.rb @@ -2,13 +2,16 @@ module DeviseHelpers # explicitly tells Devise which mapping to use # this is needed when we are testing a Devise controller bypassing the router def set_devise_mapping(context:) - env = - if context.respond_to?(:env_config) - context.env_config - elsif context.respond_to?(:env) - context.env - end + env = env_from_context(context) env['devise.mapping'] = Devise.mappings[:user] if env end + + def env_from_context(context) + if context.respond_to?(:env_config) + context.env_config + elsif context.respond_to?(:env) + context.env + end + end end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 50702a0ac88..b52b6a28c54 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -125,6 +125,13 @@ module LoginHelpers }) end + def stub_omniauth_provider(provider, context: Rails.application) + env = env_from_context(context) + + set_devise_mapping(context: context) + env['omniauth.auth'] = OmniAuth.config.mock_auth[provider] + end + def stub_omniauth_saml_config(messages) set_devise_mapping(context: Rails.application) Rails.application.routes.disable_clear_and_finalize = true -- cgit v1.2.1 From 450cb59cdd648b6657a6474d1699cd88de2150da Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 10 Jan 2018 20:27:31 -0200 Subject: Update CHANGELOG.md for 10.3.4 [ci skip] --- changelogs/unreleased/api-no-service-pw-output.yml | 5 ----- changelogs/unreleased/fix-import-rce.yml | 5 ----- changelogs/unreleased/jej-fix-disabled-oauth-access.yml | 5 ----- changelogs/unreleased/milestones-finder-order-fix.yml | 5 ----- changelogs/unreleased/projectfix.yml | 6 ------ changelogs/unreleased/security-10-3.yml | 5 ----- .../unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml | 5 ----- 7 files changed, 36 deletions(-) delete mode 100644 changelogs/unreleased/api-no-service-pw-output.yml delete mode 100644 changelogs/unreleased/fix-import-rce.yml delete mode 100644 changelogs/unreleased/jej-fix-disabled-oauth-access.yml delete mode 100644 changelogs/unreleased/milestones-finder-order-fix.yml delete mode 100644 changelogs/unreleased/projectfix.yml delete mode 100644 changelogs/unreleased/security-10-3.yml delete mode 100644 changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml diff --git a/changelogs/unreleased/api-no-service-pw-output.yml b/changelogs/unreleased/api-no-service-pw-output.yml deleted file mode 100644 index f0d0adaad1c..00000000000 --- a/changelogs/unreleased/api-no-service-pw-output.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Filter out sensitive fields from the project services API -merge_request: -author: Robert Schilling -type: security diff --git a/changelogs/unreleased/fix-import-rce.yml b/changelogs/unreleased/fix-import-rce.yml deleted file mode 100644 index c47e45fac31..00000000000 --- a/changelogs/unreleased/fix-import-rce.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix RCE via project import mechanism -merge_request: -author: -type: security diff --git a/changelogs/unreleased/jej-fix-disabled-oauth-access.yml b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml deleted file mode 100644 index 3a92c432dc6..00000000000 --- a/changelogs/unreleased/jej-fix-disabled-oauth-access.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Prevent OAuth login POST requests when a provider has been disabled -merge_request: -author: -type: security diff --git a/changelogs/unreleased/milestones-finder-order-fix.yml b/changelogs/unreleased/milestones-finder-order-fix.yml deleted file mode 100644 index 983c301a82d..00000000000 --- a/changelogs/unreleased/milestones-finder-order-fix.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Prevent a SQL injection in the MilestonesFinder -merge_request: -author: -type: security diff --git a/changelogs/unreleased/projectfix.yml b/changelogs/unreleased/projectfix.yml deleted file mode 100644 index 4d8ebe6194a..00000000000 --- a/changelogs/unreleased/projectfix.yml +++ /dev/null @@ -1,6 +0,0 @@ ---- -title: Check user authorization for source and target projects when creating a merge - request. -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-10-3.yml b/changelogs/unreleased/security-10-3.yml deleted file mode 100644 index 5c718d976cd..00000000000 --- a/changelogs/unreleased/security-10-3.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix path traversal in gitlab-ci.yml cache:key -merge_request: -author: -type: security diff --git a/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml deleted file mode 100644 index cb53a94e465..00000000000 --- a/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix writable shared deploy keys -merge_request: -author: -type: security -- cgit v1.2.1 From de3b9b9fb508636c527bbb1de33614dcfdf80e6e Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 10 Jan 2018 20:27:51 -0200 Subject: Update VERSION to 10.3.4 --- VERSION | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/VERSION b/VERSION index 80959b81ba4..cd5aa407ba8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1,5 @@ +<<<<<<< HEAD 10.4.0-pre +======= +10.3.4 +>>>>>>> Update VERSION to 10.3.4 -- cgit v1.2.1 From 3f2df9c366d80b58209e2028a3bfe4e66c6b483c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 16 Jan 2018 17:08:03 -0800 Subject: Resolve conflicts in VERSION, db/schema.rb, and spec/models/service_spec.rb --- VERSION | 4 ---- db/schema.rb | 4 ---- spec/models/service_spec.rb | 6 +++--- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/VERSION b/VERSION index cd5aa407ba8..80959b81ba4 100644 --- a/VERSION +++ b/VERSION @@ -1,5 +1 @@ -<<<<<<< HEAD 10.4.0-pre -======= -10.3.4 ->>>>>>> Update VERSION to 10.3.4 diff --git a/db/schema.rb b/db/schema.rb index 25583170851..13913b49902 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,11 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -<<<<<<< HEAD ActiveRecord::Schema.define(version: 20180105212544) do -======= -ActiveRecord::Schema.define(version: 20171215121259) do ->>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3' # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 15c1e57c9e4..79f25dc4360 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -255,7 +255,6 @@ describe Service do end end -<<<<<<< HEAD describe "#deprecated?" do let(:project) { create(:project, :repository) } @@ -279,7 +278,9 @@ describe Service do it 'returns service template' do expect(KubernetesService.find_by_template).to eq(kubernetes_service) -======= + end + end + describe '#api_field_names' do let(:fake_service) do Class.new(Service) do @@ -311,7 +312,6 @@ describe Service do it 'filters out sensitive fields' do expect(service.api_field_names).to eq(['safe_field']) ->>>>>>> Merge branch 'security-10-3-do-not-expose-passwords-or-tokens-in-service-integrations-api' into 'security-10-3' end end end -- cgit v1.2.1 From 23a20c20f826f090446587881df7008a137d2d34 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Thu, 28 Dec 2017 11:25:02 -0600 Subject: Initial work to add notification reason to emails Adds `#build_notification_recipients` to `NotificationRecipientService` that returns the `NotificationRecipient` objects in order to be able to access the new attribute `reason`. This new attribute is used in the different notifier methods in order to add the reason as a header: `X-GitLab-NotificationReason`. Only the reason with the most priority gets sent. --- app/helpers/emails_helper.rb | 16 +++++++ app/mailers/emails/issues.rb | 33 +++++++------- app/mailers/emails/merge_requests.rb | 37 +++++++-------- app/mailers/notify.rb | 2 + app/models/notification_reason.rb | 19 ++++++++ app/models/notification_recipient.rb | 36 +++++++-------- app/services/notification_recipient_service.rb | 48 +++++++++++--------- app/services/notification_service.rb | 26 ++++++----- app/views/layouts/notify.html.haml | 2 +- app/views/layouts/notify.text.erb | 2 +- changelogs/unreleased/41532-email-reason.yml | 5 +++ doc/workflow/notifications.md | 30 +++++++++++++ spec/mailers/notify_spec.rb | 49 ++++++++++++++++++++ spec/services/notification_service_spec.rb | 62 +++++++++++++++++++++++++- spec/support/email_helpers.rb | 4 ++ spec/workers/new_issue_worker_spec.rb | 5 ++- spec/workers/new_merge_request_worker_spec.rb | 6 ++- 17 files changed, 288 insertions(+), 94 deletions(-) create mode 100644 app/models/notification_reason.rb create mode 100644 changelogs/unreleased/41532-email-reason.yml diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index 878bc9b5c9c..4ddc1dbed49 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -80,4 +80,20 @@ module EmailsHelper 'text-align:center' ].join(';') end + + # "You are receiving this email because #{reason}" + def notification_reason_text(reason) + string = case reason + when NotificationReason::OWN_ACTIVITY + 'of your activity' + when NotificationReason::ASSIGNED + 'you have been assigned an item' + when NotificationReason::MENTIONED + 'you have been mentioned' + else + 'of your account' + end + + "#{string} on #{Gitlab.config.gitlab.host}" + end end diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 64ca2d2eacf..b33131becd3 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -1,54 +1,54 @@ module Emails module Issues - def new_issue_email(recipient_id, issue_id) + def new_issue_email(recipient_id, issue_id, reason = nil) setup_issue_mail(issue_id, recipient_id) - mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) + mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id, reason)) end - def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id) + def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end - def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id) + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) @previous_assignees = [] @previous_assignees = User.where(id: previous_assignee_ids) if previous_assignee_ids.any? - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end - def closed_issue_email(recipient_id, issue_id, updated_by_user_id) + def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) @updated_by = User.find(updated_by_user_id) - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end - def relabeled_issue_email(recipient_id, issue_id, label_names, updated_by_user_id) + def relabeled_issue_email(recipient_id, issue_id, label_names, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) @label_names = label_names @labels_url = project_labels_url(@project) - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end - def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) + def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) @issue_status = status @updated_by = User.find(updated_by_user_id) - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end - def issue_moved_email(recipient, issue, new_issue, updated_by_user) + def issue_moved_email(recipient, issue, new_issue, updated_by_user, reason = nil) setup_issue_mail(issue.id, recipient.id) @new_issue = new_issue @new_project = new_issue.project - mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id)) + mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id, reason)) end private @@ -61,11 +61,12 @@ module Emails @sent_notification = SentNotification.record(@issue, recipient_id, reply_key) end - def issue_thread_options(sender_id, recipient_id) + def issue_thread_options(sender_id, recipient_id, reason) { from: sender(sender_id), to: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})") + subject: subject("#{@issue.title} (##{@issue.iid})"), + 'X-GitLab-NotificationReason' => reason } end end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 3626f8ce416..5fe09cea83f 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -1,57 +1,57 @@ module Emails module MergeRequests - def new_merge_request_email(recipient_id, merge_request_id) + def new_merge_request_email(recipient_id, merge_request_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) - mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id)) + mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason)) end - def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) + def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id) + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id) + def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) @label_names = label_names @labels_url = project_labels_url(@project) - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) + def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) @updated_by = User.find(updated_by_user_id) - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) + def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id) + def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) @mr_status = status @updated_by = User.find(updated_by_user_id) - mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end - def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id) + def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) @resolved_by = User.find(resolved_by_user_id) - mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id)) + mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id, reason)) end private @@ -64,11 +64,12 @@ module Emails @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key) end - def merge_request_thread_options(sender_id, recipient_id) + def merge_request_thread_options(sender_id, recipient_id, reason = nil) { from: sender(sender_id), to: recipient(recipient_id), - subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})") + subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})"), + 'X-GitLab-NotificationReason' => reason } end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index ec886e993c3..eade0fe278f 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -112,6 +112,8 @@ class Notify < BaseMailer headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key + @reason = headers['X-GitLab-NotificationReason'] + if Gitlab::IncomingEmail.enabled? && @sent_notification address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace diff --git a/app/models/notification_reason.rb b/app/models/notification_reason.rb new file mode 100644 index 00000000000..c3965565022 --- /dev/null +++ b/app/models/notification_reason.rb @@ -0,0 +1,19 @@ +# Holds reasons for a notification to have been sent as well as a priority list to select which reason to use +# above the rest +class NotificationReason + OWN_ACTIVITY = 'own_activity'.freeze + ASSIGNED = 'assigned'.freeze + MENTIONED = 'mentioned'.freeze + + # Priority list for selecting which reason to return in the notification + REASON_PRIORITY = [ + OWN_ACTIVITY, + ASSIGNED, + MENTIONED + ].freeze + + # returns the priority of a reason as an integer + def self.priority(reason) + REASON_PRIORITY.index(reason) || REASON_PRIORITY.length + 1 + end +end diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index ab5a96209c7..472b348a545 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -1,27 +1,19 @@ class NotificationRecipient - attr_reader :user, :type - def initialize( - user, type, - custom_action: nil, - target: nil, - acting_user: nil, - project: nil, - group: nil, - skip_read_ability: false - ) - + attr_reader :user, :type, :reason + def initialize(user, type, **opts) unless NotificationSetting.levels.key?(type) || type == :subscription raise ArgumentError, "invalid type: #{type.inspect}" end - @custom_action = custom_action - @acting_user = acting_user - @target = target - @project = project || default_project - @group = group || @project&.group + @custom_action = opts[:custom_action] + @acting_user = opts[:acting_user] + @target = opts[:target] + @project = opts[:project] || default_project + @group = opts[:group] || @project&.group @user = user @type = type - @skip_read_ability = skip_read_ability + @reason = opts[:reason] + @skip_read_ability = opts[:skip_read_ability] end def notification_setting @@ -77,9 +69,15 @@ class NotificationRecipient def own_activity? return false unless @acting_user - return false if @acting_user.notified_of_own_activity? - user == @acting_user + if user == @acting_user + # if activity was generated by the same user, change reason to :own_activity + @reason = NotificationReason::OWN_ACTIVITY + # If the user wants to be notified, we must return `false` + !@acting_user.notified_of_own_activity? + else + false + end end def has_access? diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 3eb8cfcca9b..6835b14648b 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -11,11 +11,11 @@ module NotificationRecipientService end def self.build_recipients(*a) - Builder::Default.new(*a).recipient_users + Builder::Default.new(*a).notification_recipients end def self.build_new_note_recipients(*a) - Builder::NewNote.new(*a).recipient_users + Builder::NewNote.new(*a).notification_recipients end module Builder @@ -49,25 +49,24 @@ module NotificationRecipientService @recipients ||= [] end - def <<(pair) - users, type = pair - + def add_recipients(users, type, reason) if users.is_a?(ActiveRecord::Relation) users = users.includes(:notification_settings) end users = Array(users) users.compact! - recipients.concat(users.map { |u| make_recipient(u, type) }) + recipients.concat(users.map { |u| make_recipient(u, type, reason) }) end def user_scope User.includes(:notification_settings) end - def make_recipient(user, type) + def make_recipient(user, type, reason) NotificationRecipient.new( user, type, + reason: reason, project: project, custom_action: custom_action, target: target, @@ -75,14 +74,13 @@ module NotificationRecipientService ) end - def recipient_users - @recipient_users ||= + def notification_recipients + @notification_recipients ||= begin build! filter! - users = recipients.map(&:user) - users.uniq! - users.freeze + recipients = self.recipients.sort_by { |r| NotificationReason.priority(r.reason) }.uniq(&:user) + recipients.freeze end end @@ -95,13 +93,13 @@ module NotificationRecipientService def add_participants(user) return unless target.respond_to?(:participants) - self << [target.participants(user), :participating] + add_recipients(target.participants(user), :participating, nil) end def add_mentions(user, target:) return unless target.respond_to?(:mentioned_users) - self << [target.mentioned_users(user), :mention] + add_recipients(target.mentioned_users(user), :mention, NotificationReason::MENTIONED) end # Get project/group users with CUSTOM notification level @@ -119,11 +117,11 @@ module NotificationRecipientService global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global) user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action) - self << [user_scope.where(id: user_ids), :watch] + add_recipients(user_scope.where(id: user_ids), :watch, nil) end def add_project_watchers - self << [project_watchers, :watch] + add_recipients(project_watchers, :watch, nil) end # Get project users with WATCH notification level @@ -144,7 +142,7 @@ module NotificationRecipientService def add_subscribed_users return unless target.respond_to? :subscribers - self << [target.subscribers(project), :subscription] + add_recipients(target.subscribers(project), :subscription, nil) end def user_ids_notifiable_on(resource, notification_level = nil) @@ -195,7 +193,7 @@ module NotificationRecipientService return unless target.respond_to? :labels (labels || target.labels).each do |label| - self << [label.subscribers(project), :subscription] + add_recipients(label.subscribers(project), :subscription, nil) end end end @@ -222,12 +220,12 @@ module NotificationRecipientService # Re-assign is considered as a mention of the new assignee case custom_action when :reassign_merge_request - self << [previous_assignee, :mention] - self << [target.assignee, :mention] + add_recipients(previous_assignee, :mention, nil) + add_recipients(target.assignee, :mention, NotificationReason::ASSIGNED) when :reassign_issue previous_assignees = Array(previous_assignee) - self << [previous_assignees, :mention] - self << [target.assignees, :mention] + add_recipients(previous_assignees, :mention, nil) + add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED) end add_subscribed_users @@ -238,6 +236,12 @@ module NotificationRecipientService # receive them, too. add_mentions(current_user, target: target) + # Add the assigned users, if any + assignees = custom_action == :new_issue ? target.assignees : target.assignee + # We use the `:participating` notification level in order to match existing legacy behavior as captured + # in existing specs (notification_service_spec.rb ~ line 507) + add_recipients(assignees, :participating, NotificationReason::ASSIGNED) if assignees + add_labels_subscribers end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index be3b4b2ba07..8c84ccfcc92 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -85,10 +85,11 @@ class NotificationService recipients.each do |recipient| mailer.send( :reassigned_issue_email, - recipient.id, + recipient.user.id, issue.id, previous_assignee_ids, - current_user.id + current_user.id, + recipient.reason ).deliver_later end end @@ -176,7 +177,7 @@ class NotificationService action: "resolve_all_discussions") recipients.each do |recipient| - mailer.resolved_all_discussions_email(recipient.id, merge_request.id, current_user.id).deliver_later + mailer.resolved_all_discussions_email(recipient.user.id, merge_request.id, current_user.id, recipient.reason).deliver_later end end @@ -199,7 +200,7 @@ class NotificationService recipients = NotificationRecipientService.build_new_note_recipients(note) recipients.each do |recipient| - mailer.send(notify_method, recipient.id, note.id).deliver_later + mailer.send(notify_method, recipient.user.id, note.id).deliver_later end end @@ -299,7 +300,7 @@ class NotificationService recipients = NotificationRecipientService.build_recipients(issue, current_user, action: 'moved') recipients.map do |recipient| - email = mailer.issue_moved_email(recipient, issue, new_issue, current_user) + email = mailer.issue_moved_email(recipient.user, issue, new_issue, current_user, recipient.reason) email.deliver_later email end @@ -339,16 +340,16 @@ class NotificationService recipients = NotificationRecipientService.build_recipients(target, target.author, action: "new") recipients.each do |recipient| - mailer.send(method, recipient.id, target.id).deliver_later + mailer.send(method, recipient.user.id, target.id, recipient.reason).deliver_later end end def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method) recipients = NotificationRecipientService.build_recipients(target, current_user, action: "new") - recipients = recipients & new_mentioned_users + recipients = recipients.select {|r| new_mentioned_users.include?(r.user) } recipients.each do |recipient| - mailer.send(method, recipient.id, target.id, current_user.id).deliver_later + mailer.send(method, recipient.user.id, target.id, current_user.id, recipient.reason).deliver_later end end @@ -363,7 +364,7 @@ class NotificationService ) recipients.each do |recipient| - mailer.send(method, recipient.id, target.id, current_user.id).deliver_later + mailer.send(method, recipient.user.id, target.id, current_user.id, recipient.reason).deliver_later end end @@ -381,10 +382,11 @@ class NotificationService recipients.each do |recipient| mailer.send( method, - recipient.id, + recipient.user.id, target.id, previous_assignee_id, - current_user.id + current_user.id, + recipient.reason ).deliver_later end end @@ -408,7 +410,7 @@ class NotificationService recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen") recipients.each do |recipient| - mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later + mailer.send(method, recipient.user.id, target.id, status, current_user.id, recipient.reason).deliver_later end end diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 40bf45cece7..ab8b1271212 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -20,7 +20,7 @@ #{link_to "View it on GitLab", @target_url}. %br -# Don't link the host in the line below, one link in the email is easier to quickly click than two. - You're receiving this email because of your account on #{Gitlab.config.gitlab.host}. + You're receiving this email because #{notification_reason_text(@reason)}. If you'd like to receive fewer emails, you can - if @labels_url adjust your #{link_to 'label subscriptions', @labels_url}. diff --git a/app/views/layouts/notify.text.erb b/app/views/layouts/notify.text.erb index b4ce02eead8..de48f548a1b 100644 --- a/app/views/layouts/notify.text.erb +++ b/app/views/layouts/notify.text.erb @@ -9,4 +9,4 @@ <% end -%> <% end -%> -You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>. +<%= "You're receiving this email because #{notification_reason_text(@reason)}." %> diff --git a/changelogs/unreleased/41532-email-reason.yml b/changelogs/unreleased/41532-email-reason.yml new file mode 100644 index 00000000000..83c28769217 --- /dev/null +++ b/changelogs/unreleased/41532-email-reason.yml @@ -0,0 +1,5 @@ +--- +title: Initial work to add notification reason to emails +merge_request: 16160 +author: Mario de la Ossa +type: added diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 3e2e7d0f7b6..123b37abd19 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -104,3 +104,33 @@ You won't receive notifications for Issues, Merge Requests or Milestones created by yourself. You will only receive automatic notifications when somebody else comments or adds changes to the ones that you've created or mentions you. + +### Email Headers + +Notification emails include headers that provide extra content about the notification received: + +| Header | Description | +|-----------------------------|-------------------------------------------------------------------------| +| X-GitLab-Project | The name of the project the notification belongs to | +| X-GitLab-Project-Id | The ID of the project | +| X-GitLab-Project-Path | The path of the project | +| X-GitLab-(Resource)-ID | The ID of the resource the notification is for, where resource is `Issue`, `MergeRequest`, `Commit`, etc| +| X-GitLab-Discussion-ID | Only in comment emails, the ID of the discussion the comment is from | +| X-GitLab-Pipeline-Id | Only in pipeline emails, the ID of the pipeline the notification is for | +| X-GitLab-Reply-Key | A unique token to support reply by email | +| X-GitLab-NotificationReason | The reason for being notified. "mentioned", "assigned", etc | + +#### X-GitLab-NotificationReason +This header holds the reason for the notification to have been sent out, +where reason can be `mentioned`, `assigned`, `own_activity`, etc. +Only one reason is sent out according to its priority: +- `own_activity` +- `assigned` +- `mentioned` + +The reason in this header will also be shown in the footer of the notification email. For example an email with the +reason `assigned` will have this sentence in the footer: +`"You are receiving this email because you have been assigned an item on {configured GitLab hostname}"` + +**Note: Only reasons listed above have been implemented so far** +Further implementation is [being discussed here](https://gitlab.com/gitlab-org/gitlab-ce/issues/42062) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index cbc8c67da61..7a8e798e3c9 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -71,6 +71,18 @@ describe Notify do is_expected.to have_html_escaped_body_text issue.description end + it 'does not add a reason header' do + is_expected.not_to have_header('X-GitLab-NotificationReason', /.+/) + end + + context 'when sent with a reason' do + subject { described_class.new_issue_email(issue.assignees.first.id, issue.id, NotificationReason::ASSIGNED) } + + it 'includes the reason in a header' do + is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + end + context 'when enabled email_author_in_body' do before do stub_application_setting(email_author_in_body: true) @@ -108,6 +120,14 @@ describe Notify do is_expected.to have_body_text(project_issue_path(project, issue)) end end + + context 'when sent with a reason' do + subject { described_class.reassigned_issue_email(recipient.id, issue.id, [previous_assignee.id], current_user.id, NotificationReason::ASSIGNED) } + + it 'includes the reason in a header' do + is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + end end describe 'that have been relabeled' do @@ -226,6 +246,14 @@ describe Notify do is_expected.to have_html_escaped_body_text merge_request.description end + context 'when sent with a reason' do + subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id, NotificationReason::ASSIGNED) } + + it 'includes the reason in a header' do + is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + end + context 'when enabled email_author_in_body' do before do stub_application_setting(email_author_in_body: true) @@ -263,6 +291,27 @@ describe Notify do is_expected.to have_html_escaped_body_text(assignee.name) end end + + context 'when sent with a reason' do + subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::ASSIGNED) } + + it 'includes the reason in a header' do + is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + + it 'includes the reason in the footer' do + text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::ASSIGNED) + is_expected.to have_body_text(text) + + new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::MENTIONED) + text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::MENTIONED) + expect(new_subject).to have_body_text(text) + + new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, nil) + text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(nil) + expect(new_subject).to have_body_text(text) + end + end end describe 'that have been relabeled' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 43e2643f709..5c59455e3e1 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe NotificationService, :mailer do + include EmailSpec::Matchers + let(:notification) { described_class.new } let(:assignee) { create(:user) } @@ -31,6 +33,14 @@ describe NotificationService, :mailer do send_notifications(@u_disabled) should_not_email_anyone end + + it 'sends the proper notification reason header' do + send_notifications(@u_watcher) + should_only_email(@u_watcher) + email = find_email_for(@u_watcher) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::MENTIONED) + end end # Next shared examples are intended to test notifications of "participants" @@ -511,12 +521,35 @@ describe NotificationService, :mailer do should_not_email(issue.assignees.first) end + it 'properly prioritizes notification reason' do + # have assignee be both assigned and mentioned + issue.update_attribute(:description, "/cc #{assignee.to_reference} #{@u_mentioned.to_reference}") + + notification.new_issue(issue, @u_disabled) + + email = find_email_for(assignee) + expect(email).to have_header('X-GitLab-NotificationReason', 'assigned') + + email = find_email_for(@u_mentioned) + expect(email).to have_header('X-GitLab-NotificationReason', 'mentioned') + end + + it 'adds "assigned" reason for assignees if any' do + notification.new_issue(issue, @u_disabled) + + email = find_email_for(assignee) + + expect(email).to have_header('X-GitLab-NotificationReason', 'assigned') + end + it "emails any mentioned users with the mention level" do issue.description = @u_mentioned.to_reference notification.new_issue(issue, @u_disabled) - should_email(@u_mentioned) + email = find_email_for(@u_mentioned) + expect(email).not_to be_nil + expect(email).to have_header('X-GitLab-NotificationReason', 'mentioned') end it "emails the author if they've opted into notifications about their activity" do @@ -620,6 +653,14 @@ describe NotificationService, :mailer do should_not_email(@u_lazy_participant) end + it 'adds "assigned" reason for new assignee' do + notification.reassigned_issue(issue, @u_disabled, [assignee]) + + email = find_email_for(assignee) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + it 'emails previous assignee even if he has the "on mention" notif level' do issue.assignees = [@u_mentioned] notification.reassigned_issue(issue, @u_disabled, [@u_watcher]) @@ -910,6 +951,14 @@ describe NotificationService, :mailer do should_not_email(@u_lazy_participant) end + it 'adds "assigned" reason for assignee, if any' do + notification.new_merge_request(merge_request, @u_disabled) + + email = find_email_for(merge_request.assignee) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + it "emails any mentioned users with the mention level" do merge_request.description = @u_mentioned.to_reference @@ -924,6 +973,9 @@ describe NotificationService, :mailer do notification.new_merge_request(merge_request, merge_request.author) should_email(merge_request.author) + + email = find_email_for(merge_request.author) + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::OWN_ACTIVITY) end it "doesn't email the author if they haven't opted into notifications about their activity" do @@ -1009,6 +1061,14 @@ describe NotificationService, :mailer do should_not_email(@u_lazy_participant) end + it 'adds "assigned" reason for new assignee' do + notification.reassigned_merge_request(merge_request, merge_request.author) + + email = find_email_for(merge_request.assignee) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end + it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { merge_request } diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb index b39052923dd..1fb8252459f 100644 --- a/spec/support/email_helpers.rb +++ b/spec/support/email_helpers.rb @@ -30,4 +30,8 @@ module EmailHelpers def email_recipients(kind: :to) ActionMailer::Base.deliveries.flat_map(&kind) end + + def find_email_for(user) + ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email) } + end end diff --git a/spec/workers/new_issue_worker_spec.rb b/spec/workers/new_issue_worker_spec.rb index 4e15ccc534b..baa8ddb59e5 100644 --- a/spec/workers/new_issue_worker_spec.rb +++ b/spec/workers/new_issue_worker_spec.rb @@ -44,8 +44,9 @@ describe NewIssueWorker do expect { worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1) end - it 'creates a notification for the assignee' do - expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id).and_return(double(deliver_later: true)) + it 'creates a notification for the mentioned user' do + expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id, NotificationReason::MENTIONED) + .and_return(double(deliver_later: true)) worker.perform(issue.id, user.id) end diff --git a/spec/workers/new_merge_request_worker_spec.rb b/spec/workers/new_merge_request_worker_spec.rb index 9e0cbde45b1..c3f29a40d58 100644 --- a/spec/workers/new_merge_request_worker_spec.rb +++ b/spec/workers/new_merge_request_worker_spec.rb @@ -46,8 +46,10 @@ describe NewMergeRequestWorker do expect { worker.perform(merge_request.id, user.id) }.to change { Event.count }.from(0).to(1) end - it 'creates a notification for the assignee' do - expect(Notify).to receive(:new_merge_request_email).with(mentioned.id, merge_request.id).and_return(double(deliver_later: true)) + it 'creates a notification for the mentioned user' do + expect(Notify).to receive(:new_merge_request_email) + .with(mentioned.id, merge_request.id, NotificationReason::MENTIONED) + .and_return(double(deliver_later: true)) worker.perform(merge_request.id, user.id) end -- cgit v1.2.1 From 9b4a8b499e5c151e34943884b8de275d211b9d20 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 16 Jan 2018 21:01:54 -0800 Subject: Add missing 9.5.10 CHANGELOG entries --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c29c289310d..87260744190 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -992,6 +992,11 @@ entry. - Added type to CHANGELOG entries. (Jacopo Beschi @jacopo-beschi) - [BUGIFX] Improves subgroup creation permissions. !13418 +## 9.5.10 (2017-11-08) + +- [SECURITY] Add SSRF protections for hostnames that will never resolve but will still connect to localhost +- [SECURITY] Include X-Content-Type-Options (XCTO) header into API responses + ## 9.5.9 (2017-10-16) - [SECURITY] Move project repositories between namespaces when renaming users. -- cgit v1.2.1 From 7e9484e9fab052642bc57947ce4610c164da2b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Kadlecova=CC=81?= Date: Fri, 12 Jan 2018 09:39:34 +0100 Subject: Set target_branch to the ref branch when creating MR from issue --- .../merge_requests/create_from_issue_service.rb | 1 + changelogs/unreleased/41727-target-branch-name.yml | 5 +++++ .../merge_requests/create_from_issue_service_spec.rb | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 changelogs/unreleased/41727-target-branch-name.yml diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb index 89dab1dd028..cf687b71d16 100644 --- a/app/services/merge_requests/create_from_issue_service.rb +++ b/app/services/merge_requests/create_from_issue_service.rb @@ -54,6 +54,7 @@ module MergeRequests source_project_id: project.id, source_branch: branch_name, target_project_id: project.id, + target_branch: ref, milestone_id: issue.milestone_id } end diff --git a/changelogs/unreleased/41727-target-branch-name.yml b/changelogs/unreleased/41727-target-branch-name.yml new file mode 100644 index 00000000000..aaedf6f1d12 --- /dev/null +++ b/changelogs/unreleased/41727-target-branch-name.yml @@ -0,0 +1,5 @@ +--- +title: Set target_branch to the ref branch when creating MR from issue +merge_request: +author: +type: fixed diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 623b182b205..75553afc033 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -112,5 +112,24 @@ describe MergeRequests::CreateFromIssueService do expect(result[:merge_request].assignee).to eq(user) end + + context 'when ref branch is set' do + subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'feature').execute } + + it 'sets the merge request source branch to the new issue branch' do + expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) + end + + it 'sets the merge request target branch to the ref branch' do + expect(subject[:merge_request].target_branch).to eq('feature') + end + + context 'when ref branch does not exist' do + it 'does not create a merge request' do + expect { described_class.new(project, user, issue_iid: issue.iid, ref: 'nobr').execute } + .not_to change { project.merge_requests.count } + end + end + end end end -- cgit v1.2.1 From 310d209b67938acdb1cde3cf5622440bf42c5bc4 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Thu, 11 Jan 2018 17:45:35 +0100 Subject: Adds sorting to deployments API Adds sorting to deployments API through the `order_by` and sort `fields`. --- .../41118-add-sorting-to-deployments-api.yml | 5 ++ doc/api/deployments.md | 2 + lib/api/deployments.rb | 4 +- spec/requests/api/deployments_spec.rb | 54 ++++++++++++++++++++-- 4 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/41118-add-sorting-to-deployments-api.yml diff --git a/changelogs/unreleased/41118-add-sorting-to-deployments-api.yml b/changelogs/unreleased/41118-add-sorting-to-deployments-api.yml new file mode 100644 index 00000000000..a08f75f9fb9 --- /dev/null +++ b/changelogs/unreleased/41118-add-sorting-to-deployments-api.yml @@ -0,0 +1,5 @@ +--- +title: Adds sorting to deployments API +merge_request: !16396 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/doc/api/deployments.md b/doc/api/deployments.md index ab9e63e01d3..fd11894ea8f 100644 --- a/doc/api/deployments.md +++ b/doc/api/deployments.md @@ -11,6 +11,8 @@ GET /projects/:id/deployments | Attribute | Type | Required | Description | |-----------|---------|----------|---------------------| | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `order_by`| string | no | Return deployments ordered by `id` or `iid` or `created_at` or `ref` fields. Default is `id` | +| `sort` | string | no | Return deployments sorted in `asc` or `desc` order. Default is `asc` | ```bash curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/deployments" diff --git a/lib/api/deployments.rb b/lib/api/deployments.rb index 1efee9a1324..184fae0eb76 100644 --- a/lib/api/deployments.rb +++ b/lib/api/deployments.rb @@ -15,11 +15,13 @@ module API end params do use :pagination + optional :order_by, type: String, values: %w[id iid created_at ref], default: 'id', desc: 'Return deployments ordered by `id` or `iid` or `created_at` or `ref`' + optional :sort, type: String, values: %w[asc desc], default: 'asc', desc: 'Sort by asc (ascending) or desc (descending)' end get ':id/deployments' do authorize! :read_deployment, user_project - present paginate(user_project.deployments), with: Entities::Deployment + present paginate(user_project.deployments.order(params[:order_by] => params[:sort])), with: Entities::Deployment end desc 'Gets a specific deployment' do diff --git a/spec/requests/api/deployments_spec.rb b/spec/requests/api/deployments_spec.rb index 6732c99e329..51b70fda148 100644 --- a/spec/requests/api/deployments_spec.rb +++ b/spec/requests/api/deployments_spec.rb @@ -3,24 +3,65 @@ require 'spec_helper' describe API::Deployments do let(:user) { create(:user) } let(:non_member) { create(:user) } - let(:project) { deployment.environment.project } - let!(:deployment) { create(:deployment) } before do project.add_master(user) end describe 'GET /projects/:id/deployments' do + let(:project) { create(:project) } + let!(:deployment_1) { create(:deployment, project: project, iid: 11, ref: 'master', created_at: Time.now) } + let!(:deployment_2) { create(:deployment, project: project, iid: 12, ref: 'feature', created_at: 1.day.ago) } + let!(:deployment_3) { create(:deployment, project: project, iid: 8, ref: 'feature', created_at: 2.days.ago) } + context 'as member of the project' do - it 'returns projects deployments' do + it 'returns projects deployments sorted by id asc' do get api("/projects/#{project.id}/deployments", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(json_response.size).to eq(1) - expect(json_response.first['iid']).to eq(deployment.iid) + expect(json_response.size).to eq(3) + expect(json_response.first['iid']).to eq(deployment_1.iid) expect(json_response.first['sha']).to match /\A\h{40}\z/ + expect(json_response.second['iid']).to eq(deployment_2.iid) + expect(json_response.last['iid']).to eq(deployment_3.iid) + end + + describe 'ordering' do + using RSpec::Parameterized::TableSyntax + + let(:order_by) { nil } + let(:sort) { nil } + + subject { get api("/projects/#{project.id}/deployments?order_by=#{order_by}&sort=#{sort}", user) } + + def expect_deployments(ordered_deployments) + json_response.each_with_index do |deployment_json, index| + expect(deployment_json['id']).to eq(public_send(ordered_deployments[index]).id) + end + end + + before do + subject + end + + where(:order_by, :sort, :ordered_deployments) do + 'created_at' | 'asc' | [:deployment_3, :deployment_2, :deployment_1] + 'created_at' | 'desc' | [:deployment_1, :deployment_2, :deployment_3] + 'id' | 'asc' | [:deployment_1, :deployment_2, :deployment_3] + 'id' | 'desc' | [:deployment_3, :deployment_2, :deployment_1] + 'iid' | 'asc' | [:deployment_3, :deployment_1, :deployment_2] + 'iid' | 'desc' | [:deployment_2, :deployment_1, :deployment_3] + 'ref' | 'asc' | [:deployment_2, :deployment_3, :deployment_1] + 'ref' | 'desc' | [:deployment_1, :deployment_2, :deployment_3] + end + + with_them do + it 'returns the deployments ordered' do + expect_deployments(ordered_deployments) + end + end end end @@ -34,6 +75,9 @@ describe API::Deployments do end describe 'GET /projects/:id/deployments/:deployment_id' do + let(:project) { deployment.environment.project } + let!(:deployment) { create(:deployment) } + context 'as a member of the project' do it 'returns the projects deployment' do get api("/projects/#{project.id}/deployments/#{deployment.id}", user) -- cgit v1.2.1 From 88832ba9392d4521b7de003e6d7ea88c59f2d579 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 23 Nov 2017 21:38:25 +0100 Subject: doc for system hook merge_requests_events --- doc/system_hooks/system_hooks.md | 129 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/doc/system_hooks/system_hooks.md b/doc/system_hooks/system_hooks.md index f2a9b1d769b..b49e75fdfc1 100644 --- a/doc/system_hooks/system_hooks.md +++ b/doc/system_hooks/system_hooks.md @@ -445,6 +445,135 @@ X-Gitlab-Event: System Hook "total_commits_count": 0 } ``` + +### Merge request events + +Triggered when a new merge request is created, an existing merge request was +updated/merged/closed or a commit is added in the source branch. + +**Request header**: + +``` +X-Gitlab-Event: System Hook +``` + +```json +{ + "object_kind": "merge_request", + "user": { + "name": "Administrator", + "username": "root", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon" + }, + "project": { + "name": "Example", + "description": "", + "web_url": "http://example.com/jsmith/example", + "avatar_url": null, + "git_ssh_url": "git@example.com:jsmith/example.git", + "git_http_url": "http://example.com/jsmith/example.git", + "namespace": "Jsmith", + "visibility_level": 0, + "path_with_namespace": "jsmith/example", + "default_branch": "master", + "ci_config_path": "", + "homepage": "http://example.com/jsmith/example", + "url": "git@example.com:jsmith/example.git", + "ssh_url": "git@example.com:jsmith/example.git", + "http_url": "http://example.com/jsmith/example.git" + }, + "object_attributes": { + "id": 90, + "target_branch": "master", + "source_branch": "ms-viewport", + "source_project_id": 14, + "author_id": 51, + "assignee_id": 6, + "title": "MS-Viewport", + "created_at": "2017-09-20T08:31:45.944Z", + "updated_at": "2017-09-28T12:23:42.365Z", + "milestone_id": null, + "state": "opened", + "merge_status": "unchecked", + "target_project_id": 14, + "iid": 1, + "description": "", + "updated_by_id": 1, + "merge_error": null, + "merge_params": { + "force_remove_source_branch": "0" + }, + "merge_when_pipeline_succeeds": false, + "merge_user_id": null, + "merge_commit_sha": null, + "deleted_at": null, + "in_progress_merge_commit_sha": null, + "lock_version": 5, + "time_estimate": 0, + "last_edited_at": "2017-09-27T12:43:37.558Z", + "last_edited_by_id": 1, + "head_pipeline_id": 61, + "ref_fetched": true, + "merge_jid": null, + "source": { + "name": "Awesome Project", + "description": "", + "web_url": "http://example.com/awesome_space/awesome_project", + "avatar_url": null, + "git_ssh_url": "git@example.com:awesome_space/awesome_project.git", + "git_http_url": "http://example.com/awesome_space/awesome_project.git", + "namespace": "root", + "visibility_level": 0, + "path_with_namespace": "awesome_space/awesome_project", + "default_branch": "master", + "ci_config_path": "", + "homepage": "http://example.com/awesome_space/awesome_project", + "url": "http://example.com/awesome_space/awesome_project.git", + "ssh_url": "git@example.com:awesome_space/awesome_project.git", + "http_url": "http://example.com/awesome_space/awesome_project.git" + }, + "target": { + "name": "Awesome Project", + "description": "Aut reprehenderit ut est.", + "web_url": "http://example.com/awesome_space/awesome_project", + "avatar_url": null, + "git_ssh_url": "git@example.com:awesome_space/awesome_project.git", + "git_http_url": "http://example.com/awesome_space/awesome_project.git", + "namespace": "Awesome Space", + "visibility_level": 0, + "path_with_namespace": "awesome_space/awesome_project", + "default_branch": "master", + "ci_config_path": "", + "homepage": "http://example.com/awesome_space/awesome_project", + "url": "http://example.com/awesome_space/awesome_project.git", + "ssh_url": "git@example.com:awesome_space/awesome_project.git", + "http_url": "http://example.com/awesome_space/awesome_project.git" + }, + "last_commit": { + "id": "ba3e0d8ff79c80d5b0bbb4f3e2e343e0aaa662b7", + "message": "fixed readme", + "timestamp": "2017-09-26T16:12:57Z", + "url": "http://example.com/awesome_space/awesome_project/commits/da1560886d4f094c3e6c9ef40349f7d38b5d27d7", + "author": { + "name": "GitLab dev user", + "email": "gitlabdev@dv6700.(none)" + } + }, + "work_in_progress": false, + "total_time_spent": 0, + "human_total_time_spent": null, + "human_time_estimate": null + }, + "labels": null, + "repository": { + "name": "git-gpg-test", + "url": "git@example.com:awesome_space/awesome_project.git", + "description": "", + "homepage": "http://example.com/awesome_space/awesome_project" + } +} +``` + ## Repository Update events Triggered only once when you push to the repository (including tags). -- cgit v1.2.1 From ac92d70d9025e1c90bffa99c08bfc4cdb2fc36c9 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 4 Dec 2017 11:18:54 +0100 Subject: extract concern for hook triggers --- app/models/concerns/triggerable_hooks.rb | 31 ++++++++++++++++++++++++++ app/models/hooks/project_hook.rb | 26 ++++++++++----------- app/models/hooks/system_hook.rb | 16 ++++++------- spec/models/concerns/triggerable_hooks_spec.rb | 17 ++++++++++++++ 4 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 app/models/concerns/triggerable_hooks.rb create mode 100644 spec/models/concerns/triggerable_hooks_spec.rb diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb new file mode 100644 index 00000000000..6e02275d552 --- /dev/null +++ b/app/models/concerns/triggerable_hooks.rb @@ -0,0 +1,31 @@ +module TriggerableHooks + AVAILABLE_TRIGGERS = { + repository_update_hooks: :repository_update_events, + push_hooks: :push_events, + tag_push_hooks: :tag_push_events, + issue_hooks: :issues_events, + confidential_issue_hooks: :confidential_issues_events, + note_hooks: :note_events, + merge_request_hooks: :merge_requests_events, + job_hooks: :job_events, + pipeline_hooks: :pipeline_events, + wiki_page_hooks: :wiki_page_events + }.freeze + + extend ActiveSupport::Concern + + class_methods do + attr_reader :triggerable_hooks + + private + + def triggerable_hooks(hooks) + triggers = AVAILABLE_TRIGGERS.slice(*hooks) + const_set('TRIGGERS', triggers) + + self::TRIGGERS.each do |trigger, event| + scope trigger, -> { where(event => true) } + end + end + end +end diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index a8c424a6614..110389d5d86 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -1,19 +1,17 @@ class ProjectHook < WebHook - TRIGGERS = { - push_hooks: :push_events, - tag_push_hooks: :tag_push_events, - issue_hooks: :issues_events, - confidential_issue_hooks: :confidential_issues_events, - note_hooks: :note_events, - merge_request_hooks: :merge_requests_events, - job_hooks: :job_events, - pipeline_hooks: :pipeline_events, - wiki_page_hooks: :wiki_page_events - }.freeze + include TriggerableHooks - TRIGGERS.each do |trigger, event| - scope trigger, -> { where(event => true) } - end + triggerable_hooks only: [ + :push_hooks, + :tag_push_hooks, + :issue_hooks, + :confidential_issue_hooks, + :note_hooks, + :merge_request_hooks, + :job_hooks, + :pipeline_hooks, + :wiki_page_hooks + ] belongs_to :project validates :project, presence: true diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb index 78b053f36c3..d8d8670d5b1 100644 --- a/app/models/hooks/system_hook.rb +++ b/app/models/hooks/system_hook.rb @@ -1,14 +1,12 @@ class SystemHook < WebHook - TRIGGERS = { - repository_update_hooks: :repository_update_events, - push_hooks: :push_events, - tag_push_hooks: :tag_push_events, - merge_request_hooks: :merge_requests_events - }.freeze + include TriggerableHooks - TRIGGERS.each do |trigger, event| - scope trigger, -> { where(event => true) } - end + triggerable_hooks only: [ + :repository_update_hooks, + :push_hooks, + :tag_push_hooks, + :merge_request_hooks + ] default_value_for :push_events, false default_value_for :repository_update_events, true diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb new file mode 100644 index 00000000000..695df5e10aa --- /dev/null +++ b/spec/models/concerns/triggerable_hooks_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +RSpec.describe TriggerableHooks do + before do + class TestableHook < WebHook + include TriggerableHooks + triggerable_hooks only: [:push_hooks] + end + end + + describe 'scopes' do + it 'defines a scope for each of the requested triggers' do + expect(TestableHook).to respond_to :push_hooks + expect(TestableHook).not_to respond_to :tag_push_hooks + end + end +end -- cgit v1.2.1 From a78abf3a0071d1705adc562f23be1f3a347c6db5 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 4 Dec 2017 12:51:39 +0100 Subject: use safer .hooks_for instead of .public_send with .public_send we can't make sure that the scope on the model actually exists. --- app/models/concerns/triggerable_hooks.rb | 6 ++++++ app/models/project.rb | 2 +- app/services/system_hooks_service.rb | 2 +- spec/models/concerns/triggerable_hooks_spec.rb | 18 ++++++++++++++++++ 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb index 6e02275d552..42f40c52d29 100644 --- a/app/models/concerns/triggerable_hooks.rb +++ b/app/models/concerns/triggerable_hooks.rb @@ -17,6 +17,12 @@ module TriggerableHooks class_methods do attr_reader :triggerable_hooks + def hooks_for(trigger) + return none unless self::TRIGGERS.keys.include?(trigger) + + public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend + end + private def triggerable_hooks(hooks) diff --git a/app/models/project.rb b/app/models/project.rb index a8c634200ce..602c6a2795f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -970,7 +970,7 @@ class Project < ActiveRecord::Base def execute_hooks(data, hooks_scope = :push_hooks) run_after_commit_or_now do - hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend + hooks.hooks_for(hooks_scope).each do |hook| hook.async_execute(data, hooks_scope.to_s) end end diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index 690918b4a00..68c4597a133 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -8,7 +8,7 @@ class SystemHooksService end def execute_hooks(data, hooks_scope = :all) - SystemHook.public_send(hooks_scope).find_each do |hook| # rubocop:disable GitlabSecurity/PublicSend + SystemHook.hooks_for(hooks_scope).find_each do |hook| hook.async_execute(data, 'system_hooks') end end diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb index 695df5e10aa..3ddf477e41e 100644 --- a/spec/models/concerns/triggerable_hooks_spec.rb +++ b/spec/models/concerns/triggerable_hooks_spec.rb @@ -14,4 +14,22 @@ RSpec.describe TriggerableHooks do expect(TestableHook).not_to respond_to :tag_push_hooks end end + + describe '.hooks_for' do + context 'the model has the required trigger scope' do + it 'returns the record' do + hook = TestableHook.create!(url: 'http://example.com', push_events: true) + + expect(TestableHook.hooks_for(:push_hooks)).to eq [hook] + end + end + + context 'the model does not have the required trigger scope' do + it 'returns an empty relation' do + TestableHook.create!(url: 'http://example.com', push_events: true) + + expect(TestableHook.hooks_for(:tag_push_hooks)).to eq [] + end + end + end end -- cgit v1.2.1 From a63792b7249d8b49ef9e20ee3bbabd41c22b1ccf Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 4 Dec 2017 15:29:13 +0100 Subject: accepts `.all` as a hook scope as well --- app/models/concerns/triggerable_hooks.rb | 3 ++- spec/models/concerns/triggerable_hooks_spec.rb | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb index 42f40c52d29..4f2c8102204 100644 --- a/app/models/concerns/triggerable_hooks.rb +++ b/app/models/concerns/triggerable_hooks.rb @@ -18,7 +18,8 @@ module TriggerableHooks attr_reader :triggerable_hooks def hooks_for(trigger) - return none unless self::TRIGGERS.keys.include?(trigger) + callable_scopes = self::TRIGGERS.keys + [:all] + return none unless callable_scopes.include?(trigger) public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend end diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb index 3ddf477e41e..bea846454de 100644 --- a/spec/models/concerns/triggerable_hooks_spec.rb +++ b/spec/models/concerns/triggerable_hooks_spec.rb @@ -26,10 +26,18 @@ RSpec.describe TriggerableHooks do context 'the model does not have the required trigger scope' do it 'returns an empty relation' do - TestableHook.create!(url: 'http://example.com', push_events: true) + TestableHook.create!(url: 'http://example.com') expect(TestableHook.hooks_for(:tag_push_hooks)).to eq [] end end + + context 'the stock scope ".all" is accepted' do + it 'returns the record' do + hook = TestableHook.create!(url: 'http://example.com') + + expect(TestableHook.hooks_for(:all)).to eq [hook] + end + end end end -- cgit v1.2.1 From f99b0cc5853f10e07a8ed60caa40c07a4c677d6f Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 4 Dec 2017 15:45:47 +0100 Subject: no need for a named parameter --- app/models/hooks/project_hook.rb | 2 +- app/models/hooks/system_hook.rb | 2 +- spec/models/concerns/triggerable_hooks_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 110389d5d86..b6dd39b860b 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -1,7 +1,7 @@ class ProjectHook < WebHook include TriggerableHooks - triggerable_hooks only: [ + triggerable_hooks [ :push_hooks, :tag_push_hooks, :issue_hooks, diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb index d8d8670d5b1..0528266e5b3 100644 --- a/app/models/hooks/system_hook.rb +++ b/app/models/hooks/system_hook.rb @@ -1,7 +1,7 @@ class SystemHook < WebHook include TriggerableHooks - triggerable_hooks only: [ + triggerable_hooks [ :repository_update_hooks, :push_hooks, :tag_push_hooks, diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb index bea846454de..621d2d38eae 100644 --- a/spec/models/concerns/triggerable_hooks_spec.rb +++ b/spec/models/concerns/triggerable_hooks_spec.rb @@ -4,7 +4,7 @@ RSpec.describe TriggerableHooks do before do class TestableHook < WebHook include TriggerableHooks - triggerable_hooks only: [:push_hooks] + triggerable_hooks [:push_hooks] end end -- cgit v1.2.1 From faadd9e0e4edcb332b41aceabd6c03a796e11978 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Fri, 8 Dec 2017 14:17:15 +0100 Subject: use class reader instead of dynamic constant --- app/models/concerns/triggerable_hooks.rb | 8 +++++--- app/services/test_hooks/base_service.rb | 2 +- app/views/admin/hooks/edit.html.haml | 2 +- app/views/admin/hooks/index.html.haml | 4 ++-- app/views/projects/hooks/edit.html.haml | 2 +- app/views/projects/settings/integrations/_project_hook.html.haml | 4 ++-- 6 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb index 4f2c8102204..ec0ed3b795a 100644 --- a/app/models/concerns/triggerable_hooks.rb +++ b/app/models/concerns/triggerable_hooks.rb @@ -17,8 +17,10 @@ module TriggerableHooks class_methods do attr_reader :triggerable_hooks + attr_reader :triggers + def hooks_for(trigger) - callable_scopes = self::TRIGGERS.keys + [:all] + callable_scopes = triggers.keys + [:all] return none unless callable_scopes.include?(trigger) public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend @@ -28,9 +30,9 @@ module TriggerableHooks def triggerable_hooks(hooks) triggers = AVAILABLE_TRIGGERS.slice(*hooks) - const_set('TRIGGERS', triggers) + @triggers = triggers - self::TRIGGERS.each do |trigger, event| + triggers.each do |trigger, event| scope trigger, -> { where(event => true) } end end diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb index 20d90504bd2..e9aefb1fb75 100644 --- a/app/services/test_hooks/base_service.rb +++ b/app/services/test_hooks/base_service.rb @@ -9,7 +9,7 @@ module TestHooks end def execute - trigger_key = hook.class::TRIGGERS.key(trigger.to_sym) + trigger_key = hook.class.triggers.key(trigger.to_sym) trigger_data_method = "#{trigger}_data" if trigger_key.nil? || !self.respond_to?(trigger_data_method, true) diff --git a/app/views/admin/hooks/edit.html.haml b/app/views/admin/hooks/edit.html.haml index efb15ccc8df..629b1a9940f 100644 --- a/app/views/admin/hooks/edit.html.haml +++ b/app/views/admin/hooks/edit.html.haml @@ -13,7 +13,7 @@ = render partial: 'form', locals: { form: f, hook: @hook } .form-actions = f.submit 'Save changes', class: 'btn btn-create' - = render 'shared/web_hooks/test_button', triggers: SystemHook::TRIGGERS, hook: @hook + = render 'shared/web_hooks/test_button', triggers: SystemHook.triggers, hook: @hook = link_to 'Remove', admin_hook_path(@hook), method: :delete, class: 'btn btn-remove pull-right', data: { confirm: 'Are you sure?' } %hr diff --git a/app/views/admin/hooks/index.html.haml b/app/views/admin/hooks/index.html.haml index b6e1df5f3ac..bc02d9969d6 100644 --- a/app/views/admin/hooks/index.html.haml +++ b/app/views/admin/hooks/index.html.haml @@ -22,12 +22,12 @@ - @hooks.each do |hook| %li .controls - = render 'shared/web_hooks/test_button', triggers: SystemHook::TRIGGERS, hook: hook, button_class: 'btn-sm' + = render 'shared/web_hooks/test_button', triggers: SystemHook.triggers, hook: hook, button_class: 'btn-sm' = link_to 'Edit', edit_admin_hook_path(hook), class: 'btn btn-sm' = link_to 'Remove', admin_hook_path(hook), data: { confirm: 'Are you sure?' }, method: :delete, class: 'btn btn-remove btn-sm' .monospace= hook.url %div - - SystemHook::TRIGGERS.each_value do |event| + - SystemHook.triggers.each_value do |event| - if hook.public_send(event) %span.label.label-gray= event.to_s.titleize %span.label.label-gray SSL Verification: #{hook.enable_ssl_verification ? 'enabled' : 'disabled'} diff --git a/app/views/projects/hooks/edit.html.haml b/app/views/projects/hooks/edit.html.haml index b1219f019d7..dcc1f0e3fbe 100644 --- a/app/views/projects/hooks/edit.html.haml +++ b/app/views/projects/hooks/edit.html.haml @@ -12,7 +12,7 @@ = render partial: 'shared/web_hooks/form', locals: { form: f, hook: @hook } = f.submit 'Save changes', class: 'btn btn-create' - = render 'shared/web_hooks/test_button', triggers: ProjectHook::TRIGGERS, hook: @hook + = render 'shared/web_hooks/test_button', triggers: ProjectHook.triggers, hook: @hook = link_to 'Remove', project_hook_path(@project, @hook), method: :delete, class: 'btn btn-remove pull-right', data: { confirm: 'Are you sure?' } %hr diff --git a/app/views/projects/settings/integrations/_project_hook.html.haml b/app/views/projects/settings/integrations/_project_hook.html.haml index 82516cb4bcf..cd003107d66 100644 --- a/app/views/projects/settings/integrations/_project_hook.html.haml +++ b/app/views/projects/settings/integrations/_project_hook.html.haml @@ -3,14 +3,14 @@ .col-md-8.col-lg-7 %strong.light-header= hook.url %div - - ProjectHook::TRIGGERS.each_value do |event| + - ProjectHook.triggers.each_value do |event| - if hook.public_send(event) %span.label.label-gray.deploy-project-label= event.to_s.titleize .col-md-4.col-lg-5.text-right-lg.prepend-top-5 %span.append-right-10.inline SSL Verification: #{hook.enable_ssl_verification ? 'enabled' : 'disabled'} = link_to 'Edit', edit_project_hook_path(@project, hook), class: 'btn btn-sm' - = render 'shared/web_hooks/test_button', triggers: ProjectHook::TRIGGERS, hook: hook, button_class: 'btn-sm' + = render 'shared/web_hooks/test_button', triggers: ProjectHook.triggers, hook: hook, button_class: 'btn-sm' = link_to project_hook_path(@project, hook), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-transparent' do %span.sr-only Remove = icon('trash') -- cgit v1.2.1 From eb5e0e921c017322ad8f9122d897b0cf39c0b294 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 18 Dec 2017 10:58:18 +0100 Subject: add missing permitted param --- app/controllers/admin/hooks_controller.rb | 1 + spec/controllers/admin/hooks_controller_spec.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb index 77e3c95d197..1c90c298f66 100644 --- a/app/controllers/admin/hooks_controller.rb +++ b/app/controllers/admin/hooks_controller.rb @@ -62,6 +62,7 @@ class Admin::HooksController < Admin::ApplicationController :push_events, :tag_push_events, :repository_update_events, + :merge_requests_events, :token, :url ) diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb index e6ba596117a..090a51be7c3 100644 --- a/spec/controllers/admin/hooks_controller_spec.rb +++ b/spec/controllers/admin/hooks_controller_spec.rb @@ -14,6 +14,7 @@ describe Admin::HooksController do push_events: true, tag_push_events: true, repository_update_events: true, + merge_requests_events: true, token: "TEST TOKEN", url: "http://example.com" } -- cgit v1.2.1 From 337ced28bcdff25a2d2d4c726d37c782c4cc2d24 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 18 Dec 2017 11:14:51 +0100 Subject: reference triggers.values for permitted parameters --- app/controllers/admin/hooks_controller.rb | 7 ++---- app/controllers/projects/hooks_controller.rb | 10 +-------- spec/controllers/admin/hooks_controller_spec.rb | 7 +++--- spec/controllers/projects/hooks_controller_spec.rb | 26 ++++++++++++++++++++++ 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb index 1c90c298f66..2b47819303e 100644 --- a/app/controllers/admin/hooks_controller.rb +++ b/app/controllers/admin/hooks_controller.rb @@ -59,12 +59,9 @@ class Admin::HooksController < Admin::ApplicationController def hook_params params.require(:hook).permit( :enable_ssl_verification, - :push_events, - :tag_push_events, - :repository_update_events, - :merge_requests_events, :token, - :url + :url, + *SystemHook.triggers.values ) end end diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb index 85d35900c71..b83d6543096 100644 --- a/app/controllers/projects/hooks_controller.rb +++ b/app/controllers/projects/hooks_controller.rb @@ -63,18 +63,10 @@ class Projects::HooksController < Projects::ApplicationController def hook_params params.require(:hook).permit( - :job_events, - :pipeline_events, :enable_ssl_verification, - :issues_events, - :confidential_issues_events, - :merge_requests_events, - :note_events, - :push_events, - :tag_push_events, :token, :url, - :wiki_page_events + *ProjectHook.triggers.values ) end end diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb index 090a51be7c3..d2c1e634930 100644 --- a/spec/controllers/admin/hooks_controller_spec.rb +++ b/spec/controllers/admin/hooks_controller_spec.rb @@ -11,12 +11,13 @@ describe Admin::HooksController do it 'sets all parameters' do hook_params = { enable_ssl_verification: true, + token: "TEST TOKEN", + url: "http://example.com", + push_events: true, tag_push_events: true, repository_update_events: true, - merge_requests_events: true, - token: "TEST TOKEN", - url: "http://example.com" + merge_requests_events: true } post :create, hook: hook_params diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb index aba70c6d4c1..2d473d5bf52 100644 --- a/spec/controllers/projects/hooks_controller_spec.rb +++ b/spec/controllers/projects/hooks_controller_spec.rb @@ -18,4 +18,30 @@ describe Projects::HooksController do ) end end + + describe '#create' do + it 'sets all parameters' do + hook_params = { + enable_ssl_verification: true, + token: "TEST TOKEN", + url: "http://example.com", + + push_events: true, + tag_push_events: true, + merge_requests_events: true, + issues_events: true, + confidential_issues_events: true, + note_events: true, + job_events: true, + pipeline_events: true, + wiki_page_events: true + } + + post :create, namespace_id: project.namespace, project_id: project, hook: hook_params + + expect(response).to have_http_status(302) + expect(ProjectHook.all.size).to eq(1) + expect(ProjectHook.first).to have_attributes(hook_params) + end + end end -- cgit v1.2.1 From 6ef1b94ccf9eb60d5cff43d7e1302129d1b43cc3 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 18 Dec 2017 15:18:40 +0100 Subject: accept_confirm requires js driver --- spec/features/admin/admin_hooks_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb index 725e603d1e3..f266f2ecc54 100644 --- a/spec/features/admin/admin_hooks_spec.rb +++ b/spec/features/admin/admin_hooks_spec.rb @@ -62,7 +62,7 @@ describe 'Admin::Hooks' do end end - describe 'Remove existing hook' do + describe 'Remove existing hook', :js do before do create(:system_hook) end -- cgit v1.2.1 From 83ab63de34f1b3af1202808eb309105ed586ada0 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 17 Jan 2018 09:21:37 +0000 Subject: fixed conflicts --- .../javascripts/deploy_keys/components/key.vue | 23 +++---------- .../javascripts/notebook/cells/output/html.vue | 38 ++++++++-------------- 2 files changed, 17 insertions(+), 44 deletions(-) diff --git a/app/assets/javascripts/deploy_keys/components/key.vue b/app/assets/javascripts/deploy_keys/components/key.vue index 8211ba280c5..843564ce016 100644 --- a/app/assets/javascripts/deploy_keys/components/key.vue +++ b/app/assets/javascripts/deploy_keys/components/key.vue @@ -1,15 +1,15 @@