diff --git a/app/assets/javascripts/admin/models/admin_user.js b/app/assets/javascripts/admin/models/admin_user.js index c76df0099..853cf484a 100644 --- a/app/assets/javascripts/admin/models/admin_user.js +++ b/app/assets/javascripts/admin/models/admin_user.js @@ -235,6 +235,7 @@ Discourse.AdminUser = Discourse.User.extend({ var formData = { context: window.location.pathname }; if (block) { formData["block_email"] = true; + formData["block_urls"] = true; } Discourse.ajax("/admin/users/" + user.get('id') + '.json', { type: 'DELETE', @@ -292,7 +293,7 @@ Discourse.AdminUser = Discourse.User.extend({ "callback": function() { Discourse.ajax("/admin/users/" + user.get('id') + '.json', { type: 'DELETE', - data: {delete_posts: true, block_email: true, context: window.location.pathname} + data: {delete_posts: true, block_email: true, block_urls: true, context: window.location.pathname} }).then(function(data) { if (data.deleted) { bootbox.alert(I18n.t("admin.user.deleted"), function() { diff --git a/app/controllers/admin/blocked_emails_controller.rb b/app/controllers/admin/blocked_emails_controller.rb deleted file mode 100644 index 866672f3f..000000000 --- a/app/controllers/admin/blocked_emails_controller.rb +++ /dev/null @@ -1,8 +0,0 @@ -class Admin::BlockedEmailsController < Admin::AdminController - - def index - blocked_emails = BlockedEmail.limit(200).order('last_match_at desc').to_a - render_serialized(blocked_emails, BlockedEmailSerializer) - end - -end diff --git a/app/controllers/admin/screened_emails_controller.rb b/app/controllers/admin/screened_emails_controller.rb new file mode 100644 index 000000000..92847e39e --- /dev/null +++ b/app/controllers/admin/screened_emails_controller.rb @@ -0,0 +1,8 @@ +class Admin::ScreenedEmailsController < Admin::AdminController + + def index + screened_emails = ScreenedEmail.limit(200).order('last_match_at desc').to_a + render_serialized(screened_emails, ScreenedEmailSerializer) + end + +end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 486700a6f..d158ca427 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -118,7 +118,7 @@ class Admin::UsersController < Admin::AdminController user = User.where(id: params[:id]).first guardian.ensure_can_delete_user!(user) begin - if UserDestroyer.new(current_user).destroy(user, params.slice(:delete_posts, :block_email, :context)) + if UserDestroyer.new(current_user).destroy(user, params.slice(:delete_posts, :block_email, :block_urls, :context)) render json: {deleted: true} else render json: {deleted: false, user: AdminDetailedUserSerializer.new(user, root: false).as_json} diff --git a/app/models/blocked_email.rb b/app/models/blocked_email.rb deleted file mode 100644 index 1aa85ec43..000000000 --- a/app/models/blocked_email.rb +++ /dev/null @@ -1,54 +0,0 @@ -# A BlockedEmail record represents an email address that is being watched, -# typically when creating a new User account. If the email of the signup form -# (or some other form) matches a BlockedEmail record, an action can be -# performed based on the action_type. -class BlockedEmail < ActiveRecord::Base - - before_validation :set_defaults - - validates :email, presence: true, uniqueness: true - - def self.actions - @actions ||= Enum.new(:block, :do_nothing) - end - - def self.block(email, opts={}) - find_by_email(email) || create(opts.slice(:action_type).merge({email: email})) - end - - def self.should_block?(email) - blocked_email = BlockedEmail.where(email: email).first - blocked_email.record_match! if blocked_email - blocked_email && blocked_email.action_type == actions[:block] - end - - def set_defaults - self.action_type ||= BlockedEmail.actions[:block] - end - - def record_match! - self.match_count += 1 - self.last_match_at = Time.zone.now - save - end - -end - -# == Schema Information -# -# Table name: blocked_emails -# -# id :integer not null, primary key -# email :string(255) not null -# action_type :integer not null -# match_count :integer default(0), not null -# last_match_at :datetime -# created_at :datetime not null -# updated_at :datetime not null -# -# Indexes -# -# index_blocked_emails_on_email (email) UNIQUE -# index_blocked_emails_on_last_match_at (last_match_at) -# - diff --git a/app/models/screened_email.rb b/app/models/screened_email.rb new file mode 100644 index 000000000..05a2c5e19 --- /dev/null +++ b/app/models/screened_email.rb @@ -0,0 +1,25 @@ +require_dependency 'screening_model' + +# A ScreenedEmail record represents an email address that is being watched, +# typically when creating a new User account. If the email of the signup form +# (or some other form) matches a ScreenedEmail record, an action can be +# performed based on the action_type. +class ScreenedEmail < ActiveRecord::Base + + include ScreeningModel + + default_action :block + + validates :email, presence: true, uniqueness: true + + def self.block(email, opts={}) + find_by_email(email) || create(opts.slice(:action_type).merge({email: email})) + end + + def self.should_block?(email) + screened_email = ScreenedEmail.where(email: email).first + screened_email.record_match! if screened_email + screened_email && screened_email.action_type == actions[:block] + end + +end diff --git a/app/models/screened_url.rb b/app/models/screened_url.rb new file mode 100644 index 000000000..c27f6820d --- /dev/null +++ b/app/models/screened_url.rb @@ -0,0 +1,26 @@ +require_dependency 'screening_model' + +# A ScreenedUrl record represents a URL that is being watched. +# If the URL is found in a post, some action can be performed. + +# For now, nothing is done. We're just collecting the data and will decide +# what to do with it later. +class ScreenedUrl < ActiveRecord::Base + + include ScreeningModel + + default_action :do_nothing + + before_validation :strip_http + + validates :url, presence: true, uniqueness: true + validates :domain, presence: true + + def strip_http + self.url.gsub!(/http(s?):\/\//i, '') + end + + def self.watch(url, domain, opts={}) + find_by_url(url) || create(opts.slice(:action_type).merge(url: url, domain: domain)) + end +end diff --git a/app/serializers/blocked_email_serializer.rb b/app/serializers/screened_email_serializer.rb similarity index 57% rename from app/serializers/blocked_email_serializer.rb rename to app/serializers/screened_email_serializer.rb index 3d8686c07..22d884f88 100644 --- a/app/serializers/blocked_email_serializer.rb +++ b/app/serializers/screened_email_serializer.rb @@ -1,11 +1,11 @@ -class BlockedEmailSerializer < ApplicationSerializer +class ScreenedEmailSerializer < ApplicationSerializer attributes :email, :action, :match_count, :last_match_at, :created_at - + def action - BlockedEmail.actions.key(object.action_type).to_s + ScreenedEmail.actions.key(object.action_type).to_s end end diff --git a/config/routes.rb b/config/routes.rb index ad3374f70..5c7e8bcd3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,7 +63,7 @@ Discourse::Application.routes.draw do end scope '/logs' do - resources :blocked_emails, only: [:index, :create, :update, :destroy] + resources :screened_emails, only: [:index, :create, :update, :destroy] resources :staff_action_logs, only: [:index, :create, :update, :destroy] end diff --git a/db/migrate/20130813204212_create_screened_urls.rb b/db/migrate/20130813204212_create_screened_urls.rb new file mode 100644 index 000000000..5e613c748 --- /dev/null +++ b/db/migrate/20130813204212_create_screened_urls.rb @@ -0,0 +1,14 @@ +class CreateScreenedUrls < ActiveRecord::Migration + def change + create_table :screened_urls do |t| + t.string :url, null: false + t.string :domain, null: false + t.integer :action_type, null: false + t.integer :match_count, null: false, default: 0 + t.datetime :last_match_at + t.timestamps + end + add_index :screened_urls, :url, unique: true + add_index :screened_urls, :last_match_at + end +end diff --git a/db/migrate/20130813224817_rename_blocked_emails_to_screened_emails.rb b/db/migrate/20130813224817_rename_blocked_emails_to_screened_emails.rb new file mode 100644 index 000000000..5f593e445 --- /dev/null +++ b/db/migrate/20130813224817_rename_blocked_emails_to_screened_emails.rb @@ -0,0 +1,5 @@ +class RenameBlockedEmailsToScreenedEmails < ActiveRecord::Migration + def change + rename_table :blocked_emails, :screened_emails + end +end diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 046f1891a..e821b0145 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -31,6 +31,10 @@ module Oneboxer 1.day end + def self.oneboxer_exists_for_url?(url) + Whitelist.entry_for_url(url) || matchers.any? { |matcher| url =~ matcher.regexp } + end + # Return a oneboxer for a given URL def self.onebox_for_url(url) matchers.each do |matcher| diff --git a/lib/screening_model.rb b/lib/screening_model.rb new file mode 100644 index 000000000..5d9b86ad0 --- /dev/null +++ b/lib/screening_model.rb @@ -0,0 +1,31 @@ +module ScreeningModel + extend ActiveSupport::Concern + + module ClassMethods + def actions + @actions ||= Enum.new(:block, :do_nothing) + end + + def default_action(action_key) + @default_action = action_key + end + + def df_action + @default_action || :do_nothing + end + end + + included do + before_validation :set_default_action + end + + def set_default_action + self.action_type ||= self.class.actions[self.class.df_action] + end + + def record_match! + self.match_count += 1 + self.last_match_at = Time.zone.now + save + end +end diff --git a/lib/user_destroyer.rb b/lib/user_destroyer.rb index 8a158e6ea..b7e72dbd3 100644 --- a/lib/user_destroyer.rb +++ b/lib/user_destroyer.rb @@ -17,6 +17,13 @@ class UserDestroyer User.transaction do if opts[:delete_posts] user.posts.each do |post| + if opts[:block_urls] + post.topic_links.each do |link| + unless link.internal or Oneboxer.oneboxer_exists_for_url?(link.url) + ScreenedUrl.watch(link.url, link.domain).try(:record_match!) + end + end + end PostDestroyer.new(@staff, post).destroy end raise PostsExistError if user.reload.post_count != 0 @@ -24,7 +31,7 @@ class UserDestroyer user.destroy.tap do |u| if u if opts[:block_email] - b = BlockedEmail.block(u.email) + b = ScreenedEmail.block(u.email) b.record_match! if b end Post.with_deleted.where(user_id: user.id).update_all("nuked_user = true") diff --git a/lib/validators/email_validator.rb b/lib/validators/email_validator.rb index c2e4284b4..777498348 100644 --- a/lib/validators/email_validator.rb +++ b/lib/validators/email_validator.rb @@ -10,7 +10,7 @@ class EmailValidator < ActiveModel::EachValidator record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) end end - if record.errors[attribute].blank? and BlockedEmail.should_block?(value) + if record.errors[attribute].blank? and ScreenedEmail.should_block?(value) record.errors.add(attribute, I18n.t(:'user.email.blocked')) end end diff --git a/spec/components/user_destroyer_spec.rb b/spec/components/user_destroyer_spec.rb index 4b23a2847..8e0d4e014 100644 --- a/spec/components/user_destroyer_spec.rb +++ b/spec/components/user_destroyer_spec.rb @@ -74,13 +74,13 @@ describe UserDestroyer do shared_examples "email block list" do it "doesn't add email to block list by default" do - BlockedEmail.expects(:block).never + ScreenedEmail.expects(:block).never destroy end it "adds email to block list if block_email is true" do - b = Fabricate.build(:blocked_email, email: @user.email) - BlockedEmail.expects(:block).with(@user.email).returns(b) + b = Fabricate.build(:screened_email, email: @user.email) + ScreenedEmail.expects(:block).with(@user.email).returns(b) b.expects(:record_match!).once.returns(true) UserDestroyer.new(@admin).destroy(@user, destroy_opts.merge({block_email: true})) end @@ -164,6 +164,50 @@ describe UserDestroyer do end end end + + context 'user has posts with links' do + context 'external links' do + before do + @post = Fabricate(:post_with_external_links, user: @user) + TopicLink.extract_from(@post) + end + + it "doesn't add ScreenedUrl records by default" do + ScreenedUrl.expects(:watch).never + UserDestroyer.new(@admin).destroy(@user, {delete_posts: true}) + end + + it "adds ScreenedUrl records when :block_urls is true" do + ScreenedUrl.expects(:watch).at_least_once + UserDestroyer.new(@admin).destroy(@user, {delete_posts: true, block_urls: true}) + end + end + + context 'internal links' do + before do + @post = Fabricate(:post_with_external_links, user: @user) + TopicLink.extract_from(@post) + TopicLink.any_instance.stubs(:internal).returns(true) + end + + it "doesn't add ScreenedUrl records" do + ScreenedUrl.expects(:watch).never + UserDestroyer.new(@admin).destroy(@user, {delete_posts: true, block_urls: true}) + end + end + + context 'with oneboxed links' do + before do + @post = Fabricate(:post_with_youtube, user: @user) + TopicLink.extract_from(@post) + end + + it "doesn't add ScreenedUrl records" do + ScreenedUrl.expects(:watch).never + UserDestroyer.new(@admin).destroy(@user, {delete_posts: true, block_urls: true}) + end + end + end end end diff --git a/spec/components/validators/email_validator_spec.rb b/spec/components/validators/email_validator_spec.rb index 2591a1ec7..e310aab34 100644 --- a/spec/components/validators/email_validator_spec.rb +++ b/spec/components/validators/email_validator_spec.rb @@ -8,13 +8,13 @@ describe EmailValidator do context "blocked email" do it "doesn't add an error when email doesn't match a blocked email" do - BlockedEmail.stubs(:should_block?).with(record.email).returns(false) + ScreenedEmail.stubs(:should_block?).with(record.email).returns(false) validate record.errors[:email].should_not be_present end it "adds an error when email matches a blocked email" do - BlockedEmail.stubs(:should_block?).with(record.email).returns(true) + ScreenedEmail.stubs(:should_block?).with(record.email).returns(true) validate record.errors[:email].should be_present end diff --git a/spec/controllers/admin/blocked_emails_controller_spec.rb b/spec/controllers/admin/screened_emails_controller_spec.rb similarity index 72% rename from spec/controllers/admin/blocked_emails_controller_spec.rb rename to spec/controllers/admin/screened_emails_controller_spec.rb index 0ee48d423..e10553ea2 100644 --- a/spec/controllers/admin/blocked_emails_controller_spec.rb +++ b/spec/controllers/admin/screened_emails_controller_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' -describe Admin::BlockedEmailsController do +describe Admin::ScreenedEmailsController do it "is a subclass of AdminController" do - (Admin::BlockedEmailsController < Admin::AdminController).should be_true + (Admin::ScreenedEmailsController < Admin::AdminController).should be_true end let!(:user) { log_in(:admin) } diff --git a/spec/fabricators/blocked_email_fabricator.rb b/spec/fabricators/blocked_email_fabricator.rb deleted file mode 100644 index 7e9325026..000000000 --- a/spec/fabricators/blocked_email_fabricator.rb +++ /dev/null @@ -1,4 +0,0 @@ -Fabricator(:blocked_email) do - email { sequence(:email) { |n| "bad#{n}@spammers.org" } } - action_type BlockedEmail.actions[:block] -end diff --git a/spec/fabricators/screened_email_fabricator.rb b/spec/fabricators/screened_email_fabricator.rb new file mode 100644 index 000000000..5373e3370 --- /dev/null +++ b/spec/fabricators/screened_email_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:screened_email) do + email { sequence(:email) { |n| "bad#{n}@spammers.org" } } + action_type ScreenedEmail.actions[:block] +end diff --git a/spec/fabricators/screened_url_fabricator.rb b/spec/fabricators/screened_url_fabricator.rb new file mode 100644 index 000000000..09b6eae41 --- /dev/null +++ b/spec/fabricators/screened_url_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:screened_url) do + url { sequence(:url) { |n| "spammers#{n}.org/buy/stuff" } } + domain { sequence(:domain) { |n| "spammers#{n}.org" } } + action_type ScreenedEmail.actions[:do_nothing] +end diff --git a/spec/models/blocked_email_spec.rb b/spec/models/screened_email_spec.rb similarity index 73% rename from spec/models/blocked_email_spec.rb rename to spec/models/screened_email_spec.rb index ea473291a..3e242034b 100644 --- a/spec/models/blocked_email_spec.rb +++ b/spec/models/screened_email_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe BlockedEmail do +describe ScreenedEmail do let(:email) { 'block@spamfromhome.org' } @@ -34,7 +34,7 @@ describe BlockedEmail do end context 'email is already being blocked' do - let!(:existing) { Fabricate(:blocked_email, email: email) } + let!(:existing) { Fabricate(:screened_email, email: email) } it "doesn't create a new record" do expect { described_class.block(email) }.to_not change { described_class.count } @@ -53,25 +53,25 @@ describe BlockedEmail do subject.should be_false end - shared_examples "when a BlockedEmail record matches" do + shared_examples "when a ScreenedEmail record matches" do it "updates statistics" do Timecop.freeze(Time.zone.now) do - expect { subject }.to change { blocked_email.reload.match_count }.by(1) - blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now) + expect { subject }.to change { screened_email.reload.match_count }.by(1) + screened_email.last_match_at.should be_within_one_second_of(Time.zone.now) end end end context "action_type is :block" do - let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) } + let!(:screened_email) { Fabricate(:screened_email, email: email, action_type: described_class.actions[:block]) } it { should be_true } - include_examples "when a BlockedEmail record matches" + include_examples "when a ScreenedEmail record matches" end context "action_type is :do_nothing" do - let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:do_nothing]) } + let!(:screened_email) { Fabricate(:screened_email, email: email, action_type: described_class.actions[:do_nothing]) } it { should be_false } - include_examples "when a BlockedEmail record matches" + include_examples "when a ScreenedEmail record matches" end end diff --git a/spec/models/screened_url_spec.rb b/spec/models/screened_url_spec.rb new file mode 100644 index 000000000..14229be65 --- /dev/null +++ b/spec/models/screened_url_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe ScreenedUrl do + + let(:url) { 'http://shopppping.com/bad/drugz' } + let(:domain) { 'shopppping.com' } + + let(:valid_params) { {url: url, domain: domain} } + + describe "new record" do + it "sets a default action_type" do + described_class.create(valid_params).action_type.should == described_class.actions[:do_nothing] + end + + it "last_match_at is null" do + described_class.create(valid_params).last_match_at.should be_nil + end + + ['http://', 'HTTP://', 'https://', 'HTTPS://'].each do |prefix| + it "strips #{prefix}" do + described_class.create(valid_params.merge(url: url.gsub('http://', prefix))).url.should == url.gsub('http://', '') + end + end + end + + describe '#watch' do + context 'url is not being blocked' do + it 'creates a new record with default action of :do_nothing' do + record = described_class.watch(url, domain) + record.should_not be_new_record + record.action_type.should == described_class.actions[:do_nothing] + end + + it 'lets action_type be overriden' do + record = described_class.watch(url, domain, action_type: described_class.actions[:block]) + record.should_not be_new_record + record.action_type.should == described_class.actions[:block] + end + end + + context 'url is already being blocked' do + let!(:existing) { Fabricate(:screened_url, url: url, domain: domain) } + + it "doesn't create a new record" do + expect { described_class.watch(url, domain) }.to_not change { described_class.count } + end + + it "returns the existing record" do + described_class.watch(url, domain).should == existing + end + end + end +end