Add expires_at, to match OmniAuth auth schema #138
Conversation
OmniAuth's [Auth Hash Schema] should return an `expires_at` field as a timestamp, but this gem returns `expires_in`. For compatibility with other `oauth2` OmniAuth strategies, we should also return `expires_at`. I'm not sure if the best place to fix it is here or upstream, in `Rack::OAuth2::AccessToken`. On the one hand, the `oauth2` gem handles it in `OAuth2::AccessToken`. On the other hand, the OmniAuth strategy is the only place we can ensure minimal latency between the server response and `expires_at` computation. I chose here. 🙂 [Auth Hash Schema]: https://github.com/omniauth/omniauth/wiki/Auth-Hash-Schema n.b. I would have assumed that "timestamp" in the schema meant a Time object, but all of the gems that inherit from `omniauth-oauth2` return `Time#to_i`, which is also appropriate.
nevans
force-pushed
the
add-expires_at-to-credentials
branch
from
cb0c46a to
2e8758d
Compare
| @@ -262,6 +264,11 @@ def access_token | |||
| @access_token = client.access_token!(token_request_params) | |||
| verify_id_token!(@access_token.id_token) if configured_response_type == 'code' | |||
|
|
|||
| if @access_token.expires_in | |||
| @access_token_expires_at = Time.now.to_i + @access_token.expires_in | |||
There was a problem hiding this comment.
Can we use the AccessToken#expires_at method with an expires_latency option?
There was a problem hiding this comment.
I guess the tl;dr answer to your question is, "yes... should we?" 🙂
I don't think Rack::OAuth2::AccessToken has an #expires_at method... https://github.com/nov/rack-oauth2/blob/master/lib/rack/oauth2/access_token.rb, or am I looking in the wrong place? I could submit a PR for that upstream.
This is the question that I was referring to in the PR description:
I'm not sure if the best place to fix it is here or upstream, in
Rack::OAuth2::AccessToken. On the one hand, the oauth2 gem handles it in
OAuth2::AccessToken. On the other hand, the OmniAuth strategy is the only place we can
ensure minimal latency between the server response and expires_at computation. I chose
here. 🙂
That was the justification I gave on Dec 6th. But, in retrospect, I may have also been motivated to do it this way because it meant one PR in one repo rather than two in two repos. 😉
It makes intuitive sense to me that expires_latency should be an optional parameter to #expires_at, like you suggest. I think one benefit of applying expires_latency during construction might be that it allows code that uses access_token to remain naive, and thus simplifies application of a uniform policy. 🤷🏻 I don't feel strongly about it one way or the other, and I'll gladly apply expires_latency in the initializer (like the oauth2 gem), as an argument to the #expires_at method, or both.
What do you think? Should I make a PR to https://github.com/nov/rack-oauth2 first, and then update this PR after a new version is released there?
OmniAuth's Auth Hash Schema should return an
expires_atfield as a timestamp, but this gem returnsexpires_in. For compatibility withomniauth-oauth2strategies, this gem should also returnexpires_at.I'm not sure if the best place to fix it is here or upstream, in
Rack::OAuth2::AccessToken. On the one hand, theoauth2gem handles it inOAuth2::AccessToken. On the other hand, the OmniAuth strategy is the only place we can ensure minimal latency between the server response and expires_at computation. I chose here. 🙂n.b. I would have assumed that "timestamp" in the schema meant a Time object, but all of the gems that inherit from
omniauth-oauth2returnTime#to_i, which is also appropriate.