Fix authentication failures after going halfway through a sign-in attempt (#16607)
* Add tests * Add security-related tests My first (unpublished) attempt at fixing the issues introduced (extremely hard-to-exploit) security vulnerabilities, addressing them in a test. * Fix authentication failures after going halfway through a sign-in attempt * Refactor `authenticate_with_sign_in_token` and `authenticate_with_two_factor` to make the two authentication steps more obviousremotes/1727458204337373841/tmp_refs/heads/signup-info-prompt
parent
2ed1c92c63
commit
94bcf45321
|
@ -58,16 +58,20 @@ class Auth::SessionsController < Devise::SessionsController
|
|||
protected
|
||||
|
||||
def find_user
|
||||
if session[:attempt_user_id]
|
||||
if user_params[:email].present?
|
||||
find_user_from_params
|
||||
elsif 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
|
||||
user ||= User.find_for_authentication(email: user_params[:email])
|
||||
user
|
||||
end
|
||||
end
|
||||
|
||||
def find_user_from_params
|
||||
user = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication
|
||||
user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication
|
||||
user ||= User.find_for_authentication(email: user_params[:email])
|
||||
user
|
||||
end
|
||||
|
||||
def user_params
|
||||
params.require(:user).permit(:email, :password, :otp_attempt, :sign_in_token_attempt, credential: {})
|
||||
end
|
||||
|
|
|
@ -16,14 +16,18 @@ module SignInTokenAuthenticationConcern
|
|||
end
|
||||
|
||||
def authenticate_with_sign_in_token
|
||||
user = self.resource = find_user
|
||||
if user_params[:email].present?
|
||||
user = self.resource = find_user_from_params
|
||||
prompt_for_sign_in_token(user) if user&.external_or_valid_password?(user_params[:password])
|
||||
elsif session[:attempt_user_id]
|
||||
user = self.resource = User.find_by(id: session[:attempt_user_id])
|
||||
return if user.nil?
|
||||
|
||||
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)
|
||||
if session[:attempt_user_updated_at] != user.updated_at.to_s
|
||||
restart_session
|
||||
elsif user_params.key?(:sign_in_token_attempt)
|
||||
authenticate_with_sign_in_token_attempt(user)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -35,16 +35,20 @@ module TwoFactorAuthenticationConcern
|
|||
end
|
||||
|
||||
def authenticate_with_two_factor
|
||||
user = self.resource = find_user
|
||||
if user_params[:email].present?
|
||||
user = self.resource = find_user_from_params
|
||||
prompt_for_two_factor(user) if user&.external_or_valid_password?(user_params[:password])
|
||||
elsif session[:attempt_user_id]
|
||||
user = self.resource = User.find_by(id: session[:attempt_user_id])
|
||||
return if user.nil?
|
||||
|
||||
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.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)
|
||||
if session[:attempt_user_updated_at] != user.updated_at.to_s
|
||||
restart_session
|
||||
elsif user.webauthn_enabled? && user_params.key?(:credential)
|
||||
authenticate_with_two_factor_via_webauthn(user)
|
||||
elsif user_params.key?(:otp_attempt)
|
||||
authenticate_with_two_factor_via_otp(user)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -206,6 +206,38 @@ RSpec.describe Auth::SessionsController, type: :controller do
|
|||
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 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 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 } }
|
||||
|
@ -231,6 +263,21 @@ 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)
|
||||
|
@ -380,6 +427,52 @@ RSpec.describe Auth::SessionsController, type: :controller do
|
|||
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
|
||||
|
@ -395,6 +488,22 @@ RSpec.describe Auth::SessionsController, type: :controller do
|
|||
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 }
|
||||
|
|
Loading…
Reference in New Issue