Merge pull request #1164 from code0-tech/improve-totp-query

Improve usability of totp generate secret mutation

Author: Taucher2003

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

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

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

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

db/schema_migrations/20260726000000

@@ -0,0 +1 @@
+3152386744f06e994087067a6e0367481df2ba73f4cff9a67401074b691f3833

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;
 

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 |

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

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

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