Wrap active_record_encryption.configuration on an after initialize #48520

duduribeiro · 2023-06-19T15:32:17Z

Motivation / Background

This commit changes the active_record_encryption.configuration to be set on an after initialize block. After PR #44873, we can configure now hash_digest_class that will be used on ActiveRecord::Encryption. But the docs said that we could change it via an initializer by setting
Rails.application.config.active_record.encryption.hash_digest_class. This wasn't working because the railtie
"active_record_encryption.configuration" was being set before the initializer was loaded. This changes this behave to configure the active_record encryption after the initialize was loaded.

Additional information

I found this while debugging #48204 and I noticed that the PR #44873 added an option to configure the hash_class used to encrypt the attribute by setting the ActiveRecord::Encryption.config.hash_digest_class on the initializer. But while debugging my issue, I found that the code to use this hash_digest_class wasn't using the different hash_digest_class that I set and I noticed that the ActiveRecord::Encryption was only configured only at the active_record railtie before the custom initializers I have.

    initializer "active_record_encryption.configuration" do |app|
      ActiveRecord::Encryption.configure \
         primary_key: app.credentials.dig(:active_record_encryption, :primary_key),
         deterministic_key: app.credentials.dig(:active_record_encryption, :deterministic_key),
         key_derivation_salt: app.credentials.dig(:active_record_encryption, :key_derivation_salt),
         **config.active_record.encryption

and my new_framework_defaults_7_1 was loaded after that.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

This commit changes the active_record_encryption.configuration to be set on an after initialize block. After PR rails#44873, we can configure now hash_digest_class that will be used on ActiveRecord::Encryption. But the docs said that we could change it via an initializer by setting Rails.application.config.active_record.encryption.hash_digest_class. This wasn't working because the railtie "active_record_encryption.configuration" was being set before the initializer was loaded. This changes this behave to configure the active_record encryption after the initialize was loaded.

…initialize

duduribeiro · 2023-06-19T17:49:07Z

activerecord/lib/active_record/railtie.rb

-      if ActiveRecord::Encryption.config.add_to_filter_parameters
-        ActiveRecord::Encryption.install_auto_filtered_parameters_hook(app)
+        # Filtered params
+        if ActiveRecord::Encryption.config.add_to_filter_parameters


@jorgemanrubia the current failure https://buildkite.com/rails/rails/builds/97281#0188d44a-def3-4a39-9f69-1cfd4ab8d4cd/1089-1099 is happening because now ActiveRecord::Encryption.install_auto_filtered_parameters_hook is invoked after the initialize, but the code for install_auto_filtered_parameters_hook is a hook that relies on ActiveRecord::Encryption.on_encrypted_attribute_declared and the attributes were declared before the config.after_initialize do |app| block is run.

is there an easy way that I can get all encrypted classes / attributes so I can run this block:

filter = [("#{klass.model_name.element}" if klass.name), encrypted_attribute_name.to_s].compact.join(".") unless excluded_from_filter_parameters?(filter) app.config.filter_parameters << filter unless app.config.filter_parameters.include?(filter) klass.filter_attributes += [encrypted_attribute_name] end

on the after initialize by Getting all encrypted attributes / classes instead to rely on the hook?

My first thought was to keep the hook, change install_auto_filtered_parameters_hook to instead of filtering attribute to only add that the klass / attribute was encrypted and then on an after_initialize I really modify the filtering. But I want to see your thoughts on this.

Also another options was to run

ActiveRecord::Encryption.configure \ primary_key: app.credentials.dig(:active_record_encryption, :primary_key), deterministic_key: app.credentials.dig(:active_record_encryption, :deterministic_key), key_derivation_salt: app.credentials.dig(:active_record_encryption, :key_derivation_salt), **config.active_record.encryption # Filtered params if ActiveRecord::Encryption.config.add_to_filter_parameters ActiveRecord::Encryption.install_auto_filtered_parameters_hook(app) end

both on the non after_initialize / after_initialize block. But this looks weird for me 😄

@jorgemanrubia I made a POC of this suggestion:

Here is a version using a hash, which required me two loops to setup the attributes and this which I made with an array to avoid this two loops.

what are your thoughts?

…ms after

…filtered params after

jorgemanrubia

Some minor comments but the patch looks good @duduribeiro, thanks!

jorgemanrubia · 2023-06-19T20:21:24Z

activerecord/lib/active_record/encryption/configurable.rb

@@ -9,6 +9,7 @@ module Configurable
      included do
        mattr_reader :config, default: Config.new
        mattr_accessor :encrypted_attribute_declaration_listeners
+        mattr_reader :encrypted_attributes, default: Concurrent::Array.new


I like the hash version better you refer at 95b3e01.

Also, I'd consider renaming :encrypted_attributes to entrypted_attributes_by_class to clarify a little bit what's contained there.

jorgemanrubia · 2023-06-19T20:23:28Z

railties/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   Wrap active_record_encryption.configuration on an after initialize


In the first line here, instead of describing the internals of the change itself, I'd try to clarify what's being fixed here.

Example: Support configuring the digest class in Active Record encryption via initializer.

jorgemanrubia · 2023-06-19T20:23:41Z

railties/test/application/configuration_test.rb

@@ -4331,6 +4331,16 @@ def new(app); self; end
      assert_match(/Cannot assign to `load_defaults`, it is a configuration method/, error.message)
    end

+    test "allows initializer to set active_record_encryption.configuration" do


Great call adding a test here 👏

jorgemanrubia · 2023-06-19T20:25:48Z

Also, the PR description could contain fix #48204 to get that one closed when this gets merged.

duduribeiro · 2023-06-19T21:00:48Z

the failing test https://buildkite.com/rails/rails/builds/97291 is the one that is also failing on main branch https://buildkite.com/rails/rails/builds/97274

duduribeiro · 2023-06-19T22:35:54Z

Also, the PR description could contain fix #48204 to get that one closed when this gets merged.

@jorgemanrubia I will not do this because #48204 is something that is still happening with me and I will investigate why. I added a reproduction on a rails 7.0.5 to 7.1.0 app here #48204 (comment) and will try to investigate more but we can merge this PR as it is related with another thing.

This PR fixes the problem where the initializer couldn't set the hash_digest_class and the bug is related with maybe another problem. I will investigate more.

thanks

rafaelfranca · 2023-06-20T05:21:23Z

activerecord/lib/active_record/railtie.rb


-      ActiveSupport.on_load(:active_record) do


ActiveSupport.on_load(:active_record) should only run during eager loading phase, that is after_initialize, so this change isn't necessary. Why do you need to do this?

Why don't you move the ActiveRecord::Encryption.configure call to inside ActiveSupport.on_load(:active_record) do? I don't think we need to use after_initialize here.

hey @rafaelfranca

thanks for the tip! We are moving this PR to #48530, which also fixes the initialization problem.

about setting ActiveRecord::Encryption.configure on ActiveSupport.on_load(:active_record), I found that on on my app my ActiveRecord was loaded before my initializers got executed. See this and this for the @jorgemanrubia thoughts.

It it possible that some initializer in this app is loading an Active Record model? That will trigger the AR encryption initialization logic. application.rb runs before any initializer runs, so the fact that it works makes me suspect that something is triggering that initialization before the new defaults' initializer is running.

After some debugging I found that when I use https://github.com/basecamp/name_of_person gem it loads the active_record before running any initializer. See this

So I think @jorgemanrubia will rely on after_initialize too on his new PR

That is a bug in your application and in the name_of_person gem that should be fixed. Rails should not change its architecture because of bugs in other libraries.

rails-bot bot added activerecord railties labels Jun 19, 2023

duduribeiro mentioned this pull request Jun 19, 2023

Change new_framework_defaults_7_1.rb to use the right config for hash_digest_class #48508

Closed

4 tasks

duduribeiro added 3 commits June 19, 2023 13:07

Add a test to ensure ActiveRecord::Encryption.configure is set after …

808d10c

…initialize

Use a version other than the default to test

abe0789

Wrap everything on after_initialize

55442ba

duduribeiro commented Jun 19, 2023

View reviewed changes

duduribeiro added 4 commits June 19, 2023 15:09

Save encrypted attributes before initialize and install filtered para…

95b3e01

…ms after

Try array to save encrypted attributes before initialize and install …

2e1a3ae

…filtered params after

Fix test to match the new behaviour

ac06054

Clear the encrypted_attributes list on each test

3d9f027

jorgemanrubia reviewed Jun 19, 2023

View reviewed changes

Apply suggestions from PR

444a969

duduribeiro mentioned this pull request Jun 19, 2023

Upgrade to Rails 7.1 alpha breaks ActiveRecord attribute encryption (deterministic) #48204

Closed

rafaelfranca reviewed Jun 20, 2023

View reviewed changes

jorgemanrubia mentioned this pull request Jun 20, 2023

Add a encryption option to support previous data encrypted non-deterministically with a SHA1 hash digest #48530

Merged

5 tasks

dhh closed this in #48530 Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap active_record_encryption.configuration on an after initialize #48520

Wrap active_record_encryption.configuration on an after initialize #48520

duduribeiro commented Jun 19, 2023 •

edited

duduribeiro Jun 19, 2023

duduribeiro Jun 19, 2023

jorgemanrubia left a comment

jorgemanrubia Jun 19, 2023 •

edited

jorgemanrubia Jun 19, 2023

jorgemanrubia Jun 19, 2023

jorgemanrubia commented Jun 19, 2023

duduribeiro commented Jun 19, 2023

duduribeiro commented Jun 19, 2023 •

edited

rafaelfranca Jun 20, 2023

duduribeiro Jun 21, 2023 •

edited

rafaelfranca Jun 26, 2023

		@@ -1,3 +1,14 @@
		* Wrap active_record_encryption.configuration on an after initialize

Wrap active_record_encryption.configuration on an after initialize #48520

Wrap active_record_encryption.configuration on an after initialize #48520

Conversation

duduribeiro commented Jun 19, 2023 • edited

Motivation / Background

Additional information

Checklist

duduribeiro Jun 19, 2023

Choose a reason for hiding this comment

duduribeiro Jun 19, 2023

Choose a reason for hiding this comment

jorgemanrubia left a comment

Choose a reason for hiding this comment

jorgemanrubia Jun 19, 2023 • edited

Choose a reason for hiding this comment

jorgemanrubia Jun 19, 2023

Choose a reason for hiding this comment

jorgemanrubia Jun 19, 2023

Choose a reason for hiding this comment

jorgemanrubia commented Jun 19, 2023

duduribeiro commented Jun 19, 2023

duduribeiro commented Jun 19, 2023 • edited

rafaelfranca Jun 20, 2023

Choose a reason for hiding this comment

duduribeiro Jun 21, 2023 • edited

Choose a reason for hiding this comment

rafaelfranca Jun 26, 2023

Choose a reason for hiding this comment

duduribeiro commented Jun 19, 2023 •

edited

jorgemanrubia Jun 19, 2023 •

edited

duduribeiro commented Jun 19, 2023 •

edited

duduribeiro Jun 21, 2023 •

edited