Factor rotator tests into dedicated classes

This commit factors tests related to `ActiveSupport::Messages::Rotator`
into `MessageEncryptorRotatorTest` and `MessageVerifierRotatorTest`
classes, with a shared `MessageRotatorTests` module.  In particular,
this:

* Replaces the `test_rotating_serializer` test in the
  `MessageEncryptorTest` class because it did not actually rotate the
  serializer.

* Removes the `test_on_rotation_is_called_and_returns_modified_messages`
  test from the `MessageEncryptorWithJsonSerializerTest` class because
  its only purpose is to override a test of the same name in the
  `MessageEncryptorTest` parent class, because the expected value must
  use string keys instead of symbol keys due to the serializer.

* Removes the `test_on_rotation_can_be_passed_at_the_constructor_level`
  test from the `MessageEncryptorWithJsonSerializerTest` class for the
  same reason as above.

* Removes the `test_on_rotation_option_takes_precedence_over_the_one_given_in_constructor`
  test from the `MessageEncryptorWithJsonSerializerTest` class for the
  same reason as above.

* Removes the `test_on_rotation_is_called_and_verified_returns_message`
  test from the `JsonSerializeMarshalFallbackMessageVerifierTest` class
  because it is rotating the secret and digest, and is not specific to
  the serializer.

* Removes the `test_on_rotation_is_called_and_verified_returns_message`
  test from the `DefaultMarshalSerializerMessageVerifierTest` class for
  the same reason as above.

* Adds thorough test coverage for the `on_rotation` option for both
  `MessageVerifier` and `MessageEncryptor` (previous test coverage was
  mostly just for `MessageEncryptor`), including coverage for when the
  `on_rotation` proc should _not_ be called.

* Adds test coverage for rotating the `url_safe` option, for both
  `MessageVerifier` and `MessageEncryptor`.

Author: jonathanhefner

activesupport/test/message_encryptor_test.rb

@@ -149,99 +149,6 @@ def test_backwards_compatibility_decrypt_previously_encrypted_messages_without_m
     assert_equal "Ruby on Rails", encryptor.decrypt_and_verify(encrypted_message)
   end
 
-  def test_rotating_secret
-    old_message = ActiveSupport::MessageEncryptor.new(secrets[:old], cipher: "aes-256-gcm").encrypt_and_sign("old")
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, cipher: "aes-256-gcm")
-    encryptor.rotate secrets[:old]
-
-    assert_equal "old", encryptor.decrypt_and_verify(old_message)
-  end
-
-  def test_rotating_serializer
-    old_message = ActiveSupport::MessageEncryptor.new(secrets[:old], cipher: "aes-256-gcm", serializer: JSON).
-      encrypt_and_sign({ ahoy: :hoy })
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, cipher: "aes-256-gcm", serializer: JSON)
-    encryptor.rotate secrets[:old]
-
-    assert_equal({ "ahoy" => "hoy" }, encryptor.decrypt_and_verify(old_message))
-  end
-
-  def test_rotating_aes_cbc_secrets
-    old_encryptor = ActiveSupport::MessageEncryptor.new(secrets[:old], "old sign", cipher: "aes-256-cbc")
-    old_message = old_encryptor.encrypt_and_sign("old")
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret)
-    encryptor.rotate secrets[:old], "old sign", cipher: "aes-256-cbc"
-
-    assert_equal "old", encryptor.decrypt_and_verify(old_message)
-  end
-
-  def test_multiple_rotations
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign("older")
-    old_message = ActiveSupport::MessageEncryptor.new(secrets[:old], "old sign").encrypt_and_sign("old")
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret)
-    encryptor.rotate secrets[:old], "old sign"
-    encryptor.rotate secrets[:older], "older sign"
-
-    assert_equal "new",   encryptor.decrypt_and_verify(encryptor.encrypt_and_sign("new"))
-    assert_equal "old",   encryptor.decrypt_and_verify(old_message)
-    assert_equal "older", encryptor.decrypt_and_verify(older_message)
-  end
-
-  def test_on_rotation_is_called_and_returns_modified_messages
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret)
-    encryptor.rotate secrets[:old]
-    encryptor.rotate secrets[:older], "older sign"
-
-    rotated = false
-    message = encryptor.decrypt_and_verify(older_message, on_rotation: proc { rotated = true })
-
-    assert_equal({ encoded: "message" }, message)
-    assert rotated
-  end
-
-  def test_on_rotation_can_be_passed_at_the_constructor_level
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    rotated = rotated = false  # double assigning to suppress "assigned but unused variable" warning
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, on_rotation: proc { rotated = true })
-    encryptor.rotate secrets[:older], "older sign"
-
-    assert_changes(:rotated, from: false, to: true) do
-      message = encryptor.decrypt_and_verify(older_message)
-
-      assert_equal({ encoded: "message" }, message)
-    end
-  end
-
-  def test_on_rotation_option_takes_precedence_over_the_one_given_in_constructor
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    rotated = rotated = false  # double assigning to suppress "assigned but unused variable" warning
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, on_rotation: proc { rotated = true })
-    encryptor.rotate secrets[:older], "older sign"
-
-    assert_changes(:rotated, from: false, to: "Yes") do
-      message = encryptor.decrypt_and_verify(older_message, on_rotation: proc { rotated = "Yes" })
-
-      assert_equal({ encoded: "message" }, message)
-    end
-  end
-
-  def test_with_rotated_metadata
-    old_message = ActiveSupport::MessageEncryptor.new(secrets[:old], cipher: "aes-256-gcm").
-      encrypt_and_sign("metadata", purpose: :rotation)
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, cipher: "aes-256-gcm")
-    encryptor.rotate secrets[:old]
-
-    assert_equal "metadata", encryptor.decrypt_and_verify(old_message, purpose: :rotation)
-  end
 
   private
     def assert_aead_not_decrypted(encryptor, value)
@@ -341,48 +248,6 @@ def test_backwards_compatibility_decrypt_previously_encrypted_messages_without_m
 
     assert_equal "Ruby on Rails", encryptor.decrypt_and_verify(encrypted_message)
   end
-
-  def test_on_rotation_is_called_and_returns_modified_messages
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret)
-    encryptor.rotate secrets[:old]
-    encryptor.rotate secrets[:older], "older sign"
-
-    rotated = false
-    message = encryptor.decrypt_and_verify(older_message, on_rotation: proc { rotated = true })
-
-    assert_equal({ "encoded" => "message" }, message)
-    assert rotated
-  end
-
-  def test_on_rotation_can_be_passed_at_the_constructor_level
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    rotated = rotated = false  # double assigning to suppress "assigned but unused variable" warning
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, on_rotation: proc { rotated = true })
-    encryptor.rotate secrets[:older], "older sign"
-
-    assert_changes(:rotated, from: false, to: true) do
-      message = encryptor.decrypt_and_verify(older_message)
-
-      assert_equal({ "encoded" => "message" }, message)
-    end
-  end
-
-  def test_on_rotation_option_takes_precedence_over_the_one_given_in_constructor
-    older_message = ActiveSupport::MessageEncryptor.new(secrets[:older], "older sign").encrypt_and_sign({ encoded: "message" })
-
-    rotated = rotated = false  # double assigning to suppress "assigned but unused variable" warning
-    encryptor = ActiveSupport::MessageEncryptor.new(@secret, on_rotation: proc { rotated = true })
-    encryptor.rotate secrets[:older], "older sign"
-
-    assert_changes(:rotated, from: false, to: "Yes") do
-      message = encryptor.decrypt_and_verify(older_message, on_rotation: proc { rotated = "Yes" })
-
-      assert_equal({ "encoded" => "message" }, message)
-    end
-  end
 end
 
 class MessageEncryptorWithHybridSerializerAndWithoutMarshalDumpTest < MessageEncryptorWithJsonSerializerTest

activesupport/test/message_verifier_test.rb

@@ -99,37 +99,6 @@ def test_raise_error_when_secret_is_nil
     end
     assert_equal "Secret should not be nil.", exception.message
   end
-
-  def test_rotating_secret
-    old_message = ActiveSupport::MessageVerifier.new("old", digest: "SHA1").generate("old")
-
-    verifier = ActiveSupport::MessageVerifier.new(@secret, digest: "SHA1")
-    verifier.rotate "old"
-
-    assert_equal "old", verifier.verified(old_message)
-  end
-
-  def test_multiple_rotations
-    old_message   = ActiveSupport::MessageVerifier.new("old", digest: "SHA256").generate("old")
-    older_message = ActiveSupport::MessageVerifier.new("older", digest: "SHA1").generate("older")
-
-    verifier = ActiveSupport::MessageVerifier.new(@secret, digest: "SHA512")
-    verifier.rotate "old",   digest: "SHA256"
-    verifier.rotate "older", digest: "SHA1"
-
-    assert_equal "new",   verifier.verified(verifier.generate("new"))
-    assert_equal "old",   verifier.verified(old_message)
-    assert_equal "older", verifier.verified(older_message)
-  end
-
-  def test_rotations_with_metadata
-    old_message = ActiveSupport::MessageVerifier.new("old").generate("old", purpose: :rotation)
-
-    verifier = ActiveSupport::MessageVerifier.new(@secret)
-    verifier.rotate "old"
-
-    assert_equal "old", verifier.verified(old_message, purpose: :rotation)
-  end
 end
 
 class DefaultMarshalSerializerMessageVerifierTest < MessageVerifierTest
@@ -151,20 +120,6 @@ def test_backward_compatibility_messages_signed_without_metadata
     assert_equal @data, @verifier.verify(signed_message)
   end
 
-  def test_on_rotation_is_called_and_verified_returns_message
-    older_message = ActiveSupport::MessageVerifier.new("older", digest: "SHA1").generate({ encoded: "message" })
-
-    verifier = ActiveSupport::MessageVerifier.new(@secret, digest: "SHA512")
-    verifier.rotate "old",   digest: "SHA256"
-    verifier.rotate "older", digest: "SHA1"
-
-    rotated = false
-    message = verifier.verified(older_message, on_rotation: proc { rotated = true })
-
-    assert_equal({ encoded: "message" }, message)
-    assert rotated
-  end
-
   def test_raise_error_when_argument_class_is_not_loaded
     # To generate the valid message below:
     #
@@ -226,20 +181,6 @@ def teardown
     ActiveSupport::JsonWithMarshalFallback.fallback_to_marshal_deserialization = @default_fallback
   end
 
-  def test_on_rotation_is_called_and_verified_returns_message
-    older_message = ActiveSupport::MessageVerifier.new("older", digest: "SHA1").generate({ encoded: "message" })
-
-    verifier = ActiveSupport::MessageVerifier.new(@secret, digest: "SHA512")
-    verifier.rotate "old",   digest: "SHA256"
-    verifier.rotate "older", digest: "SHA1"
-
-    rotated = false
-    message = verifier.verified(older_message, on_rotation: proc { rotated = true })
-
-    assert_equal({ "encoded" => "message" }, message)
-    assert rotated
-  end
-
   def test_backward_compatibility_messages_signed_marshal_serialized
     marshal_serialized_signed_message = "BAh7B0kiCXNvbWUGOgZFVEkiCWRhdGEGOwBUSSIIbm93BjsAVEl1OglUaW1lDSCAG8AAAAAABjoJem9uZUkiCFVUQwY7AEY=--ae7480422168507f4a8aec6b1d68bfdfd5c6ef48"
     assert_equal @data, @verifier.verify(marshal_serialized_signed_message)

activesupport/test/messages/message_encryptor_rotator_test.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require_relative "../abstract_unit"
+require_relative "message_rotator_tests"
+
+class MessageEncryptorRotatorTest < ActiveSupport::TestCase
+  include MessageRotatorTests
+
+  test "rotate cipher" do
+    assert_rotate [cipher: "aes-256-gcm"], [cipher: "aes-256-cbc"]
+  end
+
+  test "rotate serializer" do
+    assert_rotate [serializer: JSON], [serializer: Marshal]
+  end
+
+  test "rotate serializer when message has purpose" do
+    assert_rotate [serializer: JSON], [serializer: Marshal], purpose: "purpose"
+  end
+
+  test "rotate verifier secret when using non-authenticated encryption" do
+    with_authenticated_encryption(false) do
+      assert_rotate \
+        [secret("encryption"), secret("new verifier")],
+        [secret("encryption"), secret("old verifier")],
+        [secret("encryption"), secret("older verifier")]
+    end
+  end
+
+  test "rotate verifier digest when using non-authenticated encryption" do
+    with_authenticated_encryption(false) do
+      assert_rotate [digest: "SHA256"], [digest: "SHA1"], [digest: "MD5"]
+    end
+  end
+
+  private
+    def secret(key)
+      @secrets ||= {}
+      @secrets[key] ||= SecureRandom.random_bytes(32)
+    end
+
+    def make_codec(secret = secret("secret"), verifier_secret = nil, **options)
+      ActiveSupport::MessageEncryptor.new(secret, verifier_secret, **options)
+    end
+
+    def encode(data, encryptor, **options)
+      encryptor.encrypt_and_sign(data, **options)
+    end
+
+    def decode(message, encryptor, **options)
+      encryptor.decrypt_and_verify(message, **options)
+    rescue ActiveSupport::MessageVerifier::InvalidSignature
+      nil
+    end
+
+    def with_authenticated_encryption(value = true)
+      original_value = ActiveSupport::MessageEncryptor.use_authenticated_message_encryption
+      ActiveSupport::MessageEncryptor.use_authenticated_message_encryption = value
+      yield
+    ensure
+      ActiveSupport::MessageEncryptor.use_authenticated_message_encryption = original_value
+    end
+end

activesupport/test/messages/message_rotator_tests.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+require "json"
+
+module MessageRotatorTests
+  extend ActiveSupport::Concern
+
+  included do
+    test "rotate secret" do
+      assert_rotate [secret("new")], [secret("old")], [secret("older")]
+    end
+
+    test "rotate secret when message has purpose" do
+      assert_rotate [secret("new")], [secret("old")], purpose: "purpose"
+    end
+
+    test "rotate url_safe" do
+      assert_rotate [url_safe: true], [url_safe: false]
+    end
+
+    test "rotate secret and options" do
+      assert_rotate [secret("new"), url_safe: true], [secret("old"), url_safe: false]
+    end
+
+    test "on_rotation is called on successful rotation" do
+      called = nil
+      assert_rotate [secret("new"), on_rotation: proc { called = true }], [secret("old")]
+      assert called
+    end
+
+    test "on_rotation is not called when no rotation is necessary" do
+      called = nil
+      assert_rotate [secret("same"), on_rotation: proc { called = true }], [secret("same")]
+      assert_not called
+    end
+
+    test "on_rotation is not called when no rotation is successful" do
+      called = nil
+      codec = make_codec(secret("new"), on_rotation: proc { called = true })
+      codec.rotate(secret("old"))
+      other_message = encode(DATA, make_codec(secret("other")))
+
+      assert_nil decode(other_message, codec)
+      assert_not called
+    end
+
+    test "on_rotation method option takes precedence over constructor option" do
+      called = ""
+      codec = make_codec(secret("new"), on_rotation: proc { called += "via constructor" })
+      codec.rotate(secret("old"))
+      old_message = encode(DATA, make_codec(secret("old")))
+
+      assert_equal DATA, decode(old_message, codec, on_rotation: proc { called += "via method" })
+      assert_equal "via method", called
+    end
+  end
+
+  private
+    DATA = [{ "a_boolean" => true, "a_number" => 123, "a_string" => "abc" }]
+
+    def secret(key)
+      key
+    end
+
+    def assert_rotate(current, *old, **message_metadata)
+      current_options = current.extract_options!
+      current_codec = make_codec(*current, **current_options)
+
+      old.each do |old_args|
+        old_options = old_args.extract_options!
+        current_codec.rotate(*old_args, **old_options)
+        old_codec = make_codec(*old_args, **old_options)
+        old_message = encode(DATA, old_codec, **message_metadata)
+
+        assert_equal DATA, decode(old_message, current_codec, **message_metadata)
+        assert_nil decode(old_message, current_codec) if !message_metadata.empty?
+      end
+    end
+end

activesupport/test/messages/message_verifier_rotator_test.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require_relative "../abstract_unit"
+require_relative "message_rotator_tests"
+
+class MessageVerifierRotatorTest < ActiveSupport::TestCase
+  include MessageRotatorTests
+
+  test "rotate digest" do
+    assert_rotate [digest: "SHA256"], [digest: "SHA1"], [digest: "MD5"]
+  end
+
+  private
+    def make_codec(secret = secret("secret"), **options)
+      ActiveSupport::MessageVerifier.new(secret, **options)
+    end
+
+    def encode(data, verifier, **options)
+      verifier.generate(data, **options)
+    end
+
+    def decode(message, verifier, **options)
+      verifier.verified(message, **options)
+    end
+end