From c103a4328ae25092e0699decc4b44789783661a9 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Mon, 26 Dec 2022 14:32:56 -0600 Subject: [PATCH] Refactor message metadata tests Prior to this commit, there were several places to potentially add a new message metadata test: * `SharedMessageMetadataTests` module * `MessageEncryptorMetadataTest` class (includes `SharedMessageMetadataTests`) * `MessageEncryptorMetadataMarshalTest` class (subclasses `MessageEncryptorMetadataTest`) * `MessageEncryptorMetadataJSONTest` class (subclasses `MessageEncryptorMetadataTest`) * `MessageEncryptorMetadataJsonWithMarshalFallbackTest` class (subclasses `MessageEncryptorMetadataTest`) * `MessageVerifierMetadataTest` class (includes `SharedMessageMetadataTests`) * `MessageVerifierMetadataMarshalTest` class (subclasses `MessageVerifierMetadataTest`) * `MessageVerifierMetadataJsonWithMarshalFallbackTest` class (subclasses `MessageVerifierMetadataTest`) * `MessageVerifierMetadataJsonTest` class (subclasses `MessageVerifierMetadataTest`) * `MessageVerifierMetadataCustomJSONTest` class (subclasses `MessageVerifierMetadataTest`) * `MessageEncryptorMetadataNullSerializerTest` class (subclasses `MessageVerifierMetadataTest`) This commit refactors the message metadata tests, reducing the list to: * `MessageMetadataTests` module * `MessageEncryptorMetadataTest` class (includes `MessageMetadataTests`) * `MessageVerifierMetadataTest` class (includes `MessageMetadataTests`) This makes it easier to add new tests, as well as new testing scenarios (e.g. new encryptor / verifier configurations). Additionally, this commit fixes two tests that were ineffective: * The `test_passing_expires_in_less_than_a_second_is_not_expired` test (which is now simply a part of the "message expires with :expires_in" test) did not use the `with_usec: true` option. Therefore, the time resulting from `travel 0.5.seconds` was truncated, effectively traveling 0 seconds. * The `test_verify_with_use_standard_json_time_format_as_false` test (which is now named "expiration works with ActiveSupport.use_standard_json_time_format = false") did not specify an expiration time. Therefore, the conditional branch that it was supposed to test was never exercised. --- activesupport/test/message_encryptor_test.rb | 42 ------ activesupport/test/message_verifier_test.rb | 89 ----------- .../message_encryptor_metadata_test.rb | 24 +++ .../test/messages/message_metadata_tests.rb | 139 ++++++++++++++++++ .../message_verifier_metadata_test.rb | 71 +++++++++ .../test/metadata/shared_metadata_tests.rb | 88 ----------- 6 files changed, 234 insertions(+), 219 deletions(-) create mode 100644 activesupport/test/messages/message_encryptor_metadata_test.rb create mode 100644 activesupport/test/messages/message_metadata_tests.rb create mode 100644 activesupport/test/messages/message_verifier_metadata_test.rb delete mode 100644 activesupport/test/metadata/shared_metadata_tests.rb diff --git a/activesupport/test/message_encryptor_test.rb b/activesupport/test/message_encryptor_test.rb index 4125093c868..c9443051cb1 100644 --- a/activesupport/test/message_encryptor_test.rb +++ b/activesupport/test/message_encryptor_test.rb @@ -4,7 +4,6 @@ require_relative "abstract_unit" require "openssl" require "active_support/time" require "active_support/json" -require_relative "metadata/shared_metadata_tests" class MessageEncryptorTest < ActiveSupport::TestCase class JSONSerializer @@ -404,44 +403,3 @@ class MessageEncryptorWithHybridSerializerAndWithoutMarshalDumpTest < MessageEnc super end end - -class MessageEncryptorMetadataTest < ActiveSupport::TestCase - include SharedMessageMetadataTests - - setup do - @secret = SecureRandom.random_bytes(32) - @encryptor = ActiveSupport::MessageEncryptor.new(@secret, **encryptor_options) - end - - private - def generate(message, **options) - @encryptor.encrypt_and_sign(message, **options) - end - - def parse(data, **options) - @encryptor.decrypt_and_verify(data, **options) - end - - def encryptor_options; {} end -end - -class MessageEncryptorMetadataMarshalTest < MessageEncryptorMetadataTest - private - def encryptor_options - { serializer: Marshal } - end -end - -class MessageEncryptorMetadataJSONTest < MessageEncryptorMetadataTest - private - def encryptor_options - { serializer: MessageEncryptorTest::JSONSerializer.new } - end -end - -class MessageEncryptorMetadataJsonWithMarshalFallbackTest < MessageEncryptorMetadataTest - private - def encryptor_options - { serializer: ActiveSupport::JsonWithMarshalFallback } - end -end diff --git a/activesupport/test/message_verifier_test.rb b/activesupport/test/message_verifier_test.rb index d4677253038..ad86a9314eb 100644 --- a/activesupport/test/message_verifier_test.rb +++ b/activesupport/test/message_verifier_test.rb @@ -4,7 +4,6 @@ require_relative "abstract_unit" require "openssl" require "active_support/time" require "active_support/json" -require_relative "metadata/shared_metadata_tests" class MessageVerifierTest < ActiveSupport::TestCase class JSONSerializer @@ -289,91 +288,3 @@ class DefaultJsonSerializerMessageVerifierTest < JsonSerializeAndNoFallbackMessa ActiveSupport::MessageVerifier.default_message_verifier_serializer = @default_verifier end end - -class MessageVerifierMetadataTest < ActiveSupport::TestCase - include SharedMessageMetadataTests - - setup do - @verifier = ActiveSupport::MessageVerifier.new("Hey, I'm a secret!", **verifier_options) - end - - def test_verify_raises_when_purpose_differs - assert_raise(ActiveSupport::MessageVerifier::InvalidSignature) do - @verifier.verify(generate(data, purpose: "payment"), purpose: "shipping") - end - end - - def test_verify_with_use_standard_json_time_format_as_false - format_before = ActiveSupport.use_standard_json_time_format - ActiveSupport.use_standard_json_time_format = false - assert_equal "My Name", @verifier.verify(generate("My Name")) - ensure - ActiveSupport.use_standard_json_time_format = format_before - end - - def test_verify_raises_when_expired - signed_message = generate(data, expires_in: 1.month) - - travel 2.months - assert_raise(ActiveSupport::MessageVerifier::InvalidSignature) do - @verifier.verify(signed_message) - end - end - - private - def generate(message, **options) - @verifier.generate(message, **options) - end - - def parse(message, **options) - @verifier.verified(message, **options) - end - - def verifier_options - Hash.new - end -end - -class MessageVerifierMetadataMarshalTest < MessageVerifierMetadataTest - private - def verifier_options - { serializer: Marshal } - end -end - -class MessageVerifierMetadataJsonWithMarshalFallbackTest < MessageVerifierMetadataTest - private - def verifier_options - { serializer: ActiveSupport::JsonWithMarshalFallback } - end -end - -class MessageVerifierMetadataJsonTest < MessageVerifierMetadataTest - private - def verifier_options - { serializer: JSON } - end -end - - -class MessageVerifierMetadataCustomJSONTest < MessageVerifierMetadataTest - private - def verifier_options - { serializer: MessageVerifierTest::JSONSerializer.new } - end -end - -class MessageEncryptorMetadataNullSerializerTest < MessageVerifierMetadataTest - private - def data - "string message" - end - - def null_serializing? - true - end - - def verifier_options - { serializer: ActiveSupport::MessageEncryptor::NullSerializer } - end -end diff --git a/activesupport/test/messages/message_encryptor_metadata_test.rb b/activesupport/test/messages/message_encryptor_metadata_test.rb new file mode 100644 index 00000000000..c043aeae508 --- /dev/null +++ b/activesupport/test/messages/message_encryptor_metadata_test.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require_relative "../abstract_unit" +require_relative "message_metadata_tests" + +class MessageEncryptorMetadataTest < ActiveSupport::TestCase + include MessageMetadataTests + + private + def make_codec(**options) + @secret ||= SecureRandom.random_bytes(32) + ActiveSupport::MessageEncryptor.new(@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 +end diff --git a/activesupport/test/messages/message_metadata_tests.rb b/activesupport/test/messages/message_metadata_tests.rb new file mode 100644 index 00000000000..37aa9e0dd8b --- /dev/null +++ b/activesupport/test/messages/message_metadata_tests.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require "active_support/json" +require "active_support/time" + +module MessageMetadataTests + extend ActiveSupport::Concern + + included do + test "message :purpose must match specified :purpose" do + each_scenario do |data, codec| + assert_roundtrip data, codec, { purpose: "x" }, { purpose: "x" } + + assert_no_roundtrip data, codec, { purpose: "x" }, { purpose: "y" } + assert_no_roundtrip data, codec, { purpose: "x" }, {} + assert_no_roundtrip data, codec, {}, { purpose: "x" } + end + end + + test ":purpose can be a symbol" do + each_scenario do |data, codec| + assert_roundtrip data, codec, { purpose: :x }, { purpose: :x } + assert_roundtrip data, codec, { purpose: :x }, { purpose: "x" } + assert_roundtrip data, codec, { purpose: "x" }, { purpose: :x } + end + end + + test "message expires with :expires_at" do + freeze_time do + each_scenario do |data, codec| + message = encode(data, codec, expires_at: 1.second.from_now) + + travel 0.5.seconds, with_usec: true + assert_equal data, decode(message, codec) + + travel 0.5.seconds, with_usec: true + assert_nil decode(message, codec) + end + end + end + + test "message expires with :expires_in" do + freeze_time do + each_scenario do |data, codec| + message = encode(data, codec, expires_in: 1.second) + + travel 0.5.seconds, with_usec: true + assert_equal data, decode(message, codec) + + travel 0.5.seconds, with_usec: true + assert_nil decode(message, codec) + end + end + end + + test ":expires_at overrides :expires_in" do + each_scenario do |data, codec| + message = encode(data, codec, expires_at: 1.hour.from_now, expires_in: 1.second) + + travel 1.minute + assert_equal data, decode(message, codec) + + travel 1.hour + assert_nil decode(message, codec) + end + end + + test "messages do not expire by default" do + each_scenario do |data, codec| + message = encode(data, codec, purpose: "x") + + travel 1000.years + assert_equal data, decode(message, codec, purpose: "x") + end + end + + test "expiration works with ActiveSupport.use_standard_json_time_format = false" do + original_use_standard_json_time_format = ActiveSupport.use_standard_json_time_format + ActiveSupport.use_standard_json_time_format = false + + each_scenario do |data, codec| + assert_roundtrip data, codec, { expires_at: 1.hour.from_now } + end + ensure + ActiveSupport.use_standard_json_time_format = original_use_standard_json_time_format + end + + test "metadata works with NullSerializer" do + codec = make_codec(serializer: ActiveSupport::MessageEncryptor::NullSerializer) + assert_roundtrip "a string", codec, { purpose: "x", expires_in: 1.year }, { purpose: "x" } + end + end + + private + class CustomSerializer + def self.dump(value) + JSON.dump(value) << "!" + end + + def self.load(value) + JSON.load(value.chomp!("!")) + end + end + + SERIALIZERS = [ + Marshal, + JSON, + ActiveSupport::JSON, + ActiveSupport::JsonWithMarshalFallback, + CustomSerializer, + ] + + DATA = [ + "a string", + { "a_number" => 123, "a_time" => Time.local(2004), "an_object" => { "key" => "value" } }, + ["a string", 123, Time.local(2004), { "key" => "value" }], + ] + + def each_scenario + SERIALIZERS.each do |serializer| + codec = make_codec(serializer: serializer) + DATA.each do |data| + yield data, codec + end + end + end + + def roundtrip(data, codec, encode_options = {}, decode_options = {}) + decode(encode(data, codec, **encode_options), codec, **decode_options) + end + + def assert_roundtrip(data, codec, encode_options = {}, decode_options = {}) + assert_equal data, roundtrip(data, codec, encode_options, decode_options) + end + + def assert_no_roundtrip(data, codec, encode_options = {}, decode_options = {}) + assert_nil roundtrip(data, codec, encode_options, decode_options) + end +end diff --git a/activesupport/test/messages/message_verifier_metadata_test.rb b/activesupport/test/messages/message_verifier_metadata_test.rb new file mode 100644 index 00000000000..c4cd23cc92b --- /dev/null +++ b/activesupport/test/messages/message_verifier_metadata_test.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require_relative "../abstract_unit" +require_relative "message_metadata_tests" + +class MessageVerifierMetadataTest < ActiveSupport::TestCase + include MessageMetadataTests + + test "#verify raises when :purpose does not match" do + each_scenario do |data, verifier| + assert_equal data, verifier.verify(verifier.generate(data, purpose: "x"), purpose: "x") + + assert_raises ActiveSupport::MessageVerifier::InvalidSignature do + verifier.verify(verifier.generate(data, purpose: "x"), purpose: "y") + end + + assert_raises ActiveSupport::MessageVerifier::InvalidSignature do + verifier.verify(verifier.generate(data), purpose: "y") + end + + assert_raises ActiveSupport::MessageVerifier::InvalidSignature do + verifier.verify(verifier.generate(data, purpose: "x")) + end + end + end + + test "#verify raises when message is expired via :expires_at" do + freeze_time do + each_scenario do |data, verifier| + message = verifier.generate(data, expires_at: 1.second.from_now) + + travel 0.5.seconds, with_usec: true + assert_equal data, verifier.verify(message) + + travel 0.5.seconds, with_usec: true + assert_raises ActiveSupport::MessageVerifier::InvalidSignature do + verifier.verify(message) + end + end + end + end + + test "#verify raises when message is expired via :expires_in" do + freeze_time do + each_scenario do |data, verifier| + message = verifier.generate(data, expires_in: 1.second) + + travel 0.5.seconds, with_usec: true + assert_equal data, verifier.verify(message) + + travel 0.5.seconds, with_usec: true + assert_raises ActiveSupport::MessageVerifier::InvalidSignature do + verifier.verify(message) + end + end + end + end + + private + def make_codec(**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 diff --git a/activesupport/test/metadata/shared_metadata_tests.rb b/activesupport/test/metadata/shared_metadata_tests.rb deleted file mode 100644 index 17435573c07..00000000000 --- a/activesupport/test/metadata/shared_metadata_tests.rb +++ /dev/null @@ -1,88 +0,0 @@ -# frozen_string_literal: true - -module SharedMessageMetadataTests - def null_serializing? - false - end - - def test_encryption_and_decryption_with_same_purpose - assert_equal data, parse(generate(data, purpose: "checkout"), purpose: "checkout") - assert_equal data, parse(generate(data)) - - string_message = "address: #23, main street" - assert_equal string_message, parse(generate(string_message, purpose: "shipping"), purpose: "shipping") - end - - def test_verifies_array_when_purpose_matches - skip if null_serializing? - - data = [ "credit_card_no: 5012-6748-9087-5678", { "card_holder" => "Donald", "issued_on" => Time.local(2017) }, 12345 ] - assert_equal data, parse(generate(data, purpose: :registration), purpose: :registration) - end - - def test_encryption_and_decryption_with_different_purposes_returns_nil - assert_nil parse(generate(data, purpose: "payment"), purpose: "sign up") - assert_nil parse(generate(data, purpose: "payment")) - assert_nil parse(generate(data), purpose: "sign up") - end - - def test_purpose_using_symbols - assert_equal data, parse(generate(data, purpose: :checkout), purpose: :checkout) - assert_equal data, parse(generate(data, purpose: :checkout), purpose: "checkout") - assert_equal data, parse(generate(data, purpose: "checkout"), purpose: :checkout) - end - - def test_passing_expires_at_sets_expiration_date - encrypted_message = generate(data, expires_at: 1.hour.from_now) - - travel 59.minutes - assert_equal data, parse(encrypted_message) - - travel 2.minutes - assert_nil parse(encrypted_message) - end - - def test_set_relative_expiration_date_by_passing_expires_in - encrypted_message = generate(data, expires_in: 2.hours) - - travel 1.hour - assert_equal data, parse(encrypted_message) - - travel 1.hour + 1.second - assert_nil parse(encrypted_message) - end - - def test_passing_expires_in_less_than_a_second_is_not_expired - freeze_time do - encrypted_message = generate(data, expires_in: 1.second) - - travel 0.5.seconds - assert_equal data, parse(encrypted_message) - - travel 1.second - assert_nil parse(encrypted_message) - end - end - - def test_favor_expires_at_over_expires_in - payment_related_message = generate(data, purpose: "payment", expires_at: 2.year.from_now, expires_in: 1.second) - - travel 1.year - assert_equal data, parse(payment_related_message, purpose: :payment) - - travel 1.year + 1.day - assert_nil parse(payment_related_message, purpose: "payment") - end - - def test_skip_expires_at_and_expires_in_to_disable_expiration_check - payment_related_message = generate(data, purpose: "payment") - - travel 100.years - assert_equal data, parse(payment_related_message, purpose: "payment") - end - - private - def data - { "credit_card_no" => "5012-6784-9087-5678", "card_holder" => { "name" => "Donald" } } - end -end