From 133b3f20fc60a9294eede7c4233c5272566e580e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 8 Feb 2018 22:34:56 +0800 Subject: [PATCH 01/26] Allow using a different sandbox group for QA --- qa/README.md | 8 ++++++++ qa/qa/runtime/namespace.rb | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/qa/README.md b/qa/README.md index 3c1b61900d9..29720668251 100644 --- a/qa/README.md +++ b/qa/README.md @@ -70,6 +70,14 @@ If you need to authenticate as a different user, you can provide the GITLAB_USERNAME=jsmith GITLAB_PASSWORD=password bin/qa Test::Instance https://gitlab.example.com ``` +If your user doesn't have permission to default sandbox group +`gitlab-qa-sandbox`, you could also use another sandbox group by giving +`GITLAB_SANDBOX_NAME`: + +``` +GITLAB_USERNAME=jsmith GITLAB_PASSWORD=password GITLAB_SANDBOX_NAME=jsmith-qa-sandbox bin/qa Test::Instance https://gitlab.example.com +``` + All [supported environment variables are here](https://gitlab.com/gitlab-org/gitlab-qa#supported-environment-variables). [GDK]: https://gitlab.com/gitlab-org/gitlab-development-kit/ diff --git a/qa/qa/runtime/namespace.rb b/qa/qa/runtime/namespace.rb index a72c2d21898..2ff5686c78d 100644 --- a/qa/qa/runtime/namespace.rb +++ b/qa/qa/runtime/namespace.rb @@ -16,7 +16,7 @@ module QA end def sandbox_name - 'gitlab-qa-sandbox' + ENV['GITLAB_SANDBOX_NAME'] || 'gitlab-qa-sandbox' end end end From f2ffb398d602c55f8cbd3c501adea97bb6682e65 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 8 Feb 2018 15:52:38 +0000 Subject: [PATCH 02/26] Update jquery.waitforimages & use npm version #39072 --- app/assets/javascripts/commit/image_file.js | 1 - app/assets/javascripts/commons/jquery.js | 2 +- app/assets/javascripts/issue.js | 1 - app/assets/javascripts/merge_request.js | 2 - package.json | 1 + .../javascripts/jquery.waitforimages.js | 144 ------------------ yarn.lock | 4 + 7 files changed, 6 insertions(+), 149 deletions(-) delete mode 100644 vendor/assets/javascripts/jquery.waitforimages.js diff --git a/app/assets/javascripts/commit/image_file.js b/app/assets/javascripts/commit/image_file.js index 525fbf9dac9..6504a0bbbfc 100644 --- a/app/assets/javascripts/commit/image_file.js +++ b/app/assets/javascripts/commit/image_file.js @@ -1,5 +1,4 @@ /* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-use-before-define, prefer-arrow-callback, no-else-return, consistent-return, prefer-template, quotes, one-var, one-var-declaration-per-line, no-unused-vars, no-return-assign, comma-dangle, quote-props, no-unused-expressions, no-sequences, object-shorthand, max-len */ -import 'vendor/jquery.waitforimages'; // Width where images must fits in, for 2-up this gets divided by 2 const availWidth = 900; diff --git a/app/assets/javascripts/commons/jquery.js b/app/assets/javascripts/commons/jquery.js index b93e94a3c97..a7ed175f7a4 100644 --- a/app/assets/javascripts/commons/jquery.js +++ b/app/assets/javascripts/commons/jquery.js @@ -6,5 +6,5 @@ import 'vendor/jquery.endless-scroll'; import 'vendor/jquery.caret'; import 'vendor/jquery.atwho'; import 'vendor/jquery.scrollTo'; -import 'vendor/jquery.waitforimages'; +import 'jquery.waitforimages'; import 'select2/select2'; diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index ff65ea99e9a..e3d90a24185 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -1,5 +1,4 @@ /* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, one-var, no-underscore-dangle, one-var-declaration-per-line, object-shorthand, no-unused-vars, no-new, comma-dangle, consistent-return, quotes, dot-notation, quote-props, prefer-arrow-callback, max-len */ -import 'vendor/jquery.waitforimages'; import axios from './lib/utils/axios_utils'; import { addDelimiter } from './lib/utils/text_utility'; import flash from './flash'; diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js index bedd50de1bb..a64093afcf4 100644 --- a/app/assets/javascripts/merge_request.js +++ b/app/assets/javascripts/merge_request.js @@ -1,6 +1,4 @@ /* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, quotes, no-underscore-dangle, one-var, one-var-declaration-per-line, consistent-return, dot-notation, quote-props, comma-dangle, object-shorthand, max-len, prefer-arrow-callback */ - -import 'vendor/jquery.waitforimages'; import { __ } from '~/locale'; import TaskList from './task_list'; import MergeRequestTabs from './merge_request_tabs'; diff --git a/package.json b/package.json index c508a6e9931..75d11afbbe1 100644 --- a/package.json +++ b/package.json @@ -52,6 +52,7 @@ "jed": "^1.1.1", "jquery": "^2.2.4", "jquery-ujs": "1.2.2", + "jquery.waitforimages": "^2.2.0", "js-cookie": "^2.1.3", "jszip": "^3.1.3", "jszip-utils": "^0.0.2", diff --git a/vendor/assets/javascripts/jquery.waitforimages.js b/vendor/assets/javascripts/jquery.waitforimages.js deleted file mode 100644 index 95b39c2e074..00000000000 --- a/vendor/assets/javascripts/jquery.waitforimages.js +++ /dev/null @@ -1,144 +0,0 @@ -/* - * waitForImages 1.4 - * ----------------- - * Provides a callback when all images have loaded in your given selector. - * http://www.alexanderdickson.com/ - * - * - * Copyright (c) 2011 Alex Dickson - * Licensed under the MIT licenses. - * See website for more info. - * - */ - -;(function($) { - // Namespace all events. - var eventNamespace = 'waitForImages'; - - // CSS properties which contain references to images. - $.waitForImages = { - hasImageProperties: [ - 'backgroundImage', - 'listStyleImage', - 'borderImage', - 'borderCornerImage' - ] - }; - - // Custom selector to find `img` elements that have a valid `src` attribute and have not already loaded. - $.expr[':'].uncached = function(obj) { - // Ensure we are dealing with an `img` element with a valid `src` attribute. - if ( ! $(obj).is('img[src!=""]')) { - return false; - } - - // Firefox's `complete` property will always be`true` even if the image has not been downloaded. - // Doing it this way works in Firefox. - var img = document.createElement('img'); - img.src = obj.src; - return ! img.complete; - }; - - $.fn.waitForImages = function(finishedCallback, eachCallback, waitForAll) { - - // Handle options object. - if ($.isPlainObject(arguments[0])) { - eachCallback = finishedCallback.each; - waitForAll = finishedCallback.waitForAll; - finishedCallback = finishedCallback.finished; - } - - // Handle missing callbacks. - finishedCallback = finishedCallback || $.noop; - eachCallback = eachCallback || $.noop; - - // Convert waitForAll to Boolean - waitForAll = !! waitForAll; - - // Ensure callbacks are functions. - if (!$.isFunction(finishedCallback) || !$.isFunction(eachCallback)) { - throw new TypeError('An invalid callback was supplied.'); - }; - - return this.each(function() { - // Build a list of all imgs, dependent on what images will be considered. - var obj = $(this), - allImgs = []; - - if (waitForAll) { - // CSS properties which may contain an image. - var hasImgProperties = $.waitForImages.hasImageProperties || [], - matchUrl = /url\((['"]?)(.*?)\1\)/g; - - // Get all elements, as any one of them could have a background image. - obj.find('*').each(function() { - var element = $(this); - - // If an `img` element, add it. But keep iterating in case it has a background image too. - if (element.is('img:uncached')) { - allImgs.push({ - src: element.attr('src'), - element: element[0] - }); - } - - $.each(hasImgProperties, function(i, property) { - var propertyValue = element.css(property); - // If it doesn't contain this property, skip. - if ( ! propertyValue) { - return true; - } - - // Get all url() of this element. - var match; - while (match = matchUrl.exec(propertyValue)) { - allImgs.push({ - src: match[2], - element: element[0] - }); - }; - }); - }); - } else { - // For images only, the task is simpler. - obj - .find('img:uncached') - .each(function() { - allImgs.push({ - src: this.src, - element: this - }); - }); - }; - - var allImgsLength = allImgs.length, - allImgsLoaded = 0; - - // If no images found, don't bother. - if (allImgsLength == 0) { - finishedCallback.call(obj[0]); - }; - - $.each(allImgs, function(i, img) { - - var image = new Image; - - // Handle the image loading and error with the same callback. - $(image).bind('load.' + eventNamespace + ' error.' + eventNamespace, function(event) { - allImgsLoaded++; - - // If an error occurred with loading the image, set the third argument accordingly. - eachCallback.call(img.element, allImgsLoaded, allImgsLength, event.type == 'load'); - - if (allImgsLoaded == allImgsLength) { - finishedCallback.call(obj[0]); - return false; - }; - - }); - - image.src = img.src; - }); - }); - }; -})(jQuery); diff --git a/yarn.lock b/yarn.lock index bc5c19464fb..8e86121812d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4203,6 +4203,10 @@ jquery-ujs@1.2.2: dependencies: jquery ">=1.8.0" +jquery.waitforimages@^2.2.0: + version "2.2.0" + resolved "https://registry.yarnpkg.com/jquery.waitforimages/-/jquery.waitforimages-2.2.0.tgz#63f23131055a1b060dc913e6d874bcc9b9e6b16b" + "jquery@>= 1.9.1", jquery@>=1.8.0, jquery@^2.2.4: version "2.2.4" resolved "https://registry.yarnpkg.com/jquery/-/jquery-2.2.4.tgz#2c89d6889b5eac522a7eea32c14521559c6cbf02" From fe0b3095c50db00da2edaed355dfaeb769048680 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 8 Feb 2018 18:26:57 +0000 Subject: [PATCH 03/26] Moves missing branch into a vue file --- .../states/mr_widget_missing_branch.js | 43 ------------- .../states/mr_widget_missing_branch.vue | 63 +++++++++++++++++++ .../vue_merge_request_widget/dependencies.js | 2 +- .../states/mr_widget_missing_branch_spec.js | 34 +++------- 4 files changed, 73 insertions(+), 69 deletions(-) delete mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.js create mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.js deleted file mode 100644 index 7733fb74afe..00000000000 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.js +++ /dev/null @@ -1,43 +0,0 @@ -import statusIcon from '../mr_widget_status_icon.vue'; -import tooltip from '../../../vue_shared/directives/tooltip'; -import mrWidgetMergeHelp from '../../components/mr_widget_merge_help.vue'; - -export default { - name: 'MRWidgetMissingBranch', - props: { - mr: { type: Object, required: true }, - }, - directives: { - tooltip, - }, - components: { - 'mr-widget-merge-help': mrWidgetMergeHelp, - statusIcon, - }, - computed: { - missingBranchName() { - return this.mr.sourceBranchRemoved ? 'source' : 'target'; - }, - message() { - return `If the ${this.missingBranchName} branch exists in your local repository, you can merge this merge request manually using the command line`; - }, - }, - template: ` -
- -
- - - {{missingBranchName}} - branch does not exist. - Please restore it or use a different {{missingBranchName}} branch - - -
-
- `, -}; diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue new file mode 100644 index 00000000000..94eca0e9288 --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue @@ -0,0 +1,63 @@ + + diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index 7ca15537719..5517888c3b1 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -24,7 +24,7 @@ export { default as WipState } from './components/states/mr_widget_wip'; export { default as ArchivedState } from './components/states/mr_widget_archived.vue'; export { default as ConflictsState } from './components/states/mr_widget_conflicts.vue'; export { default as NothingToMergeState } from './components/states/mr_widget_nothing_to_merge'; -export { default as MissingBranchState } from './components/states/mr_widget_missing_branch'; +export { default as MissingBranchState } from './components/states/mr_widget_missing_branch.vue'; export { default as NotAllowedState } from './components/states/mr_widget_not_allowed'; export { default as ReadyToMergeState } from './components/states/mr_widget_ready_to_merge'; export { default as SHAMismatchState } from './components/states/mr_widget_sha_mismatch'; diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js index 720effb5c1c..3d7f4abd420 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js @@ -1,38 +1,22 @@ import Vue from 'vue'; -import missingBranchComponent from '~/vue_merge_request_widget/components/states/mr_widget_missing_branch'; - -const createComponent = () => { - const Component = Vue.extend(missingBranchComponent); - const mr = { - sourceBranchRemoved: true, - }; - - return new Component({ - el: document.createElement('div'), - propsData: { mr }, - }); -}; +import missingBranchComponent from '~/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; describe('MRWidgetMissingBranch', () => { - describe('props', () => { - it('should have props', () => { - const mrProp = missingBranchComponent.props.mr; + let vm; - expect(mrProp.type instanceof Object).toBeTruthy(); - expect(mrProp.required).toBeTruthy(); - }); + beforeEach(() => { + const Component = Vue.extend(missingBranchComponent); + vm = mountComponent(Component, { mr: { sourceBranchRemoved: true } }); }); - describe('components', () => { - it('should have components added', () => { - expect(missingBranchComponent.components['mr-widget-merge-help']).toBeDefined(); - }); + afterEach(() => { + vm.$destroy(); }); describe('computed', () => { describe('missingBranchName', () => { it('should return proper branch name', () => { - const vm = createComponent(); expect(vm.missingBranchName).toEqual('source'); vm.mr.sourceBranchRemoved = false; @@ -43,7 +27,7 @@ describe('MRWidgetMissingBranch', () => { describe('template', () => { it('should have correct elements', () => { - const el = createComponent().$el; + const el = vm.$el; const content = el.textContent.replace(/\n(\s)+/g, ' ').trim(); expect(el.classList.contains('mr-widget-body')).toBeTruthy(); From 469148b23184a1dd2d5ce6ceedcce48c2b02a7d1 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Thu, 8 Feb 2018 21:06:18 +0200 Subject: [PATCH 04/26] Update vue component naming guidelines --- .../unreleased/docs-update-vue-naming-guidelines.yml | 5 +++++ doc/development/fe_guide/style_guide_js.md | 10 +++++----- 2 files changed, 10 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/docs-update-vue-naming-guidelines.yml diff --git a/changelogs/unreleased/docs-update-vue-naming-guidelines.yml b/changelogs/unreleased/docs-update-vue-naming-guidelines.yml new file mode 100644 index 00000000000..95bfd212370 --- /dev/null +++ b/changelogs/unreleased/docs-update-vue-naming-guidelines.yml @@ -0,0 +1,5 @@ +--- +title: Update vue component naming guidelines +merge_request: 17018 +author: George Tsiolis +type: other diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 02773162801..cd26baa4243 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -302,20 +302,20 @@ Please check this [rules][eslint-plugin-vue-rules] for more documentation. #### Naming 1. **Extensions**: Use `.vue` extension for Vue components. -1. **Reference Naming**: Use camelCase for their instances: +1. **Reference Naming**: Use PascalCase for their instances: ```javascript // bad - import CardBoard from 'cardBoard' + import cardBoard from 'cardBoard.vue' components: { - CardBoard: + cardBoard, }; // good - import cardBoard from 'cardBoard' + import CardBoard from 'cardBoard.vue' components: { - cardBoard: + CardBoard, }; ``` From 4e72e4563c4425b39714f068e3f94f1a6f7ed26f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 8 Feb 2018 23:07:28 -0600 Subject: [PATCH 05/26] Rename .scss files to use snake_case See https://gitlab.com/gitlab-org/gitlab-ce/issues/42908 --- app/assets/stylesheets/framework.scss | 14 +++++++------- ...dcast-messages.scss => broadcast_messages.scss} | 0 ...extual-sidebar.scss => contextual_sidebar.scss} | 0 .../{emoji-sprites.scss => emoji_sprites.scss} | 0 .../{gitlab-theme.scss => gitlab_theme.scss} | 0 .../{page-header.scss => page_header.scss} | 0 ...nts.scss => secondary_navigation_elements.scss} | 0 ...progress-bar.scss => stacked_progress_bar.scss} | 0 lib/tasks/gemojione.rake | 2 +- 9 files changed, 8 insertions(+), 8 deletions(-) rename app/assets/stylesheets/framework/{broadcast-messages.scss => broadcast_messages.scss} (100%) rename app/assets/stylesheets/framework/{contextual-sidebar.scss => contextual_sidebar.scss} (100%) rename app/assets/stylesheets/framework/{emoji-sprites.scss => emoji_sprites.scss} (100%) rename app/assets/stylesheets/framework/{gitlab-theme.scss => gitlab_theme.scss} (100%) rename app/assets/stylesheets/framework/{page-header.scss => page_header.scss} (100%) rename app/assets/stylesheets/framework/{secondary-navigation-elements.scss => secondary_navigation_elements.scss} (100%) rename app/assets/stylesheets/framework/{stacked-progress-bar.scss => stacked_progress_bar.scss} (100%) diff --git a/app/assets/stylesheets/framework.scss b/app/assets/stylesheets/framework.scss index 887879ab715..2fccfa4011c 100644 --- a/app/assets/stylesheets/framework.scss +++ b/app/assets/stylesheets/framework.scss @@ -21,7 +21,7 @@ @import "framework/flash"; @import "framework/forms"; @import "framework/gfm"; -@import "framework/gitlab-theme"; +@import "framework/gitlab_theme"; @import "framework/header"; @import "framework/highlight"; @import "framework/issue_box"; @@ -35,10 +35,10 @@ @import "framework/pagination"; @import "framework/panels"; @import "framework/popup"; -@import "framework/secondary-navigation-elements"; +@import "framework/secondary_navigation_elements"; @import "framework/selects"; @import "framework/sidebar"; -@import "framework/contextual-sidebar"; +@import "framework/contextual_sidebar"; @import "framework/tables"; @import "framework/notes"; @import "framework/tabs"; @@ -49,16 +49,16 @@ @import "framework/zen"; @import "framework/blank"; @import "framework/wells"; -@import "framework/page-header"; +@import "framework/page_header"; @import "framework/awards"; @import "framework/images"; -@import "framework/broadcast-messages"; +@import "framework/broadcast_messages"; @import "framework/emojis"; -@import "framework/emoji-sprites"; +@import "framework/emoji_sprites"; @import "framework/icons"; @import "framework/snippets"; @import "framework/memory_graph"; @import "framework/responsive_tables"; -@import "framework/stacked-progress-bar"; +@import "framework/stacked_progress_bar"; @import "framework/ci_variable_list"; @import "framework/feature_highlight"; diff --git a/app/assets/stylesheets/framework/broadcast-messages.scss b/app/assets/stylesheets/framework/broadcast_messages.scss similarity index 100% rename from app/assets/stylesheets/framework/broadcast-messages.scss rename to app/assets/stylesheets/framework/broadcast_messages.scss diff --git a/app/assets/stylesheets/framework/contextual-sidebar.scss b/app/assets/stylesheets/framework/contextual_sidebar.scss similarity index 100% rename from app/assets/stylesheets/framework/contextual-sidebar.scss rename to app/assets/stylesheets/framework/contextual_sidebar.scss diff --git a/app/assets/stylesheets/framework/emoji-sprites.scss b/app/assets/stylesheets/framework/emoji_sprites.scss similarity index 100% rename from app/assets/stylesheets/framework/emoji-sprites.scss rename to app/assets/stylesheets/framework/emoji_sprites.scss diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab_theme.scss similarity index 100% rename from app/assets/stylesheets/framework/gitlab-theme.scss rename to app/assets/stylesheets/framework/gitlab_theme.scss diff --git a/app/assets/stylesheets/framework/page-header.scss b/app/assets/stylesheets/framework/page_header.scss similarity index 100% rename from app/assets/stylesheets/framework/page-header.scss rename to app/assets/stylesheets/framework/page_header.scss diff --git a/app/assets/stylesheets/framework/secondary-navigation-elements.scss b/app/assets/stylesheets/framework/secondary_navigation_elements.scss similarity index 100% rename from app/assets/stylesheets/framework/secondary-navigation-elements.scss rename to app/assets/stylesheets/framework/secondary_navigation_elements.scss diff --git a/app/assets/stylesheets/framework/stacked-progress-bar.scss b/app/assets/stylesheets/framework/stacked_progress_bar.scss similarity index 100% rename from app/assets/stylesheets/framework/stacked-progress-bar.scss rename to app/assets/stylesheets/framework/stacked_progress_bar.scss diff --git a/lib/tasks/gemojione.rake b/lib/tasks/gemojione.rake index c2d3a6b6950..c6942d22926 100644 --- a/lib/tasks/gemojione.rake +++ b/lib/tasks/gemojione.rake @@ -115,7 +115,7 @@ namespace :gemojione do end end - style_path = Rails.root.join(*%w(app assets stylesheets framework emoji-sprites.scss)) + style_path = Rails.root.join(*%w(app assets stylesheets framework emoji_sprites.scss)) # Combine the resized assets into a packed sprite and re-generate the SCSS SpriteFactory.cssurl = "image-url('$IMAGE')" From 5f62a935c3522517bffb6f4e0f48271dd81a9f39 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 9 Feb 2018 17:25:43 +0800 Subject: [PATCH 06/26] Move all ENV to Runtime::Env --- qa/qa/runtime/env.rb | 12 ++++++++++++ qa/qa/runtime/namespace.rb | 2 +- qa/qa/runtime/user.rb | 4 ++-- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/qa/qa/runtime/env.rb b/qa/qa/runtime/env.rb index 56944e8b641..115d5ee58d1 100644 --- a/qa/qa/runtime/env.rb +++ b/qa/qa/runtime/env.rb @@ -16,6 +16,18 @@ module QA def personal_access_token ENV['PERSONAL_ACCESS_TOKEN'] end + + def user_username + ENV['GITLAB_USERNAME'] + end + + def user_password + ENV['GITLAB_PASSWORD'] + end + + def sandbox_name + ENV['GITLAB_SANDBOX_NAME'] + end end end end diff --git a/qa/qa/runtime/namespace.rb b/qa/qa/runtime/namespace.rb index 2ff5686c78d..8d05b387416 100644 --- a/qa/qa/runtime/namespace.rb +++ b/qa/qa/runtime/namespace.rb @@ -16,7 +16,7 @@ module QA end def sandbox_name - ENV['GITLAB_SANDBOX_NAME'] || 'gitlab-qa-sandbox' + Runtime::Env.sandbox_name || 'gitlab-qa-sandbox' end end end diff --git a/qa/qa/runtime/user.rb b/qa/qa/runtime/user.rb index 60027c89ab1..6b377f6b287 100644 --- a/qa/qa/runtime/user.rb +++ b/qa/qa/runtime/user.rb @@ -4,11 +4,11 @@ module QA extend self def name - ENV['GITLAB_USERNAME'] || 'root' + Runtime::Env.user_username || 'root' end def password - ENV['GITLAB_PASSWORD'] || '5iveL!fe' + Runtime::Env.user_password || '5iveL!fe' end end end From c5a3dfe1a67184fe9fc215907030506db404a3eb Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 9 Feb 2018 10:53:07 +0000 Subject: [PATCH 07/26] Improve docs about allowing some side effects on the constructor --- doc/development/fe_guide/style_guide_js.md | 33 ++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 02773162801..2e2d58b6d66 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -207,10 +207,39 @@ Do not use them anymore and feel free to remove them when refactoring legacy cod var c = pureFunction(values.foo); ``` -1. Avoid constructors with side-effects +1. Avoid constructors with side-effects. +Although we aim for code without side-effects we need some side-effects for our code to run. + +If the class won't do anything if we only instantiate it, it's ok to add side effects into the constructor (_Note:_ The following it's just an example. If the all purpose of the class is to add an event listener and handle the callback a function will be more suitable.) + +```javascript +// Bad +export class Foo { + constructor() { + this.init(); + } + init() { + document.addEventListener('click', this.handleCallback) + }, + handleCallback() { + + } +} + +// Good +export class Foo { + constructor() { + document.addEventListener() + } + handleCallback() { + } +} +``` + +On the other hand, if a class only needs to extend a third party/add event listeners in some specific cases, they should be inited oustside of the constructor. 1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` -A forEach will cause side effects, it will be mutating the array being iterated. Prefer using `.map`, +A forEach will most likely cause side effects, it will be mutating the array being iterated. Prefer using `.map`, `.reduce` or `.filter` ```javascript const users = [ { name: 'Foo' }, { name: 'Bar' } ]; From 49f4b34be31e9ca78ff2355d5e94044cf8b8daa4 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 9 Feb 2018 10:55:19 +0000 Subject: [PATCH 08/26] Remove not needed default statement --- .../components/states/mr_widget_missing_branch.vue | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue index 94eca0e9288..718c0e4b3c6 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue @@ -17,7 +17,6 @@ mr: { type: Object, required: true, - default: () => ({}), }, }, computed: { From 71c948d63743c80f50cdd929728bb074d9deeace Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Fri, 9 Feb 2018 11:14:48 +0000 Subject: [PATCH 09/26] Replace $.post in importer status with axios --- app/assets/javascripts/importer_status.js | 68 ++++++++----- .../import/bitbucket_controller.rb | 22 +++-- app/controllers/import/fogbugz_controller.rb | 16 ++-- app/controllers/import/github_controller.rb | 19 ++-- app/controllers/import/gitlab_controller.rb | 18 ++-- .../import/google_code_controller.rb | 16 ++-- app/serializers/project_serializer.rb | 3 + app/views/import/base/create.js.haml | 13 --- app/views/import/base/unauthorized.js.haml | 14 --- .../import/bitbucket_controller_spec.rb | 88 +++++++++++------ .../import/gitlab_controller_spec.rb | 85 +++++++++++------ spec/javascripts/importer_status_spec.js | 47 +++++++++ ...ubish_import_controller_shared_examples.rb | 95 ++++++++++++------- 13 files changed, 328 insertions(+), 176 deletions(-) create mode 100644 app/serializers/project_serializer.rb delete mode 100644 app/views/import/base/create.js.haml delete mode 100644 app/views/import/base/unauthorized.js.haml create mode 100644 spec/javascripts/importer_status_spec.js diff --git a/app/assets/javascripts/importer_status.js b/app/assets/javascripts/importer_status.js index 1dc70872d92..134a503864e 100644 --- a/app/assets/javascripts/importer_status.js +++ b/app/assets/javascripts/importer_status.js @@ -1,3 +1,7 @@ +import { __ } from './locale'; +import axios from './lib/utils/axios_utils'; +import flash from './flash'; + class ImporterStatus { constructor(jobsUrl, importUrl) { this.jobsUrl = jobsUrl; @@ -9,29 +13,7 @@ class ImporterStatus { initStatusPage() { $('.js-add-to-import') .off('click') - .on('click', (event) => { - const $btn = $(event.currentTarget); - const $tr = $btn.closest('tr'); - const $targetField = $tr.find('.import-target'); - const $namespaceInput = $targetField.find('.js-select-namespace option:selected'); - const id = $tr.attr('id').replace('repo_', ''); - let targetNamespace; - let newName; - if ($namespaceInput.length > 0) { - targetNamespace = $namespaceInput[0].innerHTML; - newName = $targetField.find('#path').prop('value'); - $targetField.empty().append(`${targetNamespace}/${newName}`); - } - $btn.disable().addClass('is-loading'); - - return $.post(this.importUrl, { - repo_id: id, - target_namespace: targetNamespace, - new_name: newName, - }, { - dataType: 'script', - }); - }); + .on('click', this.addToImport.bind(this)); $('.js-import-all') .off('click') @@ -44,6 +26,39 @@ class ImporterStatus { }); } + addToImport(event) { + const $btn = $(event.currentTarget); + const $tr = $btn.closest('tr'); + const $targetField = $tr.find('.import-target'); + const $namespaceInput = $targetField.find('.js-select-namespace option:selected'); + const id = $tr.attr('id').replace('repo_', ''); + let targetNamespace; + let newName; + if ($namespaceInput.length > 0) { + targetNamespace = $namespaceInput[0].innerHTML; + newName = $targetField.find('#path').prop('value'); + $targetField.empty().append(`${targetNamespace}/${newName}`); + } + $btn.disable().addClass('is-loading'); + + return axios.post(this.importUrl, { + repo_id: id, + target_namespace: targetNamespace, + new_name: newName, + }) + .then(({ data }) => { + const job = $(`tr#repo_${id}`); + job.attr('id', `project_${data.id}`); + + job.find('.import-target').html(`${data.full_path}`); + $('table.import-jobs tbody').prepend(job); + + job.addClass('active'); + job.find('.import-actions').html(' started'); + }) + .catch(() => flash(__('An error occurred while importing project'))); + } + setAutoUpdate() { return setInterval(() => $.get(this.jobsUrl, data => $.each(data, (i, job) => { const jobItem = $(`#project_${job.id}`); @@ -71,7 +86,7 @@ class ImporterStatus { } // eslint-disable-next-line consistent-return -export default function initImporterStatus() { +function initImporterStatus() { const importerStatus = document.querySelector('.js-importer-status'); if (importerStatus) { @@ -79,3 +94,8 @@ export default function initImporterStatus() { return new ImporterStatus(data.jobsImportPath, data.importPath); } } + +export { + initImporterStatus as default, + ImporterStatus, +}; diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 5ad1e116e4e..13ea736688d 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -37,24 +37,30 @@ class Import::BitbucketController < Import::BaseController def create bitbucket_client = Bitbucket::Client.new(credentials) - @repo_id = params[:repo_id].to_s - name = @repo_id.gsub('___', '/') + repo_id = params[:repo_id].to_s + name = repo_id.gsub('___', '/') repo = bitbucket_client.repo(name) - @project_name = params[:new_name].presence || repo.name + project_name = params[:new_name].presence || repo.name repo_owner = repo.owner repo_owner = current_user.username if repo_owner == bitbucket_client.user.username namespace_path = params[:new_namespace].presence || repo_owner + target_namespace = find_or_create_namespace(namespace_path, current_user) - @target_namespace = find_or_create_namespace(namespace_path, current_user) - - if current_user.can?(:create_projects, @target_namespace) + if current_user.can?(:create_projects, target_namespace) # The token in a session can be expired, we need to get most recent one because # Bitbucket::Connection class refreshes it. session[:bitbucket_token] = bitbucket_client.connection.token - @project = Gitlab::BitbucketImport::ProjectCreator.new(repo, @project_name, @target_namespace, current_user, credentials).execute + + project = Gitlab::BitbucketImport::ProjectCreator.new(repo, project_name, target_namespace, current_user, credentials).execute + + if project.persisted? + render json: ProjectSerializer.new.represent(project) + else + render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + end else - render 'unauthorized' + render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity end end diff --git a/app/controllers/import/fogbugz_controller.rb b/app/controllers/import/fogbugz_controller.rb index 5df6bd34185..669eb31a995 100644 --- a/app/controllers/import/fogbugz_controller.rb +++ b/app/controllers/import/fogbugz_controller.rb @@ -58,17 +58,17 @@ class Import::FogbugzController < Import::BaseController end def create - @repo_id = params[:repo_id] - repo = client.repo(@repo_id) + repo = client.repo(params[:repo_id]) fb_session = { uri: session[:fogbugz_uri], token: session[:fogbugz_token] } - @target_namespace = current_user.namespace - @project_name = repo.name - - namespace = @target_namespace - umap = session[:fogbugz_user_map] || client.user_map - @project = Gitlab::FogbugzImport::ProjectCreator.new(repo, fb_session, namespace, current_user, umap).execute + project = Gitlab::FogbugzImport::ProjectCreator.new(repo, fb_session, current_user.namespace, current_user, umap).execute + + if project.persisted? + render json: ProjectSerializer.new.represent(project) + else + render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + end end private diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index b8ba7921613..69fb8121ded 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -36,16 +36,21 @@ class Import::GithubController < Import::BaseController end def create - @repo_id = params[:repo_id].to_i - repo = client.repo(@repo_id) - @project_name = params[:new_name].presence || repo.name + repo = client.repo(params[:repo_id].to_i) + project_name = params[:new_name].presence || repo.name namespace_path = params[:target_namespace].presence || current_user.namespace_path - @target_namespace = find_or_create_namespace(namespace_path, current_user.namespace_path) + target_namespace = find_or_create_namespace(namespace_path, current_user.namespace_path) - if can?(current_user, :create_projects, @target_namespace) - @project = Gitlab::LegacyGithubImport::ProjectCreator.new(repo, @project_name, @target_namespace, current_user, access_params, type: provider).execute + if can?(current_user, :create_projects, target_namespace) + project = Gitlab::LegacyGithubImport::ProjectCreator.new(repo, project_name, target_namespace, current_user, access_params, type: provider).execute + + if project.persisted? + render json: ProjectSerializer.new.represent(project) + else + render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + end else - render 'unauthorized' + render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity end end diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb index 407154e59a0..18f1d20f5a9 100644 --- a/app/controllers/import/gitlab_controller.rb +++ b/app/controllers/import/gitlab_controller.rb @@ -24,15 +24,19 @@ class Import::GitlabController < Import::BaseController end def create - @repo_id = params[:repo_id].to_i - repo = client.project(@repo_id) - @project_name = repo['name'] - @target_namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username']) + repo = client.project(params[:repo_id].to_i) + target_namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username']) - if current_user.can?(:create_projects, @target_namespace) - @project = Gitlab::GitlabImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute + if current_user.can?(:create_projects, target_namespace) + project = Gitlab::GitlabImport::ProjectCreator.new(repo, target_namespace, current_user, access_params).execute + + if project.persisted? + render json: ProjectSerializer.new.represent(project) + else + render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + end else - render 'unauthorized' + render json: { errors: 'This namespace has already been taken! Please choose another one.' }, status: :unprocessable_entity end end diff --git a/app/controllers/import/google_code_controller.rb b/app/controllers/import/google_code_controller.rb index 7d7f13ce5d5..baa19fb383d 100644 --- a/app/controllers/import/google_code_controller.rb +++ b/app/controllers/import/google_code_controller.rb @@ -85,16 +85,16 @@ class Import::GoogleCodeController < Import::BaseController end def create - @repo_id = params[:repo_id] - repo = client.repo(@repo_id) - @target_namespace = current_user.namespace - @project_name = repo.name - - namespace = @target_namespace - + repo = client.repo(params[:repo_id]) user_map = session[:google_code_user_map] - @project = Gitlab::GoogleCodeImport::ProjectCreator.new(repo, namespace, current_user, user_map).execute + project = Gitlab::GoogleCodeImport::ProjectCreator.new(repo, current_user.namespace, current_user, user_map).execute + + if project.persisted? + render json: ProjectSerializer.new.represent(project) + else + render json: { errors: project.errors.full_messages }, status: :unprocessable_entity + end end private diff --git a/app/serializers/project_serializer.rb b/app/serializers/project_serializer.rb new file mode 100644 index 00000000000..74de1e79a8f --- /dev/null +++ b/app/serializers/project_serializer.rb @@ -0,0 +1,3 @@ +class ProjectSerializer < BaseSerializer + entity ProjectEntity +end diff --git a/app/views/import/base/create.js.haml b/app/views/import/base/create.js.haml deleted file mode 100644 index 4dc3a4a0acf..00000000000 --- a/app/views/import/base/create.js.haml +++ /dev/null @@ -1,13 +0,0 @@ -- if @project.persisted? - :plain - job = $("tr#repo_#{@repo_id}") - job.attr("id", "project_#{@project.id}") - target_field = job.find(".import-target") - target_field.empty() - target_field.append('#{link_to @project.full_path, project_path(@project)}') - $("table.import-jobs tbody").prepend(job) - job.addClass("active").find(".import-actions").html(" started") -- else - :plain - job = $("tr#repo_#{@repo_id}") - job.find(".import-actions").html(" Error saving project: #{escape_javascript(h(@project.errors.full_messages.join(',')))}") diff --git a/app/views/import/base/unauthorized.js.haml b/app/views/import/base/unauthorized.js.haml deleted file mode 100644 index ada5f99f4e2..00000000000 --- a/app/views/import/base/unauthorized.js.haml +++ /dev/null @@ -1,14 +0,0 @@ -:plain - tr = $("tr#repo_#{@repo_id}") - target_field = tr.find(".import-target") - import_button = tr.find(".btn-import") - origin_target = target_field.text() - project_name = "#{@project_name}" - origin_namespace = "#{@target_namespace.full_path}" - target_field.empty() - target_field.append("

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

") - target_field.append("") - target_field.append("/" + project_name) - target_field.data("project_name", project_name) - target_field.find('input').prop("value", origin_namespace) - import_button.enable().removeClass('is-loading') diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index e8707760a5a..9c7a54f9ed4 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -84,20 +84,42 @@ describe Import::BitbucketController do double(slug: "vim", owner: bitbucket_username, name: 'vim') end + let(:project) { create(:project) } + before do allow_any_instance_of(Bitbucket::Client).to receive(:repo).and_return(bitbucket_repo) allow_any_instance_of(Bitbucket::Client).to receive(:user).and_return(bitbucket_user) assign_session_tokens end + it 'returns 200 response when the project is imported successfully' do + allow(Gitlab::BitbucketImport::ProjectCreator) + .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params) + .and_return(double(execute: project)) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(200) + end + + it 'returns 422 response when the project could not be imported' do + allow(Gitlab::BitbucketImport::ProjectCreator) + .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params) + .and_return(double(execute: build(:project))) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(422) + end + context "when the repository owner is the Bitbucket user" do context "when the Bitbucket user and GitLab user's usernames match" do it "takes the current user's namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -107,9 +129,9 @@ describe Import::BitbucketController do it "takes the current user's namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -120,7 +142,7 @@ describe Import::BitbucketController do allow(controller).to receive(:current_user).and_return(user) allow(user).to receive(:can?).and_return(false) - post :create, format: :js + post :create, format: :json end end end @@ -143,9 +165,9 @@ describe Import::BitbucketController do it "takes the existing namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, existing_namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -154,7 +176,7 @@ describe Import::BitbucketController do expect(Gitlab::BitbucketImport::ProjectCreator) .not_to receive(:new) - post :create, format: :js + post :create, format: :json end end end @@ -163,17 +185,17 @@ describe Import::BitbucketController do context "when current user can create namespaces" do it "creates the namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, format: :js }.to change(Namespace, :count).by(1) + expect { post :create, format: :json }.to change(Namespace, :count).by(1) end it "takes the new namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, an_instance_of(Group), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -184,17 +206,17 @@ describe Import::BitbucketController do it "doesn't create the namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, format: :js }.not_to change(Namespace, :count) + expect { post :create, format: :json }.not_to change(Namespace, :count) end it "takes the current user's namespace" do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -212,9 +234,9 @@ describe Import::BitbucketController do it 'takes the selected namespace and name' do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, nested_namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: nested_namespace.full_path, new_name: test_name, format: :js } + post :create, { target_namespace: nested_namespace.full_path, new_name: test_name, format: :json } end end @@ -224,26 +246,26 @@ describe Import::BitbucketController do it 'takes the selected namespace and name' do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } end it 'creates the namespaces' do allow(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } } + expect { post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } } .to change { Namespace.count }.by(2) end it 'new namespace has the right parent' do allow(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } expect(Namespace.find_by_path_or_name('bar').parent.path).to eq('foo') end @@ -256,19 +278,29 @@ describe Import::BitbucketController do it 'takes the selected namespace and name' do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } end it 'creates the namespaces' do allow(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } + expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } } .to change { Namespace.count }.by(2) end end + + context 'when user can not create projects in the chosen namespace' do + it 'returns 422 response' do + other_namespace = create(:group, name: 'other_namespace') + + post :create, { target_namespace: other_namespace.name, format: :json } + + expect(response).to have_gitlab_http_status(422) + end + end end end diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb index faf1e6f63ea..d902c6210f5 100644 --- a/spec/controllers/import/gitlab_controller_spec.rb +++ b/spec/controllers/import/gitlab_controller_spec.rb @@ -57,6 +57,7 @@ describe Import::GitlabController do end describe "POST create" do + let(:project) { create(:project) } let(:gitlab_username) { user.username } let(:gitlab_user) do { username: gitlab_username }.with_indifferent_access @@ -75,14 +76,34 @@ describe Import::GitlabController do assign_session_token end + it 'returns 200 response when the project is imported successfully' do + allow(Gitlab::GitlabImport::ProjectCreator) + .to receive(:new).with(gitlab_repo, user.namespace, user, access_params) + .and_return(double(execute: project)) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(200) + end + + it 'returns 422 response when the project could not be imported' do + allow(Gitlab::GitlabImport::ProjectCreator) + .to receive(:new).with(gitlab_repo, user.namespace, user, access_params) + .and_return(double(execute: build(:project))) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(422) + end + context "when the repository owner is the GitLab.com user" do context "when the GitLab.com user and GitLab server user's usernames match" do it "takes the current user's namespace" do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -92,9 +113,9 @@ describe Import::GitlabController do it "takes the current user's namespace" do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -118,9 +139,9 @@ describe Import::GitlabController do it "takes the existing namespace" do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, existing_namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -129,7 +150,7 @@ describe Import::GitlabController do expect(Gitlab::GitlabImport::ProjectCreator) .not_to receive(:new) - post :create, format: :js + post :create, format: :json end end end @@ -138,17 +159,17 @@ describe Import::GitlabController do context "when current user can create namespaces" do it "creates the namespace" do expect(Gitlab::GitlabImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, format: :js }.to change(Namespace, :count).by(1) + expect { post :create, format: :json }.to change(Namespace, :count).by(1) end it "takes the new namespace" do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -159,17 +180,17 @@ describe Import::GitlabController do it "doesn't create the namespace" do expect(Gitlab::GitlabImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, format: :js }.not_to change(Namespace, :count) + expect { post :create, format: :json }.not_to change(Namespace, :count) end it "takes the current user's namespace" do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, user.namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -185,9 +206,9 @@ describe Import::GitlabController do it 'takes the selected namespace and name' do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, nested_namespace, user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: nested_namespace.full_path, format: :js } + post :create, { target_namespace: nested_namespace.full_path, format: :json } end end @@ -197,26 +218,26 @@ describe Import::GitlabController do it 'takes the selected namespace and name' do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', format: :js } + post :create, { target_namespace: 'foo/bar', format: :json } end it 'creates the namespaces' do allow(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/bar', format: :js } } + expect { post :create, { target_namespace: 'foo/bar', format: :json } } .to change { Namespace.count }.by(2) end it 'new namespace has the right parent' do allow(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', format: :js } + post :create, { target_namespace: 'foo/bar', format: :json } expect(Namespace.find_by_path_or_name('bar').parent.path).to eq('foo') end @@ -229,20 +250,30 @@ describe Import::GitlabController do it 'takes the selected namespace and name' do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/foobar/bar', format: :js } + post :create, { target_namespace: 'foo/foobar/bar', format: :json } end it 'creates the namespaces' do allow(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/foobar/bar', format: :js } } + expect { post :create, { target_namespace: 'foo/foobar/bar', format: :json } } .to change { Namespace.count }.by(2) end end + + context 'when user can not create projects in the chosen namespace' do + it 'returns 422 response' do + other_namespace = create(:group, name: 'other_namespace') + + post :create, { target_namespace: other_namespace.name, format: :json } + + expect(response).to have_gitlab_http_status(422) + end + end end end end diff --git a/spec/javascripts/importer_status_spec.js b/spec/javascripts/importer_status_spec.js new file mode 100644 index 00000000000..bb49c576e91 --- /dev/null +++ b/spec/javascripts/importer_status_spec.js @@ -0,0 +1,47 @@ +import { ImporterStatus } from '~/importer_status'; +import axios from '~/lib/utils/axios_utils'; +import MockAdapter from 'axios-mock-adapter'; + +describe('Importer Status', () => { + describe('addToImport', () => { + let instance; + let mock; + const importUrl = '/import_url'; + + beforeEach(() => { + setFixtures(` + + + + + + + `); + spyOn(ImporterStatus.prototype, 'initStatusPage').and.callFake(() => {}); + spyOn(ImporterStatus.prototype, 'setAutoUpdate').and.callFake(() => {}); + instance = new ImporterStatus('', importUrl); + mock = new MockAdapter(axios); + }); + + afterEach(() => { + mock.restore(); + }); + + it('sets table row to active after post request', (done) => { + mock.onPost(importUrl).reply(200, { + id: 1, + full_path: '/full_path', + }); + + instance.addToImport({ + currentTarget: document.querySelector('.js-add-to-import'), + }) + .then(() => { + expect(document.querySelector('tr').classList.contains('active')).toEqual(true); + done(); + }) + .catch(done.fail); + }); + }); +}); diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index a0839eefe6c..793bacf25c7 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -92,6 +92,7 @@ end shared_examples 'a GitHub-ish import controller: POST create' do let(:user) { create(:user) } + let(:project) { create(:project) } let(:provider_username) { user.username } let(:provider_user) { OpenStruct.new(login: provider_username) } let(:provider_repo) do @@ -107,14 +108,34 @@ shared_examples 'a GitHub-ish import controller: POST create' do assign_session_token(provider) end + it 'returns 200 response when the project is imported successfully' do + allow(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: project)) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(200) + end + + it 'returns 422 response when the project could not be imported' do + allow(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: build(:project))) + + post :create, format: :json + + expect(response).to have_gitlab_http_status(422) + end + context "when the repository owner is the provider user" do context "when the provider user and GitLab user's usernames match" do it "takes the current user's namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -124,9 +145,9 @@ shared_examples 'a GitHub-ish import controller: POST create' do it "takes the current user's namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -151,9 +172,9 @@ shared_examples 'a GitHub-ish import controller: POST create' do it "takes the existing namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, existing_namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end @@ -163,9 +184,9 @@ shared_examples 'a GitHub-ish import controller: POST create' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -174,17 +195,17 @@ shared_examples 'a GitHub-ish import controller: POST create' do context "when current user can create namespaces" do it "creates the namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, target_namespace: provider_repo.name, format: :js }.to change(Namespace, :count).by(1) + expect { post :create, target_namespace: provider_repo.name, format: :json }.to change(Namespace, :count).by(1) end it "takes the new namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, an_instance_of(Group), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, target_namespace: provider_repo.name, format: :js + post :create, target_namespace: provider_repo.name, format: :json end end @@ -195,17 +216,17 @@ shared_examples 'a GitHub-ish import controller: POST create' do it "doesn't create the namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) - .to receive(:new).and_return(double(execute: true)) + .to receive(:new).and_return(double(execute: project)) - expect { post :create, format: :js }.not_to change(Namespace, :count) + expect { post :create, format: :json }.not_to change(Namespace, :count) end it "takes the current user's namespace" do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, format: :js + post :create, format: :json end end end @@ -221,17 +242,17 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, test_namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: test_namespace.name, new_name: test_name, format: :js } + post :create, { target_namespace: test_namespace.name, new_name: test_name, format: :json } end it 'takes the selected name and default namespace' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { new_name: test_name, format: :js } + post :create, { new_name: test_name, format: :json } end end @@ -247,9 +268,9 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, nested_namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: nested_namespace.full_path, new_name: test_name, format: :js } + post :create, { target_namespace: nested_namespace.full_path, new_name: test_name, format: :json } end end @@ -259,26 +280,26 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } end it 'creates the namespaces' do allow(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } } + expect { post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } } .to change { Namespace.count }.by(2) end it 'new namespace has the right parent' do allow(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/bar', new_name: test_name, format: :json } expect(Namespace.find_by_path_or_name('bar').parent.path).to eq('foo') end @@ -291,19 +312,29 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } + post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } end it 'creates the namespaces' do allow(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: project)) - expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } + expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } } .to change { Namespace.count }.by(2) end end + + context 'when user can not create projects in the chosen namespace' do + it 'returns 422 response' do + other_namespace = create(:group, name: 'other_namespace') + + post :create, { target_namespace: other_namespace.name, format: :json } + + expect(response).to have_gitlab_http_status(422) + end + end end end From 27a70018842e1ac130403db319ab013270901e6e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 9 Feb 2018 12:27:55 +0100 Subject: [PATCH 10/26] Do not attach runner to a non-exsiting network in QA --- qa/qa/service/runner.rb | 10 +++++++++- qa/qa/service/shellout.rb | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/qa/qa/service/runner.rb b/qa/qa/service/runner.rb index d0ee33c69f2..c0352e0467a 100644 --- a/qa/qa/service/runner.rb +++ b/qa/qa/service/runner.rb @@ -15,6 +15,14 @@ module QA @tags = %w[qa test] end + def network + shell "docker network inspect #{@network}" + rescue CommandError + 'bridge' + else + @network + end + def pull shell "docker pull #{@image}" end @@ -22,7 +30,7 @@ module QA def register! shell <<~CMD.tr("\n", ' ') docker run -d --rm --entrypoint=/bin/sh - --network #{@network} --name #{@name} + --network #{network} --name #{@name} -e CI_SERVER_URL=#{@address} -e REGISTER_NON_INTERACTIVE=true -e REGISTRATION_TOKEN=#{@token} diff --git a/qa/qa/service/shellout.rb b/qa/qa/service/shellout.rb index 898febde63c..76fb2af6319 100644 --- a/qa/qa/service/shellout.rb +++ b/qa/qa/service/shellout.rb @@ -3,6 +3,8 @@ require 'open3' module QA module Service module Shellout + CommandError = Class.new(StandardError) + ## # TODO, make it possible to use generic QA framework classes # as a library - gitlab-org/gitlab-qa#94 @@ -14,7 +16,7 @@ module QA out.each { |line| puts line } if wait.value.exited? && wait.value.exitstatus.nonzero? - raise "Command `#{command}` failed!" + raise CommandError, "Command `#{command}` failed!" end end end From 369d18cb298356e8043554613ddf1a9853803c60 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 9 Feb 2018 11:17:58 +0000 Subject: [PATCH 11/26] Adds tooltip for environment name Adds CSS for child envrionments --- .../components/environment_item.vue | 31 ++++++++++++------- .../stylesheets/pages/environments.scss | 4 +++ .../unreleased/42922-environment-name.yml | 5 +++ 3 files changed, 29 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/42922-environment-name.yml diff --git a/app/assets/javascripts/environments/components/environment_item.vue b/app/assets/javascripts/environments/components/environment_item.vue index a9d554e549e..79326ca3487 100644 --- a/app/assets/javascripts/environments/components/environment_item.vue +++ b/app/assets/javascripts/environments/components/environment_item.vue @@ -1,8 +1,9 @@ ") + expect(result.to_html).to include("alert(1)") + end + end + context "when no language is specified" do it "highlights as plaintext" do result = filter('
def fun end
') + expect(result.to_html).to eq('
def fun end
') end + + include_examples "XSS prevention", "" end context "when a valid language is specified" do it "highlights as that language" do result = filter('
def fun end
') + expect(result.to_html).to eq('
def fun end
') end + + include_examples "XSS prevention", "ruby" end context "when an invalid language is specified" do it "highlights as plaintext" do result = filter('
This is a test
') + expect(result.to_html).to eq('
This is a test
') end + + include_examples "XSS prevention", "gnuplot" + end + + context "languages that should be passed through" do + %w(math mermaid plantuml).each do |lang| + context "when #{lang} is specified" do + it "highlights as plaintext but with the correct language attribute and class" do + result = filter(%{
This is a test
}) + + expect(result.to_html).to eq(%{
This is a test
}) + end + + include_examples "XSS prevention", lang + end + end end - context "when Rouge formatting fails" do + context "when Rouge lexing fails" do before do - allow_any_instance_of(Rouge::Formatter).to receive(:format).and_raise(StandardError) + allow_any_instance_of(Rouge::Lexers::Ruby).to receive(:stream_tokens).and_raise(StandardError) end it "highlights as plaintext" do result = filter('
This is a test
') - expect(result.to_html).to eq('
This is a test
') + + expect(result.to_html).to eq('
This is a test
') end + + include_examples "XSS prevention", "ruby" + end + + context "when Rouge lexing fails after a retry" do + before do + allow_any_instance_of(Rouge::Lexers::PlainText).to receive(:stream_tokens).and_raise(StandardError) + end + + it "does not add highlighting classes" do + result = filter('
This is a test
') + + expect(result.to_html).to eq('
This is a test
') + end + + include_examples "XSS prevention", "ruby" end end From fec9fb05a5775b864ef6768df166d39fcb2be4bc Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 18 Jan 2018 23:10:19 +0000 Subject: [PATCH 17/26] Merge branch 'security-10-4-todo-api-reveals-sensitive-information' into 'security-10-4' Restrict Todo API mark_as_done endpoint to the user's todos only --- ...security-10-4-todo-api-reveals-sensitive-information.yml | 5 +++++ lib/api/todos.rb | 2 +- lib/api/v3/todos.rb | 2 +- spec/requests/api/todos_spec.rb | 6 ++++++ spec/requests/api/v3/todos_spec.rb | 6 ++++++ 5 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml diff --git a/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml b/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml new file mode 100644 index 00000000000..329825d1e73 --- /dev/null +++ b/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml @@ -0,0 +1,5 @@ +--- +title: Restrict Todo API mark_as_done endpoint to the user's todos only +merge_request: +author: +type: security diff --git a/lib/api/todos.rb b/lib/api/todos.rb index ffccfebe752..c6dbcf84e3a 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -60,7 +60,7 @@ module API end post ':id/mark_as_done' do TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user) - todo = Todo.find(params[:id]) + todo = current_user.todos.find(params[:id]) present todo, with: Entities::Todo, current_user: current_user end diff --git a/lib/api/v3/todos.rb b/lib/api/v3/todos.rb index 2f2cf259987..3e2c61f6dbd 100644 --- a/lib/api/v3/todos.rb +++ b/lib/api/v3/todos.rb @@ -12,7 +12,7 @@ module API end delete ':id' do TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user) - todo = Todo.find(params[:id]) + todo = current_user.todos.find(params[:id]) present todo, with: ::API::Entities::Todo, current_user: current_user end diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index fb3a33cadff..2ee8d150dc8 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -129,6 +129,12 @@ describe API::Todos do post api("/todos/#{pending_1.id}/mark_as_done", john_doe) end + + it 'returns 404 if the todo does not belong to the current user' do + post api("/todos/#{pending_1.id}/mark_as_done", author_1) + + expect(response.status).to eq(404) + end end end diff --git a/spec/requests/api/v3/todos_spec.rb b/spec/requests/api/v3/todos_spec.rb index 53fd962272a..ea648e3917f 100644 --- a/spec/requests/api/v3/todos_spec.rb +++ b/spec/requests/api/v3/todos_spec.rb @@ -38,6 +38,12 @@ describe API::V3::Todos do delete v3_api("/todos/#{pending_1.id}", john_doe) end + + it 'returns 404 if the todo does not belong to the current user' do + delete v3_api("/todos/#{pending_1.id}", author_1) + + expect(response.status).to eq(404) + end end end From 68e31c098ec3984c42b921c07fec8593116e77ce Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 26 Jan 2018 15:39:10 +0000 Subject: [PATCH 18/26] Merge branch 'fix/gh-namespace-issue' into 'security-10-4' [10.4] Fix GH namespace security issue --- app/controllers/import/base_controller.rb | 24 +++----- app/services/groups/nested_create_service.rb | 10 +++- .../unreleased/fix-gh-namespace-issue.yml | 5 ++ .../import/bitbucket_controller_spec.rb | 10 +++- .../import/gitlab_controller_spec.rb | 10 +++- ...ubish_import_controller_shared_examples.rb | 57 ++++++++++++++++++- 6 files changed, 87 insertions(+), 29 deletions(-) create mode 100644 changelogs/unreleased/fix-gh-namespace-issue.yml diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 9de0297ecfd..c84fc2d305d 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -2,26 +2,16 @@ class Import::BaseController < ApplicationController private def find_or_create_namespace(names, owner) - return current_user.namespace if names == owner - return current_user.namespace unless current_user.can_create_group? - names = params[:target_namespace].presence || names - full_path_namespace = Namespace.find_by_full_path(names) - return full_path_namespace if full_path_namespace + return current_user.namespace if names == owner - names.split('/').inject(nil) do |parent, name| - begin - namespace = Group.create!(name: name, - path: name, - owner: current_user, - parent: parent) - namespace.add_owner(current_user) + group = Groups::NestedCreateService.new(current_user, group_path: names).execute - namespace - rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid - Namespace.where(parent: parent).find_by_path_or_name(name) - end - end + group.errors.any? ? current_user.namespace : group + rescue => e + Gitlab::AppLogger.error(e) + + current_user.namespace end end diff --git a/app/services/groups/nested_create_service.rb b/app/services/groups/nested_create_service.rb index d6f08fc3cce..5c337a9faa5 100644 --- a/app/services/groups/nested_create_service.rb +++ b/app/services/groups/nested_create_service.rb @@ -11,8 +11,8 @@ module Groups def execute return nil unless group_path - if group = Group.find_by_full_path(group_path) - return group + if namespace = namespace_or_group(group_path) + return namespace end if group_path.include?('/') && !Group.supports_nested_groups? @@ -40,10 +40,14 @@ module Groups ) new_params[:visibility_level] ||= Gitlab::CurrentSettings.current_application_settings.default_group_visibility - last_group = Group.find_by_full_path(partial_path) || Groups::CreateService.new(current_user, new_params).execute + last_group = namespace_or_group(partial_path) || Groups::CreateService.new(current_user, new_params).execute end last_group end + + def namespace_or_group(group_path) + Namespace.find_by_full_path(group_path) + end end end diff --git a/changelogs/unreleased/fix-gh-namespace-issue.yml b/changelogs/unreleased/fix-gh-namespace-issue.yml new file mode 100644 index 00000000000..2db7abb9d58 --- /dev/null +++ b/changelogs/unreleased/fix-gh-namespace-issue.yml @@ -0,0 +1,5 @@ +--- +title: Fix namespace access issue for GitHub, BitBucket, and GitLab.com project importers +merge_request: +author: +type: security diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index 9c7a54f9ed4..2be46049aab 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -222,7 +222,7 @@ describe Import::BitbucketController do end end - context 'user has chosen an existing nested namespace and name for the project' do + context 'user has chosen an existing nested namespace and name for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:test_name) { 'test_name' } @@ -240,7 +240,7 @@ describe Import::BitbucketController do end end - context 'user has chosen a non-existent nested namespaces and name for the project' do + context 'user has chosen a non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -271,10 +271,14 @@ describe Import::BitbucketController do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb index d902c6210f5..e958be077c2 100644 --- a/spec/controllers/import/gitlab_controller_spec.rb +++ b/spec/controllers/import/gitlab_controller_spec.rb @@ -195,7 +195,7 @@ describe Import::GitlabController do end end - context 'user has chosen an existing nested namespace for the project' do + context 'user has chosen an existing nested namespace for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } @@ -212,7 +212,7 @@ describe Import::GitlabController do end end - context 'user has chosen a non-existent nested namespaces for the project' do + context 'user has chosen a non-existent nested namespaces for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -243,10 +243,14 @@ describe Import::GitlabController do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index 793bacf25c7..e848efd402f 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -256,7 +256,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen an existing nested namespace and name for the project' do + context 'user has chosen an existing nested namespace and name for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:test_name) { 'test_name' } @@ -274,7 +274,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen a non-existent nested namespaces and name for the project' do + context 'user has chosen a non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -305,10 +305,14 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) @@ -325,6 +329,53 @@ shared_examples 'a GitHub-ish import controller: POST create' do expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } } .to change { Namespace.count }.by(2) end + + it 'does not create a new namespace under the user namespace' do + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: true)) + + expect { post :create, { target_namespace: "#{user.namespace_path}/test_group", new_name: test_name, format: :js } } + .not_to change { Namespace.count } + end + end + + context 'user cannot create a subgroup inside a group is not a member of' do + let(:test_name) { 'test_name' } + let!(:parent_namespace) { create(:group, name: 'foo') } + + it 'does not take the selected namespace and name' do + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: true)) + + post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } + end + + it 'does not create the namespaces' do + allow(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) + .and_return(double(execute: true)) + + expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } + .not_to change { Namespace.count } + end + end + + context 'user can use a group without having permissions to create a group' do + let(:test_name) { 'test_name' } + let!(:group) { create(:group, name: 'foo') } + + it 'takes the selected namespace and name' do + group.add_owner(user) + user.update!(can_create_group: false) + + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, group, user, access_params, type: provider) + .and_return(double(execute: true)) + + post :create, { target_namespace: 'foo', new_name: test_name, format: :js } + end end context 'when user can not create projects in the chosen namespace' do From 02f93da8600a77e1502e89c3a65513a806c0d847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 26 Jan 2018 17:05:04 +0000 Subject: [PATCH 19/26] Merge branch 'mc/bug/38984-wildcard-protected-tags' into 'security-10-4' Fix using wildcards in protected tags to expose protected variables --- app/models/project.rb | 5 ++++- .../unreleased/mc-bug-38984-wildcard-protected-tags.yml | 5 +++++ spec/models/ci/build_spec.rb | 8 ++++---- spec/models/group_spec.rb | 8 ++++++-- spec/models/project_spec.rb | 8 ++++++-- 5 files changed, 25 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml diff --git a/app/models/project.rb b/app/models/project.rb index 0590cc1c720..3893b1818f3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1589,8 +1589,11 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - ProtectedBranch.protected?(self, ref) || + if repository.branch_exists?(ref) + ProtectedBranch.protected?(self, ref) + elsif repository.tag_exists?(ref) ProtectedTag.protected?(self, ref) + end end def deployment_variables diff --git a/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml b/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml new file mode 100644 index 00000000000..27219b096af --- /dev/null +++ b/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml @@ -0,0 +1,5 @@ +--- +title: Fix wilcard protected tags protecting all branches +merge_request: +author: +type: security diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9e159c3f1fe..78fcbf6d47e 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1590,7 +1590,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - create(:protected_branch, project: build.project, name: build.ref) + allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -1598,7 +1598,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - create(:protected_tag, project: build.project, name: build.ref) + allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -1635,7 +1635,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - create(:protected_branch, project: build.project, name: build.ref) + allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -1643,7 +1643,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - create(:protected_tag, project: build.project, name: build.ref) + allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) end it { is_expected.to include(protected_variable) } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 338fb314ee9..4f16b73ef38 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -549,7 +549,7 @@ describe Group do context 'when the ref is a protected branch' do before do - create(:protected_branch, name: 'ref', project: project) + allow(project).to receive(:protected_for?).with('ref').and_return(true) end it_behaves_like 'ref is protected' @@ -557,7 +557,7 @@ describe Group do context 'when the ref is a protected tag' do before do - create(:protected_tag, name: 'ref', project: project) + allow(project).to receive(:protected_for?).with('ref').and_return(true) end it_behaves_like 'ref is protected' @@ -571,6 +571,10 @@ describe Group do let(:variable_child_2) { create(:ci_group_variable, group: group_child_2) } let(:variable_child_3) { create(:ci_group_variable, group: group_child_3) } + before do + allow(project).to receive(:protected_for?).with('ref').and_return(true) + end + it 'returns all variables belong to the group and parent groups' do expected_array1 = [protected_variable, secret_variable] expected_array2 = [variable_child, variable_child_2, variable_child_3] diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 50b8bb7acb3..ee04d74d848 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2092,7 +2092,7 @@ describe Project do context 'when the ref is a protected branch' do before do - create(:protected_branch, name: 'ref', project: project) + allow(project).to receive(:protected_for?).with('ref').and_return(true) end it_behaves_like 'ref is protected' @@ -2100,7 +2100,7 @@ describe Project do context 'when the ref is a protected tag' do before do - create(:protected_tag, name: 'ref', project: project) + allow(project).to receive(:protected_for?).with('ref').and_return(true) end it_behaves_like 'ref is protected' @@ -2125,6 +2125,8 @@ describe Project do context 'when the ref is a protected branch' do before do + allow(project).to receive(:repository).and_call_original + allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true) create(:protected_branch, name: 'ref', project: project) end @@ -2135,6 +2137,8 @@ describe Project do context 'when the ref is a protected tag' do before do + allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false) + allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true) create(:protected_tag, name: 'ref', project: project) end From 06e9faa3574738a2e32d8c1c1e1636cdff1c2489 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 9 Feb 2018 12:16:49 -0600 Subject: [PATCH 20/26] Remove changelogs for already-released security patches --- changelogs/unreleased/fix-gh-namespace-issue.yml | 5 ----- changelogs/unreleased/fix-stored-xss-in-code-blocks.yml | 5 ----- .../unreleased/mc-bug-38984-wildcard-protected-tags.yml | 5 ----- .../security-10-4-todo-api-reveals-sensitive-information.yml | 5 ----- 4 files changed, 20 deletions(-) delete mode 100644 changelogs/unreleased/fix-gh-namespace-issue.yml delete mode 100644 changelogs/unreleased/fix-stored-xss-in-code-blocks.yml delete mode 100644 changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml delete mode 100644 changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml diff --git a/changelogs/unreleased/fix-gh-namespace-issue.yml b/changelogs/unreleased/fix-gh-namespace-issue.yml deleted file mode 100644 index 2db7abb9d58..00000000000 --- a/changelogs/unreleased/fix-gh-namespace-issue.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix namespace access issue for GitHub, BitBucket, and GitLab.com project importers -merge_request: -author: -type: security diff --git a/changelogs/unreleased/fix-stored-xss-in-code-blocks.yml b/changelogs/unreleased/fix-stored-xss-in-code-blocks.yml deleted file mode 100644 index b595459ee6b..00000000000 --- a/changelogs/unreleased/fix-stored-xss-in-code-blocks.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix stored XSS in code blocks that ignore highlighting -merge_request: -author: -type: security diff --git a/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml b/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml deleted file mode 100644 index 27219b096af..00000000000 --- a/changelogs/unreleased/mc-bug-38984-wildcard-protected-tags.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix wilcard protected tags protecting all branches -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml b/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml deleted file mode 100644 index 329825d1e73..00000000000 --- a/changelogs/unreleased/security-10-4-todo-api-reveals-sensitive-information.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Restrict Todo API mark_as_done endpoint to the user's todos only -merge_request: -author: -type: security From b60a39f26023207a0e5a63559ec3d2592a3246f0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 9 Feb 2018 12:18:40 -0600 Subject: [PATCH 21/26] Default CI variables to unprotected See https://gitlab.com/gitlab-org/gitlab-ce/issues/42928 --- .../javascripts/ci_variable_list/ci_variable_list.js | 2 +- app/views/ci/variables/_variable_row.html.haml | 2 +- .../javascripts/ci_variable_list/ci_variable_list_spec.js | 2 +- spec/support/features/variable_list_shared_examples.rb | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/ci_variable_list/ci_variable_list.js b/app/assets/javascripts/ci_variable_list/ci_variable_list.js index d91789c2192..3467e88119b 100644 --- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js +++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js @@ -39,7 +39,7 @@ export default class VariableList { }, protected: { selector: '.js-ci-variable-input-protected', - default: 'true', + default: 'false', }, environment_scope: { // We can't use a `.js-` class here because diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml index 495a55660cb..15201780451 100644 --- a/app/views/ci/variables/_variable_row.html.haml +++ b/app/views/ci/variables/_variable_row.html.haml @@ -5,7 +5,7 @@ - id = variable&.id - key = variable&.key - value = variable&.value -- is_protected = variable && !only_key_value ? variable.protected : true +- is_protected = variable && !only_key_value ? variable.protected : false - id_input_name = "#{form_field}[variables_attributes][][id]" - destroy_input_name = "#{form_field}[variables_attributes][][_destroy]" diff --git a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js index 6ab7b50e035..8acb346901f 100644 --- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js +++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js @@ -126,7 +126,7 @@ describe('VariableList', () => { // Check for the correct default in the new row const $protectedInput = $wrapper.find('.js-row:last-child').find('.js-ci-variable-input-protected'); - expect($protectedInput.val()).toBe('true'); + expect($protectedInput.val()).toBe('false'); }) .then(done) .catch(done.fail); diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 83bf06b6727..4315bf5d037 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -41,13 +41,13 @@ shared_examples 'variable list' do end end - it 'adds new unprotected variable' do + it 'adds new protected variable' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') find('.js-ci-variable-input-value').set('key value') find('.ci-variable-protected-item .js-project-feature-toggle').click - expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end click_button('Save variables') @@ -59,7 +59,7 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do expect(find('.js-ci-variable-input-key').value).to eq('key') expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value') - expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end end @@ -143,7 +143,6 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('unprotected_key') find('.js-ci-variable-input-value').set('unprotected_value') - find('.ci-variable-protected-item .js-project-feature-toggle').click expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') end @@ -178,6 +177,7 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('protected_key') find('.js-ci-variable-input-value').set('protected_value') + find('.ci-variable-protected-item .js-project-feature-toggle').click expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end From 56a41ceb7aa3250c4ac7e6fc466a1ddb6bceda25 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 9 Feb 2018 13:14:29 -0600 Subject: [PATCH 22/26] Resolve failures in GitHub-ish import controller specs --- .../githubish_import_controller_shared_examples.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index e848efd402f..3321f920666 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -333,7 +333,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'does not create a new namespace under the user namespace' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: build_stubbed(:project))) expect { post :create, { target_namespace: "#{user.namespace_path}/test_group", new_name: test_name, format: :js } } .not_to change { Namespace.count } @@ -347,7 +347,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'does not take the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: build_stubbed(:project))) post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } end @@ -355,7 +355,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do it 'does not create the namespaces' do allow(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: build_stubbed(:project))) expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } .not_to change { Namespace.count } @@ -372,7 +372,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, group, user, access_params, type: provider) - .and_return(double(execute: true)) + .and_return(double(execute: build_stubbed(:project))) post :create, { target_namespace: 'foo', new_name: test_name, format: :js } end From 43876e5c8ce563abb46bc84c185ba46dcaaa7905 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Fri, 9 Feb 2018 21:35:32 +0100 Subject: [PATCH 23/26] Remove confirmation_input --- .../components/confirmation_input.vue | 62 ------------------ .../components/confirmation_input_spec.js | 63 ------------------- 2 files changed, 125 deletions(-) delete mode 100644 app/assets/javascripts/vue_shared/components/confirmation_input.vue delete mode 100644 spec/javascripts/vue_shared/components/confirmation_input_spec.js diff --git a/app/assets/javascripts/vue_shared/components/confirmation_input.vue b/app/assets/javascripts/vue_shared/components/confirmation_input.vue deleted file mode 100644 index 1aa03ea6317..00000000000 --- a/app/assets/javascripts/vue_shared/components/confirmation_input.vue +++ /dev/null @@ -1,62 +0,0 @@ - - - diff --git a/spec/javascripts/vue_shared/components/confirmation_input_spec.js b/spec/javascripts/vue_shared/components/confirmation_input_spec.js deleted file mode 100644 index a6a12614e77..00000000000 --- a/spec/javascripts/vue_shared/components/confirmation_input_spec.js +++ /dev/null @@ -1,63 +0,0 @@ -import Vue from 'vue'; -import confirmationInput from '~/vue_shared/components/confirmation_input.vue'; -import mountComponent from '../../helpers/vue_mount_component_helper'; - -describe('Confirmation input component', () => { - const Component = Vue.extend(confirmationInput); - const props = { - inputId: 'dummy-id', - confirmationKey: 'confirmation-key', - confirmationValue: 'confirmation-value', - }; - let vm; - - afterEach(() => { - vm.$destroy(); - }); - - describe('props', () => { - beforeEach(() => { - vm = mountComponent(Component, props); - }); - - it('sets id of the input field to inputId', () => { - expect(vm.$refs.enteredValue.id).toBe(props.inputId); - }); - - it('sets name of the input field to confirmationKey', () => { - expect(vm.$refs.enteredValue.name).toBe(props.confirmationKey); - }); - }); - - describe('computed', () => { - describe('inputLabel', () => { - it('escapes confirmationValue by default', () => { - vm = mountComponent(Component, { ...props, confirmationValue: 'nds escap"ng' }); - expect(vm.inputLabel).toBe('Type n<e></e>ds escap"ng to confirm:'); - }); - - it('does not escape confirmationValue if escapeValue is false', () => { - vm = mountComponent(Component, { ...props, confirmationValue: 'nds escap"ng', shouldEscapeConfirmationValue: false }); - expect(vm.inputLabel).toBe('Type nds escap"ng to confirm:'); - }); - }); - }); - - describe('methods', () => { - describe('hasCorrectValue', () => { - beforeEach(() => { - vm = mountComponent(Component, props); - }); - - it('returns false if entered value is incorrect', () => { - vm.$refs.enteredValue.value = 'incorrect'; - expect(vm.hasCorrectValue()).toBe(false); - }); - - it('returns true if entered value is correct', () => { - vm.$refs.enteredValue.value = props.confirmationValue; - expect(vm.hasCorrectValue()).toBe(true); - }); - }); - }); -}); From 0d9107374a2caddea61351d2fd5ba619cb69df81 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 9 Feb 2018 18:58:29 +0100 Subject: [PATCH 24/26] Return a warning string if we try to encode to unsupported encoding Fixes gitlab-development-kit#321 --- lib/gitlab/encoding_helper.rb | 6 +++--- spec/lib/gitlab/encoding_helper_spec.rb | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/encoding_helper.rb b/lib/gitlab/encoding_helper.rb index c0edcabc6fd..6659efa0961 100644 --- a/lib/gitlab/encoding_helper.rb +++ b/lib/gitlab/encoding_helper.rb @@ -28,9 +28,9 @@ module Gitlab # encode and clean the bad chars message.replace clean(message) - rescue ArgumentError - return nil - rescue + rescue ArgumentError => e + return unless e.message.include?('unknown encoding name') + encoding = detect ? detect[:encoding] : "unknown" "--broken encoding: #{encoding}" end diff --git a/spec/lib/gitlab/encoding_helper_spec.rb b/spec/lib/gitlab/encoding_helper_spec.rb index 4e9367323cb..83d431a7458 100644 --- a/spec/lib/gitlab/encoding_helper_spec.rb +++ b/spec/lib/gitlab/encoding_helper_spec.rb @@ -24,6 +24,11 @@ describe Gitlab::EncodingHelper do 'removes invalid bytes from ASCII-8bit encoded multibyte string. This can occur when a git diff match line truncates in the middle of a multibyte character. This occurs after the second word in this example. The test string is as short as we can get while still triggering the error condition when not looking at `detect[:confidence]`.', "mu ns\xC3\n Lorem ipsum dolor sit amet, consectetur adipisicing ut\xC3\xA0y\xC3\xB9abcd\xC3\xB9efg kia elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non p\n {: .normal_pn}\n \n-Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in\n# *Lorem ipsum\xC3\xB9l\xC3\xB9l\xC3\xA0 dolor\xC3\xB9k\xC3\xB9 sit\xC3\xA8b\xC3\xA8 N\xC3\xA8 amet b\xC3\xA0d\xC3\xAC*\n+# *consectetur\xC3\xB9l\xC3\xB9l\xC3\xA0 adipisicing\xC3\xB9k\xC3\xB9 elit\xC3\xA8b\xC3\xA8 N\xC3\xA8 sed do\xC3\xA0d\xC3\xAC*{: .italic .smcaps}\n \n \xEF\x9B\xA1 eiusmod tempor incididunt, ut\xC3\xAAn\xC3\xB9 labore et dolore. Tw\xC4\x83nj\xC3\xAC magna aliqua. Ut enim ad minim veniam\n {: .normal}\n@@ -9,5 +9,5 @@ quis nostrud\xC3\xAAt\xC3\xB9 exercitiation ullamco laboris m\xC3\xB9s\xC3\xB9k\xC3\xB9abc\xC3\xB9 nisi ".force_encoding('ASCII-8BIT'), "mu ns\n Lorem ipsum dolor sit amet, consectetur adipisicing ut\xC3\xA0y\xC3\xB9abcd\xC3\xB9efg kia elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non p\n {: .normal_pn}\n \n-Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in\n# *Lorem ipsum\xC3\xB9l\xC3\xB9l\xC3\xA0 dolor\xC3\xB9k\xC3\xB9 sit\xC3\xA8b\xC3\xA8 N\xC3\xA8 amet b\xC3\xA0d\xC3\xAC*\n+# *consectetur\xC3\xB9l\xC3\xB9l\xC3\xA0 adipisicing\xC3\xB9k\xC3\xB9 elit\xC3\xA8b\xC3\xA8 N\xC3\xA8 sed do\xC3\xA0d\xC3\xAC*{: .italic .smcaps}\n \n \xEF\x9B\xA1 eiusmod tempor incididunt, ut\xC3\xAAn\xC3\xB9 labore et dolore. Tw\xC4\x83nj\xC3\xAC magna aliqua. Ut enim ad minim veniam\n {: .normal}\n@@ -9,5 +9,5 @@ quis nostrud\xC3\xAAt\xC3\xB9 exercitiation ullamco laboris m\xC3\xB9s\xC3\xB9k\xC3\xB9abc\xC3\xB9 nisi " + ], + [ + 'string with detected encoding that is not supported in Ruby', + "\xFFe,i\xFF,\xB8oi,'\xB8,\xFF,-", + "--broken encoding: IBM420_ltr" ] ].each do |description, test_string, xpect| it description do From dc8cf732078dc03af52e4821683ea1c04946091e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 10 Feb 2018 14:20:44 -0800 Subject: [PATCH 25/26] GitLab QA: Add GITLAB_USER_TYPE to support different login types (e.g. standard, LDAP) GITLAB_USERNAME and GITLAB_PASSWORD may be specified for an LDAP login, but QA scenarios may need to know which type it is in order to log in successfully. --- qa/README.md | 6 ++++++ qa/qa/page/main/login.rb | 10 +++++++++- qa/qa/runtime/env.rb | 10 ++++++++++ qa/qa/runtime/user.rb | 4 ++++ qa/spec/runtime/env_spec.rb | 21 +++++++++++++++++++++ 5 files changed, 50 insertions(+), 1 deletion(-) diff --git a/qa/README.md b/qa/README.md index 44f3e262f4d..3a99a30d379 100644 --- a/qa/README.md +++ b/qa/README.md @@ -78,6 +78,12 @@ If your user doesn't have permission to default sandbox group GITLAB_USERNAME=jsmith GITLAB_PASSWORD=password GITLAB_SANDBOX_NAME=jsmith-qa-sandbox bin/qa Test::Instance https://gitlab.example.com ``` +In addition, the `GITLAB_USER_TYPE` can be set to "ldap" to sign in as an LDAP user: + +``` +GITLAB_USER_TYPE=ldap GITLAB_USERNAME=jsmith GITLAB_PASSWORD=password GITLAB_SANDBOX_NAME=jsmith-qa-sandbox bin/qa Test::Instance https://gitlab.example.com +``` + All [supported environment variables are here](https://gitlab.com/gitlab-org/gitlab-qa#supported-environment-variables). ### Building a Docker image to test diff --git a/qa/qa/page/main/login.rb b/qa/qa/page/main/login.rb index fd49b27cb1a..a8a5601dbe6 100644 --- a/qa/qa/page/main/login.rb +++ b/qa/qa/page/main/login.rb @@ -39,6 +39,14 @@ module QA end end + def sign_in_using_credentials + if Runtime::User.ldap_user? + sign_in_using_ldap_credentials + else + sign_in_using_gitlab_credentials + end + end + def sign_in_using_ldap_credentials using_wait_time 0 do set_initial_password_if_present @@ -51,7 +59,7 @@ module QA end end - def sign_in_using_credentials + def sign_in_using_gitlab_credentials using_wait_time 0 do set_initial_password_if_present diff --git a/qa/qa/runtime/env.rb b/qa/qa/runtime/env.rb index 115d5ee58d1..5401372e225 100644 --- a/qa/qa/runtime/env.rb +++ b/qa/qa/runtime/env.rb @@ -17,6 +17,16 @@ module QA ENV['PERSONAL_ACCESS_TOKEN'] end + # By default, "standard" denotes a standard GitLab user login. + # Set this to "ldap" if the user should be logged in via LDAP. + def user_type + (ENV['GITLAB_USER_TYPE'] || 'standard').tap do |type| + unless %w(ldap standard).include?(type) + raise ArgumentError.new("Invalid user type '#{type}': must be 'ldap' or 'standard'") + end + end + end + def user_username ENV['GITLAB_USERNAME'] end diff --git a/qa/qa/runtime/user.rb b/qa/qa/runtime/user.rb index 6b377f6b287..39e6adf9522 100644 --- a/qa/qa/runtime/user.rb +++ b/qa/qa/runtime/user.rb @@ -10,6 +10,10 @@ module QA def password Runtime::Env.user_password || '5iveL!fe' end + + def ldap_user? + Runtime::Env.user_type == 'ldap' + end end end end diff --git a/qa/spec/runtime/env_spec.rb b/qa/spec/runtime/env_spec.rb index 103573db6be..2b6365dbc41 100644 --- a/qa/spec/runtime/env_spec.rb +++ b/qa/spec/runtime/env_spec.rb @@ -55,4 +55,25 @@ describe QA::Runtime::Env do end end end + + describe '.user_type' do + it 'returns standard if not defined' do + expect(described_class.user_type).to eq('standard') + end + + it 'returns standard as defined' do + stub_env('GITLAB_USER_TYPE', 'standard') + expect(described_class.user_type).to eq('standard') + end + + it 'returns ldap as defined' do + stub_env('GITLAB_USER_TYPE', 'ldap') + expect(described_class.user_type).to eq('ldap') + end + + it 'returns an error if invalid user type' do + stub_env('GITLAB_USER_TYPE', 'foobar') + expect { described_class.user_type }.to raise_error(ArgumentError) + end + end end From 525d58303ba21601a03b847316642cf3e8d3c6d9 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Tue, 6 Feb 2018 17:20:07 +0200 Subject: [PATCH 26/26] Move IssuableTimeTracker vue component --- .../time_tracking/sidebar_time_tracking.js | 4 +- .../{time_tracker.js => time_tracker.vue} | 149 +++++++++--------- ...ve-issuable-time-tracker-vue-component.yml | 5 + .../javascripts/issuable_time_tracker_spec.js | 2 +- 4 files changed, 85 insertions(+), 75 deletions(-) rename app/assets/javascripts/sidebar/components/time_tracking/{time_tracker.js => time_tracker.vue} (59%) create mode 100644 changelogs/unreleased/refactor-move-issuable-time-tracker-vue-component.yml diff --git a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js index d32fe4abc7d..782e4ba4fad 100644 --- a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js +++ b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js @@ -2,7 +2,7 @@ import _ from 'underscore'; import '~/smart_interval'; -import timeTracker from './time_tracker'; +import IssuableTimeTracker from './time_tracker.vue'; import Store from '../../stores/sidebar_store'; import Mediator from '../../sidebar_mediator'; @@ -16,7 +16,7 @@ export default { }; }, components: { - 'issuable-time-tracker': timeTracker, + IssuableTimeTracker, }, methods: { listenForQuickActions() { diff --git a/app/assets/javascripts/sidebar/components/time_tracking/time_tracker.js b/app/assets/javascripts/sidebar/components/time_tracking/time_tracker.vue similarity index 59% rename from app/assets/javascripts/sidebar/components/time_tracking/time_tracker.js rename to app/assets/javascripts/sidebar/components/time_tracking/time_tracker.vue index 866178e2b23..230736a56b8 100644 --- a/app/assets/javascripts/sidebar/components/time_tracking/time_tracker.js +++ b/app/assets/javascripts/sidebar/components/time_tracking/time_tracker.vue @@ -1,3 +1,4 @@ + + + diff --git a/changelogs/unreleased/refactor-move-issuable-time-tracker-vue-component.yml b/changelogs/unreleased/refactor-move-issuable-time-tracker-vue-component.yml new file mode 100644 index 00000000000..5ed06c61817 --- /dev/null +++ b/changelogs/unreleased/refactor-move-issuable-time-tracker-vue-component.yml @@ -0,0 +1,5 @@ +--- +title: Move IssuableTimeTracker vue component +merge_request: 16948 +author: George Tsiolis +type: performance diff --git a/spec/javascripts/issuable_time_tracker_spec.js b/spec/javascripts/issuable_time_tracker_spec.js index 8ff93c4f918..365e9fe6a4b 100644 --- a/spec/javascripts/issuable_time_tracker_spec.js +++ b/spec/javascripts/issuable_time_tracker_spec.js @@ -2,7 +2,7 @@ import Vue from 'vue'; -import timeTracker from '~/sidebar/components/time_tracking/time_tracker'; +import timeTracker from '~/sidebar/components/time_tracking/time_tracker.vue'; function initTimeTrackingComponent(opts) { setFixtures(`