From 6221b36b278c02cdbf5b6d1c0753654b506b44fd Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 6 Apr 2022 20:58:12 +0200 Subject: Remove sign-in token authentication, instead send e-mail about new sign-in (#17970) --- spec/controllers/auth/sessions_controller_spec.rb | 151 ---------------------- spec/lib/suspicious_sign_in_detector_spec.rb | 57 ++++++++ spec/mailers/previews/user_mailer_preview.rb | 6 +- 3 files changed, 60 insertions(+), 154 deletions(-) create mode 100644 spec/lib/suspicious_sign_in_detector_spec.rb (limited to 'spec') diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 64ec7b794..1b8fd0b7b 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -225,22 +225,6 @@ RSpec.describe Auth::SessionsController, type: :controller do end end - context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do - let!(:other_user) do - Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) - end - - before do - post :create, params: { user: { email: other_user.email, password: other_user.password } } - post :create, params: { user: { email: user.email, password: user.password } } - end - - it 'renders two factor authentication page' do - expect(controller).to render_template("two_factor") - expect(controller).to render_template(partial: "_otp_authentication_form") - end - end - context 'using upcase email and password' do before do post :create, params: { user: { email: user.email.upcase, password: user.password } } @@ -266,21 +250,6 @@ RSpec.describe Auth::SessionsController, type: :controller do end end - context 'using a valid OTP, attempting to leverage previous half-login to bypass password auth' do - let!(:other_user) do - Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) - end - - before do - post :create, params: { user: { email: other_user.email, password: other_user.password } } - post :create, params: { user: { email: user.email, otp_attempt: user.current_otp } }, session: { attempt_user_updated_at: user.updated_at.to_s } - end - - it "doesn't log the user in" do - expect(controller.current_user).to be_nil - end - end - context 'when the server has an decryption error' do before do allow_any_instance_of(User).to receive(:validate_and_consume_otp!).and_raise(OpenSSL::Cipher::CipherError) @@ -401,126 +370,6 @@ RSpec.describe Auth::SessionsController, type: :controller do end end end - - context 'when 2FA is disabled and IP is unfamiliar' do - let!(:user) { Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', current_sign_in_at: 3.weeks.ago) } - - before do - request.remote_ip = '10.10.10.10' - request.user_agent = 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0' - - allow(UserMailer).to receive(:sign_in_token).and_return(double('email', deliver_later!: nil)) - end - - context 'using email and password' do - before do - post :create, params: { user: { email: user.email, password: user.password } } - end - - it 'renders sign in token authentication page' do - expect(controller).to render_template("sign_in_token") - end - - it 'generates sign in token' do - expect(user.reload.sign_in_token).to_not be_nil - end - - it 'sends sign in token e-mail' do - expect(UserMailer).to have_received(:sign_in_token) - end - end - - context 'using email and password after an unfinished log-in attempt to a 2FA-protected account' do - let!(:other_user) do - Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) - end - - before do - post :create, params: { user: { email: other_user.email, password: other_user.password } } - post :create, params: { user: { email: user.email, password: user.password } } - end - - it 'renders sign in token authentication page' do - expect(controller).to render_template("sign_in_token") - end - - it 'generates sign in token' do - expect(user.reload.sign_in_token).to_not be_nil - end - - it 'sends sign in token e-mail' do - expect(UserMailer).to have_received(:sign_in_token) - end - end - - context 'using email and password after an unfinished log-in attempt with a sign-in token challenge' do - let!(:other_user) do - Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) - end - - before do - post :create, params: { user: { email: other_user.email, password: other_user.password } } - post :create, params: { user: { email: user.email, password: user.password } } - end - - it 'renders sign in token authentication page' do - expect(controller).to render_template("sign_in_token") - end - - it 'generates sign in token' do - expect(user.reload.sign_in_token).to_not be_nil - end - - it 'sends sign in token e-mail' do - expect(UserMailer).to have_received(:sign_in_token).with(user, any_args) - end - end - - context 'using a valid sign in token' do - before do - user.generate_sign_in_token && user.save - post :create, params: { user: { sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } - end - - it 'redirects to home' do - expect(response).to redirect_to(root_path) - end - - it 'logs the user in' do - expect(controller.current_user).to eq user - end - end - - context 'using a valid sign in token, attempting to leverage previous half-login to bypass password auth' do - let!(:other_user) do - Fabricate(:user, email: 'z@y.com', password: 'abcdefgh', otp_required_for_login: false, current_sign_in_at: 1.month.ago) - end - - before do - user.generate_sign_in_token && user.save - post :create, params: { user: { email: other_user.email, password: other_user.password } } - post :create, params: { user: { email: user.email, sign_in_token_attempt: user.sign_in_token } }, session: { attempt_user_updated_at: user.updated_at.to_s } - end - - it "doesn't log the user in" do - expect(controller.current_user).to be_nil - end - end - - context 'using an invalid sign in token' do - before do - post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } - end - - it 'shows a login error' do - expect(flash[:alert]).to match I18n.t('users.invalid_sign_in_token') - end - - it "doesn't log the user in" do - expect(controller.current_user).to be_nil - end - end - end end describe 'GET #webauthn_options' do diff --git a/spec/lib/suspicious_sign_in_detector_spec.rb b/spec/lib/suspicious_sign_in_detector_spec.rb new file mode 100644 index 000000000..101a18aa0 --- /dev/null +++ b/spec/lib/suspicious_sign_in_detector_spec.rb @@ -0,0 +1,57 @@ +require 'rails_helper' + +RSpec.describe SuspiciousSignInDetector do + describe '#suspicious?' do + let(:user) { Fabricate(:user, current_sign_in_at: 1.day.ago) } + let(:request) { double(remote_ip: remote_ip) } + let(:remote_ip) { nil } + + subject { described_class.new(user).suspicious?(request) } + + context 'when user has 2FA enabled' do + before do + user.update!(otp_required_for_login: true) + end + + it 'returns false' do + expect(subject).to be false + end + end + + context 'when exact IP has been used before' do + let(:remote_ip) { '1.1.1.1' } + + before do + user.update!(sign_up_ip: remote_ip) + end + + it 'returns false' do + expect(subject).to be false + end + end + + context 'when similar IP has been used before' do + let(:remote_ip) { '1.1.2.2' } + + before do + user.update!(sign_up_ip: '1.1.1.1') + end + + it 'returns false' do + expect(subject).to be false + end + end + + context 'when IP is completely unfamiliar' do + let(:remote_ip) { '2.2.2.2' } + + before do + user.update!(sign_up_ip: '1.1.1.1') + end + + it 'returns true' do + expect(subject).to be true + end + end + end +end diff --git a/spec/mailers/previews/user_mailer_preview.rb b/spec/mailers/previews/user_mailer_preview.rb index 8de7d8669..95712e6cf 100644 --- a/spec/mailers/previews/user_mailer_preview.rb +++ b/spec/mailers/previews/user_mailer_preview.rb @@ -87,8 +87,8 @@ class UserMailerPreview < ActionMailer::Preview UserMailer.appeal_approved(User.first, Appeal.last) end - # Preview this email at http://localhost:3000/rails/mailers/user_mailer/sign_in_token - def sign_in_token - UserMailer.sign_in_token(User.first.tap { |user| user.generate_sign_in_token }, '127.0.0.1', 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0', Time.now.utc) + # Preview this email at http://localhost:3000/rails/mailers/user_mailer/suspicious_sign_in + def suspicious_sign_in + UserMailer.suspicious_sign_in(User.first, '127.0.0.1', 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0', Time.now.utc) end end -- cgit From 8f91e304a5adb98b657a5c096359d0423a5d7e84 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 6 Apr 2022 21:01:02 +0200 Subject: Fix spurious edits and require incoming edits to be explicitly marked as such (#17918) * Change post text edit to not be considered significant if it's identical after reformatting * We don't need to clear previous change information anymore * Require status edits to be explicit, except for poll tallies * Fix tests * Add some tests * Add poll-related tests * Add HTML-formatting related tests --- .../activitypub/process_status_update_service.rb | 50 ++++-- .../fetch_remote_status_service_spec.rb | 4 +- .../process_status_update_service_spec.rb | 178 +++++++++++++++++++++ 3 files changed, 220 insertions(+), 12 deletions(-) (limited to 'spec') diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 6dc14d8c2..3d9d9cb84 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -17,10 +17,19 @@ class ActivityPub::ProcessStatusUpdateService < BaseService # Only native types can be updated at the moment return @status if !expected_type? || already_updated_more_recently? - last_edit_date = status.edited_at.presence || status.created_at + if @status_parser.edited_at.present? && (@status.edited_at.nil? || @status_parser.edited_at > @status.edited_at) + handle_explicit_update! + else + handle_implicit_update! + end + + @status + end + + private - # Since we rely on tracking of previous changes, ensure clean slate - status.clear_changes_information + def handle_explicit_update! + last_edit_date = @status.edited_at.presence || @status.created_at # Only allow processing one create/update per status at a time RedisLock.acquire(lock_options) do |lock| @@ -45,12 +54,20 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end end - forward_activity! if significant_changes? && @status_parser.edited_at.present? && @status_parser.edited_at > last_edit_date - - @status + forward_activity! if significant_changes? && @status_parser.edited_at > last_edit_date end - private + def handle_implicit_update! + RedisLock.acquire(lock_options) do |lock| + if lock.acquired? + update_poll!(allow_significant_changes: false) + else + raise Mastodon::RaceConditionError + end + end + + queue_poll_notifications! + end def update_media_attachments! previous_media_attachments = @status.media_attachments.to_a @@ -98,7 +115,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @media_attachments_changed = true if @status.ordered_media_attachment_ids != previous_media_attachments_ids end - def update_poll! + def update_poll!(allow_significant_changes: true) previous_poll = @status.preloadable_poll @previous_expires_at = previous_poll&.expires_at poll_parser = ActivityPub::Parser::PollParser.new(@json) @@ -109,6 +126,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService # If for some reasons the options were changed, it invalidates all previous # votes, so we need to remove them @poll_changed = true if poll_parser.significantly_changes?(poll) + return if @poll_changed && !allow_significant_changes poll.last_fetched_at = Time.now.utc poll.options = poll_parser.options @@ -121,6 +139,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @status.poll_id = poll.id elsif previous_poll.present? + return unless allow_significant_changes + previous_poll.destroy! @poll_changed = true @status.poll_id = nil @@ -132,7 +152,10 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @status.spoiler_text = @status_parser.spoiler_text || '' @status.sensitive = @account.sensitized? || @status_parser.sensitive || false @status.language = @status_parser.language - @status.edited_at = @status_parser.edited_at || Time.now.utc if significant_changes? + + @significant_changes = text_significantly_changed? || @status.spoiler_text_changed? || @media_attachments_changed || @poll_changed + + @status.edited_at = @status_parser.edited_at if significant_changes? @status.save! end @@ -243,7 +266,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end def significant_changes? - @status.text_changed? || @status.text_previously_changed? || @status.spoiler_text_changed? || @status.spoiler_text_previously_changed? || @media_attachments_changed || @poll_changed + @significant_changes + end + + def text_significantly_changed? + return false unless @status.text_changed? + + old, new = @status.text_change + HtmlAwareFormatter.new(old, false).to_s != HtmlAwareFormatter.new(new, false).to_s end def already_updated_more_recently? diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 68816e554..943cb161d 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -195,7 +195,7 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do let(:existing_status) { Fabricate(:status, account: sender, text: 'Foo', uri: note[:id]) } context 'with a Note object' do - let(:object) { note } + let(:object) { note.merge(updated: '2021-09-08T22:39:25Z') } it 'updates status' do existing_status.reload @@ -211,7 +211,7 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do id: "https://#{valid_domain}/@foo/1234/create", type: 'Create', actor: ActivityPub::TagManager.instance.uri_for(sender), - object: note, + object: note.merge(updated: '2021-09-08T22:39:25Z'), } end diff --git a/spec/services/activitypub/process_status_update_service_spec.rb b/spec/services/activitypub/process_status_update_service_spec.rb index f87adcae1..481572742 100644 --- a/spec/services/activitypub/process_status_update_service_spec.rb +++ b/spec/services/activitypub/process_status_update_service_spec.rb @@ -1,5 +1,9 @@ require 'rails_helper' +def poll_option_json(name, votes) + { type: 'Note', name: name, replies: { type: 'Collection', totalItems: votes } } +end + RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do let!(:status) { Fabricate(:status, text: 'Hello world', account: Fabricate(:account, domain: 'example.com')) } @@ -46,6 +50,180 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do expect(status.reload.spoiler_text).to eq 'Show more' end + context 'when the changes are only in sanitized-out HTML' do + let!(:status) { Fabricate(:status, text: '

Hello world joinmastodon.org

', account: Fabricate(:account, domain: 'example.com')) } + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Note', + updated: '2021-09-08T22:39:25Z', + content: '

Hello world joinmastodon.org

', + } + end + + before do + subject.call(status, json) + end + + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.edited?).to be false + end + end + + context 'when the status has not been explicitly edited' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Note', + content: 'Updated text', + } + end + + before do + subject.call(status, json) + end + + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.reload.edited?).to be false + end + + it 'does not update the text' do + expect(status.reload.text).to eq 'Hello world' + end + end + + context 'when the status has not been explicitly edited and features a poll' do + let(:account) { Fabricate(:account, domain: 'example.com') } + let!(:expiration) { 10.days.from_now.utc } + let!(:status) do + Fabricate(:status, + text: 'Hello world', + account: account, + poll_attributes: { + options: %w(Foo Bar), + account: account, + multiple: false, + hide_totals: false, + expires_at: expiration + } + ) + end + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/foo', + type: 'Question', + content: 'Hello world', + endTime: expiration.iso8601, + oneOf: [ + poll_option_json('Foo', 4), + poll_option_json('Bar', 3), + ], + } + end + + before do + subject.call(status, json) + end + + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.reload.edited?).to be false + end + + it 'does not update the text' do + expect(status.reload.text).to eq 'Hello world' + end + + it 'updates tallies' do + expect(status.poll.reload.cached_tallies).to eq [4, 3] + end + end + + context 'when the status changes a poll despite being not explicitly marked as updated' do + let(:account) { Fabricate(:account, domain: 'example.com') } + let!(:expiration) { 10.days.from_now.utc } + let!(:status) do + Fabricate(:status, + text: 'Hello world', + account: account, + poll_attributes: { + options: %w(Foo Bar), + account: account, + multiple: false, + hide_totals: false, + expires_at: expiration + } + ) + end + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/foo', + type: 'Question', + content: 'Hello world', + endTime: expiration.iso8601, + oneOf: [ + poll_option_json('Foo', 4), + poll_option_json('Bar', 3), + poll_option_json('Baz', 3), + ], + } + end + + before do + subject.call(status, json) + end + + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.reload.edited?).to be false + end + + it 'does not update the text' do + expect(status.reload.text).to eq 'Hello world' + end + + it 'does not update tallies' do + expect(status.poll.reload.cached_tallies).to eq [0, 0] + end + end + + context 'when receiving an edit older than the latest processed' do + before do + status.snapshot!(at_time: status.created_at, rate_limit: false) + status.update!(text: 'Hello newer world', edited_at: Time.now.utc) + status.snapshot!(rate_limit: false) + end + + it 'does not create any edits' do + expect { subject.call(status, json) }.not_to change { status.reload.edits.pluck(&:id) } + end + + it 'does not update the text, spoiler_text or edited_at' do + expect { subject.call(status, json) }.not_to change { s = status.reload; [s.text, s.spoiler_text, s.edited_at] } + end + end + context 'with no changes at all' do let(:payload) do { -- cgit