From 8532429af749339a3ff6af4130de3743cd8d1c68 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 12 Nov 2020 23:05:01 +0100 Subject: Fix 2FA/sign-in token sessions being valid after password change (#14802) If someone tries logging in to an account and is prompted for a 2FA code or sign-in token, even if the account's password or e-mail is updated in the meantime, the session will show the prompt and allow the login process to complete with a valid 2FA code or sign-in token --- app/controllers/api/base_controller.rb | 2 +- app/controllers/auth/sessions_controller.rb | 22 ++++++++++++++- .../sign_in_token_authentication_concern.rb | 16 ++++++----- .../concerns/two_factor_authentication_concern.rb | 32 ++++++++++++---------- app/controllers/concerns/user_tracking_concern.rb | 7 ++--- app/models/user.rb | 25 +++++++++++++---- spec/controllers/auth/sessions_controller_spec.rb | 14 +++++----- 7 files changed, 78 insertions(+), 40 deletions(-) diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index e962c4e97..fe199e689 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -103,7 +103,7 @@ class Api::BaseController < ApplicationController elsif !current_user.functional? render json: { error: 'Your login is currently disabled' }, status: 403 else - set_user_activity + update_user_sign_in end end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index c1ea702ad..13d158c67 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -7,6 +7,7 @@ class Auth::SessionsController < Devise::SessionsController skip_before_action :require_no_authentication, only: [:create] skip_before_action :require_functional! + skip_before_action :update_user_sign_in include TwoFactorAuthenticationConcern include SignInTokenAuthenticationConcern @@ -24,6 +25,7 @@ class Auth::SessionsController < Devise::SessionsController def create super do |resource| + resource.update_sign_in!(request, new_sign_in: true) remember_me(resource) flash.delete(:notice) end @@ -57,7 +59,7 @@ class Auth::SessionsController < Devise::SessionsController def find_user if session[:attempt_user_id] - User.find(session[:attempt_user_id]) + User.find_by(id: 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 @@ -90,6 +92,7 @@ class Auth::SessionsController < Devise::SessionsController def require_no_authentication super + # Delete flash message that isn't entirely useful and may be confusing in # most cases because /web doesn't display/clear flash messages. flash.delete(:alert) if flash[:alert] == I18n.t('devise.failure.already_authenticated') @@ -107,13 +110,30 @@ class Auth::SessionsController < Devise::SessionsController def home_paths(resource) paths = [about_path] + if single_user_mode? && resource.is_a?(User) paths << short_account_path(username: resource.account) end + paths end def continue_after? truthy_param?(:continue) end + + def restart_session + clear_attempt_from_session + redirect_to new_user_session_path, alert: I18n.t('devise.failure.timeout') + end + + def set_attempt_session(user) + session[:attempt_user_id] = user.id + session[:attempt_user_updated_at] = user.updated_at.to_s + end + + def clear_attempt_from_session + session.delete(:attempt_user_id) + session.delete(:attempt_user_updated_at) + end end diff --git a/app/controllers/concerns/sign_in_token_authentication_concern.rb b/app/controllers/concerns/sign_in_token_authentication_concern.rb index 91f813acc..3c95a4afd 100644 --- a/app/controllers/concerns/sign_in_token_authentication_concern.rb +++ b/app/controllers/concerns/sign_in_token_authentication_concern.rb @@ -18,7 +18,9 @@ module SignInTokenAuthenticationConcern def authenticate_with_sign_in_token user = self.resource = find_user - if user_params[:sign_in_token_attempt].present? && session[:attempt_user_id] + if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s + restart_session + elsif user_params.key?(:sign_in_token_attempt) && 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) @@ -27,7 +29,7 @@ module SignInTokenAuthenticationConcern def authenticate_with_sign_in_token_attempt(user) if valid_sign_in_token_attempt?(user) - session.delete(:attempt_user_id) + clear_attempt_from_session remember_me(user) sign_in(user) else @@ -42,10 +44,10 @@ module SignInTokenAuthenticationConcern UserMailer.sign_in_token(user, request.remote_ip, request.user_agent, Time.now.utc.to_s).deliver_later! end - set_locale do - session[:attempt_user_id] = user.id - @body_classes = 'lighter' - render :sign_in_token - end + set_attempt_session(user) + + @body_classes = 'lighter' + + set_locale { 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 index 8a2a86a02..4d4ccf49c 100644 --- a/app/controllers/concerns/two_factor_authentication_concern.rb +++ b/app/controllers/concerns/two_factor_authentication_concern.rb @@ -37,9 +37,11 @@ module TwoFactorAuthenticationConcern def authenticate_with_two_factor user = self.resource = find_user - if user.webauthn_enabled? && user_params[:credential].present? && session[:attempt_user_id] + if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s + restart_session + elsif user.webauthn_enabled? && user_params.key?(:credential) && session[:attempt_user_id] authenticate_with_two_factor_via_webauthn(user) - elsif user_params[:otp_attempt].present? && session[:attempt_user_id] + elsif user_params.key?(:otp_attempt) && session[:attempt_user_id] authenticate_with_two_factor_via_otp(user) elsif user.present? && user.external_or_valid_password?(user_params[:password]) prompt_for_two_factor(user) @@ -50,7 +52,7 @@ module TwoFactorAuthenticationConcern webauthn_credential = WebAuthn::Credential.from_get(user_params[:credential]) if valid_webauthn_credential?(user, webauthn_credential) - session.delete(:attempt_user_id) + clear_attempt_from_session remember_me(user) sign_in(user) render json: { redirect_path: root_path }, status: :ok @@ -61,7 +63,7 @@ module TwoFactorAuthenticationConcern def authenticate_with_two_factor_via_otp(user) if valid_otp_attempt?(user) - session.delete(:attempt_user_id) + clear_attempt_from_session remember_me(user) sign_in(user) else @@ -71,16 +73,18 @@ module TwoFactorAuthenticationConcern end def prompt_for_two_factor(user) - set_locale do - session[:attempt_user_id] = user.id - @body_classes = 'lighter' - @webauthn_enabled = user.webauthn_enabled? - @scheme_type = if user.webauthn_enabled? && user_params[:otp_attempt].blank? - 'webauthn' - else - 'totp' - end - render :two_factor + set_attempt_session(user) + + @body_classes = 'lighter' + @webauthn_enabled = user.webauthn_enabled? + @scheme_type = begin + if user.webauthn_enabled? && user_params[:otp_attempt].blank? + 'webauthn' + else + 'totp' + end end + + set_locale { render :two_factor } end end diff --git a/app/controllers/concerns/user_tracking_concern.rb b/app/controllers/concerns/user_tracking_concern.rb index be10705fc..efda37fae 100644 --- a/app/controllers/concerns/user_tracking_concern.rb +++ b/app/controllers/concerns/user_tracking_concern.rb @@ -6,14 +6,13 @@ module UserTrackingConcern UPDATE_SIGN_IN_HOURS = 24 included do - before_action :set_user_activity + before_action :update_user_sign_in end private - def set_user_activity - return unless user_needs_sign_in_update? - current_user.update_tracked_fields!(request) + def update_user_sign_in + current_user.update_sign_in!(request) if user_needs_sign_in_update? end def user_needs_sign_in_update? diff --git a/app/models/user.rb b/app/models/user.rb index 7c8124fed..94fee999a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -63,7 +63,7 @@ class User < ApplicationRecord devise :two_factor_backupable, otp_number_of_backup_codes: 10 - devise :registerable, :recoverable, :rememberable, :trackable, :validatable, + devise :registerable, :recoverable, :rememberable, :validatable, :confirmable include Omniauthable @@ -165,6 +165,24 @@ class User < ApplicationRecord prepare_new_user! if new_user && approved? end + def update_sign_in!(request, new_sign_in: false) + old_current, new_current = current_sign_in_at, Time.now.utc + self.last_sign_in_at = old_current || new_current + self.current_sign_in_at = new_current + + old_current, new_current = current_sign_in_ip, request.remote_ip + self.last_sign_in_ip = old_current || new_current + self.current_sign_in_ip = new_current + + if new_sign_in + self.sign_in_count ||= 0 + self.sign_in_count += 1 + end + + save(validate: false) unless new_record? + prepare_returning_user! + end + def pending? !approved? end @@ -196,11 +214,6 @@ class User < ApplicationRecord prepare_new_user! end - def update_tracked_fields!(request) - super - prepare_returning_user! - end - def otp_enabled? otp_required_for_login end diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 8ad9e74fc..d3a9a11eb 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -219,7 +219,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: { attempt_user_id: user.id } + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end it 'redirects to home' do @@ -234,7 +234,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: { attempt_user_id: user.id } + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end it 'shows a login error' do @@ -248,7 +248,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: { attempt_user_id: user.id } + post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end it 'redirects to home' do @@ -262,7 +262,7 @@ RSpec.describe Auth::SessionsController, type: :controller do context 'using an invalid OTP' do before do - post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id } + post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end it 'shows a login error' do @@ -334,7 +334,7 @@ RSpec.describe Auth::SessionsController, type: :controller do before do @controller.session[:webauthn_challenge] = challenge - post :create, params: { user: { credential: fake_credential } }, session: { attempt_user_id: user.id } + post :create, params: { user: { credential: fake_credential } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end it 'instructs the browser to redirect to home' do @@ -383,7 +383,7 @@ RSpec.describe Auth::SessionsController, type: :controller do 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 } + 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 @@ -397,7 +397,7 @@ RSpec.describe Auth::SessionsController, type: :controller do 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 } + 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 -- cgit From aa10200e58ccb340b6384532ccdba6b7fbac037a Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 12 Nov 2020 23:05:24 +0100 Subject: Fix streaming API allowing connections to persist after access token invalidation (#15111) Fix #14816 --- app/lib/access_token_extension.rb | 17 ++++++++ app/models/session_activation.rb | 16 +++++--- config/application.rb | 1 + streaming/index.js | 82 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 app/lib/access_token_extension.rb diff --git a/app/lib/access_token_extension.rb b/app/lib/access_token_extension.rb new file mode 100644 index 000000000..3e184e775 --- /dev/null +++ b/app/lib/access_token_extension.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module AccessTokenExtension + extend ActiveSupport::Concern + + included do + after_commit :push_to_streaming_api + end + + def revoke(clock = Time) + update(revoked_at: clock.now.utc) + end + + def push_to_streaming_api + Redis.current.publish("timeline:access_token:#{id}", Oj.dump(event: :kill)) if revoked? || destroyed? + end +end diff --git a/app/models/session_activation.rb b/app/models/session_activation.rb index 34d25c83d..b0ce9d112 100644 --- a/app/models/session_activation.rb +++ b/app/models/session_activation.rb @@ -70,12 +70,16 @@ class SessionActivation < ApplicationRecord end def assign_access_token - superapp = Doorkeeper::Application.find_by(superapp: true) + self.access_token = Doorkeeper::AccessToken.create!(access_token_attributes) + end - self.access_token = Doorkeeper::AccessToken.create!(application_id: superapp&.id, - resource_owner_id: user_id, - scopes: 'read write follow', - expires_in: Doorkeeper.configuration.access_token_expires_in, - use_refresh_token: Doorkeeper.configuration.refresh_token_enabled?) + def access_token_attributes + { + application_id: Doorkeeper::Application.find_by(superapp: true)&.id, + resource_owner_id: user_id, + scopes: 'read write follow', + expires_in: Doorkeeper.configuration.access_token_expires_in, + use_refresh_token: Doorkeeper.configuration.refresh_token_enabled?, + } end end diff --git a/config/application.rb b/config/application.rb index de2951487..af7735221 100644 --- a/config/application.rb +++ b/config/application.rb @@ -140,6 +140,7 @@ module Mastodon Doorkeeper::AuthorizationsController.layout 'modal' Doorkeeper::AuthorizedApplicationsController.layout 'admin' Doorkeeper::Application.send :include, ApplicationExtension + Doorkeeper::AccessToken.send :include, AccessTokenExtension Devise::FailureApp.send :include, AbstractController::Callbacks Devise::FailureApp.send :include, HttpAcceptLanguage::EasyAccess Devise::FailureApp.send :include, Localized diff --git a/streaming/index.js b/streaming/index.js index 791a26941..877f45d19 100644 --- a/streaming/index.js +++ b/streaming/index.js @@ -294,7 +294,7 @@ const startWorker = (workerId) => { return; } - client.query('SELECT oauth_access_tokens.resource_owner_id, users.account_id, users.chosen_languages, oauth_access_tokens.scopes, devices.device_id FROM oauth_access_tokens INNER JOIN users ON oauth_access_tokens.resource_owner_id = users.id LEFT OUTER JOIN devices ON oauth_access_tokens.id = devices.access_token_id WHERE oauth_access_tokens.token = $1 AND oauth_access_tokens.revoked_at IS NULL LIMIT 1', [token], (err, result) => { + client.query('SELECT oauth_access_tokens.id, oauth_access_tokens.resource_owner_id, users.account_id, users.chosen_languages, oauth_access_tokens.scopes, devices.device_id FROM oauth_access_tokens INNER JOIN users ON oauth_access_tokens.resource_owner_id = users.id LEFT OUTER JOIN devices ON oauth_access_tokens.id = devices.access_token_id WHERE oauth_access_tokens.token = $1 AND oauth_access_tokens.revoked_at IS NULL LIMIT 1', [token], (err, result) => { done(); if (err) { @@ -310,6 +310,7 @@ const startWorker = (workerId) => { return; } + req.accessTokenId = result.rows[0].id; req.scopes = result.rows[0].scopes.split(' '); req.accountId = result.rows[0].account_id; req.chosenLanguages = result.rows[0].chosen_languages; @@ -450,6 +451,55 @@ const startWorker = (workerId) => { }); }; + /** + * @typedef SystemMessageHandlers + * @property {function(): void} onKill + */ + + /** + * @param {any} req + * @param {SystemMessageHandlers} eventHandlers + * @return {function(string): void} + */ + const createSystemMessageListener = (req, eventHandlers) => { + return message => { + const json = parseJSON(message); + + if (!json) return; + + const { event } = json; + + log.silly(req.requestId, `System message for ${req.accountId}: ${event}`); + + if (event === 'kill') { + log.verbose(req.requestId, `Closing connection for ${req.accountId} due to expired access token`); + eventHandlers.onKill(); + } + } + }; + + /** + * @param {any} req + * @param {any} res + */ + const subscribeHttpToSystemChannel = (req, res) => { + const systemChannelId = `timeline:access_token:${req.accessTokenId}`; + + const listener = createSystemMessageListener(req, { + + onKill () { + res.end(); + }, + + }); + + res.on('close', () => { + unsubscribe(`${redisPrefix}${systemChannelId}`, listener); + }); + + subscribe(`${redisPrefix}${systemChannelId}`, listener); + }; + /** * @param {any} req * @param {any} res @@ -462,6 +512,8 @@ const startWorker = (workerId) => { } accountFromRequest(req, alwaysRequireAuth).then(() => checkScopes(req, channelNameFromPath(req))).then(() => { + subscribeHttpToSystemChannel(req, res); + }).then(() => { next(); }).catch(err => { next(err); @@ -536,7 +588,9 @@ const startWorker = (workerId) => { const listener = message => { const json = parseJSON(message); + if (!json) return; + const { event, payload, queued_at } = json; const transmit = () => { @@ -902,6 +956,28 @@ const startWorker = (workerId) => { socket.send(JSON.stringify({ error: err.toString() })); }); + /** + * @param {WebSocketSession} session + */ + const subscribeWebsocketToSystemChannel = ({ socket, request, subscriptions }) => { + const systemChannelId = `timeline:access_token:${request.accessTokenId}`; + + const listener = createSystemMessageListener(request, { + + onKill () { + socket.close(); + }, + + }); + + subscribe(`${redisPrefix}${systemChannelId}`, listener); + + subscriptions[systemChannelId] = { + listener, + stopHeartbeat: () => {}, + }; + }; + /** * @param {string|string[]} arrayOrString * @return {string} @@ -948,7 +1024,9 @@ const startWorker = (workerId) => { ws.on('message', data => { const json = parseJSON(data); + if (!json) return; + const { type, stream, ...params } = json; if (type === 'subscribe') { @@ -960,6 +1038,8 @@ const startWorker = (workerId) => { } }); + subscribeWebsocketToSystemChannel(session); + if (location.query.stream) { subscribeWebsocketToChannel(session, firstParam(location.query.stream), location.query); } -- cgit From 0a4d0e8320ae9fc5c446828743008db3b45bcb13 Mon Sep 17 00:00:00 2001 From: ThibG Date: Fri, 13 Nov 2020 22:17:04 +0100 Subject: Fix AccountDeletionWorker not accepting keyword arguments (#15152) --- app/workers/account_deletion_worker.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/workers/account_deletion_worker.rb b/app/workers/account_deletion_worker.rb index b6016bf8c..81c3b91ad 100644 --- a/app/workers/account_deletion_worker.rb +++ b/app/workers/account_deletion_worker.rb @@ -5,7 +5,8 @@ class AccountDeletionWorker sidekiq_options queue: 'pull' - def perform(account_id, reserve_username: true) + def perform(account_id, options = {}) + reserve_username = options.with_indifferent_access.fetch(:reserve_username, true) DeleteAccountService.new.call(Account.find(account_id), reserve_username: reserve_username, reserve_email: false) rescue ActiveRecord::RecordNotFound true -- cgit