From 5d8398c8b8b51ee7363e7d45acc560f489783e34 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 2 Jun 2020 19:24:53 +0200 Subject: Add E2EE API (#13820) --- spec/fabricators/device_fabricator.rb | 8 ++++ spec/fabricators/encrypted_message_fabricator.rb | 8 ++++ spec/fabricators/one_time_key_fabricator.rb | 11 +++++ spec/fabricators/system_key_fabricator.rb | 3 ++ spec/lib/activitypub/activity/create_spec.rb | 56 ++++++++++++++++++++++++ spec/models/device_spec.rb | 5 +++ spec/models/encrypted_message_spec.rb | 5 +++ spec/models/one_time_key_spec.rb | 5 +++ spec/models/system_key_spec.rb | 5 +++ 9 files changed, 106 insertions(+) create mode 100644 spec/fabricators/device_fabricator.rb create mode 100644 spec/fabricators/encrypted_message_fabricator.rb create mode 100644 spec/fabricators/one_time_key_fabricator.rb create mode 100644 spec/fabricators/system_key_fabricator.rb create mode 100644 spec/models/device_spec.rb create mode 100644 spec/models/encrypted_message_spec.rb create mode 100644 spec/models/one_time_key_spec.rb create mode 100644 spec/models/system_key_spec.rb (limited to 'spec') diff --git a/spec/fabricators/device_fabricator.rb b/spec/fabricators/device_fabricator.rb new file mode 100644 index 000000000..b15d8248f --- /dev/null +++ b/spec/fabricators/device_fabricator.rb @@ -0,0 +1,8 @@ +Fabricator(:device) do + access_token + account + device_id { Faker::Number.number(digits: 5) } + name { Faker::App.name } + fingerprint_key { Base64.strict_encode64(Ed25519::SigningKey.generate.verify_key.to_bytes) } + identity_key { Base64.strict_encode64(Ed25519::SigningKey.generate.verify_key.to_bytes) } +end diff --git a/spec/fabricators/encrypted_message_fabricator.rb b/spec/fabricators/encrypted_message_fabricator.rb new file mode 100644 index 000000000..e65f66302 --- /dev/null +++ b/spec/fabricators/encrypted_message_fabricator.rb @@ -0,0 +1,8 @@ +Fabricator(:encrypted_message) do + device + from_account + from_device_id { Faker::Number.number(digits: 5) } + type 0 + body "" + message_franking "" +end diff --git a/spec/fabricators/one_time_key_fabricator.rb b/spec/fabricators/one_time_key_fabricator.rb new file mode 100644 index 000000000..8794baeb5 --- /dev/null +++ b/spec/fabricators/one_time_key_fabricator.rb @@ -0,0 +1,11 @@ +Fabricator(:one_time_key) do + device + key_id { Faker::Alphanumeric.alphanumeric(number: 10) } + key { Base64.strict_encode64(Ed25519::SigningKey.generate.verify_key.to_bytes) } + + signature do |attrs| + signing_key = Ed25519::SigningKey.generate + attrs[:device].update(fingerprint_key: Base64.strict_encode64(signing_key.verify_key.to_bytes)) + Base64.strict_encode64(signing_key.sign(attrs[:key])) + end +end diff --git a/spec/fabricators/system_key_fabricator.rb b/spec/fabricators/system_key_fabricator.rb new file mode 100644 index 000000000..f808495e0 --- /dev/null +++ b/spec/fabricators/system_key_fabricator.rb @@ -0,0 +1,3 @@ +Fabricator(:system_key) do + +end diff --git a/spec/lib/activitypub/activity/create_spec.rb b/spec/lib/activitypub/activity/create_spec.rb index 5220deabb..2ac4acc12 100644 --- a/spec/lib/activitypub/activity/create_spec.rb +++ b/spec/lib/activitypub/activity/create_spec.rb @@ -579,6 +579,62 @@ RSpec.describe ActivityPub::Activity::Create do end end + context 'with an encrypted message' do + let(:recipient) { Fabricate(:account) } + let(:target_device) { Fabricate(:device, account: recipient) } + + subject { described_class.new(json, sender, delivery: true, delivered_to_account_id: recipient.id) } + + let(:object_json) do + { + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, + type: 'EncryptedMessage', + attributedTo: { + type: 'Device', + deviceId: '1234', + }, + to: { + type: 'Device', + deviceId: target_device.device_id, + }, + messageType: 1, + cipherText: 'Foo', + messageFranking: 'Baz678', + digest: { + digestAlgorithm: 'Bar456', + digestValue: 'Foo123', + }, + } + end + + before do + subject.perform + end + + it 'creates an encrypted message' do + encrypted_message = target_device.encrypted_messages.reload.first + + expect(encrypted_message).to_not be_nil + expect(encrypted_message.from_device_id).to eq '1234' + expect(encrypted_message.from_account).to eq sender + expect(encrypted_message.type).to eq 1 + expect(encrypted_message.body).to eq 'Foo' + expect(encrypted_message.digest).to eq 'Foo123' + end + + it 'creates a message franking' do + encrypted_message = target_device.encrypted_messages.reload.first + message_franking = encrypted_message.message_franking + + crypt = ActiveSupport::MessageEncryptor.new(SystemKey.current_key, serializer: Oj) + json = crypt.decrypt_and_verify(message_franking) + + expect(json['source_account_id']).to eq sender.id + expect(json['target_account_id']).to eq recipient.id + expect(json['original_franking']).to eq 'Baz678' + end + end + context 'when sender is followed by local users' do subject { described_class.new(json, sender, delivery: true) } diff --git a/spec/models/device_spec.rb b/spec/models/device_spec.rb new file mode 100644 index 000000000..f56fbf978 --- /dev/null +++ b/spec/models/device_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe Device, type: :model do + +end diff --git a/spec/models/encrypted_message_spec.rb b/spec/models/encrypted_message_spec.rb new file mode 100644 index 000000000..1238d57b6 --- /dev/null +++ b/spec/models/encrypted_message_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe EncryptedMessage, type: :model do + +end diff --git a/spec/models/one_time_key_spec.rb b/spec/models/one_time_key_spec.rb new file mode 100644 index 000000000..34598334c --- /dev/null +++ b/spec/models/one_time_key_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe OneTimeKey, type: :model do + +end diff --git a/spec/models/system_key_spec.rb b/spec/models/system_key_spec.rb new file mode 100644 index 000000000..a138bc131 --- /dev/null +++ b/spec/models/system_key_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe SystemKey, type: :model do + +end -- cgit From aed3a436a2dbef40096ec8596cec08e185efe936 Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 4 Jun 2020 19:03:31 +0200 Subject: Fix serialization of replies when some of them are URIs (#13957) * Fix serialization of replies when some of them are URIs Fixes #13956 * Add test --- app/serializers/activitypub/collection_serializer.rb | 11 +++++++++++ spec/controllers/activitypub/replies_controller_spec.rb | 17 +++++++++++++++++ 2 files changed, 28 insertions(+) (limited to 'spec') diff --git a/app/serializers/activitypub/collection_serializer.rb b/app/serializers/activitypub/collection_serializer.rb index 00c7b786a..ea7af5433 100644 --- a/app/serializers/activitypub/collection_serializer.rb +++ b/app/serializers/activitypub/collection_serializer.rb @@ -1,6 +1,15 @@ # frozen_string_literal: true class ActivityPub::CollectionSerializer < ActivityPub::Serializer + class StringSerializer < ActiveModel::Serializer + # Despite the name, it does not return a hash, but the same can be said of + # the ActiveModel::Serializer::CollectionSerializer class which handles + # arrays. + def serializable_hash(*_args) + object + end + end + def self.serializer_for(model, options) case model.class.name when 'Status' @@ -9,6 +18,8 @@ class ActivityPub::CollectionSerializer < ActivityPub::Serializer ActivityPub::DeviceSerializer when 'ActivityPub::CollectionPresenter' ActivityPub::CollectionSerializer + when 'String' + StringSerializer else super end diff --git a/spec/controllers/activitypub/replies_controller_spec.rb b/spec/controllers/activitypub/replies_controller_spec.rb index a5ed14180..d956e1b35 100644 --- a/spec/controllers/activitypub/replies_controller_spec.rb +++ b/spec/controllers/activitypub/replies_controller_spec.rb @@ -4,6 +4,7 @@ require 'rails_helper' RSpec.describe ActivityPub::RepliesController, type: :controller do let(:status) { Fabricate(:status, visibility: parent_visibility) } + let(:remote_reply_id) { nil } let(:remote_account) { nil } before do @@ -14,6 +15,8 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do Fabricate(:status, thread: status, visibility: :private) Fabricate(:status, account: status.account, thread: status, visibility: :public) Fabricate(:status, account: status.account, thread: status, visibility: :private) + + Fabricate(:status, account: remote_account, thread: status, visibility: :public, uri: remote_reply_id) if remote_reply_id end describe 'GET #index' do @@ -110,6 +113,20 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do expect(json[:first][:items].size).to eq 2 expect(json[:first][:items].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true end + + context 'with remote responses' do + let(:remote_reply_id) { 'foo' } + + it 'returned items are all inlined local toots or are ids' do + json = body_as_json + + expect(json[:first]).to be_a Hash + expect(json[:first][:items]).to be_an Array + expect(json[:first][:items].size).to eq 3 + expect(json[:first][:items].all? { |item| item.is_a?(Hash) ? ActivityPub::TagManager.instance.local_uri?(item[:id]) : item.is_a?(String) }).to be true + expect(json[:first][:items]).to include remote_reply_id + end + end end end -- cgit From 72a7cfaa395bbddabd0f0a712165fd7babf5d58c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 9 Jun 2020 10:23:06 +0200 Subject: Add e-mail-based sign in challenge for users with disabled 2FA (#14013) --- app/controllers/auth/sessions_controller.rb | 50 +--------- .../sign_in_token_authentication_concern.rb | 49 ++++++++++ .../concerns/two_factor_authentication_concern.rb | 47 +++++++++ app/mailers/user_mailer.rb | 17 ++++ app/models/user.rb | 28 +++++- app/views/auth/sessions/sign_in_token.html.haml | 14 +++ app/views/user_mailer/sign_in_token.html.haml | 105 +++++++++++++++++++++ app/views/user_mailer/sign_in_token.text.erb | 17 ++++ config/locales/en.yml | 9 ++ config/locales/simple_form.en.yml | 1 + .../20200608113046_add_sign_in_token_to_users.rb | 6 ++ db/schema.rb | 5 +- spec/controllers/auth/sessions_controller_spec.rb | 66 ++++++++++++- spec/mailers/previews/user_mailer_preview.rb | 5 + 14 files changed, 368 insertions(+), 51 deletions(-) create mode 100644 app/controllers/concerns/sign_in_token_authentication_concern.rb create mode 100644 app/controllers/concerns/two_factor_authentication_concern.rb create mode 100644 app/views/auth/sessions/sign_in_token.html.haml create mode 100644 app/views/user_mailer/sign_in_token.html.haml create mode 100644 app/views/user_mailer/sign_in_token.text.erb create mode 100644 db/migrate/20200608113046_add_sign_in_token_to_users.rb (limited to 'spec') diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index e95909447..2415e2ef3 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -8,7 +8,8 @@ class Auth::SessionsController < Devise::SessionsController skip_before_action :require_no_authentication, only: [:create] skip_before_action :require_functional! - prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] + include TwoFactorAuthenticationConcern + include SignInTokenAuthenticationConcern before_action :set_instance_presenter, only: [:new] before_action :set_body_classes @@ -39,8 +40,8 @@ class Auth::SessionsController < Devise::SessionsController protected def find_user - if session[:otp_user_id] - User.find(session[:otp_user_id]) + if session[:attempt_user_id] + User.find(session[:attempt_user_id]) else user = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication @@ -49,7 +50,7 @@ class Auth::SessionsController < Devise::SessionsController end def user_params - params.require(:user).permit(:email, :password, :otp_attempt) + params.require(:user).permit(:email, :password, :otp_attempt, :sign_in_token_attempt) end def after_sign_in_path_for(resource) @@ -70,47 +71,6 @@ class Auth::SessionsController < Devise::SessionsController super end - def two_factor_enabled? - find_user&.otp_required_for_login? - end - - def valid_otp_attempt?(user) - user.validate_and_consume_otp!(user_params[:otp_attempt]) || - user.invalidate_otp_backup_code!(user_params[:otp_attempt]) - rescue OpenSSL::Cipher::CipherError - false - end - - def authenticate_with_two_factor - user = self.resource = find_user - - if user_params[:otp_attempt].present? && session[:otp_user_id] - authenticate_with_two_factor_via_otp(user) - elsif user.present? && (user.encrypted_password.blank? || user.valid_password?(user_params[:password])) - # If encrypted_password is blank, we got the user from LDAP or PAM, - # so credentials are already valid - - prompt_for_two_factor(user) - end - end - - def authenticate_with_two_factor_via_otp(user) - if valid_otp_attempt?(user) - session.delete(:otp_user_id) - remember_me(user) - sign_in(user) - else - flash.now[:alert] = I18n.t('users.invalid_otp_token') - prompt_for_two_factor(user) - end - end - - def prompt_for_two_factor(user) - session[:otp_user_id] = user.id - @body_classes = 'lighter' - render :two_factor - end - def require_no_authentication super # Delete flash message that isn't entirely useful and may be confusing in diff --git a/app/controllers/concerns/sign_in_token_authentication_concern.rb b/app/controllers/concerns/sign_in_token_authentication_concern.rb new file mode 100644 index 000000000..a177aacaf --- /dev/null +++ b/app/controllers/concerns/sign_in_token_authentication_concern.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module SignInTokenAuthenticationConcern + extend ActiveSupport::Concern + + included do + prepend_before_action :authenticate_with_sign_in_token, if: :sign_in_token_required?, only: [:create] + end + + def sign_in_token_required? + find_user&.suspicious_sign_in?(request.remote_ip) + end + + def valid_sign_in_token_attempt?(user) + Devise.secure_compare(user.sign_in_token, user_params[:sign_in_token_attempt]) + end + + def authenticate_with_sign_in_token + user = self.resource = find_user + + if user_params[:sign_in_token_attempt].present? && session[:attempt_user_id] + authenticate_with_sign_in_token_attempt(user) + elsif user.present? && user.external_or_valid_password?(user_params[:password]) + prompt_for_sign_in_token(user) + end + end + + def authenticate_with_sign_in_token_attempt(user) + if valid_sign_in_token_attempt?(user) + session.delete(:attempt_user_id) + remember_me(user) + sign_in(user) + else + flash.now[:alert] = I18n.t('users.invalid_sign_in_token') + prompt_for_sign_in_token(user) + end + end + + def prompt_for_sign_in_token(user) + if user.sign_in_token_expired? + user.generate_sign_in_token && user.save + UserMailer.sign_in_token(user, request.remote_ip, request.user_agent, Time.now.utc.to_s).deliver_later! + end + + session[:attempt_user_id] = user.id + @body_classes = 'lighter' + render :sign_in_token + end +end diff --git a/app/controllers/concerns/two_factor_authentication_concern.rb b/app/controllers/concerns/two_factor_authentication_concern.rb new file mode 100644 index 000000000..cdd8d14af --- /dev/null +++ b/app/controllers/concerns/two_factor_authentication_concern.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module TwoFactorAuthenticationConcern + extend ActiveSupport::Concern + + included do + prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] + end + + def two_factor_enabled? + find_user&.otp_required_for_login? + end + + def valid_otp_attempt?(user) + user.validate_and_consume_otp!(user_params[:otp_attempt]) || + user.invalidate_otp_backup_code!(user_params[:otp_attempt]) + rescue OpenSSL::Cipher::CipherError + false + end + + def authenticate_with_two_factor + user = self.resource = find_user + + if user_params[:otp_attempt].present? && session[:attempt_user_id] + authenticate_with_two_factor_attempt(user) + elsif user.present? && user.external_or_valid_password?(user_params[:password]) + prompt_for_two_factor(user) + end + end + + def authenticate_with_two_factor_attempt(user) + if valid_otp_attempt?(user) + session.delete(:attempt_user_id) + remember_me(user) + sign_in(user) + else + flash.now[:alert] = I18n.t('users.invalid_otp_token') + prompt_for_two_factor(user) + end + end + + def prompt_for_two_factor(user) + session[:attempt_user_id] = user.id + @body_classes = 'lighter' + render :two_factor + end +end diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 88a11f761..2cd58e60a 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -126,4 +126,21 @@ class UserMailer < Devise::Mailer reply_to: Setting.site_contact_email end end + + def sign_in_token(user, remote_ip, user_agent, timestamp) + @resource = user + @instance = Rails.configuration.x.local_domain + @remote_ip = remote_ip + @user_agent = user_agent + @detection = Browser.new(user_agent) + @timestamp = timestamp.to_time.utc + + return if @resource.disabled? + + I18n.with_locale(@resource.locale || I18n.default_locale) do + mail to: @resource.email, + subject: I18n.t('user_mailer.sign_in_token.subject'), + reply_to: Setting.site_contact_email + end + end end diff --git a/app/models/user.rb b/app/models/user.rb index ae0cdf29d..306e2d435 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,6 +38,8 @@ # chosen_languages :string is an Array # created_by_application_id :bigint(8) # approved :boolean default(TRUE), not null +# sign_in_token :string +# sign_in_token_sent_at :datetime # class User < ApplicationRecord @@ -113,7 +115,7 @@ class User < ApplicationRecord :advanced_layout, :use_blurhash, :use_pending_items, :trends, :crop_images, to: :settings, prefix: :setting, allow_nil: false - attr_reader :invite_code + attr_reader :invite_code, :sign_in_token_attempt attr_writer :external def confirmed? @@ -167,6 +169,10 @@ class User < ApplicationRecord true end + def suspicious_sign_in?(ip) + !otp_required_for_login? && current_sign_in_at.present? && current_sign_in_at < 2.weeks.ago && !recent_ip?(ip) + end + def functional? confirmed? && approved? && !disabled? && !account.suspended? && account.moved_to_account_id.nil? end @@ -269,6 +275,13 @@ class User < ApplicationRecord super end + def external_or_valid_password?(compare_password) + # If encrypted_password is blank, we got the user from LDAP or PAM, + # so credentials are already valid + + encrypted_password.blank? || valid_password?(compare_password) + end + def send_reset_password_instructions return false if encrypted_password.blank? @@ -304,6 +317,15 @@ class User < ApplicationRecord end end + def sign_in_token_expired? + sign_in_token_sent_at.nil? || sign_in_token_sent_at < 5.minutes.ago + end + + def generate_sign_in_token + self.sign_in_token = Devise.friendly_token(6) + self.sign_in_token_sent_at = Time.now.utc + end + protected def send_devise_notification(notification, *args) @@ -320,6 +342,10 @@ class User < ApplicationRecord private + def recent_ip?(ip) + recent_ips.any? { |(_, recent_ip)| recent_ip == ip } + end + def send_pending_devise_notifications pending_devise_notifications.each do |notification, args| render_and_send_devise_message(notification, *args) diff --git a/app/views/auth/sessions/sign_in_token.html.haml b/app/views/auth/sessions/sign_in_token.html.haml new file mode 100644 index 000000000..8923203cd --- /dev/null +++ b/app/views/auth/sessions/sign_in_token.html.haml @@ -0,0 +1,14 @@ +- content_for :page_title do + = t('auth.login') + += simple_form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| + %p.hint.otp-hint= t('users.suspicious_sign_in_confirmation') + + .fields-group + = f.input :sign_in_token_attempt, type: :number, wrapper: :with_label, label: t('simple_form.labels.defaults.sign_in_token_attempt'), input_html: { 'aria-label' => t('simple_form.labels.defaults.sign_in_token_attempt'), :autocomplete => 'off' }, autofocus: true + + .actions + = f.button :button, t('auth.login'), type: :submit + + - if Setting.site_contact_email.present? + %p.hint.subtle-hint= t('users.generic_access_help_html', email: mail_to(Setting.site_contact_email, nil)) diff --git a/app/views/user_mailer/sign_in_token.html.haml b/app/views/user_mailer/sign_in_token.html.haml new file mode 100644 index 000000000..826b34e7c --- /dev/null +++ b/app/views/user_mailer/sign_in_token.html.haml @@ -0,0 +1,105 @@ +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.hero + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center.padded + %table.hero-icon.alert-icon{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td + = image_tag full_pack_url('media/images/mailer/icon_email.png'), alt: '' + + %h1= t 'user_mailer.sign_in_token.title' + %p.lead= t 'user_mailer.sign_in_token.explanation' + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.content-start + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.input-cell + %table.input{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td= @resource.sign_in_token + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center + %p= t 'user_mailer.sign_in_token.details' + %tr + %td.column-cell.text-center + %p + %strong= "#{t('sessions.ip')}:" + = @remote_ip + %br/ + %strong= "#{t('sessions.browser')}:" + %span{ title: @user_agent }= t 'sessions.description', browser: t("sessions.browsers.#{@detection.id}", default: "#{@detection.id}"), platform: t("sessions.platforms.#{@detection.platform.id}", default: "#{@detection.platform.id}") + %br/ + = l(@timestamp) + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center + %p= t 'user_mailer.sign_in_token.further_actions' + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.button-cell + %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.button-primary + = link_to edit_user_registration_url do + %span= t 'settings.account_settings' diff --git a/app/views/user_mailer/sign_in_token.text.erb b/app/views/user_mailer/sign_in_token.text.erb new file mode 100644 index 000000000..2539ddaf6 --- /dev/null +++ b/app/views/user_mailer/sign_in_token.text.erb @@ -0,0 +1,17 @@ +<%= t 'user_mailer.sign_in_token.title' %> + +=== + +<%= t 'user_mailer.sign_in_token.explanation' %> + +=> <%= @resource.sign_in_token %> + +<%= t 'user_mailer.sign_in_token.details' %> + +<%= t('sessions.ip') %>: <%= @remote_ip %> +<%= t('sessions.browser') %>: <%= t('sessions.description', browser: t("sessions.browsers.#{@detection.id}", default: "#{@detection.id}"), platform: t("sessions.platforms.#{@detection.platform.id}", default: "#{@detection.platform.id}")) %> +<%= l(@timestamp) %> + +<%= t 'user_mailer.sign_in_token.further_actions' %> + +=> <%= edit_user_registration_url %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 20d87057f..a33605049 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1273,6 +1273,12 @@ en: explanation: You requested a full backup of your Mastodon account. It's now ready for download! subject: Your archive is ready for download title: Archive takeout + sign_in_token: + details: 'Here are details of the attempt:' + explanation: 'We detected an attempt to sign in to your account from an unrecognized IP address. If this is you, please enter the security code below on the sign in challenge page:' + further_actions: 'If this wasn''t you, please change your password and enable two-factor authentication on your account. You can do so here:' + subject: Please confirm attempted sign in + title: Sign in attempt warning: explanation: disable: While your account is frozen, your account data remains intact, but you cannot perform any actions until it is unlocked. @@ -1310,11 +1316,14 @@ en: title: Welcome aboard, %{name}! users: follow_limit_reached: You cannot follow more than %{limit} people + generic_access_help_html: Trouble accessing your account? You may get in touch with %{email} for assistance invalid_email: The e-mail address is invalid invalid_otp_token: Invalid two-factor code + invalid_sign_in_token: Invalid security code otp_lost_help_html: If you lost access to both, you may get in touch with %{email} seamless_external_login: You are logged in via an external service, so password and e-mail settings are not available. signed_in_as: 'Signed in as:' + suspicious_sign_in_confirmation: You appear to not have logged in from this device before, and you haven't logged in for a while, so we're sending a security code to your e-mail address to confirm that it's you. verification: explanation_html: 'You can verify yourself as the owner of the links in your profile metadata. For that, the linked website must contain a link back to your Mastodon profile. The link back must have a rel="me" attribute. The text content of the link does not matter. Here is an example:' verification: Verification diff --git a/config/locales/simple_form.en.yml b/config/locales/simple_form.en.yml index fd56a35bf..f84b6c884 100644 --- a/config/locales/simple_form.en.yml +++ b/config/locales/simple_form.en.yml @@ -151,6 +151,7 @@ en: setting_use_blurhash: Show colorful gradients for hidden media setting_use_pending_items: Slow mode severity: Severity + sign_in_token_attempt: Security code type: Import type username: Username username_or_email: Username or Email diff --git a/db/migrate/20200608113046_add_sign_in_token_to_users.rb b/db/migrate/20200608113046_add_sign_in_token_to_users.rb new file mode 100644 index 000000000..baa63c10f --- /dev/null +++ b/db/migrate/20200608113046_add_sign_in_token_to_users.rb @@ -0,0 +1,6 @@ +class AddSignInTokenToUsers < ActiveRecord::Migration[5.2] + def change + add_column :users, :sign_in_token, :string + add_column :users, :sign_in_token_sent_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index beda93c01..df5d48f44 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,8 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_06_05_155027) do +ActiveRecord::Schema.define(version: 2020_06_08_113046) do + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -869,6 +870,8 @@ ActiveRecord::Schema.define(version: 2020_06_05_155027) do t.string "chosen_languages", array: true t.bigint "created_by_application_id" t.boolean "approved", default: true, null: false + t.string "sign_in_token" + t.datetime "sign_in_token_sent_at" t.index ["account_id"], name: "index_users_on_account_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id" diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 1950c173a..c387842cd 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -215,7 +215,7 @@ RSpec.describe Auth::SessionsController, type: :controller do context 'using a valid OTP' do before do - post :create, params: { user: { otp_attempt: user.current_otp } }, session: { otp_user_id: user.id } + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id } end it 'redirects to home' do @@ -230,7 +230,7 @@ RSpec.describe Auth::SessionsController, type: :controller do 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) - post :create, params: { user: { otp_attempt: user.current_otp } }, session: { otp_user_id: user.id } + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id } end it 'shows a login error' do @@ -244,7 +244,7 @@ RSpec.describe Auth::SessionsController, type: :controller do context 'using a valid recovery code' do before do - post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { otp_user_id: user.id } + post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { attempt_user_id: user.id } end it 'redirects to home' do @@ -258,7 +258,7 @@ RSpec.describe Auth::SessionsController, type: :controller do context 'using an invalid OTP' do before do - post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { otp_user_id: user.id } + post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id } end it 'shows a login error' do @@ -270,5 +270,63 @@ 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, current_sign_in_ip: '0.0.0.0') } + + 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 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 } + 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 an invalid sign in token' do + before do + post :create, params: { user: { sign_in_token_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id } + 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 end diff --git a/spec/mailers/previews/user_mailer_preview.rb b/spec/mailers/previews/user_mailer_preview.rb index 464f177d0..313666412 100644 --- a/spec/mailers/previews/user_mailer_preview.rb +++ b/spec/mailers/previews/user_mailer_preview.rb @@ -59,4 +59,9 @@ class UserMailerPreview < ActionMailer::Preview def warning UserMailer.warning(User.first, AccountWarning.new(text: '', action: :silence), [Status.first.id]) 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) + end end -- cgit From 89f40b6c3ec525b09d02f21e9b45276084167d8d Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 9 Jun 2020 10:32:00 +0200 Subject: Make domain block/silence/reject-media code more robust (#13424) * Split media cleanup from reject-media domain blocks to its own service * Slightly improve ClearDomainMediaService error handling * Lower DomainClearMediaWorker to lowest-priority queue * Do not catch ActiveRecord::RecordNotFound in domain block workers * Fix DomainBlockWorker spec labels * Add some specs * Change domain blocks to immediately mark accounts as suspended Rather than doing so sequentially, account after account, while cleaning their data. This doesn't change much about the time the block takes to complete, but it immediately prevents interaction with the blocked domain, while up to now, it would only be guaranteed when the process ends. --- app/services/block_domain_service.rb | 53 +----------------- app/services/clear_domain_media_service.rb | 70 ++++++++++++++++++++++++ app/workers/domain_block_worker.rb | 7 ++- app/workers/domain_clear_media_worker.rb | 14 +++++ spec/services/clear_domain_media_service_spec.rb | 23 ++++++++ spec/workers/domain_block_worker_spec.rb | 4 +- spec/workers/domain_clear_media_worker_spec.rb | 26 +++++++++ 7 files changed, 142 insertions(+), 55 deletions(-) create mode 100644 app/services/clear_domain_media_service.rb create mode 100644 app/workers/domain_clear_media_worker.rb create mode 100644 spec/services/clear_domain_media_service_spec.rb create mode 100644 spec/workers/domain_clear_media_worker_spec.rb (limited to 'spec') diff --git a/app/services/block_domain_service.rb b/app/services/block_domain_service.rb index 9f0860674..dc23ef8d8 100644 --- a/app/services/block_domain_service.rb +++ b/app/services/block_domain_service.rb @@ -26,59 +26,20 @@ class BlockDomainService < BaseService suspend_accounts! end - clear_media! if domain_block.reject_media? - end - - def invalidate_association_caches! - # Normally, associated models of a status are immutable (except for accounts) - # so they are aggressively cached. After updating the media attachments to no - # longer point to a local file, we need to clear the cache to make those - # changes appear in the API and UI - @affected_status_ids.each { |id| Rails.cache.delete_matched("statuses/#{id}-*") } + DomainClearMediaWorker.perform_async(domain_block.id) if domain_block.reject_media? end def silence_accounts! blocked_domain_accounts.without_silenced.in_batches.update_all(silenced_at: @domain_block.created_at) end - def clear_media! - @affected_status_ids = [] - - clear_account_images! - clear_account_attachments! - clear_emojos! - - invalidate_association_caches! - end - def suspend_accounts! - blocked_domain_accounts.without_suspended.reorder(nil).find_each do |account| + blocked_domain_accounts.without_suspended.in_batches.update_all(suspended_at: @domain_block.created_at) + blocked_domain_accounts.where(suspended_at: @domain_block.created_at).reorder(nil).find_each do |account| SuspendAccountService.new.call(account, reserve_username: true, suspended_at: @domain_block.created_at) end end - def clear_account_images! - blocked_domain_accounts.reorder(nil).find_each do |account| - account.avatar.destroy if account.avatar.exists? - account.header.destroy if account.header.exists? - account.save - end - end - - def clear_account_attachments! - media_from_blocked_domain.reorder(nil).find_each do |attachment| - @affected_status_ids << attachment.status_id if attachment.status_id.present? - - attachment.file.destroy if attachment.file.exists? - attachment.type = :unknown - attachment.save - end - end - - def clear_emojos! - emojis_from_blocked_domains.destroy_all - end - def blocked_domain domain_block.domain end @@ -86,12 +47,4 @@ class BlockDomainService < BaseService def blocked_domain_accounts Account.by_domain_and_subdomains(blocked_domain) end - - def media_from_blocked_domain - MediaAttachment.joins(:account).merge(blocked_domain_accounts).reorder(nil) - end - - def emojis_from_blocked_domains - CustomEmoji.by_domain_and_subdomains(blocked_domain) - end end diff --git a/app/services/clear_domain_media_service.rb b/app/services/clear_domain_media_service.rb new file mode 100644 index 000000000..704cfb71a --- /dev/null +++ b/app/services/clear_domain_media_service.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +class ClearDomainMediaService < BaseService + attr_reader :domain_block + + def call(domain_block) + @domain_block = domain_block + clear_media! if domain_block.reject_media? + end + + private + + def invalidate_association_caches! + # Normally, associated models of a status are immutable (except for accounts) + # so they are aggressively cached. After updating the media attachments to no + # longer point to a local file, we need to clear the cache to make those + # changes appear in the API and UI + @affected_status_ids.each { |id| Rails.cache.delete_matched("statuses/#{id}-*") } + end + + def clear_media! + @affected_status_ids = [] + + begin + clear_account_images! + clear_account_attachments! + clear_emojos! + ensure + invalidate_association_caches! + end + end + + def clear_account_images! + blocked_domain_accounts.reorder(nil).find_each do |account| + account.avatar.destroy if account.avatar&.exists? + account.header.destroy if account.header&.exists? + account.save + end + end + + def clear_account_attachments! + media_from_blocked_domain.reorder(nil).find_each do |attachment| + @affected_status_ids << attachment.status_id if attachment.status_id.present? + + attachment.file.destroy if attachment.file&.exists? + attachment.type = :unknown + attachment.save + end + end + + def clear_emojos! + emojis_from_blocked_domains.destroy_all + end + + def blocked_domain + domain_block.domain + end + + def blocked_domain_accounts + Account.by_domain_and_subdomains(blocked_domain) + end + + def media_from_blocked_domain + MediaAttachment.joins(:account).merge(blocked_domain_accounts).reorder(nil) + end + + def emojis_from_blocked_domains + CustomEmoji.by_domain_and_subdomains(blocked_domain) + end +end diff --git a/app/workers/domain_block_worker.rb b/app/workers/domain_block_worker.rb index 35518d6b5..3c601cd83 100644 --- a/app/workers/domain_block_worker.rb +++ b/app/workers/domain_block_worker.rb @@ -4,8 +4,9 @@ class DomainBlockWorker include Sidekiq::Worker def perform(domain_block_id, update = false) - BlockDomainService.new.call(DomainBlock.find(domain_block_id), update) - rescue ActiveRecord::RecordNotFound - true + domain_block = DomainBlock.find_by(id: domain_block_id) + return true if domain_block.nil? + + BlockDomainService.new.call(domain_block, update) end end diff --git a/app/workers/domain_clear_media_worker.rb b/app/workers/domain_clear_media_worker.rb new file mode 100644 index 000000000..971934a08 --- /dev/null +++ b/app/workers/domain_clear_media_worker.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class DomainClearMediaWorker + include Sidekiq::Worker + + sidekiq_options queue: 'pull' + + def perform(domain_block_id) + domain_block = DomainBlock.find_by(id: domain_block_id) + return true if domain_block.nil? + + ClearDomainMediaService.new.call(domain_block) + end +end diff --git a/spec/services/clear_domain_media_service_spec.rb b/spec/services/clear_domain_media_service_spec.rb new file mode 100644 index 000000000..8e58c6039 --- /dev/null +++ b/spec/services/clear_domain_media_service_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +RSpec.describe ClearDomainMediaService, type: :service do + let!(:bad_account) { Fabricate(:account, username: 'badguy666', domain: 'evil.org') } + let!(:bad_status1) { Fabricate(:status, account: bad_account, text: 'You suck') } + let!(:bad_status2) { Fabricate(:status, account: bad_account, text: 'Hahaha') } + let!(:bad_attachment) { Fabricate(:media_attachment, account: bad_account, status: bad_status2, file: attachment_fixture('attachment.jpg')) } + + subject { ClearDomainMediaService.new } + + describe 'for a silence with reject media' do + before do + subject.call(DomainBlock.create!(domain: 'evil.org', severity: :silence, reject_media: true)) + end + + it 'leaves the domains status and attachements, but clears media' do + expect { bad_status1.reload }.not_to raise_error + expect { bad_status2.reload }.not_to raise_error + expect { bad_attachment.reload }.not_to raise_error + expect(bad_attachment.file.exists?).to be false + end + end +end diff --git a/spec/workers/domain_block_worker_spec.rb b/spec/workers/domain_block_worker_spec.rb index 48b3e38c4..bd8fc4a62 100644 --- a/spec/workers/domain_block_worker_spec.rb +++ b/spec/workers/domain_block_worker_spec.rb @@ -8,7 +8,7 @@ describe DomainBlockWorker do describe 'perform' do let(:domain_block) { Fabricate(:domain_block) } - it 'returns true for non-existent domain block' do + it 'calls domain block service for relevant domain block' do service = double(call: nil) allow(BlockDomainService).to receive(:new).and_return(service) result = subject.perform(domain_block.id) @@ -17,7 +17,7 @@ describe DomainBlockWorker do expect(service).to have_received(:call).with(domain_block, false) end - it 'calls domain block service for relevant domain block' do + it 'returns true for non-existent domain block' do result = subject.perform('aaa') expect(result).to eq(true) diff --git a/spec/workers/domain_clear_media_worker_spec.rb b/spec/workers/domain_clear_media_worker_spec.rb new file mode 100644 index 000000000..36251b1ec --- /dev/null +++ b/spec/workers/domain_clear_media_worker_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DomainClearMediaWorker do + subject { described_class.new } + + describe 'perform' do + let(:domain_block) { Fabricate(:domain_block, severity: :silence, reject_media: true) } + + it 'calls domain clear media service for relevant domain block' do + service = double(call: nil) + allow(ClearDomainMediaService).to receive(:new).and_return(service) + result = subject.perform(domain_block.id) + + expect(result).to be_nil + expect(service).to have_received(:call).with(domain_block) + end + + it 'returns true for non-existent domain block' do + result = subject.perform('aaa') + + expect(result).to eq(true) + end + end +end -- cgit