diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue index 71a1fc31315..052bb3dcb53 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue @@ -42,6 +42,7 @@ export default { :width="imgSize" :class="`s${imgSize}`" class="avatar avatar-inline m-0" + data-qa-selector="avatar_image" /> diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_title.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_title.vue index f4dac38b9e1..b107e9789a7 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignee_title.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_title.vue @@ -32,13 +32,14 @@ export default { }; - + {{ assigneeTitle }} - + {{ hiddenAssigneesLabel }} diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index bf6abdb8c4b..177778a7278 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -76,7 +76,7 @@ module ProjectsHelper link_to(author_html, user_path(author), class: "author-link js-user-link #{"#{opts[:extra_class]}" if opts[:extra_class]} #{"#{opts[:mobile_classes]}" if opts[:mobile_classes]}", data: data_attrs).html_safe else title = opts[:title].sub(":name", sanitize(author.name)) - link_to(author_html, user_path(author), class: "author-link has-tooltip", title: title, data: { container: 'body' }).html_safe + link_to(author_html, user_path(author), class: "author-link has-tooltip", title: title, data: { container: 'body', qa_selector: 'assignee_link' }).html_safe end end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 3ff4b4046d3..10bbeecc2f7 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -80,4 +80,9 @@ module Spammable def check_for_spam? true end + + # Override in Spammable if differs + def allow_possible_spam? + Feature.enabled?(:allow_possible_spam, project) + end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 1e84b9fa12e..4e693a94c6b 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -14,6 +14,7 @@ class Snippet < ApplicationRecord include Editable include Gitlab::SQL::Pattern include FromUnion + extend ::Gitlab::Utils::Override cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description @@ -191,6 +192,12 @@ class Snippet < ApplicationRecord (public? && (title_changed? || content_changed?)) end + # snippers are the biggest sources of spam + override :allow_possible_spam? + def allow_possible_spam? + false + end + def spammable_entity_type 'snippet' end diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index f2f133dae28..babe69cfdc8 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -37,7 +37,8 @@ class SpamService else # Otherwise, it goes to Akismet and check if it's a spam. If that's the # case, it assigns spammable record as "spam" and create a SpamLog record. - spammable.spam = check(api) + possible_spam = check(api) + spammable.spam = possible_spam unless spammable.allow_possible_spam? spammable.spam_log = spam_log end end diff --git a/app/views/shared/issuable/_assignees.html.haml b/app/views/shared/issuable/_assignees.html.haml index 24734ed66cf..cec865ec8de 100644 --- a/app/views/shared/issuable/_assignees.html.haml +++ b/app/views/shared/issuable/_assignees.html.haml @@ -7,4 +7,4 @@ = link_to_member(@project, assignee, name: false, title: "Assigned to :name") - if more_assignees_count.positive? - %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{more_assignees_count} more assignees" } } +#{more_assignees_count} + %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{more_assignees_count} more assignees", qa_selector: 'avatar_counter' } } +#{more_assignees_count} diff --git a/doc/development/i18n/externalization.md b/doc/development/i18n/externalization.md index a97719fe901..ff75ba8c73b 100644 --- a/doc/development/i18n/externalization.md +++ b/doc/development/i18n/externalization.md @@ -313,17 +313,22 @@ Developer documentation][mdn]. ## Updating the PO files with the new content -Now that the new content is marked for translation, we need to update the PO -files with the following command: +Now that the new content is marked for translation, we need to update +`locale/gitlab.pot` files with the following command: ```sh bin/rake gettext:regenerate ``` -This command will update the `locale/gitlab.pot` file with the newly externalized +This command will update `locale/gitlab.pot` file with the newly externalized strings and remove any strings that aren't used anymore. You should check this file in. Once the changes are on master, they will be picked up by -[Crowdin](http://translate.gitlab.com) and be presented for translation. +[Crowdin](http://translate.gitlab.com) and be presented for +translation. + +We don't need to check in any changes to the +`locale/[language]/gitlab.po` files. Those will be updated in a [when +translations from Crowdin are merged](merging_translations.md). If there are merge conflicts in the `gitlab.pot` file, you can delete the file and regenerate it using the same command. diff --git a/doc/user/clusters/applications.md b/doc/user/clusters/applications.md index 3da6424170f..3218a18d8f8 100644 --- a/doc/user/clusters/applications.md +++ b/doc/user/clusters/applications.md @@ -293,7 +293,7 @@ The applications below can be uninstalled. | ----------- | -------------- | ----- | | Cert-Manager | 12.2+ | The associated private key will be deleted and cannot be restored. Deployed applications will continue to use HTTPS, but certificates will not be renewed. Before uninstalling, you may wish to [back up your configuration](https://docs.cert-manager.io/en/latest/tasks/backup-restore-crds.html) or [revoke your certificates](https://letsencrypt.org/docs/revoking/) | | GitLab Runner | 12.2+ | Any running pipelines will be canceled. | -| Helm | 12.2+ | The associated Tiller pod will be deleted and cannot be restored. | +| Helm | 12.2+ | The associated Tiller pod, the `gitlab-managed-apps` namespace, and all of its resources will be deleted and cannot be restored. | | Ingress | 12.1+ | The associated load balancer and IP will be deleted and cannot be restored. Furthermore, it can only be uninstalled if JupyterHub is not installed. | | JupyterHub | 12.1+ | All data not committed to GitLab will be deleted and cannot be restored. | | Knative | 12.1+ | The associated IP will be deleted and cannot be restored. | diff --git a/qa/qa/page/base.rb b/qa/qa/page/base.rb index 104c9b19e2a..94e046fd3bf 100644 --- a/qa/qa/page/base.rb +++ b/qa/qa/page/base.rb @@ -131,6 +131,10 @@ module QA has_no_css?('.fa-spinner', wait: Capybara.default_max_wait_time) end + def finished_loading_block? + has_no_css?('.fa-spinner.block-loading', wait: Capybara.default_max_wait_time) + end + def wait_for_animated_element(name) # It would be ideal if we could detect when the animation is complete # but in some cases there's nothing we can easily access via capybara diff --git a/qa/qa/page/project/issue/index.rb b/qa/qa/page/project/issue/index.rb index f74366f6967..befee25b37a 100644 --- a/qa/qa/page/project/issue/index.rb +++ b/qa/qa/page/project/issue/index.rb @@ -5,14 +5,30 @@ module QA module Project module Issue class Index < Page::Base + view 'app/helpers/projects_helper.rb' do + element :assignee_link + end + view 'app/views/projects/issues/_issue.html.haml' do element :issue_link, 'link_to issue.title' # rubocop:disable QA/ElementWithPattern end + view 'app/views/shared/issuable/_assignees.html.haml' do + element :avatar_counter + end + view 'app/views/shared/issuable/_nav.html.haml' do element :closed_issues_link end + def assignee_link_count + all_elements(:assignee_link).count + end + + def avatar_counter + find_element(:avatar_counter) + end + def click_issue_link(title) click_link(title) end diff --git a/qa/qa/page/project/issue/show.rb b/qa/qa/page/project/issue/show.rb index e5e26b1864b..9264289b603 100644 --- a/qa/qa/page/project/issue/show.rb +++ b/qa/qa/page/project/issue/show.rb @@ -22,24 +22,54 @@ module QA element :noteable_note_item end + view 'app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue' do + element :avatar_image + end + + view 'app/assets/javascripts/sidebar/components/assignees/assignee_title.vue' do + element :assignee_edit_link + element :assignee_title + end + + view 'app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue' do + element :more_assignees_link + end + view 'app/helpers/dropdowns_helper.rb' do element :dropdown_input_field end + view 'app/views/shared/issuable/_close_reopen_button.html.haml' do + element :reopen_issue_button + end + + view 'app/views/shared/issuable/_sidebar.html.haml' do + element :assignee_block + element :labels_block + element :edit_link_labels + element :dropdown_menu_labels + element :milestone_link + end + view 'app/views/shared/notes/_form.html.haml' do element :new_note_form, 'new-note' # rubocop:disable QA/ElementWithPattern element :new_note_form, 'attr: :note' # rubocop:disable QA/ElementWithPattern end - view 'app/views/shared/issuable/_sidebar.html.haml' do - element :labels_block - element :edit_link_labels - element :dropdown_menu_labels - element :milestone_link + def assign(user) + click_element(:assignee_edit_link) + select_user(user.username) + click_body end - view 'app/views/shared/issuable/_close_reopen_button.html.haml' do - element :reopen_issue_button + def assignee_title + find_element(:assignee_title) + end + + def avatar_image_count + wait_assignees_block_finish_loading do + all_elements(:avatar_image).count + end end def click_milestone_link @@ -66,6 +96,10 @@ module QA end end + def more_assignees_link + find_element(:more_assignees_link) + end + def select_all_activities_filter select_filter_with_text('Show all activity') end @@ -103,6 +137,10 @@ module QA find_element(:labels_block) end + def toggle_more_assignees_link + click_element(:more_assignees_link) + end + private def select_filter_with_text(text) @@ -112,6 +150,20 @@ module QA find_element(:filter_options, text: text).click end end + + def select_user(username) + find("#{element_selector_css(:assignee_block)} input").set(username) + find('.dropdown-menu-user-link', text: "@#{username}").click + end + + def wait_assignees_block_finish_loading + within_element(:assignee_block) do + wait(reload: false, max: 10, interval: 1) do + finished_loading_block? + yield + end + end + end end end end diff --git a/qa/qa/runtime/env.rb b/qa/qa/runtime/env.rb index 594e5712ab2..a437c83100a 100644 --- a/qa/qa/runtime/env.rb +++ b/qa/qa/runtime/env.rb @@ -145,6 +145,38 @@ module QA ENV['GITLAB_QA_PASSWORD_2'] end + def gitlab_qa_username_3 + ENV['GITLAB_QA_USERNAME_3'] || 'gitlab-qa-user3' + end + + def gitlab_qa_password_3 + ENV['GITLAB_QA_PASSWORD_3'] + end + + def gitlab_qa_username_4 + ENV['GITLAB_QA_USERNAME_4'] || 'gitlab-qa-user4' + end + + def gitlab_qa_password_4 + ENV['GITLAB_QA_PASSWORD_4'] + end + + def gitlab_qa_username_5 + ENV['GITLAB_QA_USERNAME_5'] || 'gitlab-qa-user5' + end + + def gitlab_qa_password_5 + ENV['GITLAB_QA_PASSWORD_5'] + end + + def gitlab_qa_username_6 + ENV['GITLAB_QA_USERNAME_6'] || 'gitlab-qa-user6' + end + + def gitlab_qa_password_6 + ENV['GITLAB_QA_PASSWORD_6'] + end + def knapsack? !!(ENV['KNAPSACK_GENERATE_REPORT'] || ENV['KNAPSACK_REPORT_PATH'] || ENV['KNAPSACK_TEST_FILE_PATTERN']) end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index ad57c29850b..2edc0aa5536 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -408,6 +408,7 @@ describe Projects::IssuesController do context 'when user has access to update issue' do before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) project.add_developer(user) end @@ -421,14 +422,30 @@ describe Projects::IssuesController do 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) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end - it 'renders json with recaptcha_html' do - subject + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - expect(json_response).to have_key('recaptcha_html') + it 'renders json with recaptcha_html' do + subject + + expect(json_response).to have_key('recaptcha_html') + end + end + + context 'when allow_possible_spam feature flag is true' do + it 'updates the issue' do + subject + + expect(response).to have_http_status(:ok) + expect(issue.reload.title).to eq('New title') + end end end end @@ -681,13 +698,13 @@ describe Projects::IssuesController do before do project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) stub_application_setting(recaptcha_enabled: true) - allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) end context 'when an issue is not identified as spam' do before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: false) + end end it 'normally updates the issue' do @@ -696,45 +713,64 @@ describe Projects::IssuesController do end context 'when an issue is identified as spam' do - before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) - end - context 'when captcha is not verified' do before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end - it 'rejects an issue recognized as a spam' do - expect { update_issue }.not_to change { issue.reload.title } + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end + + it 'rejects an issue recognized as a spam' do + expect { update_issue }.not_to change { issue.reload.title } + end + + it 'rejects an issue recognized as a spam when recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + expect { update_issue }.not_to change { issue.reload.title } + end + + it 'creates a spam log' do + expect { update_issue(issue_params: { title: 'Spam title' }) } + .to log_spam(title: 'Spam title', noteable_type: 'Issue') + end + + it 'renders recaptcha_html json response' do + update_issue + + expect(json_response).to have_key('recaptcha_html') + end + + it 'returns 200 status' do + update_issue + + expect(response).to have_gitlab_http_status(200) + end end - it 'rejects an issue recognized as a spam when recaptcha disabled' do - stub_application_setting(recaptcha_enabled: false) + context 'when allow_possible_spam feature flag is true' do + it 'updates the issue recognized as spam' do + expect { update_issue }.to change { issue.reload.title } + end - expect { update_issue }.not_to change { issue.reload.title } - end + it 'creates a spam log' do + expect { update_issue(issue_params: { title: 'Spam title' }) } + .to log_spam( + title: 'Spam title', description: issue.description, + noteable_type: 'Issue', recaptcha_verified: false + ) + end - it 'creates a spam log' do - update_issue(issue_params: { title: 'Spam title' }) + it 'returns 200 status' do + update_issue - spam_logs = SpamLog.all - - expect(spam_logs.count).to eq(1) - expect(spam_logs.first.title).to eq('Spam title') - expect(spam_logs.first.recaptcha_verified).to be_falsey - end - - it 'renders recaptcha_html json response' do - update_issue - - expect(json_response).to have_key('recaptcha_html') - end - - it 'returns 200 status' do - update_issue - - expect(response).to have_gitlab_http_status(200) + expect(response).to have_gitlab_http_status(200) + end end end @@ -748,11 +784,6 @@ describe Projects::IssuesController do additional_params: { spam_log_id: spam_logs.last.id, recaptcha_verification: true }) end - before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha) - .and_return(true) - end - it 'returns 200 status' do expect(response).to have_gitlab_http_status(200) end @@ -917,55 +948,72 @@ describe Projects::IssuesController do context 'Akismet is enabled' do before do stub_application_setting(recaptcha_enabled: true) - allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) end context 'when an issue is not identified as spam' do before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) + stub_feature_flags(allow_possible_spam: false) + + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: false) + end end - it 'does not create an issue' do - expect { post_new_issue(title: '') }.not_to change(Issue, :count) + it 'creates an issue' do + expect { post_new_issue(title: 'Some title') }.to change(Issue, :count) end end context 'when an issue is identified as spam' do - before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) - end - context 'when captcha is not verified' do def post_spam_issue post_new_issue(title: 'Spam Title', description: 'Spam lives here') end before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end - it 'rejects an issue recognized as a spam' do - expect { post_spam_issue }.not_to change(Issue, :count) + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end + + it 'rejects an issue recognized as a spam' do + expect { post_spam_issue }.not_to change(Issue, :count) + end + + it 'creates a spam log' do + expect { post_spam_issue } + .to log_spam(title: 'Spam Title', noteable_type: 'Issue', recaptcha_verified: false) + end + + it 'does not create an issue when it is not valid' do + expect { post_new_issue(title: '') }.not_to change(Issue, :count) + end + + it 'does not create an issue when recaptcha is not enabled' do + stub_application_setting(recaptcha_enabled: false) + + expect { post_spam_issue }.not_to change(Issue, :count) + end end - it 'creates a spam log' do - post_spam_issue - spam_logs = SpamLog.all + context 'when allow_possible_spam feature flag is true' do + it 'creates an issue recognized as spam' do + expect { post_spam_issue }.to change(Issue, :count) + end - expect(spam_logs.count).to eq(1) - expect(spam_logs.first.title).to eq('Spam Title') - expect(spam_logs.first.recaptcha_verified).to be_falsey - end + it 'creates a spam log' do + expect { post_spam_issue } + .to log_spam(title: 'Spam Title', noteable_type: 'Issue', recaptcha_verified: false) + end - it 'does not create an issue when it is not valid' do - expect { post_new_issue(title: '') }.not_to change(Issue, :count) - end - - it 'does not create an issue when recaptcha is not enabled' do - stub_application_setting(recaptcha_enabled: false) - - expect { post_spam_issue }.not_to change(Issue, :count) + it 'does not create an issue when it is not valid' do + expect { post_new_issue(title: '') }.not_to change(Issue, :count) + end end end @@ -977,7 +1025,7 @@ describe Projects::IssuesController do end before do - allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(true) + expect(controller).to receive_messages(verify_recaptcha: true) end it 'accepts an issue after recaptcha is verified' do @@ -1030,8 +1078,12 @@ describe Projects::IssuesController do describe 'POST #mark_as_spam' do context 'properly submits to Akismet' do before do - allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true) - allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(submit_spam: true) + end + expect_next_instance_of(ApplicationSetting) do |setting| + expect(setting).to receive_messages(akismet_enabled: true) + end end def post_spam @@ -1266,7 +1318,9 @@ describe Projects::IssuesController do end it "shows error when upload fails" do - allow_any_instance_of(UploadService).to receive(:execute).and_return(nil) + expect_next_instance_of(UploadService) do |upload_service| + expect(upload_service).to receive(:execute).and_return(nil) + end import_csv diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index b13534b9088..042a5542786 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -111,7 +111,7 @@ describe Projects::SnippetsController do it 'creates a spam log' do expect { create_snippet(project, visibility_level: Snippet::PUBLIC) } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Title', user_id: user.id, noteable_type: 'ProjectSnippet') end it 'renders :new with recaptcha disabled' do @@ -192,7 +192,7 @@ describe Projects::SnippetsController do it 'creates a spam log' do expect { update_snippet(title: 'Foo') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'ProjectSnippet') end it 'renders :edit with recaptcha disabled' do @@ -237,7 +237,7 @@ describe Projects::SnippetsController do it 'creates a spam log' do expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'ProjectSnippet') end it 'renders :edit with recaptcha disabled' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 1b3a8965342..e892c736c69 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -269,7 +269,7 @@ describe SnippetsController do it 'creates a spam log' do expect { create_snippet(visibility_level: Snippet::PUBLIC) } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Title', user: user, noteable_type: 'PersonalSnippet') end it 'renders :new with recaptcha disabled' do @@ -345,7 +345,7 @@ describe SnippetsController do it 'creates a spam log' do expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user: user, noteable_type: 'PersonalSnippet') end it 'renders :edit with recaptcha disabled' do @@ -389,8 +389,8 @@ describe SnippetsController do end it 'creates a spam log' do - expect { update_snippet(title: 'Foo') } - .to change { SpamLog.count }.by(1) + expect {update_snippet(title: 'Foo') } + .to log_spam(title: 'Foo', user: user, noteable_type: 'PersonalSnippet') end it 'renders :edit with recaptcha disabled' do diff --git a/spec/features/issues/spam_issues_spec.rb b/spec/features/issues/spam_issues_spec.rb index 0d009f47fff..18245f249fd 100644 --- a/spec/features/issues/spam_issues_spec.rb +++ b/spec/features/issues/spam_issues_spec.rb @@ -30,21 +30,47 @@ describe 'New issue', :js do visit new_project_issue_path(project) end - it 'creates an issue after solving reCaptcha' do - fill_in 'issue_title', with: 'issue title' - fill_in 'issue_description', with: 'issue description' + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - click_button 'Submit issue' + it 'creates an issue after solving reCaptcha' do + fill_in 'issue_title', with: 'issue title' + fill_in 'issue_description', with: 'issue description' - # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha - # recaptcha verification is skipped in test environment and it always returns true - expect(page).not_to have_content('issue title') - expect(page).to have_css('.recaptcha') + click_button 'Submit issue' - click_button 'Submit issue' + # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha + # recaptcha verification is skipped in test environment and it always returns true + expect(page).not_to have_content('issue title') + expect(page).to have_css('.recaptcha') - expect(page.find('.issue-details h2.title')).to have_content('issue title') - expect(page.find('.issue-details .description')).to have_content('issue description') + click_button 'Submit issue' + + expect(page.find('.issue-details h2.title')).to have_content('issue title') + expect(page.find('.issue-details .description')).to have_content('issue description') + end + end + + context 'when allow_possible_spam feature flag is true' do + before do + fill_in 'issue_title', with: 'issue title' + fill_in 'issue_description', with: 'issue description' + end + + it 'creates an issue without a need to solve reCaptcha' do + click_button 'Submit issue' + + expect(page).not_to have_css('.recaptcha') + expect(page.find('.issue-details h2.title')).to have_content('issue title') + expect(page.find('.issue-details .description')).to have_content('issue description') + end + + it 'creates a spam log record' do + expect { click_button 'Submit issue' } + .to log_spam(title: 'issue title', description: 'issue description', user_id: user.id, noteable_type: 'Issue') + end end end diff --git a/spec/features/snippets/spam_snippets_spec.rb b/spec/features/snippets/spam_snippets_spec.rb new file mode 100644 index 00000000000..3e71a4e7879 --- /dev/null +++ b/spec/features/snippets/spam_snippets_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'User creates snippet', :js do + let(:user) { create(:user) } + + before do + stub_feature_flags(allow_possible_spam: false) + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + + Gitlab::CurrentSettings.update!( + akismet_enabled: true, + akismet_api_key: 'testkey', + recaptcha_enabled: true, + recaptcha_site_key: 'test site key', + recaptcha_private_key: 'test private key' + ) + + sign_in(user) + visit new_snippet_path + + fill_in 'personal_snippet_title', with: 'My Snippet Title' + fill_in 'personal_snippet_description', with: 'My Snippet **Description**' + find('#personal_snippet_visibility_level_20').set(true) + page.within('.file-editor') do + find('.ace_text-input', visible: false).send_keys 'Hello World!' + end + end + + shared_examples 'solve recaptcha' do + it 'creates a snippet after solving reCaptcha' do + click_button('Create snippet') + wait_for_requests + + # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha + # recaptcha verification is skipped in test environment and it always returns true + expect(page).not_to have_content('My Snippet Title') + expect(page).to have_css('.recaptcha') + click_button('Submit personal snippet') + + expect(page).to have_content('My Snippet Title') + end + end + + context 'when identified as a spam' do + before do + WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200) + end + + context 'when allow_possible_spam feature flag is false' do + it_behaves_like 'solve recaptcha' + end + + context 'when allow_possible_spam feature flag is true' do + it_behaves_like 'solve recaptcha' + end + end + + context 'when not identified as a spam' do + before do + WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "false", status: 200) + end + + it 'creates a snippet' do + click_button('Create snippet') + wait_for_requests + + expect(page).not_to have_css('.recaptcha') + expect(page).to have_content('My Snippet Title') + end + end +end diff --git a/spec/requests/api/issues/post_projects_issues_spec.rb b/spec/requests/api/issues/post_projects_issues_spec.rb index b74e8867310..3a55b437ead 100644 --- a/spec/requests/api/issues/post_projects_issues_spec.rb +++ b/spec/requests/api/issues/post_projects_issues_spec.rb @@ -374,9 +374,17 @@ describe API::Issues do end describe 'POST /projects/:id/issues with spam filtering' do + def post_issue + post api("/projects/#{project.id}/issues", user), params: params + end + before do - allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(AkismetService).to receive_messages(spam?: true) + expect_next_instance_of(SpamService) do |spam_service| + expect(spam_service).to receive_messages(check_for_spam?: true) + end + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end let(:params) do @@ -387,17 +395,43 @@ describe API::Issues do } end - it 'does not create a new project issue' do - expect { post api("/projects/#{project.id}/issues", user), params: params }.not_to change(Issue, :count) - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']).to eq({ 'error' => 'Spam detected' }) + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - spam_logs = SpamLog.all - expect(spam_logs.count).to eq(1) - expect(spam_logs[0].title).to eq('new issue') - expect(spam_logs[0].description).to eq('content here') - expect(spam_logs[0].user).to eq(user) - expect(spam_logs[0].noteable_type).to eq('Issue') + it 'does not create a new project issue' do + expect { post_issue }.not_to change(Issue, :count) + end + + it 'returns correct status and message' do + post_issue + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq({ 'error' => 'Spam detected' }) + end + + it 'creates a new spam log entry' do + expect { post_issue } + .to log_spam(title: 'new issue', description: 'content here', user_id: user.id, noteable_type: 'Issue') + end + end + + context 'when allow_possible_spam feature flag is true' do + it 'does creates a new project issue' do + expect { post_issue }.to change(Issue, :count).by(1) + end + + it 'returns correct status' do + post_issue + + expect(response).to have_gitlab_http_status(201) + end + + it 'creates a new spam log entry' do + expect { post_issue } + .to log_spam(title: 'new issue', description: 'content here', user_id: user.id, noteable_type: 'Issue') + end end end diff --git a/spec/requests/api/issues/put_projects_issues_spec.rb b/spec/requests/api/issues/put_projects_issues_spec.rb index 267cba93713..43f302ed194 100644 --- a/spec/requests/api/issues/put_projects_issues_spec.rb +++ b/spec/requests/api/issues/put_projects_issues_spec.rb @@ -181,6 +181,10 @@ describe API::Issues do end describe 'PUT /projects/:id/issues/:issue_iid with spam filtering' do + def update_issue + put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: params + end + let(:params) do { title: 'updated title', @@ -189,21 +193,52 @@ describe API::Issues do } end - it 'does not create a new project issue' do - allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true) - allow_any_instance_of(AkismetService).to receive_messages(spam?: true) + before do + expect_next_instance_of(SpamService) do |spam_service| + expect(spam_service).to receive_messages(check_for_spam?: true) + end + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end + end - put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: params + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']).to eq({ 'error' => 'Spam detected' }) + it 'does not update a project issue' do + expect { update_issue }.not_to change { issue.reload.title } + end - spam_logs = SpamLog.all - expect(spam_logs.count).to eq(1) - expect(spam_logs[0].title).to eq('updated title') - expect(spam_logs[0].description).to eq('content here') - expect(spam_logs[0].user).to eq(user) - expect(spam_logs[0].noteable_type).to eq('Issue') + it 'returns correct status and message' do + update_issue + + expect(response).to have_gitlab_http_status(400) + expect(json_response).to include('message' => { 'error' => 'Spam detected' }) + end + + it 'creates a new spam log entry' do + expect { update_issue } + .to log_spam(title: 'updated title', description: 'content here', user_id: user.id, noteable_type: 'Issue') + end + end + + context 'when allow_possible_spam feature flag is true' do + it 'updates a project issue' do + expect { update_issue }.to change { issue.reload.title } + end + + it 'returns correct status and message' do + update_issue + + expect(response).to have_gitlab_http_status(200) + end + + it 'creates a new spam log entry' do + expect { update_issue } + .to log_spam(title: 'updated title', description: 'content here', user_id: user.id, noteable_type: 'Issue') + end end end diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 2e6e13aa927..ef0cabad4b0 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -198,7 +198,7 @@ describe API::ProjectSnippets do it 'creates a spam log' do expect { create_snippet(project, visibility: 'public') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Test Title', user_id: user.id, noteable_type: 'ProjectSnippet') end end end @@ -289,7 +289,7 @@ describe API::ProjectSnippets do it 'creates a spam log' do expect { update_snippet(title: 'Foo') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet') end end @@ -306,7 +306,7 @@ describe API::ProjectSnippets do it 'creates a spam log' do expect { update_snippet(title: 'Foo', visibility: 'public') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet') end end end diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index 515912cb305..e7eaaea2418 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -254,7 +254,7 @@ describe API::Snippets do it 'creates a spam log' do expect { create_snippet(visibility: 'public') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Test Title', user_id: user.id, noteable_type: 'PersonalSnippet') end end end @@ -344,8 +344,7 @@ describe API::Snippets do end it 'creates a spam log' do - expect { update_snippet(title: 'Foo') } - .to change { SpamLog.count }.by(1) + expect { update_snippet(title: 'Foo') }.to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet') end end @@ -359,7 +358,7 @@ describe API::Snippets do it 'creates a spam log' do expect { update_snippet(title: 'Foo', visibility: 'public') } - .to change { SpamLog.count }.by(1) + .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet') end end end diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb index 7d2491b3a49..1751029a78c 100644 --- a/spec/services/create_snippet_service_spec.rb +++ b/spec/services/create_snippet_service_spec.rb @@ -3,26 +3,28 @@ require 'spec_helper' describe CreateSnippetService do - before do - @user = create :user - @admin = create :user, admin: true - @opts = { + let(:user) { create(:user) } + let(:admin) { create(:user, :admin) } + let(:opts) { base_opts.merge(extra_opts) } + let(:base_opts) do + { title: 'Test snippet', file_name: 'snippet.rb', content: 'puts "hello world"', visibility_level: Gitlab::VisibilityLevel::PRIVATE } end + let(:extra_opts) { {} } context 'When public visibility is restricted' do + let(:extra_opts) { { visibility_level: Gitlab::VisibilityLevel::PUBLIC } } + before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) - - @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) end it 'non-admins are not able to create a public snippet' do - snippet = create_snippet(nil, @user, @opts) + snippet = create_snippet(nil, user, opts) expect(snippet.errors.messages).to have_key(:visibility_level) expect(snippet.errors.messages[:visibility_level].first).to( match('has been restricted') @@ -30,37 +32,81 @@ describe CreateSnippetService do end it 'admins are able to create a public snippet' do - snippet = create_snippet(nil, @admin, @opts) + snippet = create_snippet(nil, admin, opts) expect(snippet.errors.any?).to be_falsey expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) end describe "when visibility level is passed as a string" do + let(:extra_opts) { { visibility: 'internal' } } + before do - @opts[:visibility] = 'internal' - @opts.delete(:visibility_level) + base_opts.delete(:visibility_level) end it "assigns the correct visibility level" do - snippet = create_snippet(nil, @user, @opts) + snippet = create_snippet(nil, user, opts) expect(snippet.errors.any?).to be_falsey expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) end end end + context 'checking spam' do + shared_examples 'marked as spam' do + let(:snippet) { create_snippet(nil, admin, opts) } + + it 'marks a snippet as a spam ' do + expect(snippet).to be_spam + end + + it 'invalidates the snippet' do + expect(snippet).to be_invalid + end + + it 'creates a new spam_log' do + expect { snippet } + .to log_spam(title: snippet.title, noteable_type: 'PersonalSnippet') + end + + it 'assigns a spam_log to an issue' do + expect(snippet.spam_log).to eq(SpamLog.last) + end + end + + let(:extra_opts) do + { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) } + end + + before do + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end + end + + [true, false, nil].each do |allow_possible_spam| + context "when recaptcha_disabled flag is #{allow_possible_spam.inspect}" do + before do + stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil? + end + + it_behaves_like 'marked as spam' + end + end + end + describe 'usage counter' do let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } it 'increments count' do expect do - create_snippet(nil, @admin, @opts) + create_snippet(nil, admin, opts) end.to change { counter.read(:create) }.by 1 end it 'does not increment count if create fails' do expect do - create_snippet(nil, @admin, {}) + create_snippet(nil, admin, {}) end.not_to change { counter.read(:create) } end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index b7bedc2f97e..5dc6b6176ee 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -344,7 +344,7 @@ describe Issues::CreateService do end before do - allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) + stub_feature_flags(allow_possible_spam: false) end context 'when recaptcha was verified' do @@ -384,31 +384,67 @@ describe Issues::CreateService do end context 'when recaptcha was not verified' do + before do + expect_next_instance_of(SpamService) do |spam_service| + expect(spam_service).to receive_messages(check_for_spam?: true) + end + end + context 'when akismet detects spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end - it 'marks an issue as a spam ' do - expect(issue).to be_spam + context 'when issuables_recaptcha_enabled feature flag is true' do + it 'marks an issue as a spam ' do + expect(issue).to be_spam + end + + it 'invalidates the issue' do + expect(issue).to be_invalid + end + + it 'creates a new spam_log' do + expect { issue } + .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') + end + + it 'assigns a spam_log to an issue' do + expect(issue.spam_log).to eq(SpamLog.last) + end end - it 'an issue is not valid ' do - expect(issue.valid?).to be_falsey - end + context 'when issuable_recaptcha_enabled feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: true) + end - it 'creates a new spam_log' do - expect {issue}.to change {SpamLog.count}.from(0).to(1) - end + it 'does not mark an issue as a spam ' do + expect(issue).not_to be_spam + end - it 'assigns a spam_log to an issue' do - expect(issue.spam_log).to eq(SpamLog.last) + it 'accepts the ​issue as valid' do + expect(issue).to be_valid + end + + it 'creates a new spam_log' do + expect { issue } + .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') + end + + it 'assigns a spam_log to an issue' do + expect(issue.spam_log).to eq(SpamLog.last) + end end end context 'when akismet does not detect spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: false) + end end it 'does not mark an issue as a spam ' do diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb index b9e5e844c1f..76f77583612 100644 --- a/spec/services/spam_service_spec.rb +++ b/spec/services/spam_service_spec.rb @@ -44,30 +44,50 @@ describe SpamService do end context 'when indicated as spam by akismet' do + shared_examples 'akismet spam' do + it 'doesnt check as spam when request is missing' do + check_spam(issue, nil, false) + + expect(issue).not_to be_spam + end + + it 'creates a spam log' do + expect { check_spam(issue, request, false) } + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + end + + it 'does not yield to the block' do + expect(check_spam(issue, request, false)) + .to eql(SpamLog.last) + end + end + before do allow(AkismetService).to receive(:new).and_return(double(spam?: true)) end - it 'doesnt check as spam when request is missing' do - check_spam(issue, nil, false) + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - expect(issue.spam).to be_falsey + it_behaves_like 'akismet spam' + + it 'checks as spam' do + check_spam(issue, request, false) + + expect(issue.spam).to be_truthy + end end - it 'checks as spam' do - check_spam(issue, request, false) + context 'when allow_possible_spam feature flag is true' do + it_behaves_like 'akismet spam' - expect(issue.spam).to be_truthy - end + it 'does not check as spam' do + check_spam(issue, request, false) - it 'creates a spam log' do - expect { check_spam(issue, request, false) } - .to change { SpamLog.count }.from(0).to(1) - end - - it 'doesnt yield block' do - expect(check_spam(issue, request, false)) - .to eql(SpamLog.last) + expect(issue.spam).to be_nil + end end end diff --git a/spec/support/matchers/log_spam.rb b/spec/support/matchers/log_spam.rb new file mode 100644 index 00000000000..541cacf558c --- /dev/null +++ b/spec/support/matchers/log_spam.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# This matcher checkes if one spam log with provided attributes was created +# +# Example: +# +# expect { create_issue }.to log_spam +RSpec::Matchers.define :log_spam do |expected| + def spam_logs + SpamLog.all + end + + match do |block| + block.call + + expect(spam_logs).to contain_exactly( + have_attributes(expected) + ) + end + + description do + count = spam_logs.count + + if count == 1 + keys = expected.keys.map(&:to_s) + actual = spam_logs.first.attributes.slice(*keys) + "create a spam log with #{expected} attributes. #{actual} created instead." + else + "create exactly 1 spam log with #{expected} attributes. #{count} spam logs created instead." + end + end + + supports_block_expectations +end