Merge branch 'bwill/add-otp-secret-ttl' into 'master'

Add expiration time to user otp_secrets

See merge request gitlab-org/gitlab!84985

app/controllers/profiles/two_factor_auths_controller.rb

@@ -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])
 

app/models/user.rb

@@ -37,6 +37,9 @@ class User < ApplicationRecord
 
   COUNT_CACHE_VALIDITY_PERIOD = 24.hours
 
+  OTP_SECRET_LENGTH = 32
+  OTP_SECRET_TTL = 2.minutes
+
   MAX_USERNAME_LENGTH = 255
   MIN_USERNAME_LENGTH = 2
 
@@ -954,6 +957,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_expires_at
+
+    otp_secret_expires_at < Time.current
+  end
+
+  def update_otp_secret!
+    self.otp_secret = User.generate_otp_secret(OTP_SECRET_LENGTH)
+    self.otp_secret_expires_at = Time.current + OTP_SECRET_TTL
+  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.'))

db/migrate/20220412171810_add_otp_secret_expires_at.rb

+# frozen_string_literal: true
+
+class AddOtpSecretExpiresAt < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    # rubocop: disable Migration/AddColumnsToWideTables
+    add_column :users, :otp_secret_expires_at, :datetime_with_timezone
+    # rubocop: enable Migration/AddColumnsToWideTables
+  end
+end

db/schema_migrations/20220412171810

+efba00e36821c5ebe92ba39ad40dd165ab46c97b1b18becdec0d192470c2e8ca
\ No newline at end of file

db/structure.sql

@@ -21480,6 +21480,7 @@ CREATE TABLE users (
     role smallint,
     user_type smallint,
     static_object_token_encrypted text,
+    otp_secret_expires_at timestamp with time zone,
     CONSTRAINT check_7bde697e8e CHECK ((char_length(static_object_token_encrypted) <= 255))
 );
 

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -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_expires_at: 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_expires_at: 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

spec/models/user_spec.rb

@@ -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_expires_at is nil' do
+        expect(user.needs_new_otp_secret?).to eq(true)
+      end
+
+      it 'returns true if the otp_secret_expires_at has passed' do
+        user.update!(otp_secret_expires_at: 10.minutes.ago)
+
+        expect(user.reload.needs_new_otp_secret?).to eq(true)
+      end
+
+      it 'returns false if the otp_secret_expires_at has not passed' do
+        user.update!(otp_secret_expires_at: 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_expires_at = 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_expires_at is nil' do
+      expect(user.otp_secret_expired?).to eq(true)
+    end
+
+    it 'returns true if the otp_secret_expires_at has passed' do
+      user.otp_secret_expires_at = 10.minutes.ago
+
+      expect(user.otp_secret_expired?).to eq(true)
+    end
+
+    it 'returns false if the otp_secret_expires_at has not passed' do
+      user.otp_secret_expires_at = 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_expires_at' do
+      expect(user.otp_secret_expires_at).to eq(Time.current + described_class::OTP_SECRET_TTL)
+    end
+  end
+
   describe 'projects' do
     before do
       @user = create(:user)