New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document metadata support for MessageEncryptor #29892

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
6 participants
@assain
Contributor

assain commented Jul 22, 2017

@kaspth

This pull request adds the necessary documentation for the metadata support added to the MessageEncryptor: #29599

@rails-bot

This comment has been minimized.

rails-bot commented Jul 22, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

activesupport/lib/active_support/message_encryptor.rb Outdated
@@ -21,7 +21,29 @@ module ActiveSupport
# key = ActiveSupport::KeyGenerator.new('password').generate_key(salt, len) # => "\x89\xE0\x156\xAC..."
# crypt = ActiveSupport::MessageEncryptor.new(key) # => #<ActiveSupport::MessageEncryptor ...>
# encrypted_data = crypt.encrypt_and_sign('my secret data') # => "NlFBTTMwOUV5UlA1QlNEN2xkY2d6eThYWWh..."
# crypt.decrypt_and_verify(encrypted_data) # => "my secret data"
# crypt.decrypt_and_verify(encrypted_data) ` # => "my secret data"

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

You've added a back-tick char by mistake between the code and the #.

This comment has been minimized.

@assain

assain Jul 22, 2017

Contributor

Oops! I'll correct it right away 😅

activesupport/lib/active_support/message_encryptor.rb Outdated
#
# * <tt>:purpose</tt> - The purpose for which the message is intended to be used.
# * <tt>:expires_at</tt> - The expiration date of the message.
# * <tt>:expires_in</tt> - The relative expiration date. Note that, :expires_at takes

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Maybe you can surround :expires_at and :expires_in with a + sign to make them formatted on the generated documentation.

activesupport/lib/active_support/message_encryptor.rb Outdated
# encrypted_data = crypt.encrypt_and_sign(address, purpose: "shipping", expires_at: 1.year.from_now) # => "B/yFvIcSzfaVQcjQYU+pGq..."
#
# Trying to use a message for a different purpose or trying to use an expired message
# would fail and return +nil+

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Nit-picky but you can add a period at the end of this sentence.

activesupport/lib/active_support/message_encryptor.rb Outdated
# would fail and return +nil+
#
# different_purpose = crypt.decrypt_and_verify(encrypted_data, purpose: "login") # => nil
# address = crypt.decrypt_and_verify(encrypted_data, purpose: :shipping) # => "#23 main street"

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

What do you think about removing the variables ? They complicate a bit the example and without them, we can easily see that only the :purpose key is different between the two lines.

activesupport/lib/active_support/message_encryptor.rb Outdated
# different_purpose = crypt.decrypt_and_verify(encrypted_data, purpose: "login") # => nil
# address = crypt.decrypt_and_verify(encrypted_data, purpose: :shipping) # => "#23 main street"
#
# You could use the expires_in option to set the expiration date relatively.

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

You can also use +:expires_in+ here to properly format the text.

activesupport/lib/active_support/message_encryptor.rb Outdated
#
# encrypted_data = crypt.encrypt_and_sign(address, expires_in: 2.months) # => "Dwx8ht2FWhtQbA
#
# This generates an encrypted message that expires in 2 months from now.

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

What do you think about:

This generates an encrypted message that will expire in 2 months.

@robin850 robin850 assigned kaspth and unassigned eileencodes Jul 22, 2017

@assain

This comment has been minimized.

Contributor

assain commented Jul 22, 2017

Hi, @robin850
I've made the suggested changes 😄

@robin850

Thanks for the quick update ! :-) Left a few comments ; could you also add [ci skip] to the commit message ? This will prevent Travis CI from running a build.

activesupport/lib/active_support/message_encryptor.rb Outdated
#
# * <tt>+:purpose+</tt> - The purpose for which the message is intended to be used.
# * <tt>+:expires_at+</tt> - The expiration date of the message.
# * <tt>+:expires_in+</tt> - The relative expiration date. Note that, :expires_at takes

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Whoops, sorry, my comment wasn't clear. Actually, RDoc allows you to write formatted code snippet through <tt>code</tt> or with +code+. Though, the latter, while simpler, doesn't support a lot of fashions.

My original comment was about the :expires_at and :expires_in references in this sentence that could be wrapped around plus signs.

activesupport/lib/active_support/message_encryptor.rb Outdated
# * <tt>+:purpose+</tt> - The purpose for which the message is intended to be used.
# * <tt>+:expires_at+</tt> - The expiration date of the message.
# * <tt>+:expires_in+</tt> - The relative expiration date. Note that, :expires_at takes
# precedence over :expires_in.

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Could you align the beginning of this line with the beginning of the above one ? Otherwise, RDoc will think that this is a code snippet.

You can check-out how the documentation you write will look on the API site running bundle exec rake rdoc and by opening the index.html file under doc/rdoc. :-)

activesupport/lib/active_support/message_encryptor.rb Outdated
# precedence over :expires_in.
#
# address = "#23 main street"
# encrypted_data = crypt.encrypt_and_sign(address, purpose: "shipping", expires_at: 1.year.from_now) # => "B/yFvIcSzfaVQcjQYU+pGq..."

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

I missed it the first time around but actually it's four spaces to create a code block.

activesupport/lib/active_support/message_encryptor.rb Outdated
# crypt.decrypt_and_verify(encrypted_data, purpose: "login") # => nil
# crypt.decrypt_and_verify(encrypted_data, purpose: :shipping) # => "#23 main street"
#
# You could use the +expires_in+ option to set the expiration date relatively.

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Missing : in front of :expires_in.

activesupport/lib/active_support/message_encryptor.rb Outdated
# Trying to use a message for a different purpose or trying to use an expired message
# would fail and return +nil+.
#
# crypt.decrypt_and_verify(encrypted_data, purpose: "login") # => nil

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Nit-picky but could you align the two # please ? 😊

activesupport/lib/active_support/message_encryptor.rb Outdated
# * <tt>+:expires_at+</tt> - The expiration date of the message.
# * <tt>+:expires_in+</tt> - The relative expiration date. Note that, :expires_at takes
# precedence over :expires_in.
#

This comment has been minimized.

@robin850

robin850 Jul 22, 2017

Member

Could you add a tiny "For example:" here ?

@assain

This comment has been minimized.

Contributor

assain commented Jul 22, 2017

Hi @robin850 , sorry for the trouble, I've made the requested changes! 😄

activesupport/lib/active_support/message_encryptor.rb Outdated
# You could use the +:expires_in+ option, to set the expiration date relatively.
#
# # Generates an encrypted message that expires in 2 months.
# encrypted_data = crypt.encrypt_and_sign(address, expires_in: 2.months) # => "Dwx8ht2FWhtQb.."

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Personally, I'm not too much of a fan of the raw options list and prefer to read things more as a "story" kind of.

How does this sound?

# === Confining messages to a specific purpose
#
# By default any message can be used throughout your app. But they can also be
# confined to a specific +:purpose+.
# 
#   token = crypt.encrypt_and_sign("this is the chair", purpose: :login)
#
# Then that same purpose must be passed when verifying to get the data back out:
#
#   crypt.decrypt_and_verify(token, purpose: :login)    # => "this is the chair"
#   crypt.decrypt_and_verify(token, purpose: :shipping) # => nil
#   crypt.decrypt_and_verify(token)                     # => nil
#
# Likewise, if a message has no purpose it won't be returned when verifying with
# a specific purpose.
# 
#   token = crypt.encrypt_and_sign("the conversation is lively")
#   crypt.decrypt_and_verify(token, purpose: :scare_tactics) # => nil
#   crypt.decrypt_and_verify(token)                          # => "the conversation is lively"
#
# === Making messages expire
#
# By default messages last forever and verifying one years from now will still
# return the original value. But messages can be set to expire at a given
# time with +:expires_in+ or +:expires_at+.
#
#   crypt.encrypt_and_sign(parcel, expires_in: 1.month)
#   crypt.encrypt_and_sign(doowad, expires_at: Time.now.end_of_year)
#
# Then the messages can be verified and returned upto the expire time.
# Thereafter, verifying returns +nil+.

(Note: we don't indent code by 4 spaces, only 3 after #)

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

Wow, this looks elegant 😄

@robin850

This comment has been minimized.

Member

robin850 commented Jul 23, 2017

(No problem @assain ; I am sorry for wasting your time, I should've waited for Kasper's feedback. 😊 )

Ugh, and regarding 3 spaces for code blocks, yeah, it looks like I had a bug with RDoc parsing your example.

@assain

This comment has been minimized.

Contributor

assain commented Jul 23, 2017

Hi, @kaspth. I've added the suggested changes. 😄

activesupport/lib/active_support/message_verifier.rb Outdated
#
# Then the messages can be verified and returned upto the expire time.
# Thereafter, the +verified+ method returns +nil+ and the +verify+ method would
# raise +InvalidSignature+.

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Are we sure that the verify method would raise? Let's be sure with a test in the MessageVerifierMetadataTest please.

(If yes, the InvalidSignature exception should also be fully qualified to <tt>ActiveSupport::MessageVerifier::InvalidSignature</tt>)

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

We're mixing present and future tense here too (returns then later would raise).

Let's also ditch the the x method construct and just say x.

# Thereafter, +verified+ returns +nil+ while +verify+ raises
# <tt>ActiveSupport::MessageVerifier::InvalidSignature</tt>.
activesupport/CHANGELOG.md Outdated
#29599 - Added expiry and purpose to MessageEncryptor
#29854 - Added expiry and purpose to MessageVerifier
*Assain Jaleel guided by Kasper Timm Hansen*

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Just put your name here 😉

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

I was just wondering whether I could leave it this way? 😅

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

I appreciate the sentiment, but from a GSoC point of view I'd like you to stand on your own two feet :)

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

👍 thanks

activesupport/CHANGELOG.md Outdated
@verifier.verify(token, purpose: :shipping)
#29599 - Added expiry and purpose to MessageEncryptor
#29854 - Added expiry and purpose to MessageVerifier

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Let's just put these down like Pull requests: #29599, #29854 (add this one too), as people can see the title when they click on it.

activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,28 @@
* Add expiry and purpose to MessageEncryptor & MessageVerifier.

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Let's say purpose and expiry support and wrap the verifier and encryptor in backticks.

activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,28 @@
* Add expiry and purpose to MessageEncryptor & MessageVerifier.

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Also let's make it verifier first, encryptor second throughout the changelog since signing is less complex than encrypting (which for the MessageEncryptor is really encrypt and sign).

activesupport/CHANGELOG.md Outdated
token = @verifier.generate(address, purpose: "shipping", expires_in: 1.month)
@verifier.verified(token, purpose: "shipping")
@verifier.verify(token, purpose: :shipping)

This comment has been minimized.

@kaspth

kaspth Jul 23, 2017

Member

Think this is too detailed for a changelog entry. How does this attempt at condensing the description sound?

For instance, to ensure a message is only usable for one intended purpose:

    token = @verifier.generate("x", purpose: :shipping)

    @verifier.verified(token, purpose: :shipping) # => "x"
    @verifier.verified(token)                     # => nil

Or make it expire after a set time:

    @verifier.generate("x", expires_in: 1.month)
    @verifier.generate("y", expires_at: Time.now.end_of_year)

Showcased with `ActiveSupport::MessageVerifier`, but works the same for 
`ActiveSupport::MessageEncryptor`'s `encrypt_and_sign` and `decrypt_and_verify`.

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

This is much better style 👍

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

I see all the changelog entries using backticks for class and method names, but I still don't get why? 😅

@assain

This comment has been minimized.

Contributor

assain commented Jul 23, 2017

Just wondering 😅 , how come the tests pass, although there's an issue with one of my commits

activesupport/lib/active_support/message_verifier.rb Outdated
# @verifier.verified(token) # => nil
#
# @verifier.verify(token, purpose: :login) # => "this is the chair"
# @verifier.verify(token, purpose: shipping) # => ActiveSupport::MessageVerifier::InvalidSignature

This comment has been minimized.

@morgoth

morgoth Jul 23, 2017

Member

: is missing before shipping

This comment has been minimized.

@assain

assain Jul 23, 2017

Contributor

Thanks for pointing it out 😄

@assain

This comment has been minimized.

Contributor

assain commented Jul 23, 2017

Hi, @kaspth . I've made the suggested changes, again, 😅

@assain

This comment has been minimized.

Contributor

assain commented Jul 24, 2017

Hello, @kaspth 😄

I've got the tests passing. Is it okay if I add the new tests to MessageVerifier via a new PR?

@kaspth kaspth merged commit e4bf6b0 into rails:master Jul 24, 2017

1 check passed

codeclimate All good!
Details
@kaspth

This comment has been minimized.

Member

kaspth commented Jul 24, 2017

Yup, that's fine 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment