Add ActiveSupport::MessageVerifier#expired?

[ghiculescu]

Motivation / Background

I'd like to add support for rails/globalid#141. As noted in that issue, the expired? method needs to be added to Active Support first. This PR adds it.

Detail

verifier = ActiveSupport::MessageVerifier.new("secret")
message = verifier.generate("hello", expires_at: Time.now.tomorrow)
verifier.expired?(message) # false
# 24 hours later...
verifier.expired?(message) # true

[jonathanhefner]

With the current implementation in this PR, writing something like

if verifier.expired?(message)
  # ...
else
  data = verifier.verify(message)
end

will decode and verify the message twice (i.e. double the work).

I think avoiding that would require a deeper implementation of expired?.

Furthermore, MessageVerifier#expired? suggests we should also have MessageEncryptor#expired?, and there is no way MessageEncryptor#expired? could avoid (double) decrypting the message.

Reading rails/globalid#141, it seems like the user doesn't actually require expired?; they just need a way to determine whether a nil result is due to expiration vs a missing record. There are a few ways I think we could accommodate that in the MessageVerifier (and MessageEncryptor) API.

One way would be to make verified (and decrypt_and_verify) accept a block:

verifier.verified(valid_message) do |data|
  "is valid"
end
# => "is valid"

verifier.verified(invalid_message) do |data|
  raise "never reached"
end
# => nil

[jonathanhefner]

Reading rails/globalid#141, it seems like the user doesn't actually require expired?; they just need a way to determine whether a nil result is due to expiration vs a missing record. There are a few ways I think we could accommodate that in the MessageVerifier (and MessageEncryptor) API.

One way would be to make verified (and decrypt_and_verify) accept a block:

Though even that may not be necessary... Unlike arbitrary data, the signed value of an SGID should never be nil, right? So if verified returns nil then the message is definitely invalid rather than being nil data.

It seems like find_signed could implement the relevant behavior instead. (For some reason, I thought SGID delegated to find_signed.)

[ghiculescu]

So if verified returns nil then the message is definitely invalid rather than being nil data.

If verified returns nil, then it's hit a throw statement for :invalid_message_format or :invalid_message_content. But I don't think that lets you distinguish between messages that were once valid and have now expired, and other issues.

[jonathanhefner]

But I don't think that lets you distinguish between messages that were once valid and have now expired, and other issues.

Why would you want to? An expired message can be from a malicious actor reusing old messages just as much as a message with a mismatched purpose.

[ghiculescu]

Here's an example of why: rails/globalid#141 (comment)

we want to know if & when a globalid expired to show users an info like "this link expired on XXX"

[jonathanhefner]

we want to know if & when a globalid expired to show users an info like "this link expired on XXX"

That will not be possible with expired?. At most, you could show "this link expired", which is very similar to "this link is invalid". In fact, you could show "this link expired" for all invalid messages — if someone is using a tampered message, there is no reason to show them an accurate error.

[casperisfine]

will decode and verify the message twice (i.e. double the work).

Seems like the proper way would be a more finely grained API that start an object representing the message that you can then query:

• Message#expired?
• Message#expires_at
• Message#payload
• Message#verified?

etc.

[jonathanhefner]

Seems like the proper way would be a more finely grained API that start an object representing the message that you can then query:

I agree that is the way to go if we want to expose expires_at. However, if there is app logic that depends on the expiration, then, in my opinion, the expiration should be a part of the domain model. For example, instead of writing:

message = verifier.generate("data", expires_at: 1.day.from_now)

data = verifier.verified(message)

if data.nil?
  # Show generic error
end

You would write:

message = verifier.generate(["data", 1.day.from_now])

data, expiration = verifier.verified(message)

if expiration&.past?
  # Show expiration
end

(Though you may need to write Time.parse(expiration).past? if the serializer does not deserialize time objects.)

[casperisfine]

Yeah, I guess your suggestion is simpler.