Skip to content
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

Signing... with an expiration date #27

Closed
jeremy opened this issue Aug 20, 2014 · 15 comments
Closed

Signing... with an expiration date #27

jeremy opened this issue Aug 20, 2014 · 15 comments

Comments

@jeremy
Copy link
Member

jeremy commented Aug 20, 2014

Once we can sign with purpose, we'll also want to be explicit about how long the signed Global ID is valid. It needs an expiration date!

See the work in progress on expiration @ rails/rails#16462 - they can use some help on this as well ❤️

We'll want to be able to pass :expires_in or :expires_at when we create signed Global IDs. When we parse a sgid, we'll rely on the MessageVerifier to raise when it's past the expiration date. We'll have to rescue that error and return nil.

Furthermore, we'll want expiration by default, so we'll never inadvertently send out forever-valid signed Global IDs. So, SignedGlobalID.expires_in = 1.month for example, and expose config.global_id.expires_in = ... to the Railtie. Allow passing expires_in: nil to override and use no expiry.

@jeremy jeremy added this to the 1.0 milestone Aug 20, 2014
@tony612
Copy link
Contributor

tony612 commented Aug 21, 2014

It seems we should wait for the merging of rails/rails#16462 although we can implement this like http://api.rubyonrails.org/classes/ActiveSupport/MessageVerifier.html , right?

@jeremy
Copy link
Member Author

jeremy commented Aug 21, 2014

That PR needs more work, so it may take some time. We could implement it here, much like we're handling purpose, and port it to use MessageVerifier support when it's available.

@tony612
Copy link
Contributor

tony612 commented Aug 21, 2014

@jeremy Good idea.I'd like to work on this cause if my purpose PR can be merged :)

@tony612
Copy link
Contributor

tony612 commented Aug 21, 2014

Besides expires_in, should we implement expires_at too?

@jeremy
Copy link
Member Author

jeremy commented Aug 21, 2014

👍 to expires_at, yes. But only expires_in for the global defaults 😁

@tony612
Copy link
Contributor

tony612 commented Aug 21, 2014

@jeremy But I have a question: If we accepts both expires_at and expires_in, which one should we use?

@jeremy
Copy link
Member Author

jeremy commented Aug 21, 2014

@tony612 Passing both... shouldn't happen. Suppose an explicit :expires_at should take precedence.

@tony612
Copy link
Contributor

tony612 commented Aug 22, 2014

@jeremy Glad to see @kaspth has done much work 👍 . I'll stop my work though I just wrote the tests yesterday.

@jeremy
Copy link
Member Author

jeremy commented Aug 22, 2014

@tony612 Thank you! Your review and feedback are welcome on #29 👍

@kaspth
Copy link
Contributor

kaspth commented Aug 22, 2014

If you want, you can push those tests to a branch which I can rebase off of.
We're gonna need @jeremy to create a branch on the main repo, which you can PR against.

Kasper

Den 22/08/2014 kl. 02.29 skrev Tony Han notifications@github.com:

@jeremy Glad to see @kaspth has done much work . I'll stop my work though I just wrote the tests yesterday.


Reply to this email directly or view it on GitHub.

@tony612
Copy link
Contributor

tony612 commented Aug 22, 2014

@kaspth It doesn't matter, I'll review your code and give some feedback if I find something 😃

@kaspth
Copy link
Contributor

kaspth commented Aug 22, 2014

@tony612 Sweet! ❤️

@jeremy
Copy link
Member Author

jeremy commented Aug 25, 2014

Implemented by @kaspth ❤️

@jeremy jeremy closed this as completed Aug 25, 2014
@tony612
Copy link
Contributor

tony612 commented Aug 25, 2014

cool!!

@kaspth
Copy link
Contributor

kaspth commented Aug 25, 2014

Sweet! Thanks, @jeremy ❤️
I'll follow the rails issue and keep giving feedback there.

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

No branches or pull requests

3 participants