diff options
author | Rémy Coutable <remy@rymai.me> | 2017-02-02 16:29:04 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-02-02 16:29:04 +0000 |
commit | 29a2843c229ab5c8be4884e63f062a134eb04bef (patch) | |
tree | 773d1f02d1652005e93c457a17dddfe91f3cb32f | |
parent | ee59a64b2035622b02760d5ab928ea59a35cd45f (diff) | |
parent | 34918d94c011e8f81bd962d43d67fe8bd9f21e3e (diff) | |
download | gitlab-ce-29a2843c229ab5c8be4884e63f062a134eb04bef.tar.gz |
Merge branch 'snippet-spam' into 'master'
Snippet spam
Closes #26276
See merge request !8911
25 files changed, 286 insertions, 26 deletions
diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 99acd98ae13..562f92bd83c 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -7,7 +7,7 @@ module SpammableActions def mark_as_spam if SpamService.new(spammable).mark_as_spam! - redirect_to spammable, notice: "#{spammable.class} was submitted to Akismet successfully." + redirect_to spammable, notice: "#{spammable.spammable_entity_type.titlecase} was submitted to Akismet successfully." else redirect_to spammable, alert: 'Error with Akismet. Please check the logs for more info.' end diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 02a97c1c574..5d193f26a8e 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -1,8 +1,9 @@ class Projects::SnippetsController < Projects::ApplicationController include ToggleAwardEmoji + include SpammableActions before_action :module_enabled - before_action :snippet, only: [:show, :edit, :destroy, :update, :raw, :toggle_award_emoji] + before_action :snippet, only: [:show, :edit, :destroy, :update, :raw, :toggle_award_emoji, :mark_as_spam] # Allow read any snippet before_action :authorize_read_project_snippet!, except: [:new, :create, :index] @@ -36,8 +37,8 @@ class Projects::SnippetsController < Projects::ApplicationController end def create - @snippet = CreateSnippetService.new(@project, current_user, - snippet_params).execute + create_params = snippet_params.merge(request: request) + @snippet = CreateSnippetService.new(@project, current_user, create_params).execute if @snippet.valid? respond_with(@snippet, @@ -88,6 +89,7 @@ class Projects::SnippetsController < Projects::ApplicationController @snippet ||= @project.snippets.find(params[:id]) end alias_method :awardable, :snippet + alias_method :spammable, :snippet def authorize_read_project_snippet! return render_404 unless can?(current_user, :read_project_snippet, @snippet) diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index dee57e4a388..b169d993688 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -1,5 +1,6 @@ class SnippetsController < ApplicationController include ToggleAwardEmoji + include SpammableActions before_action :snippet, only: [:show, :edit, :destroy, :update, :raw, :download] @@ -40,8 +41,8 @@ class SnippetsController < ApplicationController end def create - @snippet = CreateSnippetService.new(nil, current_user, - snippet_params).execute + create_params = snippet_params.merge(request: request) + @snippet = CreateSnippetService.new(nil, current_user, create_params).execute respond_with @snippet.becomes(Snippet) end @@ -96,6 +97,7 @@ class SnippetsController < ApplicationController end end alias_method :awardable, :snippet + alias_method :spammable, :snippet def authorize_read_snippet! authenticate_user! unless can?(current_user, :read_personal_snippet, @snippet) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 3a83ae15dd8..fc93acfe63e 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -93,10 +93,6 @@ module VisibilityLevelHelper current_application_settings.default_project_visibility end - def default_snippet_visibility - current_application_settings.default_snippet_visibility - end - def default_group_visibility current_application_settings.default_group_visibility end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 1aa97debe42..1acff093aa1 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -34,7 +34,13 @@ module Spammable end def check_for_spam - self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? + if spam? + self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam and has been discarded.") + end + end + + def spammable_entity_type + self.class.name.underscore end def spam_title diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb index 25b5d777641..9bb456eee24 100644 --- a/app/models/project_snippet.rb +++ b/app/models/project_snippet.rb @@ -9,4 +9,8 @@ class ProjectSnippet < Snippet participant :author participant :notes_with_associations + + def check_for_spam? + super && project.public? + end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 771a7350556..2665a7249a3 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -7,6 +7,7 @@ class Snippet < ActiveRecord::Base include Sortable include Awardable include Mentionable + include Spammable cache_markdown_field :title, pipeline: :single_line cache_markdown_field :content @@ -17,7 +18,7 @@ class Snippet < ActiveRecord::Base default_content_html_invalidator || file_name_changed? end - default_value_for :visibility_level, Snippet::PRIVATE + default_value_for(:visibility_level) { current_application_settings.default_snippet_visibility } belongs_to :author, class_name: 'User' belongs_to :project @@ -46,6 +47,9 @@ class Snippet < ActiveRecord::Base participant :author participant :notes_with_associations + attr_spammable :title, spam_title: true + attr_spammable :content, spam_description: true + def self.reference_prefix '$' end @@ -127,6 +131,14 @@ class Snippet < ActiveRecord::Base notes.includes(:author) end + def check_for_spam? + public? + end + + def spammable_entity_type + 'snippet' + end + class << self # Searches for snippets with a matching title or file name. # diff --git a/app/services/create_snippet_service.rb b/app/services/create_snippet_service.rb index 95cc9baf406..14f5ba064ff 100644 --- a/app/services/create_snippet_service.rb +++ b/app/services/create_snippet_service.rb @@ -1,5 +1,8 @@ class CreateSnippetService < BaseService def execute + request = params.delete(:request) + api = params.delete(:api) + snippet = if project project.snippets.build(params) else @@ -12,8 +15,12 @@ class CreateSnippetService < BaseService end snippet.author = current_user + snippet.spam = SpamService.new(snippet, request).check(api) + + if snippet.save + UserAgentDetailService.new(snippet, request).create + end - snippet.save snippet end end diff --git a/app/views/projects/snippets/_actions.html.haml b/app/views/projects/snippets/_actions.html.haml index 068a6610350..e2a5107a883 100644 --- a/app/views/projects/snippets/_actions.html.haml +++ b/app/views/projects/snippets/_actions.html.haml @@ -8,6 +8,8 @@ - if can?(current_user, :create_project_snippet, @project) = link_to new_namespace_project_snippet_path(@project.namespace, @project), class: 'btn btn-grouped btn-inverted btn-create', title: "New snippet" do New snippet + - if @snippet.submittable_as_spam? && current_user.admin? + = link_to 'Submit as spam', mark_as_spam_namespace_project_snippet_path(@project.namespace, @project, @snippet), method: :post, class: 'btn btn-grouped btn-spam', title: 'Submit as spam' - if can?(current_user, :create_project_snippet, @project) || can?(current_user, :update_project_snippet, @snippet) .visible-xs-block.dropdown %button.btn.btn-default.btn-block.append-bottom-0.prepend-top-5{ data: { toggle: "dropdown" } } @@ -27,3 +29,6 @@ %li = link_to edit_namespace_project_snippet_path(@project.namespace, @project, @snippet) do Edit + - if @snippet.submittable_as_spam? && current_user.admin? + %li + = link_to 'Submit as spam', mark_as_spam_namespace_project_snippet_path(@project.namespace, @project, @snippet), method: :post diff --git a/app/views/projects/snippets/edit.html.haml b/app/views/projects/snippets/edit.html.haml index 216f70f5605..fb39028529d 100644 --- a/app/views/projects/snippets/edit.html.haml +++ b/app/views/projects/snippets/edit.html.haml @@ -3,4 +3,4 @@ %h3.page-title Edit Snippet %hr -= render "shared/snippets/form", url: namespace_project_snippet_path(@project.namespace, @project, @snippet), visibility_level: @snippet.visibility_level += render "shared/snippets/form", url: namespace_project_snippet_path(@project.namespace, @project, @snippet) diff --git a/app/views/projects/snippets/new.html.haml b/app/views/projects/snippets/new.html.haml index 772a594269c..cfed3a79bc5 100644 --- a/app/views/projects/snippets/new.html.haml +++ b/app/views/projects/snippets/new.html.haml @@ -3,4 +3,4 @@ %h3.page-title New Snippet %hr -= render "shared/snippets/form", url: namespace_project_snippets_path(@project.namespace, @project, @snippet), visibility_level: default_snippet_visibility += render "shared/snippets/form", url: namespace_project_snippets_path(@project.namespace, @project, @snippet) diff --git a/app/views/shared/snippets/_form.html.haml b/app/views/shared/snippets/_form.html.haml index 0c788032020..2d22782eb36 100644 --- a/app/views/shared/snippets/_form.html.haml +++ b/app/views/shared/snippets/_form.html.haml @@ -11,7 +11,7 @@ .col-sm-10 = f.text_field :title, class: 'form-control', required: true, autofocus: true - = render 'shared/visibility_level', f: f, visibility_level: visibility_level, can_change_visibility_level: true, form_model: @snippet + = render 'shared/visibility_level', f: f, visibility_level: @snippet.visibility_level, can_change_visibility_level: true, form_model: @snippet .file-editor .form-group @@ -34,4 +34,3 @@ = link_to "Cancel", namespace_project_snippets_path(@project.namespace, @project), class: "btn btn-cancel" - else = link_to "Cancel", snippets_path(@project), class: "btn btn-cancel" - diff --git a/app/views/snippets/_actions.html.haml b/app/views/snippets/_actions.html.haml index 95fc7198104..9a9a3ff9220 100644 --- a/app/views/snippets/_actions.html.haml +++ b/app/views/snippets/_actions.html.haml @@ -8,6 +8,8 @@ - if current_user = link_to new_snippet_path, class: "btn btn-grouped btn-inverted btn-create", title: "New snippet" do New snippet + - if @snippet.submittable_as_spam? && current_user.admin? + = link_to 'Submit as spam', mark_as_spam_snippet_path(@snippet), method: :post, class: 'btn btn-grouped btn-spam', title: 'Submit as spam' - if current_user .visible-xs-block.dropdown %button.btn.btn-default.btn-block.append-bottom-0.prepend-top-5{ data: { toggle: "dropdown" } } @@ -26,3 +28,6 @@ %li = link_to edit_snippet_path(@snippet) do Edit + - if @snippet.submittable_as_spam? && current_user.admin? + %li + = link_to 'Submit as spam', mark_as_spam_snippet_path(@snippet), method: :post diff --git a/app/views/snippets/edit.html.haml b/app/views/snippets/edit.html.haml index 82f44a9a5c3..915bf98eb3e 100644 --- a/app/views/snippets/edit.html.haml +++ b/app/views/snippets/edit.html.haml @@ -2,4 +2,4 @@ %h3.page-title Edit Snippet %hr -= render 'shared/snippets/form', url: snippet_path(@snippet), visibility_level: @snippet.visibility_level += render 'shared/snippets/form', url: snippet_path(@snippet) diff --git a/app/views/snippets/new.html.haml b/app/views/snippets/new.html.haml index 79e2392490d..ca8afb4bb6a 100644 --- a/app/views/snippets/new.html.haml +++ b/app/views/snippets/new.html.haml @@ -2,4 +2,4 @@ %h3.page-title New Snippet %hr -= render "shared/snippets/form", url: snippets_path(@snippet), visibility_level: default_snippet_visibility += render "shared/snippets/form", url: snippets_path(@snippet) diff --git a/changelogs/unreleased/snippet-spam.yml b/changelogs/unreleased/snippet-spam.yml new file mode 100644 index 00000000000..4867f088953 --- /dev/null +++ b/changelogs/unreleased/snippet-spam.yml @@ -0,0 +1,4 @@ +--- +title: Check public snippets for spam +merge_request: +author: diff --git a/config/routes/project.rb b/config/routes/project.rb index f36febc6e04..efe2fbc521d 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -64,6 +64,7 @@ constraints(ProjectUrlConstrainer.new) do resources :snippets, concerns: :awardable, constraints: { id: /\d+/ } do member do get 'raw' + post :mark_as_spam end end diff --git a/config/routes/snippets.rb b/config/routes/snippets.rb index 3ca096f31ba..ce0d1314292 100644 --- a/config/routes/snippets.rb +++ b/config/routes/snippets.rb @@ -2,6 +2,7 @@ resources :snippets, concerns: :awardable do member do get 'raw' get 'download' + post :mark_as_spam end end diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 9d8c5b63685..dcc0c82ee27 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -58,7 +58,7 @@ module API end post ":id/snippets" do authorize! :create_project_snippet, user_project - snippet_params = declared_params + snippet_params = declared_params.merge(request: request, api: true) snippet_params[:content] = snippet_params.delete(:code) snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index e096e636806..eb9ece49e7f 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -64,7 +64,7 @@ module API desc: 'The visibility level of the snippet' end post do - attrs = declared_params(include_missing: false) + attrs = declared_params(include_missing: false).merge(request: request, api: true) snippet = CreateSnippetService.new(nil, current_user, attrs).execute if snippet.persisted? diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 32b0e42c3cd..19e948d8fb8 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -6,8 +6,8 @@ describe Projects::SnippetsController do let(:user2) { create(:user) } before do - project.team << [user, :master] - project.team << [user2, :master] + project.add_master(user) + project.add_master(user2) end describe 'GET #index' do @@ -69,6 +69,86 @@ describe Projects::SnippetsController do end end + describe 'POST #create' do + def create_snippet(project, snippet_params = {}) + sign_in(user) + + project.add_developer(user) + + post :create, { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) + } + end + + context 'when the snippet is spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the project is private' do + let(:private_project) { create(:project_empty_repo, :private) } + + context 'when the snippet is public' do + it 'creates the snippet' do + expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }. + to change { Snippet.count }.by(1) + end + end + end + + context 'when the project is public' do + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) + end + end + + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + expect(response).to render_template(:new) + end + + it 'creates a spam log' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end + end + end + + describe 'POST #mark_as_spam' do + let(:snippet) { create(:project_snippet, :private, project: project, author: user) } + + before do + allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true) + stub_application_setting(akismet_enabled: true) + end + + def mark_as_spam + admin = create(:admin) + create(:user_agent_detail, subject: snippet) + project.add_master(admin) + sign_in(admin) + + post :mark_as_spam, + namespace_id: project.namespace.path, + project_id: project.path, + id: snippet.id + end + + it 'updates the snippet' do + mark_as_spam + + expect(snippet.reload).not_to be_submittable_as_spam + end + end + %w[show raw].each do |action| describe "GET ##{action}" do context 'when the project snippet is private' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index d76fe9f580f..dadcb90cfc2 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -138,6 +138,65 @@ describe SnippetsController do end end + describe 'POST #create' do + def create_snippet(snippet_params = {}) + sign_in(user) + + post :create, { + personal_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) + } + end + + context 'when the snippet is spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) + end + end + + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + expect(response).to render_template(:new) + end + + it 'creates a spam log' do + expect { create_snippet(visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end + end + + describe 'POST #mark_as_spam' do + let(:snippet) { create(:personal_snippet, :public, author: user) } + + before do + allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true) + stub_application_setting(akismet_enabled: true) + end + + def mark_as_spam + admin = create(:admin) + create(:user_agent_detail, subject: snippet) + sign_in(admin) + + post :mark_as_spam, id: snippet.id + end + + it 'updates the snippet' do + mark_as_spam + + expect(snippet.reload).not_to be_submittable_as_spam + end + end + %w(raw download).each do |action| describe "GET #{action}" do context 'when the personal snippet is private' do diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 7fb6829f582..20241d4d63e 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -52,6 +52,7 @@ snippets: - project - notes - award_emoji +- user_agent_detail releases: - project project_members: diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 01032c0929b..45d5ae267c5 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -4,6 +4,7 @@ describe API::ProjectSnippets, api: true do include ApiHelpers let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } let(:admin) { create(:admin) } describe 'GET /projects/:project_id/snippets/:id' do @@ -22,7 +23,7 @@ describe API::ProjectSnippets, api: true do let(:user) { create(:user) } it 'returns all snippets available to team member' do - project.team << [user, :developer] + project.add_developer(user) public_snippet = create(:project_snippet, :public, project: project) internal_snippet = create(:project_snippet, :internal, project: project) private_snippet = create(:project_snippet, :private, project: project) @@ -50,7 +51,7 @@ describe API::ProjectSnippets, api: true do title: 'Test Title', file_name: 'test.rb', code: 'puts "hello world"', - visibility_level: Gitlab::VisibilityLevel::PUBLIC + visibility_level: Snippet::PUBLIC } end @@ -72,6 +73,51 @@ describe API::ProjectSnippets, api: true do expect(response).to have_http_status(400) end + + context 'when the snippet is spam' do + def create_snippet(project, snippet_params = {}) + project.add_developer(user) + + post api("/projects/#{project.id}/snippets", user), params.merge(snippet_params) + end + + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the project is private' do + let(:private_project) { create(:project_empty_repo, :private) } + + context 'when the snippet is public' do + it 'creates the snippet' do + expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }. + to change { Snippet.count }.by(1) + end + end + end + + context 'when the project is public' do + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) + end + end + + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + expect(response).to have_http_status(400) + end + + it 'creates a spam log' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end + end end describe 'PUT /projects/:project_id/snippets/:id/' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index f6fb6ea5506..6b9a739b439 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -80,7 +80,7 @@ describe API::Snippets, api: true do title: 'Test Title', file_name: 'test.rb', content: 'puts "hello world"', - visibility_level: Gitlab::VisibilityLevel::PUBLIC + visibility_level: Snippet::PUBLIC } end @@ -101,6 +101,36 @@ describe API::Snippets, api: true do expect(response).to have_http_status(400) end + + context 'when the snippet is spam' do + def create_snippet(snippet_params = {}) + post api('/snippets', user), params.merge(snippet_params) + end + + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) + end + end + + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + expect(response).to have_http_status(400) + end + + it 'creates a spam log' do + expect { create_snippet(visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end end describe 'PUT /snippets/:id' do |