diff --git a/app/graphql/mutations/users/mfa/totp/generate_secret.rb b/app/graphql/mutations/users/mfa/totp/generate_secret.rb index 9bd83e316..7a54624f2 100644 --- a/app/graphql/mutations/users/mfa/totp/generate_secret.rb +++ b/app/graphql/mutations/users/mfa/totp/generate_secret.rb @@ -7,11 +7,19 @@ module Totp class GenerateSecret < BaseMutation description 'Generates an encrypted totp secret' - field :secret, String, null: true, description: 'The created and signed secret' + field :secret, String, null: true, description: 'The created secret' + field :signed_secret, String, null: true, description: 'The created and signed secret' def resolve - ::Users::Mfa::Totp::GenerateSecretService.new(current_authentication).execute - .to_mutation_response(success_key: :secret) + response = ::Users::Mfa::Totp::GenerateSecretService.new(current_authentication).execute + + return response.to_mutation_response unless response.success? + + { + secret: response.payload[:secret], + signed_secret: response.payload[:signed_secret], + errors: [], + } end end end diff --git a/app/models/user.rb b/app/models/user.rb index 63093bd4a..9169d7b29 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -24,6 +24,9 @@ class User < ApplicationRecord validates :firstname, length: { maximum: 50 } validates :lastname, length: { maximum: 50 } + + encrypts :totp_secret + validates :totp_secret, length: { maximum: 32 } has_many :backup_codes, inverse_of: :user diff --git a/app/services/users/mfa/totp/generate_secret_service.rb b/app/services/users/mfa/totp/generate_secret_service.rb index 3c7d7f29f..ac5a17e2c 100644 --- a/app/services/users/mfa/totp/generate_secret_service.rb +++ b/app/services/users/mfa/totp/generate_secret_service.rb @@ -22,8 +22,12 @@ def execute totp_secret = ROTP::Base32.random ServiceResponse.success(message: 'TOTP secret generated', - payload: Rails.application.message_verifier(:totp_secret) - .generate(totp_secret)) + payload: { + signed_secret: Rails.application + .message_verifier(:totp_secret) + .generate(totp_secret, expires_in: 30.minutes), + secret: totp_secret, + }) end end end diff --git a/db/migrate/20260726000000_encrypt_totp_secret_on_users.rb b/db/migrate/20260726000000_encrypt_totp_secret_on_users.rb new file mode 100644 index 000000000..36c72dd0f --- /dev/null +++ b/db/migrate/20260726000000_encrypt_totp_secret_on_users.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class EncryptTotpSecretOnUsers < Code0::ZeroTrack::Database::Migration[1.0] + def up + remove_index :users, :totp_secret, name: 'index_users_on_totp_secret' + end + + def down + add_index :users, :totp_secret, unique: true, where: 'totp_secret IS NOT NULL', + name: 'index_users_on_totp_secret' + end +end diff --git a/db/schema_migrations/20260726000000 b/db/schema_migrations/20260726000000 new file mode 100644 index 000000000..9f3f21ff0 --- /dev/null +++ b/db/schema_migrations/20260726000000 @@ -0,0 +1 @@ +3152386744f06e994087067a6e0367481df2ba73f4cff9a67401074b691f3833 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index d67162bf9..69a8f0916 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1703,8 +1703,6 @@ CREATE UNIQUE INDEX "index_users_on_LOWER_email" ON users USING btree (lower(ema CREATE UNIQUE INDEX "index_users_on_LOWER_username" ON users USING btree (lower(username)); -CREATE UNIQUE INDEX index_users_on_totp_secret ON users USING btree (totp_secret) WHERE (totp_secret IS NOT NULL); - ALTER TABLE ONLY node_parameters ADD CONSTRAINT fk_rails_0d79310cfa FOREIGN KEY (node_function_id) REFERENCES node_functions(id) ON DELETE CASCADE; diff --git a/docs/graphql/mutation/usersmfatotpgeneratesecret.md b/docs/graphql/mutation/usersmfatotpgeneratesecret.md index 173b77e05..d15c4ec48 100644 --- a/docs/graphql/mutation/usersmfatotpgeneratesecret.md +++ b/docs/graphql/mutation/usersmfatotpgeneratesecret.md @@ -16,4 +16,5 @@ Generates an encrypted totp secret |------|------|-------------| | `clientMutationId` | [`String`](../scalar/string.md) | A unique identifier for the client performing the mutation. | | `errors` | [`[Error!]!`](../object/error.md) | Errors encountered during execution of the mutation. | -| `secret` | [`String`](../scalar/string.md) | The created and signed secret | +| `secret` | [`String`](../scalar/string.md) | The created secret | +| `signedSecret` | [`String`](../scalar/string.md) | The created and signed secret | diff --git a/spec/requests/graphql/mutation/users/mfa/totp/generate_secret_mutation_spec.rb b/spec/requests/graphql/mutation/users/mfa/totp/generate_secret_mutation_spec.rb index b4706fc4a..c406c8976 100644 --- a/spec/requests/graphql/mutation/users/mfa/totp/generate_secret_mutation_spec.rb +++ b/spec/requests/graphql/mutation/users/mfa/totp/generate_secret_mutation_spec.rb @@ -13,6 +13,7 @@ usersMfaTotpGenerateSecret(input: {}) { #{error_query} secret + signedSecret } } QUERY @@ -25,7 +26,13 @@ it 'generates secret' do mutate! - expect(graphql_data_at(:users_mfa_totp_generate_secret, :secret)).to be_present + secret = graphql_data_at(:users_mfa_totp_generate_secret, :secret) + signed_secret = graphql_data_at(:users_mfa_totp_generate_secret, :signed_secret) + + expect(secret).to be_present + + signed_totp = Rails.application.message_verifier(:totp_secret).verified(signed_secret) + expect(signed_totp).to eq(secret) end end end diff --git a/spec/requests/graphql/mutation/users/mfa/totp/validate_secret_mutation_spec.rb b/spec/requests/graphql/mutation/users/mfa/totp/validate_secret_mutation_spec.rb index 6d71bfcdf..593138d4e 100644 --- a/spec/requests/graphql/mutation/users/mfa/totp/validate_secret_mutation_spec.rb +++ b/spec/requests/graphql/mutation/users/mfa/totp/validate_secret_mutation_spec.rb @@ -33,7 +33,7 @@ context 'when user is valid' do let(:current_user) { create(:user) } let(:secret) { ROTP::Base32.random } - let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret) } + let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret, expires_in: 30.minutes) } let(:current_totp) { ROTP::TOTP.new(secret).now } it 'validates secret and enables totp' do diff --git a/spec/services/users/mfa/totp/generate_secret_service_spec.rb b/spec/services/users/mfa/totp/generate_secret_service_spec.rb index 0a8fb9605..f0b6076ab 100644 --- a/spec/services/users/mfa/totp/generate_secret_service_spec.rb +++ b/spec/services/users/mfa/totp/generate_secret_service_spec.rb @@ -21,13 +21,31 @@ end context 'when totp secret is not set' do + include ActiveSupport::Testing::TimeHelpers + let(:current_user) { create(:user) } it { is_expected.to be_success } - it 'is valid totp secret' do - totp = ROTP::TOTP.new(service_response.payload.split('--').first) - expect(totp.secret.length).to eq(48) + it 'returns a valid totp secret' do + secret = service_response.payload[:secret] + totp = ROTP::TOTP.new(secret) + expect(totp.now).to be_present + end + + it 'returns a matching secret' do + signed_secret = Rails.application.message_verifier(:totp_secret).verified( + service_response.payload[:signed_secret] + ) + expect(signed_secret).to eq(service_response.payload[:secret]) + end + + it 'returns a signed secret that expires' do + signed_secret = service_response.payload[:signed_secret] + + travel_to 31.minutes.from_now do + expect(Rails.application.message_verifier(:totp_secret).verified(signed_secret)).to be_nil + end end end end diff --git a/spec/services/users/mfa/totp/validate_secret_service_spec.rb b/spec/services/users/mfa/totp/validate_secret_service_spec.rb index 045e935c9..6e21dceb4 100644 --- a/spec/services/users/mfa/totp/validate_secret_service_spec.rb +++ b/spec/services/users/mfa/totp/validate_secret_service_spec.rb @@ -28,17 +28,37 @@ context 'when user and secret is valid but totp is not' do let(:current_user) { create(:user) } let(:secret) { ROTP::Base32.random } - let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret) } + let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret, expires_in: 30.minutes) } let(:current_totp) { '00000' } it { is_expected.not_to be_success } it { is_expected.not_to create_audit_event } end + context 'when signed secret has expired' do + include ActiveSupport::Testing::TimeHelpers + + # rubocop:disable RSpec/LetSetup -- referenced in the subject + let!(:current_user) { create(:user) } + let!(:secret) { ROTP::Base32.random } + let!(:signed_secret) do + Rails.application.message_verifier(:totp_secret).generate(secret, expires_in: 30.minutes) + end + let!(:current_totp) { ROTP::TOTP.new(secret).now } + # rubocop:enable RSpec/LetSetup + + it 'returns invalid_totp_secret error' do + travel_to 31.minutes.from_now do + expect(service_response).not_to be_success + expect(service_response.payload[:error_code]).to eq(:invalid_totp_secret) + end + end + end + context 'when user is valid and secret is valid and totp is valid' do let(:current_user) { create(:user) } let(:secret) { ROTP::Base32.random } - let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret) } + let(:signed_secret) { Rails.application.message_verifier(:totp_secret).generate(secret, expires_in: 30.minutes) } let(:current_totp) { ROTP::TOTP.new(secret).now } it { is_expected.to be_success }