From c707ef49d9b13932f4d98c127ec3148a5cdc3479 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 15 Sep 2019 21:08:39 +0200 Subject: Fix 2FA challenge and password challenge for non-database users (#11831) * Fix 2FA challenge not appearing for non-database users Fix #11685 * Fix account deletion not working when using external login Fix #11691 --- spec/controllers/auth/sessions_controller_spec.rb | 24 ++++++++--------------- 1 file changed, 8 insertions(+), 16 deletions(-) (limited to 'spec') diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 87ef4f2bb..7ed5edde0 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -5,11 +5,11 @@ require 'rails_helper' RSpec.describe Auth::SessionsController, type: :controller do render_views - describe 'GET #new' do - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end + before do + request.env['devise.mapping'] = Devise.mappings[:user] + end + describe 'GET #new' do it 'returns http success' do get :new expect(response).to have_http_status(200) @@ -19,10 +19,6 @@ RSpec.describe Auth::SessionsController, type: :controller do describe 'DELETE #destroy' do let(:user) { Fabricate(:user) } - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end - context 'with a regular user' do it 'redirects to home after sign out' do sign_in(user, scope: :user) @@ -51,10 +47,6 @@ RSpec.describe Auth::SessionsController, type: :controller do end describe 'POST #create' do - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end - context 'using PAM authentication', if: ENV['PAM_ENABLED'] == 'true' do context 'using a valid password' do before do @@ -191,11 +183,11 @@ RSpec.describe Auth::SessionsController, type: :controller do end context 'using two-factor authentication' do - let(:user) do - Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', - otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + let!(:user) do + Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) end - let(:recovery_codes) do + + let!(:recovery_codes) do codes = user.generate_otp_backup_codes! user.save return codes -- cgit From a4b60e9ba4874b9ab427bec41d8b2cd252ec4782 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 18 Sep 2019 02:48:40 +0200 Subject: Fix TOTP codes not being filtered from logs during enabling/disabling (#11877) Not a serious issue because they are meaningless past single use --- .../settings/two_factor_authentication/confirmations_controller.rb | 4 ++-- app/controllers/settings/two_factor_authentications_controller.rb | 6 +++--- app/models/form/two_factor_confirmation.rb | 2 +- .../settings/two_factor_authentication/confirmations/new.html.haml | 2 +- app/views/settings/two_factor_authentications/show.html.haml | 2 +- .../two_factor_authentication/confirmations_controller_spec.rb | 6 +++--- .../settings/two_factor_authentications_controller_spec.rb | 4 ++-- 7 files changed, 13 insertions(+), 13 deletions(-) (limited to 'spec') diff --git a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb index 3145e092d..46c90bf74 100644 --- a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb +++ b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb @@ -15,7 +15,7 @@ module Settings end def create - if current_user.validate_and_consume_otp!(confirmation_params[:code]) + if current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt]) flash.now[:notice] = I18n.t('two_factor_authentication.enabled_success') current_user.otp_required_for_login = true @@ -33,7 +33,7 @@ module Settings private def confirmation_params - params.require(:form_two_factor_confirmation).permit(:code) + params.require(:form_two_factor_confirmation).permit(:otp_attempt) end def prepare_two_factor_form diff --git a/app/controllers/settings/two_factor_authentications_controller.rb b/app/controllers/settings/two_factor_authentications_controller.rb index 6904076e4..c93b17577 100644 --- a/app/controllers/settings/two_factor_authentications_controller.rb +++ b/app/controllers/settings/two_factor_authentications_controller.rb @@ -34,7 +34,7 @@ module Settings private def confirmation_params - params.require(:form_two_factor_confirmation).permit(:code) + params.require(:form_two_factor_confirmation).permit(:otp_attempt) end def verify_otp_required @@ -42,8 +42,8 @@ module Settings end def acceptable_code? - current_user.validate_and_consume_otp!(confirmation_params[:code]) || - current_user.invalidate_otp_backup_code!(confirmation_params[:code]) + current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt]) || + current_user.invalidate_otp_backup_code!(confirmation_params[:otp_attempt]) end end end diff --git a/app/models/form/two_factor_confirmation.rb b/app/models/form/two_factor_confirmation.rb index b8cf76d05..27ada6533 100644 --- a/app/models/form/two_factor_confirmation.rb +++ b/app/models/form/two_factor_confirmation.rb @@ -3,5 +3,5 @@ class Form::TwoFactorConfirmation include ActiveModel::Model - attr_accessor :code + attr_accessor :otp_attempt end diff --git a/app/views/settings/two_factor_authentication/confirmations/new.html.haml b/app/views/settings/two_factor_authentication/confirmations/new.html.haml index e64155299..86cf1f695 100644 --- a/app/views/settings/two_factor_authentication/confirmations/new.html.haml +++ b/app/views/settings/two_factor_authentication/confirmations/new.html.haml @@ -12,7 +12,7 @@ %samp.qr-alternative__code= current_user.otp_secret.scan(/.{4}/).join(' ') .fields-group - = f.input :code, wrapper: :with_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true + = f.input :otp_attempt, wrapper: :with_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true .actions = f.button :button, t('two_factor_authentication.enable'), type: :submit diff --git a/app/views/settings/two_factor_authentications/show.html.haml b/app/views/settings/two_factor_authentications/show.html.haml index 259bcd1ef..93509e022 100644 --- a/app/views/settings/two_factor_authentications/show.html.haml +++ b/app/views/settings/two_factor_authentications/show.html.haml @@ -10,7 +10,7 @@ %hr/ = simple_form_for @confirmation, url: settings_two_factor_authentication_path, method: :delete do |f| - = f.input :code, wrapper: :with_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true + = f.input :otp_attempt, wrapper: :with_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true .actions = f.button :button, t('two_factor_authentication.disable'), type: :submit diff --git a/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb b/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb index 2222a7559..2e5a9325c 100644 --- a/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb @@ -68,7 +68,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do true end - post :create, params: { form_two_factor_confirmation: { code: '123456' } } + post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } expect(assigns(:recovery_codes)).to eq otp_backup_codes expect(flash[:notice]).to eq 'Two-factor authentication successfully enabled' @@ -85,7 +85,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do false end - post :create, params: { form_two_factor_confirmation: { code: '123456' } } + post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } end it 'renders the new view' do @@ -99,7 +99,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do context 'when not signed in' do it 'redirects if not signed in' do - post :create, params: { form_two_factor_confirmation: { code: '123456' } } + post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } expect(response).to redirect_to('/auth/sign_in') end end diff --git a/spec/controllers/settings/two_factor_authentications_controller_spec.rb b/spec/controllers/settings/two_factor_authentications_controller_spec.rb index f7c628756..922231ded 100644 --- a/spec/controllers/settings/two_factor_authentications_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentications_controller_spec.rb @@ -91,7 +91,7 @@ describe Settings::TwoFactorAuthenticationsController do true end - post :destroy, params: { form_two_factor_confirmation: { code: '123456' } } + post :destroy, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } expect(response).to redirect_to(settings_two_factor_authentication_path) user.reload @@ -105,7 +105,7 @@ describe Settings::TwoFactorAuthenticationsController do false end - post :destroy, params: { form_two_factor_confirmation: { code: '057772' } } + post :destroy, params: { form_two_factor_confirmation: { otp_attempt: '057772' } } user.reload expect(user.otp_required_for_login).to eq(true) -- cgit From 4f6af87906175d9ea802ef0c6f050388eac890fa Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 18 Sep 2019 12:53:13 +0200 Subject: Change spam check to apply to local accounts and add a threshold (#11806) Instead of detecting spam on first duplicate message, add a threshold of 5 such messages to reduce false positives --- app/lib/activitypub/activity/create.rb | 10 +------ app/lib/spam_check.rb | 46 ++++++++++++++++++++++++++------ app/services/process_mentions_service.rb | 5 ++++ spec/lib/spam_check_spec.rb | 34 ++++++++++++++--------- 4 files changed, 66 insertions(+), 29 deletions(-) (limited to 'spec') diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index dea7fd43c..e69193b71 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -408,15 +408,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity end def check_for_spam - spam_check = SpamCheck.new(@status) - - return if spam_check.skip? - - if spam_check.spam? - spam_check.flag! - else - spam_check.remember! - end + SpamCheck.perform(@status) end def forward_for_reply diff --git a/app/lib/spam_check.rb b/app/lib/spam_check.rb index 0cf1b8790..441697364 100644 --- a/app/lib/spam_check.rb +++ b/app/lib/spam_check.rb @@ -4,9 +4,25 @@ class SpamCheck include Redisable include ActionView::Helpers::TextHelper + # Threshold over which two Nilsimsa values are considered + # to refer to the same text NILSIMSA_COMPARE_THRESHOLD = 95 - NILSIMSA_MIN_SIZE = 10 - EXPIRE_SET_AFTER = 1.week.seconds + + # Nilsimsa doesn't work well on small inputs, so below + # this size, we check only for exact matches with MD5 + NILSIMSA_MIN_SIZE = 10 + + # How long to keep the trail of digests between updates, + # there is no reason to store it forever + EXPIRE_SET_AFTER = 1.week.seconds + + # How many digests to keep in an account's trail. If it's + # too small, spam could rotate around different message templates + MAX_TRAIL_SIZE = 10 + + # How many detected duplicates to allow through before + # considering the message as spam + THRESHOLD = 5 def initialize(status) @account = status.account @@ -21,9 +37,9 @@ class SpamCheck if insufficient_data? false elsif nilsimsa? - any_other_digest?('nilsimsa') { |_, other_digest| nilsimsa_compare_value(digest, other_digest) >= NILSIMSA_COMPARE_THRESHOLD } + digests_over_threshold?('nilsimsa') { |_, other_digest| nilsimsa_compare_value(digest, other_digest) >= NILSIMSA_COMPARE_THRESHOLD } else - any_other_digest?('md5') { |_, other_digest| other_digest == digest } + digests_over_threshold?('md5') { |_, other_digest| other_digest == digest } end end @@ -38,7 +54,7 @@ class SpamCheck # get the correct status ID back, we have to save it in the string value redis.zadd(redis_key, @status.id, digest_with_algorithm) - redis.zremrangebyrank(redis_key, '0', '-10') + redis.zremrangebyrank(redis_key, 0, -(MAX_TRAIL_SIZE + 1)) redis.expire(redis_key, EXPIRE_SET_AFTER) end @@ -78,6 +94,20 @@ class SpamCheck end end + class << self + def perform(status) + spam_check = new(status) + + return if spam_check.skip? + + if spam_check.spam? + spam_check.flag! + else + spam_check.remember! + end + end + end + private def disabled? @@ -149,14 +179,14 @@ class SpamCheck redis.zrange(redis_key, 0, -1) end - def any_other_digest?(filter_algorithm) - other_digests.any? do |record| + def digests_over_threshold?(filter_algorithm) + other_digests.select do |record| algorithm, other_digest, status_id = record.split(':') next unless algorithm == filter_algorithm yield algorithm, other_digest, status_id - end + end.size >= THRESHOLD end def matching_status_ids diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index 90dca9740..2f7a9e985 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -33,6 +33,7 @@ class ProcessMentionsService < BaseService end status.save! + check_for_spam(status) mentions.each { |mention| create_notification(mention) } end @@ -61,4 +62,8 @@ class ProcessMentionsService < BaseService def resolve_account_service ResolveAccountService.new end + + def check_for_spam(status) + SpamCheck.perform(status) + end end diff --git a/spec/lib/spam_check_spec.rb b/spec/lib/spam_check_spec.rb index 9e0989216..4cae46111 100644 --- a/spec/lib/spam_check_spec.rb +++ b/spec/lib/spam_check_spec.rb @@ -86,23 +86,33 @@ RSpec.describe SpamCheck do end it 'returns true for duplicate statuses to the same recipient' do - status1 = status_with_html('@alice Hello') - described_class.new(status1).remember! + described_class::THRESHOLD.times do + status1 = status_with_html('@alice Hello') + described_class.new(status1).remember! + end + status2 = status_with_html('@alice Hello') expect(described_class.new(status2).spam?).to be true end it 'returns true for duplicate statuses to different recipients' do - status1 = status_with_html('@alice Hello') - described_class.new(status1).remember! + described_class::THRESHOLD.times do + status1 = status_with_html('@alice Hello') + described_class.new(status1).remember! + end + status2 = status_with_html('@bob Hello') expect(described_class.new(status2).spam?).to be true end it 'returns true for nearly identical statuses with random numbers' do source_text = 'Sodium, atomic number 11, was first isolated by Humphry Davy in 1807. A chemical component of salt, he named it Na in honor of the saltiest region on earth, North America.' - status1 = status_with_html('@alice ' + source_text + ' 1234') - described_class.new(status1).remember! + + described_class::THRESHOLD.times do + status1 = status_with_html('@alice ' + source_text + ' 1234') + described_class.new(status1).remember! + end + status2 = status_with_html('@bob ' + source_text + ' 9568') expect(described_class.new(status2).spam?).to be true end @@ -140,9 +150,9 @@ RSpec.describe SpamCheck do let(:redis_key) { spam_check.send(:redis_key) } it 'remembers' do - expect do - spam_check.remember! - end.to change { Redis.current.exists(redis_key) }.from(false).to(true) + expect(Redis.current.exists(redis_key)).to be true + spam_check.remember! + expect(Redis.current.exists(redis_key)).to be true end end @@ -156,9 +166,9 @@ RSpec.describe SpamCheck do end it 'resets' do - expect do - spam_check.reset! - end.to change { Redis.current.exists(redis_key) }.from(true).to(false) + expect(Redis.current.exists(redis_key)).to be true + spam_check.reset! + expect(Redis.current.exists(redis_key)).to be false end end -- cgit