Commit 18d316ae authored by Brian Williams's avatar Brian Williams Committed by Stan Hu

Add ttl expiration to user otp_secrets

Changelog: added
parent 87bf25e6
......@@ -4,6 +4,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
skip_before_action :check_two_factor_requirement
before_action :ensure_verified_primary_email, only: [:show, :create]
before_action :validate_current_password, only: [:create, :codes, :destroy], if: :current_password_required?
before_action :update_current_user_otp!, only: [:show]
helper_method :current_password_required?
......@@ -14,16 +15,6 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
feature_category :authentication_and_authorization
def show
unless current_user.two_factor_enabled?
current_user.otp_secret = User.generate_otp_secret(32)
end
unless current_user.otp_grace_period_started_at && two_factor_grace_period
current_user.otp_grace_period_started_at = Time.current
end
Users::UpdateService.new(current_user, user: current_user).execute!
if two_factor_authentication_required? && !current_user.two_factor_enabled?
two_factor_authentication_reason(
global: lambda do
......@@ -139,6 +130,18 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
private
def update_current_user_otp!
if current_user.needs_new_otp_secret?
current_user.update_otp_secret!
end
unless current_user.otp_grace_period_started_at && two_factor_grace_period
current_user.otp_grace_period_started_at = Time.current
end
Users::UpdateService.new(current_user, user: current_user).execute!
end
def validate_current_password
return if current_user.valid_password?(params[:current_password])
......
......@@ -37,6 +37,9 @@ class User < ApplicationRecord
COUNT_CACHE_VALIDITY_PERIOD = 24.hours
OTP_SECRET_LENGTH = 32
OTP_SECRET_TTL_LENGTH = 2.minutes
MAX_USERNAME_LENGTH = 255
MIN_USERNAME_LENGTH = 2
......@@ -68,6 +71,7 @@ class User < ApplicationRecord
mode: :per_attribute_iv_and_salt,
insecure_mode: true,
algorithm: 'aes-256-cbc'
attr_accessor :otp_secret_ttl
devise :two_factor_authenticatable,
otp_secret_encryption_key: Gitlab::Application.secrets.otp_key_base
......@@ -954,6 +958,21 @@ class User < ApplicationRecord
(webauthn_registrations.loaded? && webauthn_registrations.any?) || (!webauthn_registrations.loaded? && webauthn_registrations.exists?)
end
def needs_new_otp_secret?
!two_factor_enabled? && otp_secret_expired?
end
def otp_secret_expired?
return true unless otp_secret_ttl
otp_secret_ttl < Time.current
end
def update_otp_secret!
self.otp_secret = User.generate_otp_secret(OTP_SECRET_LENGTH)
self.otp_secret_ttl = Time.current + OTP_SECRET_TTL_LENGTH
end
def namespace_move_dir_allowed
if namespace&.any_project_has_container_registry_tags?
errors.add(:username, _('cannot be changed if a personal project has container registry tags.'))
......
# frozen_string_literal: true
class AddOtpSecretTtl < Gitlab::Database::Migration[1.0]
def change
# rubocop: disable Migration/AddColumnsToWideTables
add_column :users, :otp_secret_ttl, :datetime_with_timezone
# rubocop: enable Migration/AddColumnsToWideTables
end
end
efba00e36821c5ebe92ba39ad40dd165ab46c97b1b18becdec0d192470c2e8ca
\ No newline at end of file
......@@ -21480,6 +21480,7 @@ CREATE TABLE users (
role smallint,
user_type smallint,
static_object_token_encrypted text,
otp_secret_ttl timestamp with time zone,
CONSTRAINT check_7bde697e8e CHECK ((char_length(static_object_token_encrypted) <= 255))
);
......@@ -107,14 +107,26 @@ RSpec.describe Profiles::TwoFactorAuthsController do
expect(assigns[:qr_code]).to eq(code)
end
it 'generates a unique otp_secret every time the page is loaded' do
expect(User).to receive(:generate_otp_secret).with(32).and_call_original.twice
it 'generates a single otp_secret with multiple page loads', :freeze_time do
expect(User).to receive(:generate_otp_secret).with(32).and_call_original.once
user.update!(otp_secret: nil, otp_secret_ttl: nil)
2.times do
get :show
end
end
it 'generates a new otp_secret once the ttl has expired' do
expect(User).to receive(:generate_otp_secret).with(32).and_call_original.once
user.update!(otp_secret: "FT7KAVNU63YZH7PBRVPVL7CPSAENXY25", otp_secret_ttl: 2.minutes.from_now)
travel_to(10.minutes.from_now) do
get :show
end
end
it_behaves_like 'user must first verify their primary email address' do
let(:go) { get :show }
end
......
......@@ -2089,6 +2089,74 @@ RSpec.describe User do
end
end
describe 'needs_new_otp_secret?', :freeze_time do
let(:user) { create(:user) }
context 'when two-factor is not enabled' do
it 'returns true if otp_secret_ttl is nil' do
expect(user.needs_new_otp_secret?).to eq(true)
end
it 'returns true if the otp_secret_ttl has passed' do
user.update!(otp_secret_ttl: 10.minutes.ago)
expect(user.reload.needs_new_otp_secret?).to eq(true)
end
it 'returns false if the otp_secret_ttl has not passed' do
user.update!(otp_secret_ttl: 10.minutes.from_now)
expect(user.reload.needs_new_otp_secret?).to eq(false)
end
end
context 'when two-factor is enabled' do
let(:user) { create(:user, :two_factor) }
it 'returns false even if ttl is expired' do
user.otp_secret_ttl = 10.minutes.ago
expect(user.needs_new_otp_secret?).to eq(false)
end
end
end
describe 'otp_secret_expired?', :freeze_time do
let(:user) { create(:user) }
it 'returns true if otp_secret_ttl is nil' do
expect(user.otp_secret_expired?).to eq(true)
end
it 'returns true if the otp_secret_ttl has passed' do
user.otp_secret_ttl = 10.minutes.ago
expect(user.otp_secret_expired?).to eq(true)
end
it 'returns false if the otp_secret_ttl has not passed' do
user.otp_secret_ttl = 20.minutes.from_now
expect(user.otp_secret_expired?).to eq(false)
end
end
describe 'update_otp_secret!', :freeze_time do
let(:user) { create(:user) }
before do
user.update_otp_secret!
end
it 'sets the otp_secret' do
expect(user.otp_secret).to have_attributes(length: described_class::OTP_SECRET_LENGTH)
end
it 'updates the otp_secret_ttl' do
expect(user.otp_secret_ttl).to eq(Time.current + described_class::OTP_SECRET_TTL_LENGTH)
end
end
describe 'projects' do
before do
@user = create(:user)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment