When I said metadata I didn't mean users could any metadata, but a name to convey what the purpose and expiry are at a higher level 😊

For this to work we also need to add support for purpose when decrypting and verifying, so the original purpose within the message can be compared against (the expiry has the info within it). See the SignedGlobalID class for the API we need.

A note about backwardscompatibility: the tests still output the same encrypted messages now, but should they fail that means we've broken backwardscompatibility. So keep an eye out for that 😊

✂️

Needs to be a keyword arg: def decrypt_and_verify(value, purpose: nil)

_decrypt deals with the raw decryption process. I think the extra verifications should be in decrypt_and_verify

Something rubs me the wrong way about the encryptor being happy to throw an exception for every occasion. What would the ramifications be if the encryptor just returned nil if it didn't pass verification?

People also need to be able to pass in :purpose, :expires_in and :expires_at when calling encrypt_and_sign otherwise it's rather secret that we have the support.

In fact let's wrap the payload metadata handling in an ActiveSupport::Messages::Metadata class. Here's the rough structure for it (as I see it):

# active_support/messages/metadata.rb
module ActiveSupport
  module Messages
    class Metadata # :nodoc:
      def initialize(expires_at, purpose)
        @expires_at, @purpose = expires_at, purpose
      end

      class << self
        def wrap(message, expires_at: nil, expires_in: nil, purpose: nil)
          { "value" => message, "_rails" => { "exp" => pick_expiry(expires_at, expires_in), "pur" => purpose } }
        end

        def verify(message, purpose)
          metadata = extract_metadata(message)
          message["value"] if metadata.nil? || (metadata.match?(purpose) && metadata.fresh?)
        end

        private
          def pick_expiry(expires_at, expires_in)
            # Needs to use iso8601(3) and have expires_at take precedence. See SignedGlobalID and Active Storage for inspiration.
          end

          def extract_metadata(message)
            # Check if the metadata came from us and create a new instance.
          end
      end

      def match?(purpose)
      end

      def fresh?
      end
    end
  end
end

We'll need to use string keys throughout such that messages encoded with JSON will work when decoded. We can then use this class in both the verifier and encryptor.

The _rails key stores the metadata and lets us handle backwardscompatibility. E.g. if that key is missing we can't provide the extra metadata verification, so we should just skip it.

This should be encryptor.encrypt_and_sign(message, purpose: "bar") and the decrypted message should then just be "foo" (remember to keep case consistency too).

We prefer standard ifs to ternaries (especially the hard to read double-ternaries). This would be much clearer to read (albeit the extra lines):

metadata = extract_metadata(message)

if metadata && metadata.match?(purpose) && metadata.fresh?
  message["value"]
else
  message
end

Delete the newline after private.

Again, double ternaries are extremely tough for others to read. That cost isn't worth the terseness, so just use a standard if … elsif.

self here is Metadata so we can just say new instead of Metadata.new.

I don't think this extra abstraction is pulling its weight. When I read the next line I'm confused as to where message["_rails"] came from. I'd just put the method body here.

Ruby has deprecated has_key? in favor of key?.

I think == is the more apt method to compare with here.

Use assert_nil to spare us an exception on the eventual Minitest 6.

We're already in the ActiveSupport namespace here. So I think we can shorten this to Messages::Metadata. Same above.

Nicely remembered 👍

Let's move this if into wrap and make it return the original message in case no metadata options have been passed. It makes sense for the metadata object to handle whether or not to wrap — and we're going to need this if for the verifier as well.

Lets just inline this message variable.

Lets rename value to data — clarifies that this decrypts raw encrypted data.

Ruby returns nil by default, so skip the else branch.

Add a newline before here.

Newline above here as well.

Newline above here, before the other travel and the first assertion. Did you mean to write 1.1.hours?

That's not true. It's "message without expire time never expires".

Oops! I phrased it wrong

We only need one for 100.years — if there's a bug and it hasn't rightly expired by then, it probably didn't expire before hand either.

This suggests we should pull out the metadata tests into a shared tests module that we then include in some test case subclasses.

module SharedMessageMetadataTests
  def setup
    @secret    = SecureRandom.random_bytes(32)
    @encryptor = ActiveSupport::MessageEncryptor.new(@secret, encryptor_options)

    super
  end

  def test_xxx
    # …
  end

  # More metadata tests below…
end

class MessageEncryptorMetadataMarshalTest < ActiveSupport::TestCase
  include SharedMessageMetadataTests

  def encryptor_options
    { serializer: Marshal }
  end
end

class MessageEncryptorMetadataJSONTest < ActiveSupport::TestCase
  # All yours…
end

This ensures we'll always have complete compatibility between the two serializers. Consider what would happen if we added a new test for Marshal, but then forgot to add one for JSON as well? 😉

👆 for more tips find @jeremy's recent cleanup PR for the Active Support cache store tests.

Slight addendum to the newlines between assertions from yesterday, when there's 2 or more cases like this, it's usually easier to group the act and assertions together:

message = "a security question's answer"

login = @encryptor.encrypt_and_sign(message, purpose: "login")
assert_equal message, @encryptor.decrypt_and_verify(login, purpose: "login")

no_purpose = @encryptor.encrypt_and_sign(message)
assert_equal message, @encryptor.decrypt_and_verify(no_purpose)

(It can also be nice not to reuse the variable name :))

Remember to use double quoted strings.

Let's also add:

assert_nil @encryptor.decrypt_and_verify(encrypted_message, purpose: "sign up") # Right below here
assert_nil @encryptor.decrypt_and_verify(encrypted_message, purpose: "")

Bang on 👍

You don't have to get so elaborate with the messages. I'd assign a @message ivar in setup and use that throughout. I'd probably also make @message a hash, so we know the complex scenario encryption scenario works.

We should also check that we correctly wrap and unpack arrays too.

travel 0.5.seconds?

Might be good to add freeze_time above here because this test is so time sensitive.

I couldn't find any doc on how to use freeze_time 😅

😊

Remember to rebase on master to get it!

Sorry, I didn't get you! 😅

Yeah, got it! It was connectivity issue.

✂️

Let's say MessageEncryptorTest::JSONSerializer.new here. https://github.com/assain/rails/blob/288b3d60b3b46d636425f3ba2c712cf0103f6870/activesupport/test/message_encryptor_test.rb#L9

It's rather that expiry supports subsecond precision 😊

Remember to call super.

👍

Temporarily, I need some cleaning to do.

If the keys deserialized aren't left as strings wouldn't backward compatibility be affected? (JSON)

encrypt_and_sign(message, options)?

Same deal. Just pass the options straight along.

You're in an ActiveSupport::TestCase now. Let's use setup do.

Why do we need the cipher? We've changed the default to be aes-256-gcm, right?

I still think we should go with:

class MessageEncryptorMetadataMarshalTest < MessageEncryptorMetadataTest
  private
    def encryptor_options
      { serializer: Marshal } # Assain, just pass it in.
    end
end

When I do instance_variable_get(:@Cipher) I get aes-256-cbc in the tests 😅

Use JSON.

Call super!

@string_message and @array_message are only used in one test. Let's just inline them to that test (remember newlines too).

Call super!