diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-12-08 12:26:40 +0000 |
---|---|---|
committer | LUKE BENNETT <lbennett@gitlab.com> | 2017-12-13 13:30:07 +0000 |
commit | 2121e67bd04c169d7e930bb23c8f4df2b48022b5 (patch) | |
tree | f002d6364f69d3e6ba47fd36fce1b0fce08dbc09 | |
parent | 83e01aba58826ab0775d3ecf73717470da81bd2a (diff) | |
download | gitlab-ce-2121e67bd04c169d7e930bb23c8f4df2b48022b5.tar.gz |
Merge branch '29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected' into 'master'cherry-pick-d32032de
Resolve "No feedback when checking on checklist if potential spam was detected"
Closes #29483
See merge request gitlab-org/gitlab-ce!15408
(cherry picked from commit d32032de08585476c3b303803a307ff94d910ebd)
efc86c26 Mock spam?
8e9a9030 Merge remote-tracking branch 'origin/master' into…
019db0fd TEMP mock askismet spam check
11e51be6 respond_to json with json version of verify view in recaptcha_check_with_fallback
18955cdc Differentiate between spam related error and other update errors
9dcf093c Start recaptcha modal and render partial instead
64b13133 Append and remove api script when reCAPTCHA is needed
c9a67fab put-over-post the recaptcha form
cb22ae66 remove comments
20517d64 remove comments
e174d02e Review changes
7a7d4a2a Add spec
8f617b45 Added changelog
db3f1a47 Review changes
4d0c78de Add to tasklist
36ea921b Remove duplicate TaskList
6bffe620 Add app_spec for spam
d95350bc Finished specs
cf951cba FOR REVIEWERS
6c50e850 FOR MERGE
25931f73 Allow script src to be mockable
c3463e00 Merge remote-tracking branch 'origin/master' into…
2c170b8d fix eslint
a318c757 Fix redirect
8a02fbd0 BE review changes
dda03f4d Fix spec
910c7205 Fix spec
fafbd7e5 remove duplicate destroy
16 files changed, 363 insertions, 68 deletions
diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index 7de07e9403d..91b5ef1c10a 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -8,18 +8,7 @@ import IssuablesHelper from './helpers/issuables_helper'; export default class Issue { constructor() { - if ($('a.btn-close').length) { - this.taskList = new TaskList({ - dataType: 'issue', - fieldName: 'description', - selector: '.detail-page-description', - onSuccess: (result) => { - document.querySelector('#task_status').innerText = result.task_status; - document.querySelector('#task_status_short').innerText = result.task_status_short; - } - }); - this.initIssueBtnEventListeners(); - } + if ($('a.btn-close').length) this.initIssueBtnEventListeners(); Issue.$btnNewBranch = $('#new-branch'); Issue.createMrDropdownWrap = document.querySelector('.create-mr-dropdown-wrap'); diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index 5bdc7c99503..c7ce16bb623 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/app/assets/javascripts/issue_show/components/app.vue @@ -9,6 +9,7 @@ import descriptionComponent from './description.vue'; import editedComponent from './edited.vue'; import formComponent from './form.vue'; import '../../lib/utils/url_utility'; +import RecaptchaDialogImplementor from '../../vue_shared/mixins/recaptcha_dialog_implementor'; export default { props: { @@ -149,6 +150,11 @@ export default { editedComponent, formComponent, }, + + mixins: [ + RecaptchaDialogImplementor, + ], + methods: { openForm() { if (!this.showForm) { @@ -164,9 +170,11 @@ export default { closeForm() { this.showForm = false; }, + updateIssuable() { this.service.updateIssuable(this.store.formState) .then(res => res.json()) + .then(data => this.checkForSpam(data)) .then((data) => { if (location.pathname !== data.web_url) { gl.utils.visitUrl(data.web_url); @@ -179,11 +187,24 @@ export default { this.store.updateState(data); eventHub.$emit('close.form'); }) - .catch(() => { - eventHub.$emit('close.form'); - window.Flash(`Error updating ${this.issuableType}`); + .catch((error) => { + if (error && error.name === 'SpamError') { + this.openRecaptcha(); + } else { + eventHub.$emit('close.form'); + window.Flash(`Error updating ${this.issuableType}`); + } }); }, + + closeRecaptchaDialog() { + this.store.setFormState({ + updateLoading: false, + }); + + this.closeRecaptcha(); + }, + deleteIssuable() { this.service.deleteIssuable() .then(res => res.json()) @@ -237,9 +258,9 @@ export default { </script> <template> - <div> +<div> + <div v-if="canUpdate && showForm"> <form-component - v-if="canUpdate && showForm" :form-state="formState" :can-destroy="canDestroy" :issuable-templates="issuableTemplates" @@ -251,30 +272,37 @@ export default { :can-attach-file="canAttachFile" :enable-autocomplete="enableAutocomplete" /> - <div v-else> - <title-component - :issuable-ref="issuableRef" - :can-update="canUpdate" - :title-html="state.titleHtml" - :title-text="state.titleText" - :show-inline-edit-button="showInlineEditButton" - /> - <description-component - v-if="state.descriptionHtml" - :can-update="canUpdate" - :description-html="state.descriptionHtml" - :description-text="state.descriptionText" - :updated-at="state.updatedAt" - :task-status="state.taskStatus" - :issuable-type="issuableType" - :update-url="updateEndpoint" - /> - <edited-component - v-if="hasUpdated" - :updated-at="state.updatedAt" - :updated-by-name="state.updatedByName" - :updated-by-path="state.updatedByPath" - /> - </div> + + <recaptcha-dialog + v-show="showRecaptcha" + :html="recaptchaHTML" + @close="closeRecaptchaDialog" + /> + </div> + <div v-else> + <title-component + :issuable-ref="issuableRef" + :can-update="canUpdate" + :title-html="state.titleHtml" + :title-text="state.titleText" + :show-inline-edit-button="showInlineEditButton" + /> + <description-component + v-if="state.descriptionHtml" + :can-update="canUpdate" + :description-html="state.descriptionHtml" + :description-text="state.descriptionText" + :updated-at="state.updatedAt" + :task-status="state.taskStatus" + :issuable-type="issuableType" + :update-url="updateEndpoint" + /> + <edited-component + v-if="hasUpdated" + :updated-at="state.updatedAt" + :updated-by-name="state.updatedByName" + :updated-by-path="state.updatedByPath" + /> </div> +</div> </template> diff --git a/app/assets/javascripts/issue_show/components/description.vue b/app/assets/javascripts/issue_show/components/description.vue index b7559ced946..feb73481422 100644 --- a/app/assets/javascripts/issue_show/components/description.vue +++ b/app/assets/javascripts/issue_show/components/description.vue @@ -1,9 +1,14 @@ <script> import animateMixin from '../mixins/animate'; import TaskList from '../../task_list'; + import RecaptchaDialogImplementor from '../../vue_shared/mixins/recaptcha_dialog_implementor'; export default { - mixins: [animateMixin], + mixins: [ + animateMixin, + RecaptchaDialogImplementor, + ], + props: { canUpdate: { type: Boolean, @@ -51,6 +56,7 @@ this.updateTaskStatusText(); }, }, + methods: { renderGFM() { $(this.$refs['gfm-content']).renderGFM(); @@ -61,9 +67,19 @@ dataType: this.issuableType, fieldName: 'description', selector: '.detail-page-description', + onSuccess: this.taskListUpdateSuccess.bind(this), }); } }, + + taskListUpdateSuccess(data) { + try { + this.checkForSpam(data); + } catch (error) { + if (error && error.name === 'SpamError') this.openRecaptcha(); + } + }, + updateTaskStatusText() { const taskRegexMatches = this.taskStatus.match(/(\d+) of ((?!0)\d+)/); const $issuableHeader = $('.issuable-meta'); @@ -109,5 +125,11 @@ :data-update-url="updateUrl" > </textarea> + + <recaptcha-dialog + v-show="showRecaptcha" + :html="recaptchaHTML" + @close="closeRecaptcha" + /> </div> </template> diff --git a/app/assets/javascripts/vue_shared/components/popup_dialog.vue b/app/assets/javascripts/vue_shared/components/popup_dialog.vue index 47efee64c6e..6d15bbd84ba 100644 --- a/app/assets/javascripts/vue_shared/components/popup_dialog.vue +++ b/app/assets/javascripts/vue_shared/components/popup_dialog.vue @@ -38,7 +38,8 @@ export default { }, primaryButtonLabel: { type: String, - required: true, + required: false, + default: '', }, submitDisabled: { type: Boolean, @@ -113,8 +114,9 @@ export default { {{ closeButtonLabel }} </button> <button + v-if="primaryButtonLabel" type="button" - class="btn pull-right" + class="btn pull-right js-primary-button" :disabled="submitDisabled" :class="btnKindClass" @click="emitSubmit(true)"> diff --git a/app/assets/javascripts/vue_shared/components/recaptcha_dialog.vue b/app/assets/javascripts/vue_shared/components/recaptcha_dialog.vue new file mode 100644 index 00000000000..3ec50f14eb4 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/recaptcha_dialog.vue @@ -0,0 +1,85 @@ +<script> +import PopupDialog from './popup_dialog.vue'; + +export default { + name: 'recaptcha-dialog', + + props: { + html: { + type: String, + required: false, + default: '', + }, + }, + + data() { + return { + script: {}, + scriptSrc: 'https://www.google.com/recaptcha/api.js', + }; + }, + + components: { + PopupDialog, + }, + + methods: { + appendRecaptchaScript() { + this.removeRecaptchaScript(); + + const script = document.createElement('script'); + script.src = this.scriptSrc; + script.classList.add('js-recaptcha-script'); + script.async = true; + script.defer = true; + + this.script = script; + + document.body.appendChild(script); + }, + + removeRecaptchaScript() { + if (this.script instanceof Element) this.script.remove(); + }, + + close() { + this.removeRecaptchaScript(); + this.$emit('close'); + }, + + submit() { + this.$el.querySelector('form').submit(); + }, + }, + + watch: { + html() { + this.appendRecaptchaScript(); + }, + }, + + mounted() { + window.recaptchaDialogCallback = this.submit.bind(this); + }, +}; +</script> + +<template> +<popup-dialog + kind="warning" + class="recaptcha-dialog js-recaptcha-dialog" + :hide-footer="true" + :title="__('Please solve the reCAPTCHA')" + @toggle="close" +> + <div slot="body"> + <p> + {{__('We want to be sure it is you, please confirm you are not a robot.')}} + </p> + <div + ref="recaptcha" + v-html="html" + ></div> + </div> +</popup-dialog> +</template> diff --git a/app/assets/javascripts/vue_shared/mixins/recaptcha_dialog_implementor.js b/app/assets/javascripts/vue_shared/mixins/recaptcha_dialog_implementor.js new file mode 100644 index 00000000000..ef70f9432e3 --- /dev/null +++ b/app/assets/javascripts/vue_shared/mixins/recaptcha_dialog_implementor.js @@ -0,0 +1,36 @@ +import RecaptchaDialog from '../components/recaptcha_dialog.vue'; + +export default { + data() { + return { + showRecaptcha: false, + recaptchaHTML: '', + }; + }, + + components: { + RecaptchaDialog, + }, + + methods: { + openRecaptcha() { + this.showRecaptcha = true; + }, + + closeRecaptcha() { + this.showRecaptcha = false; + }, + + checkForSpam(data) { + if (!data.recaptcha_html) return data; + + this.recaptchaHTML = data.recaptcha_html; + + const spamError = new Error(data.error_message); + spamError.name = 'SpamError'; + spamError.message = 'SpamError'; + + throw spamError; + }, + }, +}; diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 5c9838c1029..ce551e6b7ce 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -48,3 +48,10 @@ body.modal-open { display: block; } +.recaptcha-dialog .recaptcha-form { + display: inline-block; + + .recaptcha { + margin: 0; + } +} diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 744e448e8df..ecac9be0360 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -25,7 +25,7 @@ module IssuableActions end format.json do - render_entity_json + recaptcha_check_with_fallback(false) { render_entity_json } end end diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index ada0dde87fb..03d8e188093 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -23,8 +23,8 @@ module SpammableActions @spam_config_loaded = Gitlab::Recaptcha.load_configurations! end - def recaptcha_check_with_fallback(&fallback) - if spammable.valid? + def recaptcha_check_with_fallback(should_redirect = true, &fallback) + if should_redirect && spammable.valid? redirect_to spammable_path elsif render_recaptcha? ensure_spam_config_loaded! @@ -33,7 +33,18 @@ module SpammableActions flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' end - render :verify + respond_to do |format| + format.html do + render :verify + end + + format.json do + locals = { spammable: spammable, script: false, has_submit: false } + recaptcha_html = render_to_string(partial: 'shared/recaptcha_form', formats: :html, locals: locals) + + render json: { recaptcha_html: recaptcha_html } + end + end else yield end diff --git a/app/views/layouts/_recaptcha_verification.html.haml b/app/views/layouts/_recaptcha_verification.html.haml index 77c77dc6754..e6f87ddd383 100644 --- a/app/views/layouts/_recaptcha_verification.html.haml +++ b/app/views/layouts/_recaptcha_verification.html.haml @@ -1,5 +1,4 @@ - humanized_resource_name = spammable.class.model_name.human.downcase -- resource_name = spammable.class.model_name.singular %h3.page-title Anti-spam verification @@ -8,16 +7,4 @@ %p #{"We detected potential spam in the #{humanized_resource_name}. Please solve the reCAPTCHA to proceed."} -= form_for form do |f| - .recaptcha - - params[resource_name].each do |field, value| - = hidden_field(resource_name, field, value: value) - = hidden_field_tag(:spam_log_id, spammable.spam_log.id) - = hidden_field_tag(:recaptcha_verification, true) - = recaptcha_tags - - -# Yields a block with given extra params. - = yield - - .row-content-block.footer-block - = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create' += render 'shared/recaptcha_form', spammable: spammable diff --git a/app/views/shared/_recaptcha_form.html.haml b/app/views/shared/_recaptcha_form.html.haml new file mode 100644 index 00000000000..0e816870f15 --- /dev/null +++ b/app/views/shared/_recaptcha_form.html.haml @@ -0,0 +1,19 @@ +- resource_name = spammable.class.model_name.singular +- humanized_resource_name = spammable.class.model_name.human.downcase +- script = local_assigns.fetch(:script, true) +- has_submit = local_assigns.fetch(:has_submit, true) + += form_for resource_name, method: :post, html: { class: 'recaptcha-form js-recaptcha-form' } do |f| + .recaptcha + - params[resource_name].each do |field, value| + = hidden_field(resource_name, field, value: value) + = hidden_field_tag(:spam_log_id, spammable.spam_log.id) + = hidden_field_tag(:recaptcha_verification, true) + = recaptcha_tags script: script, callback: 'recaptchaDialogCallback' + + -# Yields a block with given extra params. + = yield + + - if has_submit + .row-content-block.footer-block + = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create' diff --git a/changelogs/unreleased/29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected.yml b/changelogs/unreleased/29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected.yml new file mode 100644 index 00000000000..6bfcc5e70de --- /dev/null +++ b/changelogs/unreleased/29483-no-feedback-when-checking-on-checklist-if-potential-spam-was-detected.yml @@ -0,0 +1,5 @@ +--- +title: Add recaptcha modal to issue updates detected as spam +merge_request: 15408 +author: +type: fixed diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 4dbbaecdd6d..c5d08cb0b9d 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -272,6 +272,20 @@ describe Projects::IssuesController do expect(response).to have_http_status(:ok) expect(issue.reload.title).to eq('New title') end + + context 'when Akismet is enabled and the issue is identified as spam' do + before do + stub_application_setting(recaptcha_enabled: true) + allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) + end + + it 'renders json with recaptcha_html' do + subject + + expect(JSON.parse(response.body)).to have_key('recaptcha_html') + end + end end context 'when user does not have access to update issue' do @@ -504,17 +518,16 @@ describe Projects::IssuesController do expect(spam_logs.first.recaptcha_verified).to be_falsey end - it 'renders json errors' do + it 'renders recaptcha_html json response' do update_issue - expect(json_response) - .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."]) + expect(json_response).to have_key('recaptcha_html') end - it 'returns 422 status' do + it 'returns 200 status' do update_issue - expect(response).to have_gitlab_http_status(422) + expect(response).to have_gitlab_http_status(200) end end diff --git a/spec/javascripts/issue_show/components/app_spec.js b/spec/javascripts/issue_show/components/app_spec.js index b47a8bf705f..53b8a368d28 100644 --- a/spec/javascripts/issue_show/components/app_spec.js +++ b/spec/javascripts/issue_show/components/app_spec.js @@ -4,6 +4,7 @@ import '~/render_gfm'; import issuableApp from '~/issue_show/components/app.vue'; import eventHub from '~/issue_show/event_hub'; import issueShowData from '../mock_data'; +import setTimeoutPromise from '../../helpers/set_timeout_promise_helper'; function formatText(text) { return text.trim().replace(/\s\s+/g, ' '); @@ -55,6 +56,8 @@ describe('Issuable output', () => { Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); vm.poll.stop(); + + vm.$destroy(); }); it('should render a title/description/edited and update title/description/edited on update', (done) => { @@ -268,6 +271,52 @@ describe('Issuable output', () => { }); }); + it('opens recaptcha dialog if update rejected as spam', (done) => { + function mockScriptSrc() { + const recaptchaChild = vm.$children + .find(child => child.$options._componentTag === 'recaptcha-dialog'); // eslint-disable-line no-underscore-dangle + + recaptchaChild.scriptSrc = '//scriptsrc'; + } + + let modal; + const promise = new Promise((resolve) => { + resolve({ + json() { + return { + recaptcha_html: '<div class="g-recaptcha">recaptcha_html</div>', + }; + }, + }); + }); + + spyOn(vm.service, 'updateIssuable').and.returnValue(promise); + + vm.canUpdate = true; + vm.showForm = true; + + vm.$nextTick() + .then(() => mockScriptSrc()) + .then(() => vm.updateIssuable()) + .then(promise) + .then(() => setTimeoutPromise()) + .then(() => { + modal = vm.$el.querySelector('.js-recaptcha-dialog'); + + expect(modal.style.display).not.toEqual('none'); + expect(modal.querySelector('.g-recaptcha').textContent).toEqual('recaptcha_html'); + expect(document.body.querySelector('.js-recaptcha-script').src).toMatch('//scriptsrc'); + }) + .then(() => modal.querySelector('.close').click()) + .then(() => vm.$nextTick()) + .then(() => { + expect(modal.style.display).toEqual('none'); + expect(document.body.querySelector('.js-recaptcha-script')).toBeNull(); + }) + .then(done) + .catch(done.fail); + }); + describe('deleteIssuable', () => { it('changes URL when deleted', (done) => { spyOn(gl.utils, 'visitUrl'); diff --git a/spec/javascripts/issue_show/components/description_spec.js b/spec/javascripts/issue_show/components/description_spec.js index 163e5cdd062..2e000a1063f 100644 --- a/spec/javascripts/issue_show/components/description_spec.js +++ b/spec/javascripts/issue_show/components/description_spec.js @@ -51,6 +51,35 @@ describe('Description component', () => { }); }); + it('opens recaptcha dialog if update rejected as spam', (done) => { + let modal; + const recaptchaChild = vm.$children + .find(child => child.$options._componentTag === 'recaptcha-dialog'); // eslint-disable-line no-underscore-dangle + + recaptchaChild.scriptSrc = '//scriptsrc'; + + vm.taskListUpdateSuccess({ + recaptcha_html: '<div class="g-recaptcha">recaptcha_html</div>', + }); + + vm.$nextTick() + .then(() => { + modal = vm.$el.querySelector('.js-recaptcha-dialog'); + + expect(modal.style.display).not.toEqual('none'); + expect(modal.querySelector('.g-recaptcha').textContent).toEqual('recaptcha_html'); + expect(document.body.querySelector('.js-recaptcha-script').src).toMatch('//scriptsrc'); + }) + .then(() => modal.querySelector('.close').click()) + .then(() => vm.$nextTick()) + .then(() => { + expect(modal.style.display).toEqual('none'); + expect(document.body.querySelector('.js-recaptcha-script')).toBeNull(); + }) + .then(done) + .catch(done.fail); + }); + describe('TaskList', () => { beforeEach(() => { vm = mountComponent(DescriptionComponent, Object.assign({}, props, { @@ -86,6 +115,7 @@ describe('Description component', () => { dataType: 'issuableType', fieldName: 'description', selector: '.detail-page-description', + onSuccess: jasmine.any(Function), }); done(); }); diff --git a/spec/javascripts/vue_shared/components/popup_dialog_spec.js b/spec/javascripts/vue_shared/components/popup_dialog_spec.js new file mode 100644 index 00000000000..5c1d2a196f4 --- /dev/null +++ b/spec/javascripts/vue_shared/components/popup_dialog_spec.js @@ -0,0 +1,12 @@ +import Vue from 'vue'; +import PopupDialog from '~/vue_shared/components/popup_dialog.vue'; +import mountComponent from '../../helpers/vue_mount_component_helper'; + +describe('PopupDialog', () => { + it('does not render a primary button if no primaryButtonLabel', () => { + const popupDialog = Vue.extend(PopupDialog); + const vm = mountComponent(popupDialog); + + expect(vm.$el.querySelector('.js-primary-button')).toBeNull(); + }); +}); |