diff options
81 files changed, 1056 insertions, 926 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ba8a5c290ea..8703ef6823a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -310,7 +310,7 @@ review-docs-cleanup: when: manual script: - gem install gitlab --no-ri --no-rdoc - - ./SCRIPT_NAME cleanup + - ./$SCRIPT_NAME cleanup ## # Trigger a docker image build in CNG (Cloud Native GitLab) repository diff --git a/CHANGELOG.md b/CHANGELOG.md index eabacbc2e1d..72725122b8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.0.1 (2018-06-21) + +### Security (5 changes) + +- Fix XSS vulnerability for table of content generation. +- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability. +- HTML escape branch name in project graphs page. +- HTML escape the name of the user in ProjectsHelper#link_to_member. +- Don't show events from internal projects for anonymous users in public feed. + + ## 11.0.0 (2018-06-22) ### Security (3 changes) @@ -242,6 +253,17 @@ entry. - Workhorse to send raw diff and patch for commits. +## 10.8.5 (2018-06-21) + +### Security (5 changes) + +- Fix XSS vulnerability for table of content generation. +- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability. +- HTML escape branch name in project graphs page. +- HTML escape the name of the user in ProjectsHelper#link_to_member. +- Don't show events from internal projects for anonymous users in public feed. + + ## 10.8.4 (2018-06-06) - No changes. @@ -460,6 +482,22 @@ entry. - Gitaly handles repository forks by default. +## 10.7.6 (2018-06-21) + +### Security (6 changes) + +- Fix XSS vulnerability for table of content generation. +- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability. +- HTML escape branch name in project graphs page. +- HTML escape the name of the user in ProjectsHelper#link_to_member. +- Don't show events from internal projects for anonymous users in public feed. +- XSS fix to use safe_params instead of params in url_for helpers. + +### Other (1 change) + +- Replacing gollum libraries for gitlab custom libs. !18343 + + ## 10.7.5 (2018-05-28) ### Security (3 changes) @@ -230,7 +230,7 @@ gem 'ruby-fogbugz', '~> 0.2.1' gem 'kubeclient', '~> 3.1.0' # Sanitize user input -gem 'sanitize', '~> 2.0' +gem 'sanitize', '~> 4.6.5' gem 'babosa', '~> 1.0.2' # Sanitizes SVG input diff --git a/Gemfile.lock b/Gemfile.lock index fdc8f54e9c9..8281c1eff9a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -295,13 +295,13 @@ GEM flowdock (~> 0.7) gitlab-grit (>= 2.4.1) multi_json - gitlab-gollum-lib (4.2.7.4) + gitlab-gollum-lib (4.2.7.5) gemojione (~> 3.2) github-markup (~> 1.6) gollum-grit_adapter (~> 1.0) nokogiri (>= 1.6.1, < 2.0) rouge (~> 3.1) - sanitize (~> 2.1) + sanitize (~> 4.6.4) stringex (~> 2.6) gitlab-gollum-rugged_adapter (0.4.4.1) mime-types (>= 1.15) @@ -514,6 +514,8 @@ GEM netrc (0.11.0) nokogiri (1.8.2) mini_portile2 (~> 2.3.0) + nokogumbo (1.5.0) + nokogiri numerizer (0.1.1) oauth (0.5.4) oauth2 (1.4.0) @@ -804,8 +806,10 @@ GEM et-orbi (~> 1.0) rugged (0.27.2) safe_yaml (1.0.4) - sanitize (2.1.0) + sanitize (4.6.5) + crass (~> 1.0.2) nokogiri (>= 1.4.4) + nokogumbo (~> 1.4) sass (3.5.5) sass-listen (~> 4.0.0) sass-listen (4.0.0) @@ -1151,7 +1155,7 @@ DEPENDENCIES ruby_parser (~> 3.8) rufus-scheduler (~> 3.4) rugged (~> 0.27) - sanitize (~> 2.0) + sanitize (~> 4.6.5) sass-rails (~> 5.0.6) scss_lint (~> 0.56.0) seed-fu (~> 2.3.7) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index 679318b9be5..52388f17c7c 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -298,13 +298,13 @@ GEM flowdock (~> 0.7) gitlab-grit (>= 2.4.1) multi_json - gitlab-gollum-lib (4.2.7.4) + gitlab-gollum-lib (4.2.7.5) gemojione (~> 3.2) github-markup (~> 1.6) gollum-grit_adapter (~> 1.0) nokogiri (>= 1.6.1, < 2.0) rouge (~> 3.1) - sanitize (~> 2.1) + sanitize (~> 4.6.4) stringex (~> 2.6) gitlab-gollum-rugged_adapter (0.4.4.1) mime-types (>= 1.15) @@ -518,6 +518,8 @@ GEM nio4r (2.3.1) nokogiri (1.8.2) mini_portile2 (~> 2.3.0) + nokogumbo (1.5.0) + nokogiri numerizer (0.1.1) oauth (0.5.4) oauth2 (1.4.0) @@ -813,8 +815,10 @@ GEM et-orbi (~> 1.0) rugged (0.27.1) safe_yaml (1.0.4) - sanitize (2.1.0) + sanitize (4.6.5) + crass (~> 1.0.2) nokogiri (>= 1.4.4) + nokogumbo (~> 1.4) sass (3.5.5) sass-listen (~> 4.0.0) sass-listen (4.0.0) @@ -1162,7 +1166,7 @@ DEPENDENCIES ruby_parser (~> 3.8) rufus-scheduler (~> 3.4) rugged (~> 0.27) - sanitize (~> 2.0) + sanitize (~> 4.6.5) sass-rails (~> 5.0.6) scss_lint (~> 0.56.0) seed-fu (~> 2.3.7) diff --git a/app/assets/javascripts/boards/components/board_sidebar.js b/app/assets/javascripts/boards/components/board_sidebar.js index b717c4b0fd4..371be109229 100644 --- a/app/assets/javascripts/boards/components/board_sidebar.js +++ b/app/assets/javascripts/boards/components/board_sidebar.js @@ -6,13 +6,13 @@ import Flash from '../../flash'; import { __ } from '../../locale'; import Sidebar from '../../right_sidebar'; import eventHub from '../../sidebar/event_hub'; -import assigneeTitle from '../../sidebar/components/assignees/assignee_title.vue'; -import assignees from '../../sidebar/components/assignees/assignees.vue'; +import AssigneeTitle from '../../sidebar/components/assignees/assignee_title.vue'; +import Assignees from '../../sidebar/components/assignees/assignees.vue'; import DueDateSelectors from '../../due_date_select'; -import './sidebar/remove_issue'; +import RemoveBtn from './sidebar/remove_issue.vue'; import IssuableContext from '../../issuable_context'; import LabelsSelect from '../../labels_select'; -import subscriptions from '../../sidebar/components/subscriptions/subscriptions.vue'; +import Subscriptions from '../../sidebar/components/subscriptions/subscriptions.vue'; import MilestoneSelect from '../../milestone_select'; const Store = gl.issueBoards.BoardsStore; @@ -22,10 +22,10 @@ window.gl.issueBoards = window.gl.issueBoards || {}; gl.issueBoards.BoardSidebar = Vue.extend({ components: { - assigneeTitle, - assignees, - removeBtn: gl.issueBoards.RemoveIssueBtn, - subscriptions, + AssigneeTitle, + Assignees, + RemoveBtn, + Subscriptions, }, props: { currentUser: { diff --git a/app/assets/javascripts/boards/components/modal/header.js b/app/assets/javascripts/boards/components/modal/header.js index 5e511bb8935..cc9848058ca 100644 --- a/app/assets/javascripts/boards/components/modal/header.js +++ b/app/assets/javascripts/boards/components/modal/header.js @@ -1,12 +1,12 @@ import Vue from 'vue'; import modalFilters from './filters'; -import './tabs'; +import modalTabs from './tabs.vue'; import ModalStore from '../../stores/modal_store'; import modalMixin from '../../mixins/modal_mixins'; gl.issueBoards.ModalHeader = Vue.extend({ components: { - 'modal-tabs': gl.issueBoards.ModalTabs, + modalTabs, modalFilters, }, mixins: [modalMixin], diff --git a/app/assets/javascripts/boards/components/modal/tabs.js b/app/assets/javascripts/boards/components/modal/tabs.js deleted file mode 100644 index 9d331de8e22..00000000000 --- a/app/assets/javascripts/boards/components/modal/tabs.js +++ /dev/null @@ -1,46 +0,0 @@ -import Vue from 'vue'; -import ModalStore from '../../stores/modal_store'; -import modalMixin from '../../mixins/modal_mixins'; - -gl.issueBoards.ModalTabs = Vue.extend({ - mixins: [modalMixin], - data() { - return ModalStore.store; - }, - computed: { - selectedCount() { - return ModalStore.selectedCount(); - }, - }, - destroyed() { - this.activeTab = 'all'; - }, - template: ` - <div class="top-area prepend-top-10 append-bottom-10"> - <ul class="nav-links issues-state-filters"> - <li :class="{ 'active': activeTab == 'all' }"> - <a - href="#" - role="button" - @click.prevent="changeTab('all')"> - Open issues - <span class="badge badge-pill"> - {{ issuesCount }} - </span> - </a> - </li> - <li :class="{ 'active': activeTab == 'selected' }"> - <a - href="#" - role="button" - @click.prevent="changeTab('selected')"> - Selected issues - <span class="badge badge-pill"> - {{ selectedCount }} - </span> - </a> - </li> - </ul> - </div> - `, -}); diff --git a/app/assets/javascripts/boards/components/modal/tabs.vue b/app/assets/javascripts/boards/components/modal/tabs.vue new file mode 100644 index 00000000000..d926b080094 --- /dev/null +++ b/app/assets/javascripts/boards/components/modal/tabs.vue @@ -0,0 +1,49 @@ +<script> + import ModalStore from '../../stores/modal_store'; + import modalMixin from '../../mixins/modal_mixins'; + + export default { + mixins: [modalMixin], + data() { + return ModalStore.store; + }, + computed: { + selectedCount() { + return ModalStore.selectedCount(); + }, + }, + destroyed() { + this.activeTab = 'all'; + }, + }; +</script> +<template> + <div class="top-area prepend-top-10 append-bottom-10"> + <ul class="nav-links issues-state-filters"> + <li :class="{ 'active': activeTab == 'all' }"> + <a + href="#" + role="button" + @click.prevent="changeTab('all')" + > + Open issues + <span class="badge badge-pill"> + {{ issuesCount }} + </span> + </a> + </li> + <li :class="{ 'active': activeTab == 'selected' }"> + <a + href="#" + role="button" + @click.prevent="changeTab('selected')" + > + Selected issues + <span class="badge badge-pill"> + {{ selectedCount }} + </span> + </a> + </li> + </ul> + </div> +</template> diff --git a/app/assets/javascripts/boards/components/sidebar/remove_issue.js b/app/assets/javascripts/boards/components/sidebar/remove_issue.js deleted file mode 100644 index 0a0820ec5fd..00000000000 --- a/app/assets/javascripts/boards/components/sidebar/remove_issue.js +++ /dev/null @@ -1,73 +0,0 @@ -import Vue from 'vue'; -import Flash from '../../../flash'; -import { __ } from '../../../locale'; - -const Store = gl.issueBoards.BoardsStore; - -window.gl = window.gl || {}; -window.gl.issueBoards = window.gl.issueBoards || {}; - -gl.issueBoards.RemoveIssueBtn = Vue.extend({ - props: { - issue: { - type: Object, - required: true, - }, - list: { - type: Object, - required: true, - }, - }, - computed: { - updateUrl() { - return this.issue.path; - }, - }, - methods: { - removeIssue() { - const issue = this.issue; - const lists = issue.getLists(); - const listLabelIds = lists.map(list => list.label.id); - - let labelIds = issue.labels - .map(label => label.id) - .filter(id => !listLabelIds.includes(id)); - if (labelIds.length === 0) { - labelIds = ['']; - } - - const data = { - issue: { - label_ids: labelIds, - }, - }; - - // Post the remove data - Vue.http.patch(this.updateUrl, data).catch(() => { - Flash(__('Failed to remove issue from board, please try again.')); - - lists.forEach((list) => { - list.addIssue(issue); - }); - }); - - // Remove from the frontend store - lists.forEach((list) => { - list.removeIssue(issue); - }); - - Store.detail.issue = {}; - }, - }, - template: ` - <div - class="block list"> - <button - class="btn btn-default btn-block" - type="button" - @click="removeIssue"> - Remove from board - </button> - </div> - `, -}); diff --git a/app/assets/javascripts/boards/components/sidebar/remove_issue.vue b/app/assets/javascripts/boards/components/sidebar/remove_issue.vue new file mode 100644 index 00000000000..806e038a95f --- /dev/null +++ b/app/assets/javascripts/boards/components/sidebar/remove_issue.vue @@ -0,0 +1,72 @@ +<script> + import Vue from 'vue'; + import Flash from '../../../flash'; + import { __ } from '../../../locale'; + + const Store = gl.issueBoards.BoardsStore; + + export default { + props: { + issue: { + type: Object, + required: true, + }, + list: { + type: Object, + required: true, + }, + }, + computed: { + updateUrl() { + return this.issue.path; + }, + }, + methods: { + removeIssue() { + const issue = this.issue; + const lists = issue.getLists(); + const listLabelIds = lists.map(list => list.label.id); + + let labelIds = issue.labels.map(label => label.id).filter(id => !listLabelIds.includes(id)); + if (labelIds.length === 0) { + labelIds = ['']; + } + + const data = { + issue: { + label_ids: labelIds, + }, + }; + + // Post the remove data + Vue.http.patch(this.updateUrl, data).catch(() => { + Flash(__('Failed to remove issue from board, please try again.')); + + lists.forEach(list => { + list.addIssue(issue); + }); + }); + + // Remove from the frontend store + lists.forEach(list => { + list.removeIssue(issue); + }); + + Store.detail.issue = {}; + }, + }, + }; +</script> +<template> + <div + class="block list" + > + <button + class="btn btn-default btn-block" + type="button" + @click="removeIssue" + > + Remove from board + </button> + </div> +</template> diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index 9de57db48fd..b0f674f2c05 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -7,6 +7,16 @@ function sanitize(str) { return str.replace(/<(?:.|\n)*?>/gm, ''); } +export const defaultAutocompleteConfig = { + emojis: true, + members: true, + issues: true, + mergeRequests: true, + epics: false, + milestones: true, + labels: true, +}; + class GfmAutoComplete { constructor(dataSources) { this.dataSources = dataSources || {}; @@ -14,14 +24,7 @@ class GfmAutoComplete { this.isLoadingData = {}; } - setup(input, enableMap = { - emojis: true, - members: true, - issues: true, - milestones: true, - mergeRequests: true, - labels: true, - }) { + setup(input, enableMap = defaultAutocompleteConfig) { // Add GFM auto-completion to all input fields, that accept GFM input. this.input = input || $('.js-gfm-input'); this.enableMap = enableMap; diff --git a/app/assets/javascripts/gl_form.js b/app/assets/javascripts/gl_form.js index 9f5eba353d7..f802971a3ca 100644 --- a/app/assets/javascripts/gl_form.js +++ b/app/assets/javascripts/gl_form.js @@ -1,14 +1,14 @@ import $ from 'jquery'; import autosize from 'autosize'; -import GfmAutoComplete from './gfm_auto_complete'; +import GfmAutoComplete, * as GFMConfig from './gfm_auto_complete'; import dropzoneInput from './dropzone_input'; import { addMarkdownListeners, removeMarkdownListeners } from './lib/utils/text_markdown'; export default class GLForm { - constructor(form, enableGFM = false) { + constructor(form, enableGFM = {}) { this.form = form; this.textarea = this.form.find('textarea.js-gfm-input'); - this.enableGFM = enableGFM; + this.enableGFM = Object.assign({}, GFMConfig.defaultAutocompleteConfig, enableGFM); // Before we start, we should clean up any previous data for this form this.destroy(); // Setup the form @@ -34,14 +34,7 @@ export default class GLForm { // remove notify commit author checkbox for non-commit notes gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button, .js-note-new-discussion')); this.autoComplete = new GfmAutoComplete(gl.GfmAutoComplete && gl.GfmAutoComplete.dataSources); - this.autoComplete.setup(this.form.find('.js-gfm-input'), { - emojis: true, - members: this.enableGFM, - issues: this.enableGFM, - milestones: this.enableGFM, - mergeRequests: this.enableGFM, - labels: this.enableGFM, - }); + this.autoComplete.setup(this.form.find('.js-gfm-input'), this.enableGFM); dropzoneInput(this.form); autosize(this.textarea); } diff --git a/app/assets/javascripts/init_notes.js b/app/assets/javascripts/init_notes.js index 882aedfcc76..3c71258e53b 100644 --- a/app/assets/javascripts/init_notes.js +++ b/app/assets/javascripts/init_notes.js @@ -7,10 +7,10 @@ export default () => { notesIds, now, diffView, - autocomplete, + enableGFM, } = JSON.parse(dataEl.innerHTML); // Create a singleton so that we don't need to assign // into the window object, we can just access the current isntance with Notes.instance - Notes.initialize(notesUrl, notesIds, now, diffView, autocomplete); + Notes.initialize(notesUrl, notesIds, now, diffView, enableGFM); }; diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 2f752d2dcd6..da1a52155d8 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -20,6 +20,7 @@ import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_c import axios from './lib/utils/axios_utils'; import { getLocationHash } from './lib/utils/url_utility'; import Flash from './flash'; +import { defaultAutocompleteConfig } from './gfm_auto_complete'; import CommentTypeToggle from './comment_type_toggle'; import GLForm from './gl_form'; import loadAwardsHandler from './awards_handler'; @@ -45,7 +46,7 @@ const MAX_VISIBLE_COMMIT_LIST_COUNT = 3; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; export default class Notes { - static initialize(notes_url, note_ids, last_fetched_at, view, enableGFM = true) { + static initialize(notes_url, note_ids, last_fetched_at, view, enableGFM) { if (!this.instance) { this.instance = new Notes(notes_url, note_ids, last_fetched_at, view, enableGFM); } @@ -55,7 +56,7 @@ export default class Notes { return this.instance; } - constructor(notes_url, note_ids, last_fetched_at, view, enableGFM = true) { + constructor(notes_url, note_ids, last_fetched_at, view, enableGFM = defaultAutocompleteConfig) { this.updateTargetButtons = this.updateTargetButtons.bind(this); this.updateComment = this.updateComment.bind(this); this.visibilityChange = this.visibilityChange.bind(this); @@ -94,7 +95,7 @@ export default class Notes { this.cleanBinding(); this.addBinding(); this.setPollingInterval(); - this.setupMainTargetNoteForm(); + this.setupMainTargetNoteForm(enableGFM); this.taskList = new TaskList({ dataType: 'note', fieldName: 'note', @@ -598,14 +599,14 @@ export default class Notes { * * Sets some hidden fields in the form. */ - setupMainTargetNoteForm() { + setupMainTargetNoteForm(enableGFM) { var form; // find the form form = $('.js-new-note-form'); // Set a global clone of the form for later cloning this.formClone = form.clone(); // show the form - this.setupNoteForm(form); + this.setupNoteForm(form, enableGFM); // fix classes form.removeClass('js-new-note-form'); form.addClass('js-main-target-form'); @@ -633,9 +634,9 @@ export default class Notes { * setup GFM auto complete * show the form */ - setupNoteForm(form) { + setupNoteForm(form, enableGFM = defaultAutocompleteConfig) { var textarea, key; - this.glForm = new GLForm(form, this.enableGFM); + this.glForm = new GLForm(form, enableGFM); textarea = form.find('.js-note-text'); key = [ 'Note', diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index a62696b39b4..a4e3faa5d75 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -194,7 +194,7 @@ js-autosize markdown-area js-vue-issue-note-form js-vue-textarea" class="btn btn-cancel note-edit-cancel js-close-discussion-note-form" type="button" @click="cancelHandler()"> - Cancel + {{ __('Discard draft') }} </button> </div> </form> diff --git a/app/assets/javascripts/pages/projects/init_form.js b/app/assets/javascripts/pages/projects/init_form.js index 0b6c5c1d30b..9f20a3e4e46 100644 --- a/app/assets/javascripts/pages/projects/init_form.js +++ b/app/assets/javascripts/pages/projects/init_form.js @@ -3,5 +3,5 @@ import GLForm from '~/gl_form'; export default function ($formEl) { new ZenMode(); // eslint-disable-line no-new - new GLForm($formEl, true); // eslint-disable-line no-new + new GLForm($formEl); // eslint-disable-line no-new } diff --git a/app/assets/javascripts/pages/projects/issues/form.js b/app/assets/javascripts/pages/projects/issues/form.js index 14fddbc9a05..b2b8e5d2300 100644 --- a/app/assets/javascripts/pages/projects/issues/form.js +++ b/app/assets/javascripts/pages/projects/issues/form.js @@ -10,7 +10,7 @@ import IssuableTemplateSelectors from '~/templates/issuable_template_selectors'; export default () => { new ShortcutsNavigation(); - new GLForm($('.issue-form'), true); + new GLForm($('.issue-form')); new IssuableForm($('.issue-form')); new LabelsSelect(); new MilestoneSelect(); diff --git a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js index 406fc32f9a2..3a3c21f2202 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js +++ b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js @@ -12,7 +12,7 @@ import IssuableTemplateSelectors from '~/templates/issuable_template_selectors'; export default () => { new Diff(); new ShortcutsNavigation(); - new GLForm($('.merge-request-form'), true); + new GLForm($('.merge-request-form')); new IssuableForm($('.merge-request-form')); new LabelsSelect(); new MilestoneSelect(); diff --git a/app/assets/javascripts/pages/projects/tags/new/index.js b/app/assets/javascripts/pages/projects/tags/new/index.js index 8d0edf7e06c..b3158f7e939 100644 --- a/app/assets/javascripts/pages/projects/tags/new/index.js +++ b/app/assets/javascripts/pages/projects/tags/new/index.js @@ -5,6 +5,6 @@ import GLForm from '../../../../gl_form'; document.addEventListener('DOMContentLoaded', () => { new ZenMode(); // eslint-disable-line no-new - new GLForm($('.tag-form'), true); // eslint-disable-line no-new + new GLForm($('.tag-form')); // eslint-disable-line no-new new RefSelectDropdown($('.js-branch-select')); // eslint-disable-line no-new }); diff --git a/app/assets/javascripts/pages/projects/wikis/index.js b/app/assets/javascripts/pages/projects/wikis/index.js index 0295653cb29..0a0fe3fc137 100644 --- a/app/assets/javascripts/pages/projects/wikis/index.js +++ b/app/assets/javascripts/pages/projects/wikis/index.js @@ -12,7 +12,7 @@ document.addEventListener('DOMContentLoaded', () => { new Wikis(); // eslint-disable-line no-new new ShortcutsWiki(); // eslint-disable-line no-new new ZenMode(); // eslint-disable-line no-new - new GLForm($('.wiki-form'), true); // eslint-disable-line no-new + new GLForm($('.wiki-form')); // eslint-disable-line no-new const deleteWikiButton = document.getElementById('delete-wiki-button'); diff --git a/app/assets/javascripts/pages/snippets/form.js b/app/assets/javascripts/pages/snippets/form.js index 72d05da1069..758bbafead3 100644 --- a/app/assets/javascripts/pages/snippets/form.js +++ b/app/assets/javascripts/pages/snippets/form.js @@ -3,6 +3,13 @@ import GLForm from '~/gl_form'; import ZenMode from '~/zen_mode'; export default () => { - new GLForm($('.snippet-form'), false); // eslint-disable-line no-new + // eslint-disable-next-line no-new + new GLForm($('.snippet-form'), { + members: false, + issues: false, + mergeRequests: false, + milestones: false, + labels: false, + }); new ZenMode(); // eslint-disable-line no-new }; diff --git a/app/assets/javascripts/shared/milestones/form.js b/app/assets/javascripts/shared/milestones/form.js index 2f974d6ff9d..060f374310c 100644 --- a/app/assets/javascripts/shared/milestones/form.js +++ b/app/assets/javascripts/shared/milestones/form.js @@ -6,5 +6,13 @@ import GLForm from '../../gl_form'; export default (initGFM = true) => { new ZenMode(); // eslint-disable-line no-new new DueDateSelectors(); // eslint-disable-line no-new - new GLForm($('.milestone-form'), initGFM); // eslint-disable-line no-new + // eslint-disable-next-line no-new + new GLForm($('.milestone-form'), { + emojis: initGFM, + members: initGFM, + issues: initGFM, + mergeRequests: initGFM, + milestones: initGFM, + labels: initGFM, + }); }; diff --git a/app/assets/javascripts/vue_shared/components/markdown/field.vue b/app/assets/javascripts/vue_shared/components/markdown/field.vue index 7d26390d9bc..fba67681777 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/field.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/field.vue @@ -62,7 +62,14 @@ /* GLForm class handles all the toolbar buttons */ - return new GLForm($(this.$refs['gl-form']), this.enableAutocomplete); + return new GLForm($(this.$refs['gl-form']), { + emojis: this.enableAutocomplete, + members: this.enableAutocomplete, + issues: this.enableAutocomplete, + mergeRequests: this.enableAutocomplete, + milestones: this.enableAutocomplete, + labels: this.enableAutocomplete, + }); }, beforeDestroy() { const glForm = $(this.$refs['gl-form']).data('glForm'); diff --git a/app/assets/stylesheets/pages/repo.scss b/app/assets/stylesheets/pages/repo.scss index 0a56153203c..bd3320c8ae5 100644 --- a/app/assets/stylesheets/pages/repo.scss +++ b/app/assets/stylesheets/pages/repo.scss @@ -281,8 +281,8 @@ } .margin { - background-color: $gray-light; - border-right: 1px solid $white-normal; + background-color: $white-light; + border-right: 1px solid $theme-gray-100; .line-insert { border-right: 1px solid $line-added-dark; @@ -303,6 +303,15 @@ .multi-file-editor-holder { height: 100%; min-height: 0; + + &.is-readonly, + .editor.original { + .monaco-editor, + .monaco-editor-background, + .monaco-editor .inputarea.ime-input { + background-color: $theme-gray-50; + } + } } .preview-container { diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 001f6520093..96b7bc65ac9 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -72,10 +72,10 @@ class Admin::GroupsController < Admin::ApplicationController end def group_params - params.require(:group).permit(group_params_ce) + params.require(:group).permit(allowed_group_params) end - def group_params_ce + def allowed_group_params [ :avatar, :description, diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index bfeb5a2d097..653f3dfffc4 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -187,10 +187,10 @@ class Admin::UsersController < Admin::ApplicationController end def user_params - params.require(:user).permit(user_params_ce) + params.require(:user).permit(allowed_user_params) end - def user_params_ce + def allowed_user_params [ :access_level, :avatar, diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index ba62d2d5142..1547d4b5972 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -119,7 +119,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController set_remember_me(user) - if user.two_factor_enabled? + if user.two_factor_enabled? && !auth_user.bypass_two_factor? prompt_for_two_factor(user) else sign_in_and_redirect(user) diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb index 65d6e019746..74776b2ed1f 100644 --- a/app/finders/user_recent_events_finder.rb +++ b/app/finders/user_recent_events_finder.rb @@ -56,7 +56,7 @@ class UserRecentEventsFinder visible = target_user .project_interactions - .where(visibility_level: [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]) + .where(visibility_level: Gitlab::VisibilityLevel.levels_for_user(current_user)) .select(:id) Gitlab::SQL::Union.new([authorized, visible]).to_sql diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 5459bb63397..e1a0cf1604c 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -143,7 +143,14 @@ module NotesHelper notesIds: @notes.map(&:id), now: Time.now.to_i, diffView: diff_view, - autocomplete: autocomplete + enableGFM: { + emojis: true, + members: autocomplete, + issues: autocomplete, + mergeRequests: autocomplete, + milestones: autocomplete, + labels: autocomplete + } } end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index be3958c40a4..c7a434ea092 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -40,7 +40,8 @@ module ProjectsHelper name_tag_options[:class] << 'has-tooltip' end - content_tag(:span, sanitize(username), name_tag_options) + # NOTE: ActionView::Helpers::TagHelper#content_tag HTML escapes username + content_tag(:span, username, name_tag_options) end def link_to_member(project, author, opts = {}, &block) @@ -506,6 +507,14 @@ module ProjectsHelper end end + def sidebar_projects_paths + %w[ + projects#show + projects#activity + cycle_analytics#show + ] + end + def sidebar_settings_paths %w[ projects#edit diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f112c06e26f..6c96c8ca391 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -129,9 +129,7 @@ class MergeRequest < ActiveRecord::Base after_transition unchecked: :cannot_be_merged do |merge_request, transition| begin - # Merge request can become unmergeable due to many reasons. - # We only notify if it is due to conflict. - unless merge_request.project.repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch) + if merge_request.notify_conflict? NotificationService.new.merge_request_unmergeable(merge_request) TodoService.new.merge_request_became_unmergeable(merge_request) end @@ -708,6 +706,10 @@ class MergeRequest < ActiveRecord::Base should_remove_source_branch? || force_remove_source_branch? end + def notify_conflict? + (opened? || locked?) && !project.repository.can_be_merged?(diff_head_sha, target_branch) + end + def related_notes # Fetch comments only from last 100 commits commits_for_notes_limit = 100 diff --git a/app/models/repository.rb b/app/models/repository.rb index 3089d0162ee..3056c20516a 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -21,7 +21,7 @@ class Repository attr_accessor :full_path, :disk_path, :project, :is_wiki delegate :ref_name_for_sha, to: :raw_repository - delegate :bundle_to_disk, :create_from_bundle, to: :raw_repository + delegate :bundle_to_disk, to: :raw_repository CreateTreeError = Class.new(StandardError) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 7ec52b6ce2b..8a86e47f0ea 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -82,7 +82,7 @@ class WebHookService post_url = hook.url.gsub("#{parsed_url.userinfo}@", '') basic_auth = { username: CGI.unescape(parsed_url.user), - password: CGI.unescape(parsed_url.password) + password: CGI.unescape(parsed_url.password.presence || '') } make_request(post_url, basic_auth) end diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml index 9f8b3b86474..33416bf76d7 100644 --- a/app/views/layouts/nav/sidebar/_project.html.haml +++ b/app/views/layouts/nav/sidebar/_project.html.haml @@ -8,7 +8,7 @@ .sidebar-context-title = @project.name %ul.sidebar-top-level-items - = nav_link(path: ['projects#show', 'projects#activity', 'cycle_analytics#show'], html_options: { class: 'home' }) do + = nav_link(path: sidebar_projects_paths, html_options: { class: 'home' }) do = link_to project_path(@project), class: 'shortcuts-project' do .nav-icon-container = sprite_icon('project') @@ -29,13 +29,13 @@ = link_to activity_project_path(@project), title: _('Activity'), class: 'shortcuts-project-activity' do %span= _('Activity') + = render_if_exists 'projects/sidebar/security_dashboard' + - if can?(current_user, :read_cycle_analytics, @project) = nav_link(path: 'cycle_analytics#show') do = link_to project_cycle_analytics_path(@project), title: _('Cycle Analytics'), class: 'shortcuts-project-cycle-analytics' do %span= _('Cycle Analytics') - = render_if_exists 'projects/sidebar/security_dashboard' - - if project_nav_tab? :files = nav_link(controller: sidebar_repository_paths) do = link_to project_tree_path(@project), class: 'shortcuts-tree' do diff --git a/app/views/projects/graphs/charts.html.haml b/app/views/projects/graphs/charts.html.haml index 983cb187c2f..3f1974d05f4 100644 --- a/app/views/projects/graphs/charts.html.haml +++ b/app/views/projects/graphs/charts.html.haml @@ -30,7 +30,7 @@ #{@commits_graph.start_date.strftime('%b %d')} - end_time = capture do #{@commits_graph.end_date.strftime('%b %d')} - = (_("Commit statistics for %{ref} %{start_time} - %{end_time}") % { ref: "<strong>#{@ref}</strong>", start_time: start_time, end_time: end_time }).html_safe + = (_("Commit statistics for %{ref} %{start_time} - %{end_time}") % { ref: "<strong>#{h @ref}</strong>", start_time: start_time, end_time: end_time }).html_safe .col-md-6 .tree-ref-container diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml index c360f1ffe2a..6b2715b47a7 100644 --- a/app/views/shared/notes/_form.html.haml +++ b/app/views/shared/notes/_form.html.haml @@ -40,5 +40,5 @@ = yield(:note_actions) - %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } + %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Discard draft" } } Discard draft diff --git a/changelogs/unreleased/45933-webide-fade-uneditable-area.yml b/changelogs/unreleased/45933-webide-fade-uneditable-area.yml new file mode 100644 index 00000000000..dfb186122e7 --- /dev/null +++ b/changelogs/unreleased/45933-webide-fade-uneditable-area.yml @@ -0,0 +1,5 @@ +--- +title: Fade uneditable area in Web IDE +merge_request: 20008 +author: +type: changed diff --git a/changelogs/unreleased/46571-webhooks-nil-password.yml b/changelogs/unreleased/46571-webhooks-nil-password.yml new file mode 100644 index 00000000000..34c5f09478f --- /dev/null +++ b/changelogs/unreleased/46571-webhooks-nil-password.yml @@ -0,0 +1,5 @@ +--- +title: Fix webhook error when password is not present +merge_request: 19945 +author: Jan Beckmann +type: fixed diff --git a/changelogs/unreleased/6591-dont-load-omniauth-if-not-enabled.yml b/changelogs/unreleased/6591-dont-load-omniauth-if-not-enabled.yml new file mode 100644 index 00000000000..dd1c7e6955d --- /dev/null +++ b/changelogs/unreleased/6591-dont-load-omniauth-if-not-enabled.yml @@ -0,0 +1,5 @@ +--- +title: Only load Omniauth if enabled +merge_request: 20132 +author: +type: fixed diff --git a/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml new file mode 100644 index 00000000000..ae92c20fa1a --- /dev/null +++ b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml @@ -0,0 +1,5 @@ +--- +title: Notify conflict for only open merge request +merge_request: 20125 +author: +type: fixed diff --git a/changelogs/unreleased/security-2682-fix-xss-for-markdown-toc.yml b/changelogs/unreleased/security-2682-fix-xss-for-markdown-toc.yml new file mode 100644 index 00000000000..f595678c3c2 --- /dev/null +++ b/changelogs/unreleased/security-2682-fix-xss-for-markdown-toc.yml @@ -0,0 +1,5 @@ +--- +title: Fix XSS vulnerability for table of content generation +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fj-bumping-sanitize-gem.yml b/changelogs/unreleased/security-fj-bumping-sanitize-gem.yml new file mode 100644 index 00000000000..bec1033425d --- /dev/null +++ b/changelogs/unreleased/security-fj-bumping-sanitize-gem.yml @@ -0,0 +1,5 @@ +--- +title: Update sanitize gem to 4.6.5 to fix HTML injection vulnerability +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-html_escape_branch_name.yml b/changelogs/unreleased/security-html_escape_branch_name.yml new file mode 100644 index 00000000000..02d1065348f --- /dev/null +++ b/changelogs/unreleased/security-html_escape_branch_name.yml @@ -0,0 +1,5 @@ +--- +title: HTML escape branch name in project graphs page +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-html_escape_usernames.yml b/changelogs/unreleased/security-html_escape_usernames.yml new file mode 100644 index 00000000000..7e69e4ae266 --- /dev/null +++ b/changelogs/unreleased/security-html_escape_usernames.yml @@ -0,0 +1,5 @@ +--- +title: HTML escape the name of the user in ProjectsHelper#link_to_member +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-rd-do-not-show-internal-info-in-public-feed.yml b/changelogs/unreleased/security-rd-do-not-show-internal-info-in-public-feed.yml new file mode 100644 index 00000000000..ff78c162dff --- /dev/null +++ b/changelogs/unreleased/security-rd-do-not-show-internal-info-in-public-feed.yml @@ -0,0 +1,5 @@ +--- +title: Don't show events from internal projects for anonymous users in public feed +merge_request: +author: +type: security diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 362b9cc9a88..d051b699102 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -219,5 +219,7 @@ Devise.setup do |config| end end - Gitlab::OmniauthInitializer.new(config).execute(Gitlab.config.omniauth.providers) + if Gitlab.config.omniauth.enabled + Gitlab::OmniauthInitializer.new(config).execute(Gitlab.config.omniauth.providers) + end end diff --git a/doc/development/documentation/index.md b/doc/development/documentation/index.md index 48e1685082a..f5cdd310f6f 100644 --- a/doc/development/documentation/index.md +++ b/doc/development/documentation/index.md @@ -322,50 +322,49 @@ to EE only. ## Previewing the changes live -To preview your changes to documentation locally, please follow -this [development guide](https://gitlab.com/gitlab-com/gitlab-docs/blob/master/README.md#development). +NOTE: **Note:** +To preview your changes to documentation locally, follow this +[development guide](https://gitlab.com/gitlab-com/gitlab-docs/blob/master/README.md#development). -If you want to preview the doc changes of your merge request live, you can use -the manual `review-docs-deploy` job in your merge request. You will need at -least Maintainer permissions to be able to run it and is currently enabled for the -following projects: +The live preview is currently enabled for the following projects: - https://gitlab.com/gitlab-org/gitlab-ce - https://gitlab.com/gitlab-org/gitlab-ee +- https://gitlab.com/gitlab-org/gitlab-runner -NOTE: **Note:** -You will need to push a branch to those repositories, it doesn't work for forks. - -TIP: **Tip:** If your branch contains only documentation changes, you can use [special branch names](#branch-naming) to avoid long running pipelines. -In the mini pipeline graph, you should see an `>>` icon. Clicking on it will -reveal the `review-docs-deploy` job. Hit the play button for the job to start. +For [docs-only changes](#branch-naming), the review app is run automatically. +For all other branches, you can use the manual `review-docs-deploy-manual` job +in your merge request. You will need at least Maintainer permissions to be able +to run it. In the mini pipeline graph, you should see an `>>` icon. Clicking on it will +reveal the `review-docs-deploy-manual` job. Hit the play button for the job to start.  -This job will: +NOTE: **Note:** +You will need to push a branch to those repositories, it doesn't work for forks. + +The `review-docs-deploy*` job will: 1. Create a new branch in the [gitlab-docs](https://gitlab.com/gitlab-com/gitlab-docs) - project named after the scheme: `preview-<branch-slug>` + project named after the scheme: `$DOCS_GITLAB_REPO_SUFFIX-$CI_ENVIRONMENT_SLUG`, + where `DOCS_GITLAB_REPO_SUFFIX` is the suffix for each product, e.g, `ce` for + CE, etc. 1. Trigger a cross project pipeline and build the docs site with your changes After a few minutes, the Review App will be deployed and you will be able to preview the changes. The docs URL can be found in two places: - In the merge request widget -- In the output of the `review-docs-deploy` job, which also includes the +- In the output of the `review-docs-deploy*` job, which also includes the triggered pipeline so that you can investigate whether something went wrong In case the Review App URL returns 404, follow these steps to debug: 1. **Did you follow the URL from the merge request widget?** If yes, then check if - the link is the same as the one in the job output. It can happen that if the - branch name slug is longer than 35 characters, it is automatically - truncated. That means that the merge request widget will not show the proper - URL due to a limitation of how `environment: url` works, but you can find the - real URL from the output of the `review-docs-deploy` job. + the link is the same as the one in the job output. 1. **Did you follow the URL from the job output?** If yes, then it means that either the site is not yet deployed or something went wrong with the remote pipeline. Give it a few minutes and it should appear online, otherwise you diff --git a/doc/integration/saml.md b/doc/integration/saml.md index 3f49432ce93..db06efdae53 100644 --- a/doc/integration/saml.md +++ b/doc/integration/saml.md @@ -179,6 +179,81 @@ tell GitLab which groups are external via the `external_groups:` element: } } ``` +## Bypass two factor authentication + +If you want some SAML authentication methods to count as 2FA on a per session basis, you can register them in the +`upstream_two_factor_authn_contexts` list: + +**For Omnibus installations:** + +1. Edit `/etc/gitlab/gitlab.rb`: + + ```ruby + gitlab_rails['omniauth_providers'] = [ + { + name: 'saml', + args: { + assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', + idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', + idp_sso_target_url: 'https://login.example.com/idp', + issuer: 'https://gitlab.example.com', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent', + upstream_two_factor_authn_contexts: + %w( + urn:oasis:names:tc:SAML:2.0:ac:classes:CertificateProtectedTransport + urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS + urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN + ) + + }, + label: 'Company Login' # optional label for SAML login button, defaults to "Saml" + } + ] + ``` + +1. Save the file and [reconfigure][] GitLab for the changes to take effect. + +--- + +**For installations from source:** + +1. Edit `config/gitlab.yml`: + + ```yaml + - { + name: 'saml', + args: { + assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', + idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', + idp_sso_target_url: 'https://login.example.com/idp', + issuer: 'https://gitlab.example.com', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent', + upstream_two_factor_authn_contexts: + [ + 'urn:oasis:names:tc:SAML:2.0:ac:classes:CertificateProtectedTransport', + 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS', + 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN' + ] + + }, + label: 'Company Login' # optional label for SAML login button, defaults to "Saml" + } + ``` + +1. Save the file and [restart GitLab][] for the changes ot take effect + + +In addition to the changes in GitLab, make sure that your Idp is returning the +`AuthnContext`. For example: + +```xml + <saml:AuthnStatement> + <saml:AuthnContext> + <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:MediumStrongCertificateProtectedTransport</saml:AuthnContextClassRef> + </saml:AuthnContext> + </saml:AuthnStatement> +``` + ## Customization ### `auto_sign_in_with_provider` diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index edb0c6bdc30..5dc62a30128 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -111,7 +111,7 @@ by yourself (except when an issue is due). You will only receive automatic notifications when somebody else comments or adds changes to the ones that you've created or mentions you. -If a merge request becomes unmergeable, its author will be notified about the cause. +If an open merge request becomes unmergeable due to conflict, its author will be notified about the cause. If a user has also set the merge request to automatically merge once pipeline succeeds, then that user will also be notified. diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index 762bf616268..760cd87d4cc 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -31,7 +31,7 @@ A Todo appears in your Todos dashboard when: - you are `@mentioned` in a comment on a commit, - a job in the CI pipeline running for your merge request failed, but this job is not allowed to fail. -- a merge request becomes unmergeable, and you are either: +- an open merge request becomes unmergeable due to conflict, and you are either: - the author, or - have set it to automatically merge once pipeline succeeds. diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index 6786b9d07b6..afc2ca4e362 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -25,10 +25,11 @@ module Banzai # Only push these customizations once return if customized?(whitelist[:transformers]) - # Allow table alignment; we whitelist specific style properties in a + # Allow table alignment; we whitelist specific text-align values in a # transformer below whitelist[:attributes]['th'] = %w(style) whitelist[:attributes]['td'] = %w(style) + whitelist[:css] = { properties: ['text-align'] } # Allow span elements whitelist[:elements].push('span') diff --git a/lib/banzai/filter/table_of_contents_filter.rb b/lib/banzai/filter/table_of_contents_filter.rb index 97244159985..b32660a8341 100644 --- a/lib/banzai/filter/table_of_contents_filter.rb +++ b/lib/banzai/filter/table_of_contents_filter.rb @@ -92,7 +92,7 @@ module Banzai def text return '' unless node - @text ||= node.text + @text ||= EscapeUtils.escape_html(node.text) end private diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb index 6c5d0788a0a..e7283b2f9e8 100644 --- a/lib/gitlab/auth/o_auth/user.rb +++ b/lib/gitlab/auth/o_auth/user.rb @@ -74,6 +74,10 @@ module Gitlab gl_user end + def bypass_two_factor? + false + end + protected def should_save? diff --git a/lib/gitlab/auth/saml/auth_hash.rb b/lib/gitlab/auth/saml/auth_hash.rb index c345a7e3f6c..3bc5e2864df 100644 --- a/lib/gitlab/auth/saml/auth_hash.rb +++ b/lib/gitlab/auth/saml/auth_hash.rb @@ -6,6 +6,17 @@ module Gitlab Array.wrap(get_raw(Gitlab::Auth::Saml::Config.groups)) end + def authn_context + response_object = auth_hash.extra[:response_object] + return nil if response_object.blank? + + document = response_object.decrypted_document + document ||= response_object.document + return nil if document.blank? + + extract_authn_context(document) + end + private def get_raw(key) @@ -13,6 +24,10 @@ module Gitlab # otherwise just the first value is returned auth_hash.extra[:raw_info].all[key] end + + def extract_authn_context(document) + REXML::XPath.first(document, "//saml:AuthnStatement/saml:AuthnContext/saml:AuthnContextClassRef/text()").to_s + end end end end diff --git a/lib/gitlab/auth/saml/config.rb b/lib/gitlab/auth/saml/config.rb index 5fa9581f837..625dab7c6f4 100644 --- a/lib/gitlab/auth/saml/config.rb +++ b/lib/gitlab/auth/saml/config.rb @@ -7,6 +7,10 @@ module Gitlab Gitlab::Auth::OAuth::Provider.config_for('saml') end + def upstream_two_factor_authn_contexts + options.args[:upstream_two_factor_authn_contexts] + end + def groups options[:groups_attribute] end diff --git a/lib/gitlab/auth/saml/user.rb b/lib/gitlab/auth/saml/user.rb index b8c84c37cd5..6c3b75f3eb0 100644 --- a/lib/gitlab/auth/saml/user.rb +++ b/lib/gitlab/auth/saml/user.rb @@ -34,6 +34,10 @@ module Gitlab gl_user.changed? || gl_user.identities.any?(&:changed?) end + def bypass_two_factor? + saml_config.upstream_two_factor_authn_contexts&.include?(auth_hash.authn_context) + end + protected def saml_config diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb index 156d077a69c..604bb11e712 100644 --- a/lib/gitlab/git/blob.rb +++ b/lib/gitlab/git/blob.rb @@ -21,13 +21,31 @@ module Gitlab attr_accessor :name, :path, :size, :data, :mode, :id, :commit_id, :loaded_size, :binary class << self - def find(repository, sha, path) - Gitlab::GitalyClient.migrate(:project_raw_show) do |is_enabled| - if is_enabled - find_by_gitaly(repository, sha, path) - else - find_by_rugged(repository, sha, path, limit: MAX_DATA_DISPLAY_SIZE) - end + def find(repository, sha, path, limit: MAX_DATA_DISPLAY_SIZE) + return unless path + + path = path.sub(%r{\A/*}, '') + path = '/' if path.empty? + name = File.basename(path) + + # Gitaly will think that setting the limit to 0 means unlimited, while + # the client might only need the metadata and thus set the limit to 0. + # In this method we'll then set the limit to 1, but clear the byte of data + # that we got back so for the outside world it looks like the limit was + # actually 0. + req_limit = limit == 0 ? 1 : limit + + entry = Gitlab::GitalyClient::CommitService.new(repository).tree_entry(sha, path, req_limit) + return unless entry + + entry.data = "" if limit == 0 + + case entry.type + when :COMMIT + new(id: entry.oid, name: name, size: 0, data: '', path: path, commit_id: sha) + when :BLOB + new(id: entry.oid, name: name, size: entry.size, data: entry.data.dup, mode: entry.mode.to_s(8), + path: path, commit_id: sha, binary: binary?(entry.data)) end end @@ -56,7 +74,7 @@ module Gitlab repository.gitaly_blob_client.get_blobs(blob_references, blob_size_limit).to_a else blob_references.map do |sha, path| - find_by_rugged(repository, sha, path, limit: blob_size_limit) + find(repository, sha, path, limit: blob_size_limit) end end end @@ -136,85 +154,6 @@ module Gitlab ) end - def find_by_gitaly(repository, sha, path, limit: MAX_DATA_DISPLAY_SIZE) - return unless path - - path = path.sub(%r{\A/*}, '') - path = '/' if path.empty? - name = File.basename(path) - - # Gitaly will think that setting the limit to 0 means unlimited, while - # the client might only need the metadata and thus set the limit to 0. - # In this method we'll then set the limit to 1, but clear the byte of data - # that we got back so for the outside world it looks like the limit was - # actually 0. - req_limit = limit == 0 ? 1 : limit - - entry = Gitlab::GitalyClient::CommitService.new(repository).tree_entry(sha, path, req_limit) - return unless entry - - entry.data = "" if limit == 0 - - case entry.type - when :COMMIT - new( - id: entry.oid, - name: name, - size: 0, - data: '', - path: path, - commit_id: sha - ) - when :BLOB - new( - id: entry.oid, - name: name, - size: entry.size, - data: entry.data.dup, - mode: entry.mode.to_s(8), - path: path, - commit_id: sha, - binary: binary?(entry.data) - ) - end - end - - def find_by_rugged(repository, sha, path, limit:) - return unless path - - # Strip any leading / characters from the path - path = path.sub(%r{\A/*}, '') - - rugged_commit = repository.lookup(sha) - root_tree = rugged_commit.tree - - blob_entry = find_entry_by_path(repository, root_tree.oid, *path.split('/')) - - return nil unless blob_entry - - if blob_entry[:type] == :commit - submodule_blob(blob_entry, path, sha) - else - blob = repository.lookup(blob_entry[:oid]) - - if blob - new( - id: blob.oid, - name: blob_entry[:name], - size: blob.size, - # Rugged::Blob#content is expensive; don't call it if we don't have to. - data: limit.zero? ? '' : blob.content(limit), - mode: blob_entry[:filemode].to_s(8), - path: path, - commit_id: sha, - binary: blob.binary? - ) - end - end - rescue Rugged::ReferenceError - nil - end - def rugged_raw(repository, sha, limit:) blob = repository.lookup(sha) diff --git a/lib/gitlab/git/remote_mirror.rb b/lib/gitlab/git/remote_mirror.rb index ebe46722890..e4743b4db0a 100644 --- a/lib/gitlab/git/remote_mirror.rb +++ b/lib/gitlab/git/remote_mirror.rb @@ -7,81 +7,8 @@ module Gitlab end def update(only_branches_matching: []) - @repository.gitaly_migrate(:remote_update_remote_mirror) do |is_enabled| - if is_enabled - gitaly_update(only_branches_matching) - else - rugged_update(only_branches_matching) - end - end - end - - private - - def gitaly_update(only_branches_matching) - @repository.gitaly_remote_client.update_remote_mirror(@ref_name, only_branches_matching) - end - - def rugged_update(only_branches_matching) - local_branches = refs_obj(@repository.local_branches, only_refs_matching: only_branches_matching) - remote_branches = refs_obj(@repository.remote_branches(@ref_name), only_refs_matching: only_branches_matching) - - updated_branches = changed_refs(local_branches, remote_branches) - push_branches(updated_branches.keys) if updated_branches.present? - - delete_refs(local_branches, remote_branches) - - local_tags = refs_obj(@repository.tags) - remote_tags = refs_obj(@repository.remote_tags(@ref_name)) - - updated_tags = changed_refs(local_tags, remote_tags) - @repository.push_remote_branches(@ref_name, updated_tags.keys) if updated_tags.present? - - delete_refs(local_tags, remote_tags) - end - - def refs_obj(refs, only_refs_matching: []) - refs.each_with_object({}) do |ref, refs| - next if only_refs_matching.present? && !only_refs_matching.include?(ref.name) - - refs[ref.name] = ref - end - end - - def changed_refs(local_refs, remote_refs) - local_refs.select do |ref_name, ref| - remote_ref = remote_refs[ref_name] - - remote_ref.nil? || ref.dereferenced_target != remote_ref.dereferenced_target - end - end - - def push_branches(branches) - default_branch, branches = branches.partition do |branch| - @repository.root_ref == branch - end - - # Push the default branch first so it works fine when remote mirror is empty. - branches.unshift(*default_branch) - - @repository.push_remote_branches(@ref_name, branches) - end - - def delete_refs(local_refs, remote_refs) - refs = refs_to_delete(local_refs, remote_refs) - - @repository.delete_remote_branches(@ref_name, refs.keys) if refs.present? - end - - def refs_to_delete(local_refs, remote_refs) - default_branch_id = @repository.commit.id - - remote_refs.select do |remote_ref_name, remote_ref| - next false if local_refs[remote_ref_name] # skip if branch or tag exist in local repo - - remote_ref_id = remote_ref.dereferenced_target.try(:id) - - remote_ref_id && @repository.rugged_is_ancestor?(remote_ref_id, default_branch_id) + @repository.wrapped_gitaly_errors do + @repository.gitaly_remote_client.update_remote_mirror(@ref_name, only_branches_matching) end end end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 0904e1c2973..b3016c1a637 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -549,24 +549,9 @@ module Gitlab end end - # Gitaly note: JV: check gitlab-ee before removing this method. - def rugged_is_ancestor?(ancestor_id, descendant_id) - return false if ancestor_id.nil? || descendant_id.nil? - - rugged_merge_base(ancestor_id, descendant_id) == ancestor_id - rescue Rugged::OdbError - false - end - # Returns true is +from+ is direct ancestor to +to+, otherwise false def ancestor?(from, to) - Gitlab::GitalyClient.migrate(:is_ancestor) do |is_enabled| - if is_enabled - gitaly_commit_client.ancestor?(from, to) - else - rugged_is_ancestor?(from, to) - end - end + gitaly_commit_client.ancestor?(from, to) end def merged_branch_names(branch_names = []) @@ -978,29 +963,8 @@ module Gitlab end def languages(ref = nil) - gitaly_migrate(:commit_languages, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| - if is_enabled - gitaly_commit_client.languages(ref) - else - ref ||= rugged.head.target_id - languages = Linguist::Repository.new(rugged, ref).languages - total = languages.map(&:last).sum - - languages = languages.map do |language| - name, share = language - color = Linguist::Language[name].color || "##{Digest::SHA256.hexdigest(name)[0...6]}" - { - value: (share.to_f * 100 / total).round(2), - label: name, - color: color, - highlight: color - } - end - - languages.sort do |x, y| - y[:value] <=> x[:value] - end - end + wrapped_gitaly_errors do + gitaly_commit_client.languages(ref) end end @@ -1158,16 +1122,7 @@ module Gitlab end def create_from_bundle(bundle_path) - gitaly_migrate(:create_repo_from_bundle) do |is_enabled| - if is_enabled - gitaly_repository_client.create_from_bundle(bundle_path) - else - run_git!(%W(clone --bare -- #{bundle_path} #{path}), chdir: nil) - self.class.create_hooks(path, File.expand_path(Gitlab.config.gitlab_shell.hooks_path)) - end - end - - true + gitaly_repository_client.create_from_bundle(bundle_path) end def create_from_snapshot(url, auth) @@ -1268,16 +1223,10 @@ module Gitlab return unless full_path.present? # This guard avoids Gitaly log/error spam - unless exists? - raise NoRepository, 'repository does not exist' - end + raise NoRepository, 'repository does not exist' unless exists? - gitaly_migrate(:write_config) do |is_enabled| - if is_enabled - gitaly_repository_client.write_config(full_path: full_path) - else - rugged_write_config(full_path: full_path) - end + wrapped_gitaly_errors do + gitaly_repository_client.write_config(full_path: full_path) end end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 5f0e8c5eca9..b23f183fec8 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -1,127 +1,162 @@ require 'spec_helper' -describe OmniauthCallbacksController do +describe OmniauthCallbacksController, type: :controller do include LoginHelpers - let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) } - - before do - mock_auth_hash(provider.to_s, extern_uid, user.email) - stub_omniauth_provider(provider, context: request) - end - - context 'when the user is on the last sign in attempt' do - let(:extern_uid) { 'my-uid' } + describe 'omniauth' do + let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) } before do - user.update(failed_attempts: User.maximum_attempts.pred) - subject.response = ActionDispatch::Response.new + mock_auth_hash(provider.to_s, extern_uid, user.email) + stub_omniauth_provider(provider, context: request) end - context 'when using a form based provider' do - let(:provider) { :ldap } - - it 'locks the user when sign in fails' do - allow(subject).to receive(:params).and_return(ActionController::Parameters.new(username: user.username)) - request.env['omniauth.error.strategy'] = OmniAuth::Strategies::LDAP.new(nil) - - subject.send(:failure) + context 'when the user is on the last sign in attempt' do + let(:extern_uid) { 'my-uid' } - expect(user.reload).to be_access_locked + before do + user.update(failed_attempts: User.maximum_attempts.pred) + subject.response = ActionDispatch::Response.new end - end - context 'when using a button based provider' do - let(:provider) { :github } + context 'when using a form based provider' do + let(:provider) { :ldap } - it 'does not lock the user when sign in fails' do - request.env['omniauth.error.strategy'] = OmniAuth::Strategies::GitHub.new(nil) + it 'locks the user when sign in fails' do + allow(subject).to receive(:params).and_return(ActionController::Parameters.new(username: user.username)) + request.env['omniauth.error.strategy'] = OmniAuth::Strategies::LDAP.new(nil) - subject.send(:failure) + subject.send(:failure) - expect(user.reload).not_to be_access_locked + expect(user.reload).to be_access_locked + end end - end - end - context 'strategies' do - context 'github' do - let(:extern_uid) { 'my-uid' } - let(:provider) { :github } + context 'when using a button based provider' do + let(:provider) { :github } - it 'allows sign in' do - post provider + it 'does not lock the user when sign in fails' do + request.env['omniauth.error.strategy'] = OmniAuth::Strategies::GitHub.new(nil) - expect(request.env['warden']).to be_authenticated - end - - shared_context 'sign_up' do - let(:user) { double(email: 'new@example.com') } + subject.send(:failure) - before do - stub_omniauth_setting(block_auto_created_users: false) + expect(user.reload).not_to be_access_locked end end + end - context 'sign up' do - include_context 'sign_up' + context 'strategies' do + context 'github' do + let(:extern_uid) { 'my-uid' } + let(:provider) { :github } - it 'is allowed' do + it 'allows sign in' do post provider expect(request.env['warden']).to be_authenticated end - end - - context 'when OAuth is disabled' do - before do - stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') - settings = Gitlab::CurrentSettings.current_application_settings - settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) - end - it 'prevents login via POST' do - post provider + shared_context 'sign_up' do + let(:user) { double(email: 'new@example.com') } - expect(request.env['warden']).not_to be_authenticated + before do + stub_omniauth_setting(block_auto_created_users: false) + end end - it 'shows warning when attempting login' do - post provider - - expect(response).to redirect_to new_user_session_path - expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') - end + context 'sign up' do + include_context 'sign_up' - it 'allows linking the disabled provider' do - user.identities.destroy_all - sign_in(user) + it 'is allowed' do + post provider - expect { post provider }.to change { user.reload.identities.count }.by(1) + expect(request.env['warden']).to be_authenticated + end end - context 'sign up' do - include_context 'sign_up' + context 'when OAuth is disabled' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + settings = Gitlab::CurrentSettings.current_application_settings + settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) + end - it 'is prevented' do + it 'prevents login via POST' do post provider expect(request.env['warden']).not_to be_authenticated end + + it 'shows warning when attempting login' do + post provider + + expect(response).to redirect_to new_user_session_path + expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + end + + it 'allows linking the disabled provider' do + user.identities.destroy_all + sign_in(user) + + expect { post provider }.to change { user.reload.identities.count }.by(1) + end + + context 'sign up' do + include_context 'sign_up' + + it 'is prevented' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + end + end + end + + context 'auth0' do + let(:extern_uid) { '' } + let(:provider) { :auth0 } + + it 'does not allow sign in without extern_uid' do + post 'auth0' + + expect(request.env['warden']).not_to be_authenticated + expect(response.status).to eq(302) + expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') end end end + end + + describe '#saml' do + let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') } + let(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') } + let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts } + + before do + stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], + providers: [saml_config] }) + mock_auth_hash('saml', 'my-uid', user.email, mock_saml_response) + request.env["devise.mapping"] = Devise.mappings[:user] + request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth'] + post :saml, params: { SAMLResponse: mock_saml_response } + end - context 'auth0' do - let(:extern_uid) { '' } - let(:provider) { :auth0 } + context 'when worth two factors' do + let(:mock_saml_response) do + File.read('spec/fixtures/authentication/saml_response.xml') + .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN') + end - it 'does not allow sign in without extern_uid' do - post 'auth0' + it 'expects user to be signed_in' do + expect(request.env['warden']).to be_authenticated + end + end + context 'when not worth two factors' do + it 'expects user to provide second factor' do + expect(response).to render_template('devise/sessions/two_factor') expect(request.env['warden']).not_to be_authenticated - expect(response.status).to eq(302) - expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') end end end diff --git a/spec/dependencies/omniauth_saml_spec.rb b/spec/dependencies/omniauth_saml_spec.rb new file mode 100644 index 00000000000..ccc604dc230 --- /dev/null +++ b/spec/dependencies/omniauth_saml_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' +require 'omniauth/strategies/saml' + +describe 'processing of SAMLResponse in dependencies' do + let(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') } + let(:saml_strategy) { OmniAuth::Strategies::SAML.new({}) } + let(:session_mock) { {} } + let(:settings) { OpenStruct.new({ soft: false, idp_cert_fingerprint: 'something' }) } + let(:auth_hash) { Gitlab::Auth::Saml::AuthHash.new(saml_strategy) } + + subject { auth_hash.authn_context } + + before do + allow(saml_strategy).to receive(:session).and_return(session_mock) + allow_any_instance_of(OneLogin::RubySaml::Response).to receive(:is_valid?).and_return(true) + saml_strategy.send(:handle_response, mock_saml_response, {}, settings ) { } + end + + it 'can extract AuthnContextClassRef from SAMLResponse param' do + is_expected.to eq 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password' + end +end diff --git a/spec/features/projects/commit/comments/user_adds_comment_spec.rb b/spec/features/projects/commit/comments/user_adds_comment_spec.rb index 6397df086a7..53866c32c69 100644 --- a/spec/features/projects/commit/comments/user_adds_comment_spec.rb +++ b/spec/features/projects/commit/comments/user_adds_comment_spec.rb @@ -62,7 +62,7 @@ describe "User adds a comment on a commit", :js do click_diff_line(sample_commit.line_code) expect(page).to have_css(".js-temp-notes-holder form.new-note") - .and have_css(".js-close-discussion-note-form", text: "Cancel") + .and have_css(".js-close-discussion-note-form", text: "Discard draft") # The `Cancel` button closes the current form. The page should not have any open forms after that. find(".js-close-discussion-note-form").click diff --git a/spec/features/projects/graph_spec.rb b/spec/features/projects/graph_spec.rb index 57172610aed..335174b7729 100644 --- a/spec/features/projects/graph_spec.rb +++ b/spec/features/projects/graph_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe 'Project Graph', :js do let(:user) { create :user } let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:branch_name) { 'master' } before do project.add_master(user) @@ -12,7 +13,7 @@ describe 'Project Graph', :js do shared_examples 'page should have commits graphs' do it 'renders commits' do - expect(page).to have_content('Commit statistics for master') + expect(page).to have_content("Commit statistics for #{branch_name}") expect(page).to have_content('Commits per day of month') end end @@ -57,6 +58,23 @@ describe 'Project Graph', :js do it_behaves_like 'page should have languages graphs' end + context 'chart graph with HTML escaped branch name' do + let(:branch_name) { '<h1>evil</h1>' } + + before do + project.repository.create_branch(branch_name, 'master') + + visit charts_project_graph_path(project, branch_name) + end + + it_behaves_like 'page should have commits graphs' + + it 'HTML escapes branch name' do + expect(page.body).to include("Commit statistics for <strong>#{ERB::Util.html_escape(branch_name)}</strong>") + expect(page.body).not_to include(branch_name) + end + end + context 'when CI enabled' do before do project.enable_ci diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index 1f8d31a5c88..24a2c89f50b 100644 --- a/spec/features/users/login_spec.rb +++ b/spec/features/users/login_spec.rb @@ -177,14 +177,35 @@ feature 'Login' do end context 'logging in via OAuth' do - it 'shows 2FA prompt after OAuth login' do - stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config]) - user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') - gitlab_sign_in_via('saml', user, 'my-uid') + let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml')} + let(:mock_saml_response) do + File.read('spec/fixtures/authentication/saml_response.xml') + end - expect(page).to have_content('Two-Factor Authentication') - enter_code(user.current_otp) - expect(current_path).to eq root_path + before do + stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], + providers: [mock_saml_config_with_upstream_two_factor_authn_contexts]) + gitlab_sign_in_via('saml', user, 'my-uid', mock_saml_response) + end + + context 'when authn_context is worth two factors' do + let(:mock_saml_response) do + File.read('spec/fixtures/authentication/saml_response.xml') + .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS') + end + + it 'signs user in without prompting for second factor' do + expect(page).not_to have_content('Two-Factor Authentication') + expect(current_path).to eq root_path + end + end + + context 'when authn_context is not worth two factors' do + it 'shows 2FA prompt after OAuth login' do + expect(page).to have_content('Two-Factor Authentication') + enter_code(user.current_otp) + expect(current_path).to eq root_path + end end end end diff --git a/spec/finders/user_recent_events_finder_spec.rb b/spec/finders/user_recent_events_finder_spec.rb index 3ca0f7c3c89..da043f94021 100644 --- a/spec/finders/user_recent_events_finder_spec.rb +++ b/spec/finders/user_recent_events_finder_spec.rb @@ -1,31 +1,50 @@ require 'spec_helper' describe UserRecentEventsFinder do - let(:user) { create(:user) } - let(:project) { create(:project) } - let(:project_owner) { project.creator } - let!(:event) { create(:event, project: project, author: project_owner) } + let(:current_user) { create(:user) } + let(:project_owner) { create(:user) } + let(:private_project) { create(:project, :private, creator: project_owner) } + let(:internal_project) { create(:project, :internal, creator: project_owner) } + let(:public_project) { create(:project, :public, creator: project_owner) } + let!(:private_event) { create(:event, project: private_project, author: project_owner) } + let!(:internal_event) { create(:event, project: internal_project, author: project_owner) } + let!(:public_event) { create(:event, project: public_project, author: project_owner) } - subject(:finder) { described_class.new(user, project_owner) } + subject(:finder) { described_class.new(current_user, project_owner) } describe '#execute' do - it 'does not include the event when a user does not have access to the project' do - expect(finder.execute).to be_empty + context 'current user does not have access to projects' do + it 'returns public and internal events' do + records = finder.execute + + expect(records).to include(public_event, internal_event) + expect(records).not_to include(private_event) + end end - context 'when the user has access to a project' do + context 'when current user has access to the projects' do before do - project.add_developer(user) + private_project.add_developer(current_user) + internal_project.add_developer(current_user) + public_project.add_developer(current_user) end - it 'includes the event' do - expect(finder.execute).to include(event) + it 'returns all the events' do + expect(finder.execute).to include(private_event, internal_event, public_event) end - it 'does not include the event if the user cannot read cross project' do - expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } + it 'does not include the events if the user cannot read cross project' do + expect(Ability).to receive(:allowed?).with(current_user, :read_cross_project) { false } expect(finder.execute).to be_empty end end + + context 'when current user is anonymous' do + let(:current_user) { nil } + + it 'returns public events only' do + expect(finder.execute).to eq([public_event]) + end + end end end diff --git a/spec/fixtures/authentication/saml_response.xml b/spec/fixtures/authentication/saml_response.xml new file mode 100644 index 00000000000..ac7b662be22 --- /dev/null +++ b/spec/fixtures/authentication/saml_response.xml @@ -0,0 +1,42 @@ +<?xml version='1.0'?> +<samlp:Response xmlns:samlp='urn:oasis:names:tc:SAML:2.0:protocol' xmlns:saml='urn:oasis:names:tc:SAML:2.0:assertion' ID='pfxb9b71715-2202-9a51-8ae5-689d5b9dd25a' Version='2.0' IssueInstant='2014-07-17T01:01:48Z' Destination='http://sp.example.com/demo1/index.php?acs' InResponseTo='ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685'> + <saml:Issuer>http://idp.example.com/metadata.php</saml:Issuer><ds:Signature xmlns:ds='http://www.w3.org/2000/09/xmldsig#'> + <ds:SignedInfo><ds:CanonicalizationMethod Algorithm='http://www.w3.org/2001/10/xml-exc-c14n#'/> + <ds:SignatureMethod Algorithm='http://www.w3.org/2000/09/xmldsig#rsa-sha1'/> + <ds:Reference URI='#pfxb9b71715-2202-9a51-8ae5-689d5b9dd25a'><ds:Transforms><ds:Transform Algorithm='http://www.w3.org/2000/09/xmldsig#enveloped-signature'/><ds:Transform Algorithm='http://www.w3.org/2001/10/xml-exc-c14n#'/></ds:Transforms><ds:DigestMethod Algorithm='http://www.w3.org/2000/09/xmldsig#sha1'/><ds:DigestValue>z0Y25hsUHVJJnYhgB5LzPVjqbgM=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>NSdsZopzNX4kJETipLNbU+7dG4GPTj5e40iSBaUeUMc1UUSX4UCe9Qx6R9ADEkEQgNekgYaCFOuY90kLNh9Ky0Czq8gd4w7ykQJEVJ7VF7LakmG8dPedHAKyAMAuZ8y3mNGye31vtR9frYaznCVoxB3eAi9rbVOXkQtdOTRMHec=</ds:SignatureValue> + <ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIICajCCAdOgAwIBAgIBADANBgkqhkiG9w0BAQ0FADBSMQswCQYDVQQGEwJ1czETMBEGA1UECAwKQ2FsaWZvcm5pYTEVMBMGA1UECgwMT25lbG9naW4gSW5jMRcwFQYDVQQDDA5zcC5leGFtcGxlLmNvbTAeFw0xNDA3MTcxNDEyNTZaFw0xNTA3MTcxNDEyNTZaMFIxCzAJBgNVBAYTAnVzMRMwEQYDVQQIDApDYWxpZm9ybmlhMRUwEwYDVQQKDAxPbmVsb2dpbiBJbmMxFzAVBgNVBAMMDnNwLmV4YW1wbGUuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDZx+ON4IUoIWxgukTb1tOiX3bMYzYQiwWPUNMp+Fq82xoNogso2bykZG0yiJm5o8zv/sd6pGouayMgkx/2FSOdc36T0jGbCHuRSbtia0PEzNIRtmViMrt3AeoWBidRXmZsxCNLwgIV6dn2WpuE5Az0bHgpZnQxTKFek0BMKU/d8wIDAQABo1AwTjAdBgNVHQ4EFgQUGHxYqZYyX7cTxKVODVgZwSTdCnwwHwYDVR0jBBgwFoAUGHxYqZYyX7cTxKVODVgZwSTdCnwwDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQ0FAAOBgQByFOl+hMFICbd3DJfnp2Rgd/dqttsZG/tyhILWvErbio/DEe98mXpowhTkC04ENprOyXi7ZbUqiicF89uAGyt1oqgTUCD1VsLahqIcmrzgumNyTwLGWo17WDAa1/usDhetWAMhgzF/Cnf5ek0nK00m0YZGyc4LzgD0CROMASTWNg==</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature> + <samlp:Status> + <samlp:StatusCode Value='urn:oasis:names:tc:SAML:2.0:status:Success'/> + </samlp:Status> + <saml:Assertion xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:xs='http://www.w3.org/2001/XMLSchema' ID='_d71a3a8e9fcc45c9e9d248ef7049393fc8f04e5f75' Version='2.0' IssueInstant='2014-07-17T01:01:48Z'> + <saml:Issuer>http://idp.example.com/metadata.php</saml:Issuer> + <saml:Subject> + <saml:NameID SPNameQualifier='http://sp.example.com/demo1/metadata.php' Format='urn:oasis:names:tc:SAML:2.0:nameid-format:transient'>_ce3d2948b4cf20146dee0a0b3dd6f69b6cf86f62d7</saml:NameID> + <saml:SubjectConfirmation Method='urn:oasis:names:tc:SAML:2.0:cm:bearer'> + <saml:SubjectConfirmationData NotOnOrAfter='2024-01-18T06:21:48Z' Recipient='http://sp.example.com/demo1/index.php?acs' InResponseTo='ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685'/> + </saml:SubjectConfirmation> + </saml:Subject> + <saml:Conditions NotBefore='2014-07-17T01:01:18Z' NotOnOrAfter='2024-01-18T06:21:48Z'> + <saml:AudienceRestriction> + <saml:Audience>http://sp.example.com/demo1/metadata.php</saml:Audience> + </saml:AudienceRestriction> + </saml:Conditions> + <saml:AuthnStatement AuthnInstant='2014-07-17T01:01:48Z' SessionNotOnOrAfter='2024-07-17T09:01:48Z' SessionIndex='_be9967abd904ddcae3c0eb4189adbe3f71e327cf93'> + <saml:AuthnContext> + <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef> + </saml:AuthnContext> + </saml:AuthnStatement> + <saml:AttributeStatement> + <saml:Attribute Name='uid' NameFormat='urn:oasis:names:tc:SAML:2.0:attrname-format:basic'> + <saml:AttributeValue xsi:type='xs:string'>test</saml:AttributeValue> + </saml:Attribute> + <saml:Attribute Name='mail' NameFormat='urn:oasis:names:tc:SAML:2.0:attrname-format:basic'> + <saml:AttributeValue xsi:type='xs:string'>test@example.com</saml:AttributeValue> + </saml:Attribute> + <saml:Attribute Name='eduPersonAffiliation' NameFormat='urn:oasis:names:tc:SAML:2.0:attrname-format:basic'> + <saml:AttributeValue xsi:type='xs:string'>users</saml:AttributeValue> + <saml:AttributeValue xsi:type='xs:string'>examplerole1</saml:AttributeValue> + </saml:Attribute> + </saml:AttributeStatement> + </saml:Assertion> +</samlp:Response> diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 5cf9e9e8f12..80147b13739 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -248,7 +248,7 @@ describe ProjectsHelper do describe '#link_to_member' do let(:group) { build_stubbed(:group) } let(:project) { build_stubbed(:project, group: group) } - let(:user) { build_stubbed(:user) } + let(:user) { build_stubbed(:user, name: '<h1>Administrator</h1>') } describe 'using the default options' do it 'returns an HTML link to the user' do @@ -256,6 +256,13 @@ describe ProjectsHelper do expect(link).to match(%r{/#{user.username}}) end + + it 'HTML escapes the name of the user' do + link = helper.link_to_member(project, user) + + expect(link).to include(ERB::Util.html_escape(user.name)) + expect(link).not_to include(user.name) + end end end diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 17a620ef603..d930c608b18 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -93,6 +93,16 @@ describe Banzai::Filter::SanitizationFilter do expect(doc.at_css('td')['style']).to eq 'text-align: center' end + it 'disallows `text-align` property in `style` attribute on other elements' do + html = <<~HTML + <div style="text-align: center">Text</div> + HTML + + doc = filter(html) + + expect(doc.at_css('div')['style']).to be_nil + end + it 'allows `span` elements' do exp = act = %q{<span>Hello</span>} expect(filter(act).to_html).to eq exp @@ -224,7 +234,7 @@ describe Banzai::Filter::SanitizationFilter do 'protocol-based JS injection: spaces and entities' => { input: '<a href="  javascript:alert(\'XSS\');">foo</a>', - output: '<a href="">foo</a>' + output: '<a href>foo</a>' }, 'protocol whitespace' => { diff --git a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb index 0cfef4ff5bf..7213cd58ea7 100644 --- a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb +++ b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb @@ -139,5 +139,14 @@ describe Banzai::Filter::TableOfContentsFilter do expect(items[5].ancestors).to include(items[4]) end end + + context 'header text contains escaped content' do + let(:content) { '<img src="x" onerror="alert(42)">' } + let(:results) { result(header(1, content)) } + + it 'outputs escaped content' do + expect(doc.inner_html).to include(content) + end + end end end diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index 64f3d09a25b..3a8667e434d 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -779,4 +779,12 @@ describe Gitlab::Auth::OAuth::User do end end end + + describe '#bypass_two_factor?' do + subject { oauth_user.bypass_two_factor? } + + it 'returns always false' do + is_expected.to be_falsey + end + end end diff --git a/spec/lib/gitlab/auth/saml/auth_hash_spec.rb b/spec/lib/gitlab/auth/saml/auth_hash_spec.rb index bb950e6bbf8..76f49e778fb 100644 --- a/spec/lib/gitlab/auth/saml/auth_hash_spec.rb +++ b/spec/lib/gitlab/auth/saml/auth_hash_spec.rb @@ -37,4 +37,55 @@ describe Gitlab::Auth::Saml::AuthHash do end end end + + describe '#authn_context' do + let(:auth_hash_data) do + { + provider: 'saml', + uid: 'some_uid', + info: + { + name: 'mockuser', + email: 'mock@email.ch', + image: 'mock_user_thumbnail_url' + }, + credentials: + { + token: 'mock_token', + secret: 'mock_secret' + }, + extra: + { + raw_info: + { + info: + { + name: 'mockuser', + email: 'mock@email.ch', + image: 'mock_user_thumbnail_url' + } + } + } + } + end + + subject(:saml_auth_hash) { described_class.new(OmniAuth::AuthHash.new(auth_hash_data)) } + + context 'with response_object' do + before do + auth_hash_data[:extra][:response_object] = { document: + saml_xml(File.read('spec/fixtures/authentication/saml_response.xml')) } + end + + it 'can extract authn_context' do + expect(saml_auth_hash.authn_context).to eq 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password' + end + end + + context 'without response_object' do + it 'returns an empty string' do + expect(saml_auth_hash.authn_context).to be_nil + end + end + end end diff --git a/spec/lib/gitlab/auth/saml/user_spec.rb b/spec/lib/gitlab/auth/saml/user_spec.rb index 62514ca0688..c523f5e177f 100644 --- a/spec/lib/gitlab/auth/saml/user_spec.rb +++ b/spec/lib/gitlab/auth/saml/user_spec.rb @@ -400,4 +400,45 @@ describe Gitlab::Auth::Saml::User do end end end + + describe '#bypass_two_factor?' do + let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts } + + subject { saml_user.bypass_two_factor? } + + context 'with authn_contexts_worth_two_factors configured' do + before do + stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) + end + + it 'returns true when authn_context is worth two factors' do + allow(saml_user.auth_hash).to receive(:authn_context).and_return('urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS') + is_expected.to be_truthy + end + + it 'returns false when authn_context is not worth two factors' do + allow(saml_user.auth_hash).to receive(:authn_context).and_return('urn:oasis:names:tc:SAML:2.0:ac:classes:Password') + is_expected.to be_falsey + end + + it 'returns false when authn_context is blank' do + is_expected.to be_falsey + end + end + + context 'without auth_contexts_worth_two_factors_configured' do + before do + stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config]) + end + + it 'returns false when authn_context is present' do + allow(saml_user.auth_hash).to receive(:authn_context).and_return('urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS') + is_expected.to be_falsey + end + + it 'returns false when authn_context is blank' do + is_expected.to be_falsey + end + end + end end diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb index 6015086f002..b6061df349d 100644 --- a/spec/lib/gitlab/git/blob_spec.rb +++ b/spec/lib/gitlab/git/blob_spec.rb @@ -15,7 +15,7 @@ describe Gitlab::Git::Blob, seed_helper: true do end end - shared_examples 'finding blobs' do + describe '.find' do context 'nil path' do let(:blob) { Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, nil) } @@ -125,16 +125,6 @@ describe Gitlab::Git::Blob, seed_helper: true do end end - describe '.find' do - context 'when project_raw_show Gitaly feature is enabled' do - it_behaves_like 'finding blobs' - end - - context 'when project_raw_show Gitaly feature is disabled', :skip_gitaly_mock do - it_behaves_like 'finding blobs' - end - end - shared_examples 'finding blobs by ID' do let(:raw_blob) { Gitlab::Git::Blob.raw(repository, SeedRepo::RubyBlob::ID) } let(:bad_blob) { Gitlab::Git::Blob.raw(repository, SeedRepo::BigCommit::ID) } diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 45f0006dc85..b78fe4ba310 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1871,49 +1871,39 @@ describe Gitlab::Git::Repository, seed_helper: true do repository_rugged.config["gitlab.fullpath"] = repository_path end - shared_examples 'writing repo config' do - context 'is given a path' do - it 'writes it to disk' do - repository.write_config(full_path: "not-the/real-path.git") + context 'is given a path' do + it 'writes it to disk' do + repository.write_config(full_path: "not-the/real-path.git") - config = File.read(File.join(repository_path, "config")) + config = File.read(File.join(repository_path, "config")) - expect(config).to include("[gitlab]") - expect(config).to include("fullpath = not-the/real-path.git") - end + expect(config).to include("[gitlab]") + expect(config).to include("fullpath = not-the/real-path.git") end + end - context 'it is given an empty path' do - it 'does not write it to disk' do - repository.write_config(full_path: "") + context 'it is given an empty path' do + it 'does not write it to disk' do + repository.write_config(full_path: "") - config = File.read(File.join(repository_path, "config")) + config = File.read(File.join(repository_path, "config")) - expect(config).to include("[gitlab]") - expect(config).to include("fullpath = #{repository_path}") - end + expect(config).to include("[gitlab]") + expect(config).to include("fullpath = #{repository_path}") end + end - context 'repository does not exist' do - it 'raises NoRepository and does not call Gitaly WriteConfig' do - repository = Gitlab::Git::Repository.new('default', 'does/not/exist.git', '') + context 'repository does not exist' do + it 'raises NoRepository and does not call Gitaly WriteConfig' do + repository = Gitlab::Git::Repository.new('default', 'does/not/exist.git', '') - expect(repository.gitaly_repository_client).not_to receive(:write_config) + expect(repository.gitaly_repository_client).not_to receive(:write_config) - expect do - repository.write_config(full_path: 'foo/bar.git') - end.to raise_error(Gitlab::Git::Repository::NoRepository) - end + expect do + repository.write_config(full_path: 'foo/bar.git') + end.to raise_error(Gitlab::Git::Repository::NoRepository) end end - - context "when gitaly_write_config is enabled" do - it_behaves_like "writing repo config" - end - - context "when gitaly_write_config is disabled", :disable_gitaly do - it_behaves_like "writing repo config" - end end describe '#merge' do @@ -2160,43 +2150,33 @@ describe Gitlab::Git::Repository, seed_helper: true do end describe '#create_from_bundle' do - shared_examples 'creating repo from bundle' do - let(:bundle_path) { File.join(Dir.tmpdir, "repo-#{SecureRandom.hex}.bundle") } - let(:project) { create(:project) } - let(:imported_repo) { project.repository.raw } - - before do - expect(repository.bundle_to_disk(bundle_path)).to be true - end - - after do - FileUtils.rm_rf(bundle_path) - end + let(:bundle_path) { File.join(Dir.tmpdir, "repo-#{SecureRandom.hex}.bundle") } + let(:project) { create(:project) } + let(:imported_repo) { project.repository.raw } - it 'creates a repo from a bundle file' do - expect(imported_repo).not_to exist + before do + expect(repository.bundle_to_disk(bundle_path)).to be_truthy + end - result = imported_repo.create_from_bundle(bundle_path) + after do + FileUtils.rm_rf(bundle_path) + end - expect(result).to be true - expect(imported_repo).to exist - expect { imported_repo.fsck }.not_to raise_exception - end + it 'creates a repo from a bundle file' do + expect(imported_repo).not_to exist - it 'creates a symlink to the global hooks dir' do - imported_repo.create_from_bundle(bundle_path) - hooks_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { File.join(imported_repo.path, 'hooks') } + result = imported_repo.create_from_bundle(bundle_path) - expect(File.readlink(hooks_path)).to eq(Gitlab.config.gitlab_shell.hooks_path) - end + expect(result).to be_truthy + expect(imported_repo).to exist + expect { imported_repo.fsck }.not_to raise_exception end - context 'when Gitaly create_repo_from_bundle feature is enabled' do - it_behaves_like 'creating repo from bundle' - end + it 'creates a symlink to the global hooks dir' do + imported_repo.create_from_bundle(bundle_path) + hooks_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { File.join(imported_repo.path, 'hooks') } - context 'when Gitaly create_repo_from_bundle feature is disabled', :disable_gitaly do - it_behaves_like 'creating repo from bundle' + expect(File.readlink(hooks_path)).to eq(Gitlab.config.gitlab_shell.hooks_path) end end diff --git a/spec/lib/gitlab/import_export/repo_restorer_spec.rb b/spec/lib/gitlab/import_export/repo_restorer_spec.rb index 013b8895f67..7ffa84f906d 100644 --- a/spec/lib/gitlab/import_export/repo_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/repo_restorer_spec.rb @@ -30,7 +30,7 @@ describe Gitlab::ImportExport::RepoRestorer do end it 'restores the repo successfully' do - expect(restorer.restore).to be true + expect(restorer.restore).to be_truthy end it 'has the webhooks' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2c75816654e..ec72fefd137 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2134,8 +2134,7 @@ describe MergeRequest do describe 'transition to cannot_be_merged' do let(:notification_service) { double(:notification_service) } let(:todo_service) { double(:todo_service) } - - subject { create(:merge_request, merge_status: :unchecked) } + subject { create(:merge_request, state, merge_status: :unchecked) } before do allow(NotificationService).to receive(:new).and_return(notification_service) @@ -2144,33 +2143,52 @@ describe MergeRequest do allow(subject.project.repository).to receive(:can_be_merged?).and_return(false) end - it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do - expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once - expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once + [:opened, :locked].each do |state| + context state do + let(:state) { state } - subject.mark_as_unmergeable - subject.mark_as_unchecked - subject.mark_as_unmergeable - end + it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do + expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once + expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_unmergeable + end + + it 'notifies conflict, whenever newly unmergeable' do + expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice + expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_mergeable + subject.mark_as_unchecked + subject.mark_as_unmergeable + end + + it 'does not notify whenever merge request is newly unmergeable due to other reasons' do + allow(subject.project.repository).to receive(:can_be_merged?).and_return(true) - it 'notifies conflict, whenever newly unmergeable' do - expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice - expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice + expect(notification_service).not_to receive(:merge_request_unmergeable) + expect(todo_service).not_to receive(:merge_request_became_unmergeable) - subject.mark_as_unmergeable - subject.mark_as_unchecked - subject.mark_as_mergeable - subject.mark_as_unchecked - subject.mark_as_unmergeable + subject.mark_as_unmergeable + end + end end - it 'does not notify whenever merge request is newly unmergeable due to other reasons' do - allow(subject.project.repository).to receive(:can_be_merged?).and_return(true) + [:closed, :merged].each do |state| + let(:state) { state } - expect(notification_service).not_to receive(:merge_request_unmergeable) - expect(todo_service).not_to receive(:merge_request_became_unmergeable) + context state do + it 'does not notify' do + expect(notification_service).not_to receive(:merge_request_unmergeable) + expect(todo_service).not_to receive(:merge_request_became_unmergeable) - subject.mark_as_unmergeable + subject.mark_as_unmergeable + end + end end end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 723cb374c37..5c2e79ff9af 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Projects::UpdateRemoteMirrorService do - let(:project) { create(:project, :repository) } + set(:project) { create(:project, :repository) } + let(:owner) { project.owner } let(:remote_project) { create(:forked_project_with_submodules) } let(:repository) { project.repository } let(:raw_repository) { repository.raw } @@ -9,13 +10,11 @@ describe Projects::UpdateRemoteMirrorService do subject { described_class.new(project, project.creator) } - describe "#execute", :skip_gitaly_mock do + describe "#execute" do before do - create_branch(repository, 'existing-branch') - allow(raw_repository).to receive(:remote_tags) do - generate_tags(repository, 'v1.0.0', 'v1.1.0') - end - allow(raw_repository).to receive(:push_remote_branches).and_return(true) + repository.add_branch(owner, 'existing-branch', 'master') + + allow(remote_mirror).to receive(:update_repository).and_return(true) end it "fetches the remote repository" do @@ -34,307 +33,57 @@ describe Projects::UpdateRemoteMirrorService do expect(result[:status]).to eq(:success) end - describe 'Syncing branches' do + context 'when syncing all branches' do it "push all the branches the first time" do allow(repository).to receive(:fetch_remote) - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, local_branch_names) - - subject.execute(remote_mirror) - end - - it "does not push anything is remote is up to date" do - allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) } - - expect(raw_repository).not_to receive(:push_remote_branches) - - subject.execute(remote_mirror) - end - - it "sync new branches" do - # call local_branch_names early so it is not called after the new branch has been created - current_branches = local_branch_names - allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, current_branches) } - create_branch(repository, 'my-new-branch') - - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['my-new-branch']) - - subject.execute(remote_mirror) - end - - it "sync updated branches" do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - update_branch(repository, 'existing-branch') - end - - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['existing-branch']) + expect(remote_mirror).to receive(:update_repository).with({}) subject.execute(remote_mirror) end - - context 'when push only protected branches option is set' do - let(:unprotected_branch_name) { 'existing-branch' } - let(:protected_branch_name) do - project.repository.branch_names.find { |n| n != unprotected_branch_name } - end - let!(:protected_branch) do - create(:protected_branch, project: project, name: protected_branch_name) - end - - before do - project.reload - remote_mirror.only_protected_branches = true - end - - it "sync updated protected branches" do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - update_branch(repository, protected_branch_name) - end - - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, [protected_branch_name]) - - subject.execute(remote_mirror) - end - - it 'does not sync unprotected branches' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - update_branch(repository, unprotected_branch_name) - end - - expect(raw_repository).not_to receive(:push_remote_branches).with(remote_mirror.remote_name, [unprotected_branch_name]) - - subject.execute(remote_mirror) - end - end - - context 'when branch exists in local and remote repo' do - context 'when it has diverged' do - it 'syncs branches' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - update_remote_branch(repository, remote_mirror.remote_name, 'markdown') - end - - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['markdown']) - - subject.execute(remote_mirror) - end - end - end - - describe 'for delete' do - context 'when branch exists in local and remote repo' do - it 'deletes the branch from remote repo' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - delete_branch(repository, 'existing-branch') - end - - expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch']) - - subject.execute(remote_mirror) - end - end - - context 'when push only protected branches option is set' do - before do - remote_mirror.only_protected_branches = true - end - - context 'when branch exists in local and remote repo' do - let!(:protected_branch_name) { local_branch_names.first } - - before do - create(:protected_branch, project: project, name: protected_branch_name) - project.reload - end - - it 'deletes the protected branch from remote repo' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - delete_branch(repository, protected_branch_name) - end - - expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name]) - - subject.execute(remote_mirror) - end - - it 'does not delete the unprotected branch from remote repo' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - delete_branch(repository, 'existing-branch') - end - - expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch']) - - subject.execute(remote_mirror) - end - end - - context 'when branch only exists on remote repo' do - let!(:protected_branch_name) { 'remote-branch' } - - before do - create(:protected_branch, project: project, name: protected_branch_name) - end - - context 'when it has diverged' do - it 'does not delete the remote branch' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - - rev = repository.find_branch('markdown').dereferenced_target - create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id) - end - - expect(raw_repository).not_to receive(:delete_remote_branches) - - subject.execute(remote_mirror) - end - end - - context 'when it has not diverged' do - it 'deletes the remote branch' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - - masterrev = repository.find_branch('master').dereferenced_target - create_remote_branch(repository, remote_mirror.remote_name, protected_branch_name, masterrev.id) - end - - expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name]) - - subject.execute(remote_mirror) - end - end - end - end - - context 'when branch only exists on remote repo' do - context 'when it has diverged' do - it 'does not delete the remote branch' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - - rev = repository.find_branch('markdown').dereferenced_target - create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id) - end - - expect(raw_repository).not_to receive(:delete_remote_branches) - - subject.execute(remote_mirror) - end - end - - context 'when it has not diverged' do - it 'deletes the remote branch' do - allow(repository).to receive(:fetch_remote) do - sync_remote(repository, remote_mirror.remote_name, local_branch_names) - - masterrev = repository.find_branch('master').dereferenced_target - create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', masterrev.id) - end - - expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['remote-branch']) - - subject.execute(remote_mirror) - end - end - end - end end - describe 'Syncing tags' do - before do - allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) } + context 'when only syncing protected branches' do + let(:unprotected_branch_name) { 'existing-branch' } + let(:protected_branch_name) do + project.repository.branch_names.find { |n| n != unprotected_branch_name } end - - context 'when there are not tags to push' do - it 'does not try to push tags' do - allow(repository).to receive(:remote_tags) { {} } - allow(repository).to receive(:tags) { [] } - - expect(repository).not_to receive(:push_tags) - - subject.execute(remote_mirror) - end + let!(:protected_branch) do + create(:protected_branch, project: project, name: protected_branch_name) end - context 'when there are some tags to push' do - it 'pushes tags to remote' do - allow(raw_repository).to receive(:remote_tags) { {} } - - expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['v1.0.0', 'v1.1.0']) - - subject.execute(remote_mirror) - end + before do + project.reload + remote_mirror.only_protected_branches = true end - context 'when there are some tags to delete' do - it 'deletes tags from remote' do - remote_tags = generate_tags(repository, 'v1.0.0', 'v1.1.0') - allow(raw_repository).to receive(:remote_tags) { remote_tags } - - repository.rm_tag(create(:user), 'v1.0.0') - - expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['v1.0.0']) + it "sync updated protected branches" do + allow(repository).to receive(:fetch_remote) + expect(remote_mirror).to receive(:update_repository).with(only_branches_matching: [protected_branch_name]) - subject.execute(remote_mirror) - end + subject.execute(remote_mirror) end end end - def create_branch(repository, branch_name) - rugged = repository.rugged - masterrev = repository.find_branch('master').dereferenced_target - parentrev = repository.commit(masterrev).parent_id - - rugged.references.create("refs/heads/#{branch_name}", parentrev) - - repository.expire_branches_cache - end - - def create_remote_branch(repository, remote_name, branch_name, source_id) - rugged = repository.rugged - - rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", source_id) - end - def sync_remote(repository, remote_name, local_branch_names) - rugged = repository.rugged - local_branch_names.each do |branch| - target = repository.find_branch(branch).try(:dereferenced_target) - rugged.references.create("refs/remotes/#{remote_name}/#{branch}", target.id) if target + commit = repository.commit(branch) + repository.write_ref("refs/remotes/#{remote_name}/#{branch}", commit.id) if commit end end def update_remote_branch(repository, remote_name, branch) - rugged = repository.rugged - masterrev = repository.find_branch('master').dereferenced_target.id + masterrev = repository.commit('master').id - rugged.references.create("refs/remotes/#{remote_name}/#{branch}", masterrev, force: true) + repository.write_ref("refs/remotes/#{remote_name}/#{branch}", masterrev, force: true) repository.expire_branches_cache end def update_branch(repository, branch) - rugged = repository.rugged - masterrev = repository.find_branch('master').dereferenced_target.id - - # Updated existing branch - rugged.references.create("refs/heads/#{branch}", masterrev, force: true) - repository.expire_branches_cache - end - - def delete_branch(repository, branch) - rugged = repository.rugged + masterrev = repository.commit('master').id - rugged.references.delete("refs/heads/#{branch}") + repository.write_ref("refs/heads/#{branch}", masterrev, force: true) repository.expire_branches_cache end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 7995f2c9ae7..622e56e1da5 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -60,6 +60,36 @@ describe WebHookService do ).once end + context 'when auth credentials are present' do + let(:url) {'https://example.org'} + let(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } + + it 'uses the credentials' do + WebMock.stub_request(:post, url) + + service_instance.execute + + expect(WebMock).to have_requested(:post, url).with( + headers: headers.merge('Authorization' => 'Basic ZGVtbzpkZW1v') + ).once + end + end + + context 'when auth credentials are partial present' do + let(:url) {'https://example.org'} + let(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } + + it 'uses the credentials anyways' do + WebMock.stub_request(:post, url) + + service_instance.execute + + expect(WebMock).to have_requested(:post, url).with( + headers: headers.merge('Authorization' => 'Basic ZGVtbzo=') + ).once + end + end + it 'catches exceptions' do WebMock.stub_request(:post, project_hook.url).to_raise(StandardError.new('Some error')) diff --git a/spec/support/helpers/login_helpers.rb b/spec/support/helpers/login_helpers.rb index 329f18cd288..87cfb6c04dc 100644 --- a/spec/support/helpers/login_helpers.rb +++ b/spec/support/helpers/login_helpers.rb @@ -46,8 +46,8 @@ module LoginHelpers @current_user = user end - def gitlab_sign_in_via(provider, user, uid) - mock_auth_hash(provider, uid, user.email) + def gitlab_sign_in_via(provider, user, uid, saml_response = nil) + mock_auth_hash(provider, uid, user.email, saml_response) visit new_user_session_path click_link provider end @@ -87,7 +87,7 @@ module LoginHelpers click_link "oauth-login-#{provider}" end - def mock_auth_hash(provider, uid, email) + def mock_auth_hash(provider, uid, email, saml_response = nil) # The mock_auth configuration allows you to set per-provider (or default) # authentication hashes to return during integration testing. OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({ @@ -109,12 +109,21 @@ module LoginHelpers email: email, image: 'mock_user_thumbnail_url' } + }, + response_object: { + document: saml_xml(saml_response) } } }) Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym] end + def saml_xml(raw_saml_response) + return '' if raw_saml_response.blank? + + XMLSecurity::SignedDocument.new(raw_saml_response, []) + end + def mock_saml_config OpenStruct.new(name: 'saml', label: 'saml', args: { assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', @@ -125,6 +134,14 @@ module LoginHelpers }) end + def mock_saml_config_with_upstream_two_factor_authn_contexts + config = mock_saml_config + config.args[:upstream_two_factor_authn_contexts] = %w(urn:oasis:names:tc:SAML:2.0:ac:classes:CertificateProtectedTransport + urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorOTPSMS + urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN) + config + end + def stub_omniauth_provider(provider, context: Rails.application) env = env_from_context(context) @@ -140,13 +157,16 @@ module LoginHelpers env['omniauth.error.strategy'] = strategy end - def stub_omniauth_saml_config(messages) - set_devise_mapping(context: Rails.application) - Rails.application.routes.disable_clear_and_finalize = true - Rails.application.routes.draw do + def stub_omniauth_saml_config(messages, context: Rails.application) + set_devise_mapping(context: context) + routes = Rails.application.routes + routes.disable_clear_and_finalize = true + routes.formatter.clear + routes.draw do post '/users/auth/saml' => 'omniauth_callbacks#saml' end - allow(Gitlab::Auth::OAuth::Provider).to receive_messages(providers: [:saml], config_for: mock_saml_config) + saml_config = messages.key?(:providers) ? messages[:providers].first : mock_saml_config + allow(Gitlab::Auth::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) stub_omniauth_setting(messages) stub_saml_authorize_path_helpers end diff --git a/spec/workers/delete_user_worker_spec.rb b/spec/workers/delete_user_worker_spec.rb index 36594515005..06d9e125105 100644 --- a/spec/workers/delete_user_worker_spec.rb +++ b/spec/workers/delete_user_worker_spec.rb @@ -5,15 +5,17 @@ describe DeleteUserWorker do let!(:current_user) { create(:user) } it "calls the DeleteUserWorker with the params it was given" do - expect_any_instance_of(Users::DestroyService).to receive(:execute) - .with(user, {}) + expect_next_instance_of(Users::DestroyService) do |service| + expect(service).to receive(:execute).with(user, {}) + end described_class.new.perform(current_user.id, user.id) end it "uses symbolized keys" do - expect_any_instance_of(Users::DestroyService).to receive(:execute) - .with(user, test: "test") + expect_next_instance_of(Users::DestroyService) do |service| + expect(service).to receive(:execute).with(user, test: "test") + end described_class.new.perform(current_user.id, user.id, "test" => "test") end |