-
Notifications
You must be signed in to change notification settings - Fork 123
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
Use ActiveSupport::MessageVerifier's expiry and purpose feature in SignedGlobalID #107
Use ActiveSupport::MessageVerifier's expiry and purpose feature in SignedGlobalID #107
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @jeremy (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. Please see the contribution instructions for more information. |
160e485
to
2fdd484
Compare
@kaspth ActiveSupport::MessageVerfifier added support for expiry and purpose in Rails 5.2 so I updated the Gemfile.lock and fixed the SignedGlobalID tests and they work fine now. However, could you please help me connect the dots:
Is there something I'm missing here? 😄 |
2fdd484
to
b94caab
Compare
@kaspth If we're using a new Rails app then this works well. But considering the case of an old Rails app which uses gid's and upgrades to the latest Rails and Global ID version, shouldn't they still be parsable? |
b94caab
to
31e8b33
Compare
test/cases/railtie_test.rb
Outdated
@@ -38,7 +38,7 @@ def setup | |||
|
|||
test 'SignedGlobalID.verifier defaults to nil when secret_token is not present' do | |||
@app.initialize! | |||
assert_nil SignedGlobalID.verifier | |||
assert_not_nil SignedGlobalID.verifier | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secret_token is deprecated
Yep, they should be parsable. We pretty much always want to keep backwardscompatibility. |
test/cases/signed_global_id_test.rb
Outdated
end | ||
|
||
test 'sign with purpose when :for is provided' do | ||
assert_equal "eyJnaWQiOiJnaWQ6Ly9iY3gvUGVyc29uLzUiLCJwdXJwb3NlIjoibG9naW4iLCJleHBpcmVzX2F0IjpudWxsfQ==--4b9630f3a1fb3d7d6584d95d4fac96433ec2deef", @login_sgid.to_s | ||
assert_equal "eyJfcmFpbHMiOnsibWVzc2FnZSI6IkltZHBaRG92TDJKamVDOVFaWEp6YjI0dk5TST0iLCJleHAiOm51bGwsInB1ciI6ImxvZ2luIn19--c39de01a211a37d62b4773d1da7bff94ba2ec176", @login_sgid.to_s | ||
assert_not_equal @login_sgid, @like_sgid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just do:
assert_not_equal @login_sgid, SignedGlobalID.create(Person.new(5), for: 'like-button')
test/cases/signed_global_id_test.rb
Outdated
@@ -227,7 +229,7 @@ class SignedGlobalIDCustomParamsTest < ActiveSupport::TestCase | |||
end | |||
|
|||
test 'parse custom params' do | |||
sgid = SignedGlobalID.parse('eyJnaWQiOiJnaWQ6Ly9iY3gvUGVyc29uLzU/aGVsbG89d29ybGQiLCJwdXJwb3NlIjoiZGVmYXVsdCIsImV4cGlyZXNfYXQiOm51bGx9--7c042f09483dec470fa1088b76d9fd946eb30ffa') | |||
sgid = SignedGlobalID.parse('eyJfcmFpbHMiOnsibWVzc2FnZSI6IkltZHBaRG92TDJKamVDOVFaWEp6YjI0dk5UOW9aV3hzYnoxM2IzSnNaQ0k9IiwiZXhwIjpudWxsLCJwdXIiOiJkZWZhdWx0In19--09eb7f8baa6e0e19012dc4d1fd749e0c0cd33314') | |||
assert_equal 'world', sgid.params[:hello] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be a good idea to keep the old printed SGIDs around in separate tests to check that we can parse old gids.
lib/global_id/signed_global_id.rb
Outdated
@@ -30,6 +32,9 @@ def pick_purpose(options) | |||
|
|||
private | |||
def verify(sgid, options) | |||
gid = pick_verifier(options).verify(sgid, purpose: pick_purpose(options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about clarifying the flow and its purpose?
verify_with_verifier_validated_metadata(sgid, options) || verify_with_self_validated_metadata(sgid, options)
b70d098
to
ba9acac
Compare
…gnedGlobalID. Use ActiveSupport::MessageVerifier to handle SignedGlobalID's metadata.
ba9acac
to
300daca
Compare
Merged in 9d15bf2 |
No description provided.