Refactor config/initializers/01_secret_token.rb and fix tests

Signed-off-by: Rémy Coutable <remy@rymai.me>

config/initializers/01_secret_token.rb

@@ -10,30 +10,41 @@
 
 require 'securerandom'
 
-def create_tokens
-  # Inspired by https://github.com/rails/rails/blob/v7.0.8.4/railties/lib/rails/secrets.rb#L25-L36
-  raw_secrets = begin
-    YAML.safe_load(File.read(Rails.root.join('config/secrets.yml')))
-  rescue Errno::ENOENT, Psych::SyntaxError
-    {}
-  end
-  raw_secrets ||= {}
+def rails_secrets_config_file
+  Rails.root.join('config/secrets.yml')
+end
 
-  secrets = {}
-  secrets.merge!(raw_secrets["shared"].deep_symbolize_keys) if raw_secrets["shared"]
-  secrets.merge!(raw_secrets[Rails.env].deep_symbolize_keys) if raw_secrets[Rails.env]
+def load_secrets_from_file
+  YAML.safe_load_file(rails_secrets_config_file)
+rescue Errno::ENOENT, Psych::SyntaxError
+  {}
+end
 
-  # Copy secrets into credentials since Rails.application.secrets is populated from config/secrets.yml
-  # Later, once config/secrets.yml won't be read automatically, we'll need to do it manually, and set
+def set_credentials_from_file_and_env!
+  # Inspired by https://github.com/rails/rails/blob/v7.0.8.4/railties/lib/rails/secrets.rb#L25-L36
+  # Later, once config/secrets.yml won't be read automatically, we'll need to do it manually, so
+  # we anticipate and do it ourselves here.
+  file_secrets = load_secrets_from_file
+  secrets = file_secrets.fetch("shared", {}).deep_symbolize_keys
+    .merge(file_secrets.fetch(Rails.env, {}).deep_symbolize_keys)
+
+  # Copy secrets from config/secrets.yml into Rails.application.credentials
+  # If we support native Rails.application.credentials later
+  # (e.g. config.credentials.yml.enc + config/master.key ), this loop would
+  # become a no-op as long as credentials are migrated to config.credentials.yml.enc.
   secrets.each do |key, value|
+    next if Rails.application.credentials.public_send(key).present?
+
     Rails.application.credentials[key] = value
   end
 
-  # Historically, ENV['SECRET_KEY_BASE'] takes precedence over secrets.yml, so we maintain that
-  # behavior by ensuring the environment variable always overrides secrets.yml.
+  # Historically, ENV['SECRET_KEY_BASE'] takes precedence over config/secrets.yml, so we maintain that
+  # behavior by ensuring the environment variable always overrides the value from config/secrets.yml.
   env_secret_key = ENV['SECRET_KEY_BASE']
   Rails.application.credentials.secret_key_base = env_secret_key if env_secret_key.present?
+end
 
+def set_missing_from_defaults!
   defaults = {
     secret_key_base: generate_new_secure_token,
     otp_key_base: generate_new_secure_token,
@@ -43,12 +54,16 @@ def create_tokens
 
   # encrypted_settings_key_base is optional for now
   if ENV['GITLAB_GENERATE_ENCRYPTED_SETTINGS_KEY_BASE']
-    defaults[:encrypted_settings_key_base] =
-      generate_new_secure_token
+    defaults[:encrypted_settings_key_base] = generate_new_secure_token
   end
 
   missing_secrets = set_missing_keys(defaults)
-  write_secrets_yml(missing_secrets) unless missing_secrets.empty?
+  write_secrets_yml!(missing_secrets) if missing_secrets.any?
+end
+
+def create_tokens
+  set_credentials_from_file_and_env!
+  set_missing_from_defaults!
 end
 
 def generate_new_secure_token
@@ -62,7 +77,8 @@ def generate_new_rsa_private_key
 def warn_missing_secret(secret)
   return if Rails.env.test?
 
-  warn "Missing Rails.application.credentials.#{secret} for #{Rails.env} environment. The secret will be generated and stored in config/secrets.yml."
+  warn "Missing Rails.application.credentials.#{secret} for #{Rails.env} environment. " \
+    "The secret will be generated and stored in config/secrets.yml."
 end
 
 def set_missing_keys(defaults)
@@ -74,15 +90,12 @@ def set_missing_keys(defaults)
   end
 end
 
-def write_secrets_yml(missing_secrets)
-  secrets_yml = Rails.root.join('config/secrets.yml')
+def write_secrets_yml!(missing_secrets)
   rails_env = Rails.env.to_s
-  secrets = YAML.load_file(secrets_yml) if File.exist?(secrets_yml)
-  secrets ||= {}
+  secrets = load_secrets_from_file
   secrets[rails_env] ||= {}
-
   secrets[rails_env].merge!(missing_secrets)
-  File.write(secrets_yml, YAML.dump(secrets), mode: 'w', perm: 0o600)
+  File.write(rails_secrets_config_file, YAML.dump(secrets), mode: 'w', perm: 0o600)
 end
 
 create_tokens

spec/initializers/secret_token_spec.rb

@@ -6,18 +6,32 @@
 RSpec.describe 'create_tokens' do
   include StubENV
 
-  let(:secrets) { ActiveSupport::OrderedOptions.new }
+  let(:allowed_keys) do
+    %w[
+      secret_key_base
+      db_key_base
+      otp_key_base
+      openid_connect_signing_key
+    ]
+  end
+
   let(:hex_key) { /\h{128}/ }
   let(:rsa_key) { /\A-----BEGIN RSA PRIVATE KEY-----\n.+\n-----END RSA PRIVATE KEY-----\n\Z/m }
 
+  around do |example|
+    original_credentials = Rails.application.credentials
+    # ensure we clear any existing `encrypted_settings_key_base` credential
+    allowed_keys.each do |key|
+      Rails.application.credentials.public_send(:"#{key}=", nil)
+    end
+    example.run
+    Rails.application.credentials = original_credentials
+  end
+
   before do
-    allow(Rails).to receive_message_chain(:application, :credentials).and_return(secrets)
     allow(Rails).to receive_message_chain(:root, :join) { |string| string }
-
     allow(File).to receive(:write).and_call_original
-    allow(File).to receive(:write).with(Rails.root.join('config/secrets.yml'))
-    allow(self).to receive(:warn)
-    allow(self).to receive(:exit)
+    allow(File).to receive(:write).with('config/secrets.yml')
   end
 
   describe 'ensure acknowledged secrets in any installations' do
@@ -45,182 +59,177 @@
     end
   end
 
-  context 'setting secret keys' do
-    context 'when none of the secrets exist' do
-      before do
-        stub_env('SECRET_KEY_BASE', nil)
-        allow(File).to receive(:exist?).with('config/secrets.yml').and_return(false)
-        allow(self).to receive(:warn_missing_secret)
+  context 'when none of the secrets exist' do
+    before do
+      # ensure we clear any existing `encrypted_settings_key_base` credential
+      allowed_keys.each do |key|
+        Rails.application.credentials.public_send(:"#{key}=", nil)
       end
 
-      it 'generates different hashes for secret_key_base, otp_key_base, and db_key_base' do
-        create_tokens
+      allow(self).to receive(:load_secrets_from_file).and_return({})
+      stub_env('SECRET_KEY_BASE', nil)
+    end
 
-        keys = secrets.values_at(:secret_key_base, :otp_key_base, :db_key_base)
+    it 'generates different hashes for secret_key_base, otp_key_base, and db_key_base' do
+      create_tokens
 
-        expect(keys.uniq).to eq(keys)
-        expect(keys).to all(match(hex_key))
-      end
+      keys = Rails.application.credentials.values_at(:secret_key_base, :otp_key_base, :db_key_base)
 
-      it 'generates an RSA key for openid_connect_signing_key' do
-        create_tokens
+      expect(keys.uniq).to eq(keys)
+      expect(keys).to all(match(hex_key))
+    end
 
-        keys = secrets.values_at(:openid_connect_signing_key)
+    it 'generates an RSA key for openid_connect_signing_key' do
+      create_tokens
 
-        expect(keys.uniq).to eq(keys)
-        expect(keys).to all(match(rsa_key))
-      end
+      keys = Rails.application.credentials.values_at(:openid_connect_signing_key)
 
-      it 'warns about the secrets to add to secrets.yml' do
-        expect(self).to receive(:warn_missing_secret).with('secret_key_base')
-        expect(self).to receive(:warn_missing_secret).with('otp_key_base')
-        expect(self).to receive(:warn_missing_secret).with('db_key_base')
-        expect(self).to receive(:warn_missing_secret).with('openid_connect_signing_key')
+      expect(keys.uniq).to eq(keys)
+      expect(keys).to all(match(rsa_key))
+    end
 
-        create_tokens
+    it 'warns about the secrets to add to secrets.yml' do
+      allowed_keys.each do |key|
+        expect(self).to receive(:warn_missing_secret).with(key)
       end
 
-      it 'writes the secrets to secrets.yml' do
-        expect(File).to receive(:write).with('config/secrets.yml', any_args) do |filename, contents, options|
-          new_secrets = YAML.safe_load(contents)[Rails.env]
+      create_tokens
+    end
+
+    it 'writes the secrets to secrets.yml' do
+      expect(File).to receive(:write).with('config/secrets.yml', any_args) do |_filename, contents, _options|
+        new_secrets = YAML.safe_load(contents)['test']
 
-          expect(new_secrets['secret_key_base']).to eq(secrets.secret_key_base)
-          expect(new_secrets['otp_key_base']).to eq(secrets.otp_key_base)
-          expect(new_secrets['db_key_base']).to eq(secrets.db_key_base)
-          expect(new_secrets['openid_connect_signing_key']).to eq(secrets.openid_connect_signing_key)
-          expect(new_secrets['encrypted_settings_key_base']).to eq(secrets.encrypted_settings_key_base)
+        allowed_keys.each do |key|
+          expect(new_secrets[key]).to eq(Rails.application.credentials.values_at(key.to_sym).first)
         end
-
-        create_tokens
+        expect(new_secrets['encrypted_settings_key_base']).to be_nil # encrypted_settings_key_base is optional
       end
+
+      create_tokens
     end
 
-    context 'when the other secrets all exist' do
-      before do
-        secrets.db_key_base = 'db_key_base'
-        secrets.openid_connect_signing_key = 'openid_connect_signing_key'
-        secrets.encrypted_settings_key_base = 'encrypted_settings_key_base'
+    context 'when GITLAB_GENERATE_ENCRYPTED_SETTINGS_KEY_BASE is set' do
+      let(:allowed_keys) do
+        super() + ['encrypted_settings_key_base']
       end
 
-      context 'when secret_key_base exists in the environment and secrets.yml' do
-        before do
-          stub_env('SECRET_KEY_BASE', 'env_key')
-          secrets.secret_key_base = 'secret_key_base'
-          secrets.otp_key_base = 'otp_key_base'
-          secrets.openid_connect_signing_key = 'openid_connect_signing_key'
-        end
+      before do
+        stub_env('GITLAB_GENERATE_ENCRYPTED_SETTINGS_KEY_BASE', '1')
+        allow(self).to receive(:warn_missing_secret)
+      end
 
-        it 'does not issue a warning' do
-          expect(self).not_to receive(:warn)
+      it 'writes the encrypted_settings_key_base secret' do
+        expect(self).to receive(:warn_missing_secret).with('encrypted_settings_key_base')
+        expect(File).to receive(:write).with('config/secrets.yml', any_args) do |_filename, contents, _options|
+          new_secrets = YAML.safe_load(contents)['test']
 
-          create_tokens
+          expect(new_secrets['encrypted_settings_key_base']).to eq(Rails.application.credentials.encrypted_settings_key_base)
         end
 
-        it 'uses the environment variable' do
-          create_tokens
+        create_tokens
+      end
+    end
+  end
 
-          expect(secrets.secret_key_base).to eq('env_key')
-        end
+  shared_examples 'credentials are properly set' do
+    it 'sets Rails.application.credentials' do
+      create_tokens
 
-        it 'does not update secrets.yml' do
-          expect(File).not_to receive(:write)
+      expect(Rails.application.credentials.values_at(*allowed_keys.map(&:to_sym))).to eq(allowed_keys)
+    end
 
-          create_tokens
-        end
-      end
+    it 'does not issue warnings' do
+      expect(self).not_to receive(:warn_missing_secret)
 
-      context 'when secret_key_base and otp_key_base exist' do
-        before do
-          secrets.secret_key_base = 'secret_key_base'
-          secrets.otp_key_base = 'otp_key_base'
-          secrets.openid_connect_signing_key = 'openid_connect_signing_key'
-        end
+      create_tokens
+    end
 
-        it 'does not write any files' do
-          expect(File).not_to receive(:write)
+    it 'does not update secrets.yml' do
+      expect(File).not_to receive(:write)
 
-          create_tokens
-        end
+      create_tokens
+    end
+  end
 
-        it 'sets the keys to the values from the environment and secrets.yml' do
-          create_tokens
+  context 'when secrets exist in secrets.yml' do
+    let(:credentials) do
+      Hash[allowed_keys.zip(allowed_keys)]
+    end
 
-          expect(secrets.secret_key_base).to eq('secret_key_base')
-          expect(secrets.otp_key_base).to eq('otp_key_base')
-          expect(secrets.db_key_base).to eq('db_key_base')
-          expect(secrets.openid_connect_signing_key).to eq('openid_connect_signing_key')
-          expect(secrets.encrypted_settings_key_base).to eq('encrypted_settings_key_base')
-        end
+    before do
+      # ensure we clear any existing `encrypted_settings_key_base` credential
+      allowed_keys.each do |key|
+        Rails.application.credentials.public_send(:"#{key}=", nil)
       end
 
-      context 'when secret_key_base and otp_key_base do not exist' do
-        before do
-          allow(File).to receive(:exist?).with('config/secrets.yml').and_return(true)
-          allow(YAML).to receive(:load_file).with('config/secrets.yml').and_return('test' => secrets.to_h.stringify_keys)
-          allow(self).to receive(:warn_missing_secret)
-        end
+      allow(self).to receive(:load_secrets_from_file).and_return({
+        'test' => credentials
+      })
+    end
 
-        it 'keeps the other secrets as they were' do
-          create_tokens
+    it_behaves_like 'credentials are properly set'
 
-          expect(secrets.db_key_base).to eq('db_key_base')
-        end
+    context 'when secret_key_base also exist in the environment variable' do
+      before do
+        stub_env('SECRET_KEY_BASE', 'env_key')
+      end
 
-        it 'warns about the missing secrets' do
-          expect(self).to receive(:warn_missing_secret).with('secret_key_base')
-          expect(self).to receive(:warn_missing_secret).with('otp_key_base')
+      it 'sets Rails.application.credentials.secret_key_base from the environment variable' do
+        create_tokens
 
-          create_tokens
-        end
+        expect(Rails.application.credentials.secret_key_base).to eq('env_key')
       end
+    end
+  end
 
-      context 'when rotated_encrypted_settings_key_base does not exist' do
-        before do
-          secrets.secret_key_base = 'secret_key_base'
-          secrets.otp_key_base = 'otp_key_base'
-          secrets.openid_connect_signing_key = 'openid_connect_signing_key'
-          secrets.encrypted_settings_key_base = 'encrypted_settings_key_base'
-        end
+  context 'when secrets exist in Rails.application.credentials' do
+    before do
+      allowed_keys.each do |key|
+        Rails.application.credentials.public_send(:"#{key}=", key)
+      end
+    end
 
-        it 'does not warn about the missing secrets' do
-          expect(self).not_to receive(:warn_missing_secret).with('rotated_encrypted_settings_key_base')
+    it_behaves_like 'credentials are properly set'
 
-          create_tokens
-        end
+    context 'when secret_key_base also exist in the environment variable' do
+      before do
+        stub_env('SECRET_KEY_BASE', 'env_key')
+      end
 
-        it 'does not update secrets.yml' do
-          expect(File).not_to receive(:write)
+      it 'sets Rails.application.credentials.secret_key_base from the environment variable' do
+        create_tokens
 
-          create_tokens
-        end
+        expect(Rails.application.credentials.secret_key_base).to eq('env_key')
       end
     end
+  end
 
-    context 'when db_key_base is blank but exists in secrets.yml' do
-      before do
-        secrets.otp_key_base = 'otp_key_base'
-        secrets.secret_key_base = 'secret_key_base'
-        secrets.encrypted_settings_key_base = 'encrypted_settings_key_base'
-        yaml_secrets = secrets.to_h.stringify_keys.merge('db_key_base' => '<%= an_erb_expression %>')
-
-        allow(File).to receive(:exist?).with('.secret').and_return(false)
-        allow(File).to receive(:exist?).with('config/secrets.yml').and_return(true)
-        allow(YAML).to receive(:load_file).with('config/secrets.yml').and_return('test' => yaml_secrets)
-        allow(self).to receive(:warn_missing_secret)
-      end
+  context 'some secrets miss, some are in env, some are in Rails.application.credentials, and some are in secrets.yml' do
+    before do
+      stub_env('SECRET_KEY_BASE', 'env_key')
 
-      it 'warns about updating db_key_base' do
-        expect(self).to receive(:warn_missing_secret).with('db_key_base')
+      Rails.application.credentials.db_key_base = 'db_key_base'
 
-        create_tokens
-      end
+      allow(self).to receive(:load_secrets_from_file).and_return({
+        'test' => { 'otp_key_base' => 'otp_key_base' }
+      })
+    end
 
-      it 'does not update secrets.yml' do
-        expect(self).to receive(:exit).with(1).and_call_original
-        expect(File).not_to receive(:write)
+    it 'sets Rails.application.credentials properly, issue a warning and writes config.secrets.yml' do
+      expect(self).to receive(:warn_missing_secret).with('openid_connect_signing_key')
+      expect(File).to receive(:write).with('config/secrets.yml', any_args) do |_filename, contents, _options|
+        new_secrets = YAML.safe_load(contents)['test']
 
-        expect { create_tokens }.to raise_error(SystemExit)
+        expect(new_secrets['otp_key_base']).to eq('otp_key_base')
+        expect(new_secrets['openid_connect_signing_key']).to match(rsa_key)
       end
+
+      create_tokens
+
+      expect(Rails.application.credentials.secret_key_base).to eq('env_key')
+      expect(Rails.application.credentials.db_key_base).to eq('db_key_base')
+      expect(Rails.application.credentials.otp_key_base).to eq('otp_key_base')
     end
   end
 end