Encryption casting with encrypts before serialize (rails#52650)

* Encryption casting with `encrypts` before `serialize`

A demonstration of how we can simplify encryption serialization and
deserialization if we call `encrypts` before `serialize`.

Calling cast_type.deserialize before decrypting fixes the issue where
binary data cannot be decrypted on PostgreSQL.

We would need to update the docs to recommend that `encrypts` is called
before `serialize` to take advantage of this and possibly raise an error
if they are called the other way round.

* Nest encrypted attribute types within serialized types

Make the order in which `serializes :foo` and `encrypts :foo` are called
irrelevant by always nesting the encrypted attribute type inside the
serialized type.

This required switching from `DelegateClass` to `SimpleDelegator` so
that object we are delegating to can be replaced.

To ensure that the serialized type survives YAML serialization (there's
a test for this), we need to implement init_with to call __setobj__.

* Update docs and changelog

* Switch back to DelegateClass(ActiveModel::Type::Value)

Thanks to @rafaelfranca for the suggestion.

Author: djmb

activerecord/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Deserialize database values before decryption
+
+    PostgreSQL binary values (`ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Bytea`)
+    need to be deserialized before they are decrypted.
+
+    Additionally ensure that the order of serialization/deserialization is consistent
+    for `serialize :foo` and `encrypts :foo` whichever order they are declared in.
+
+    *Donal McBreen*
+
 *   Infer default `:inverse_of` option for `delegated_type` definitions.
 
     ```ruby

activerecord/lib/active_record/encryption/encryptable_record.rb

@@ -88,7 +88,17 @@ def encrypt_attribute(name, key_provider: nil, key: nil, deterministic: false, s
               scheme = scheme_for key_provider: key_provider, key: key, deterministic: deterministic, support_unencrypted_data: support_unencrypted_data, \
                 downcase: downcase, ignore_case: ignore_case, previous: previous, compress: compress, compressor: compressor, **context_properties
 
-              ActiveRecord::Encryption::EncryptedAttributeType.new(scheme: scheme, cast_type: cast_type, default: columns_hash[name.to_s]&.default)
+              type_options = { scheme: scheme, default: columns_hash[name.to_s]&.default }
+
+              if cast_type.serialized?
+                cast_type.tap do |serialized_type|
+                  serialized_type.replace_serialized_subtype do |current_subtype|
+                    ActiveRecord::Encryption::EncryptedAttributeType.new(cast_type: current_subtype, **type_options)
+                  end
+                end
+              else
+                ActiveRecord::Encryption::EncryptedAttributeType.new(cast_type: cast_type, **type_options)
+              end
             end
 
             preserve_original_encrypted(name) if ignore_case

activerecord/lib/active_record/encryption/encrypted_attribute_type.rb

@@ -33,7 +33,7 @@ def cast(value)
       end
 
       def deserialize(value)
-        cast_type.deserialize decrypt(value)
+        decrypt(cast_type.deserialize(value))
       end
 
       def serialize(value)
@@ -81,7 +81,7 @@ def previous_type?
           @previous_type
         end
 
-        def decrypt_as_text(value)
+        def decrypt(value)
           with_context do
             unless value.nil?
               if @default && @default == value
@@ -99,10 +99,6 @@ def decrypt_as_text(value)
           end
         end
 
-        def decrypt(value)
-          text_to_database_type decrypt_as_text(value)
-        end
-
         def try_to_deserialize_with_previous_encrypted_types(value)
           previous_types.each.with_index do |type, index|
             break type.deserialize(value)
@@ -128,12 +124,11 @@ def serialize_with_oldest(value)
         end
 
         def serialize_with_current(value)
-          casted_value = cast_type.serialize(value)
-          casted_value = casted_value&.downcase if downcase?
-          encrypt(casted_value.to_s) unless casted_value.nil?
+          value = value&.downcase if downcase?
+          cast_type.serialize(encrypt(value.to_s)) unless value.nil?
         end
 
-        def encrypt_as_text(value)
+        def encrypt(value)
           with_context do
             if encryptor.binary? && !cast_type.binary?
               raise Errors::Encoding, "Binary encoded data can only be stored in binary columns"
@@ -143,10 +138,6 @@ def encrypt_as_text(value)
           end
         end
 
-        def encrypt(value)
-          text_to_database_type encrypt_as_text(value)
-        end
-
         def encryptor
           ActiveRecord::Encryption.encryptor
         end
@@ -162,14 +153,6 @@ def decryption_options
         def clean_text_scheme
           @clean_text_scheme ||= ActiveRecord::Encryption::Scheme.new(downcase: downcase?, encryptor: ActiveRecord::Encryption::NullEncryptor.new)
         end
-
-        def text_to_database_type(value)
-          if value && cast_type.binary?
-            ActiveModel::Type::Binary::Data.new(value)
-          else
-            value
-          end
-        end
     end
   end
 end

activerecord/lib/active_record/type/serialized.rb

@@ -15,6 +15,13 @@ def initialize(subtype, coder)
         super(subtype)
       end
 
+      def init_with(coder) # :nodoc:
+        # Ensures YAML deserialization calls __setobj__
+        @subtype = coder["subtype"]
+        @coder = coder["coder"]
+        __setobj__(subtype)
+      end
+
       def deserialize(value)
         if default_value?(value)
           value
@@ -57,6 +64,12 @@ def serialized? # :nodoc:
         true
       end
 
+      def replace_serialized_subtype(&block) # :nodoc:
+        @subtype = block.call(subtype)
+        __setobj__(@subtype)
+        self
+      end
+
       private
         def default_value?(value)
           value == coder.load(nil)

activerecord/test/cases/encryption/encryptable_record_message_pack_serialized_test.rb

@@ -5,19 +5,22 @@
 require "models/book_encrypted"
 require "active_record/encryption/message_pack_message_serializer"
 
-class ActiveRecord::Encryption::EncryptableRecordTest < ActiveRecord::EncryptionTestCase
+class ActiveRecord::Encryption::EncryptableRecordMessagePackSerializedTest < ActiveRecord::EncryptionTestCase
   fixtures :encrypted_books
 
   test "binary data can be serialized with message pack" do
     all_bytes = (0..255).map(&:chr).join
-    assert_equal all_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: all_bytes).logo
+    book = EncryptedBookWithBinaryMessagePackSerialized.create!(logo: all_bytes)
+    assert_encrypted_attribute(book, :logo, all_bytes)
   end
 
   test "binary data can be encrypted uncompressed and serialized with message pack" do
+    # Strings below 140 bytes are not compressed
     low_bytes = (0..127).map(&:chr).join
     high_bytes = (128..255).map(&:chr).join
-    assert_equal low_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: low_bytes).logo
-    assert_equal high_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: high_bytes).logo
+
+    assert_encrypted_attribute(EncryptedBookWithBinaryMessagePackSerialized.create!(logo: low_bytes), :logo, low_bytes)
+    assert_encrypted_attribute(EncryptedBookWithBinaryMessagePackSerialized.create!(logo: high_bytes), :logo, high_bytes)
   end
 
   test "text columns cannot be serialized with message pack" do

activerecord/test/cases/encryption/encryptable_record_test.rb

@@ -92,6 +92,12 @@ class ActiveRecord::Encryption::EncryptableRecordTest < ActiveRecord::Encryption
     assert_encrypted_attribute(traffic_light, :state, states)
   end
 
+  test "encrypts serialized attributes where encrypts is declared first" do
+    states = ["green", "red"]
+    traffic_light = EncryptedFirstTrafficLight.create!(state: states, long_state: states)
+    assert_encrypted_attribute(traffic_light, :state, states)
+  end
+
   test "encrypts store attributes with accessors" do
     traffic_light = EncryptedTrafficLightWithStoreState.create!(color: "red", long_state: ["green", "red"])
     assert_equal "red", traffic_light.color
@@ -404,13 +410,13 @@ def name
   test "binary data can be encrypted uncompressed" do
     low_bytes = (0..127).map(&:chr).join
     high_bytes = (128..255).map(&:chr).join
-    assert_equal low_bytes, EncryptedBookWithBinary.create!(logo: low_bytes).logo
-    assert_equal high_bytes, EncryptedBookWithBinary.create!(logo: high_bytes).logo
+    assert_encrypted_attribute EncryptedBookWithBinary.create!(logo: low_bytes), :logo, low_bytes
+    assert_encrypted_attribute EncryptedBookWithBinary.create!(logo: high_bytes), :logo, high_bytes
   end
 
   test "serialized binary data can be encrypted" do
     json_bytes = (32..127).map(&:chr)
-    assert_equal json_bytes, EncryptedBookWithSerializedBinary.create!(logo: json_bytes).logo
+    assert_encrypted_attribute EncryptedBookWithSerializedBinary.create!(logo: json_bytes), :logo, json_bytes
   end
 
   test "can compress data with custom compressor" do

activerecord/test/models/traffic_light_encrypted.rb

@@ -7,6 +7,14 @@ class EncryptedTrafficLight < TrafficLight
   encrypts :state
 end
 
+class EncryptedFirstTrafficLight < ActiveRecord::Base
+  self.table_name = "traffic_lights"
+
+  encrypts :state
+  serialize :state, type: Array
+  serialize :long_state, type: Array
+end
+
 class EncryptedTrafficLightWithStoreState < TrafficLight
   store :state, accessors: %i[ color ], coder: ActiveRecord::Coders::JSON
   encrypts :state

guides/source/active_record_encryption.md

@@ -147,21 +147,7 @@ To encrypt Action Text fixtures, you should place them in `fixtures/action_text/
 
 `active_record.encryption` will serialize values using the underlying type before encrypting them, but, unless using a custom `message_serializer`, *they must be serializable as strings*. Structured types like `serialized` are supported out of the box.
 
-If you need to support a custom type, the recommended way is using a [serialized attribute](https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Serialization/ClassMethods.html). The declaration of the serialized attribute should go **before** the encryption declaration:
-
-```ruby
-# CORRECT
-class Article < ApplicationRecord
-  serialize :title, type: Title
-  encrypts :title
-end
-
-# INCORRECT
-class Article < ApplicationRecord
-  encrypts :title
-  serialize :title, type: Title
-end
-```
+If you need to support a custom type, the recommended way is using a [serialized attribute](https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Serialization/ClassMethods.html).
 
 ### Ignoring Case