Expire the token prematurely on client side

[klippx]

Our assumption was that expires_at? is inaccurate and needs to be offset by some fixed latency amount that we never expect to exceed has been tested and this change solves the described issue #393

The Kred::HttpErrors (which are due to getting two 401 in a row and failing the request) have vanished competely after the deploy (second green marker from the right).

The 10 seconds are a bit arbitrary, and in worst case (where latency is zero) will simply expire the token 10seconds too early on the client side. Which I think most people can live with since expires_at often is 300s or even 1800s.

[coveralls]

Pull Request Test Coverage Report for Build 662

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at ?%

---

Totals
Change from base Build 625:	0.0%
Covered Lines:
Relevant Lines:	0

---

💛 - Coveralls

[pboling]

@klippx Interesting solution.

What if instead of adding a constant that would globally apply for every use case, including some where it may be detrimental, we added the expiry_reduce as an option to OAuth2::AccessToken#initialize:

Current:

      opts = opts.dup
      [:refresh_token, :expires_in, :expires_at].each do |arg|
        instance_variable_set("@#{arg}", opts.delete(arg) || opts.delete(arg.to_s))
      end
      @expires_in ||= opts.delete('expires')
      @expires_in &&= @expires_in.to_i
      @expires_at &&= @expires_at.to_i
      @expires_at ||= Time.now.to_i + @expires_in if @expires_in

Update:

      opts = opts.dup
      [:refresh_token, :expires_in, :expires_at, :expiry_reduce].each do |arg|
        instance_variable_set("@#{arg}", opts.delete(arg) || opts.delete(arg.to_s))
      end
      @expires_in ||= opts.delete('expires')
      @expires_in &&= @expires_in.to_i
      @expires_at &&= @expires_at.to_i
      @expiry_reduce &&= @expiry_reduce.to_i
      @expires_at ||= Time.now.to_i + @expires_in if @expires_in
      @expires_at -= @expiry_reduce if @expiry_reduce

Then the handling of the expires_at can remain the same, from the perspective of all outside callers of this class, the expiry is just sooner than it otherwise would have been.

Would that work for your use case?

[klippx]

@pboling Sounds good! I updated with inspiration from your suggested code, and added some specs.

[pboling]

This is almost correct.
The instance variables initially get set to a value if provided, or nil.

instance_variable_set("@#{arg}", opts.delete(arg) || opts.delete(arg.to_s))

The &&= then ensures that the values stay nil, or if they are String/Integer, they are cast to integer.

@expires_in &&= @expires_in.to_i
@expires_at &&= @expires_at.to_i

This means that the values can be nil, and if nil, can't be used in math, unless cast to integer, or protected by a condition.

I see that you are defaulting expires_latency to 0, but would prefer it default to nil like the others. It is nonsensical to use &&= if it will always be a truthy value.

@expires_latency &&= @expires_latency.to_i

Additionally the code as you have it will not apply the expires_latency when the expires_at has been provided explicitly, as it is only applied after an ||=.

The following would fix that:

@expires_at ||= Time.now.to_i + @expires_in if @expires_in
@expires_at -= @expires_latency if @expires_latency

I think that this is in line with the underlying purpose of @expires_latency though.

[pboling]

I think I'd prefer having it be nil by default in keeping with the other options' nil defaults.

[pboling]

Would prefer that it be nil by default.

[klippx]

• make it nil by default

[pboling]

The code as you have it would fail a spec where expires_at and expires_latency are both provided explicitly, as expires_at would not be adjusted by the expires_latency. See my comment inline with the code for a suggestion at a small change to make expires_latency globally applicable when provided.

[klippx]

• Added a spec for this scenario

[pboling]

@klippx I like the direction of this!

[klippx]

Made the initialize code slightly more complex and had to add another rubocop exclusion I'm afraid. The initialize code is getting hairy, I have to admit.

[pboling]

I would expect this setting to always apply if provided. I don't see a reason to have it be dependent on the @expires_in being set. Other methods of getting an @expires_at set are possible.

Was there a reason for structuring it this way?

I prefer to have features that work independent of other features, for less confusing, less complex logic. Something like:

@expires_at ||= Time.now.to_i + @expires_in if @expires_in
@expires_at -= @expires_latency if @expires_at && @expires_latency

[pboling]

Perhaps one more additional test - reduces expires_at by the given amount if expires_at is not provided as option?

[klippx]

I think reduces expires_at by the given amount covers this case since expires_at is not provided by default and this test explicitly says that it is provided, and adds it to the hash.

[klippx]

I added further changes as per review. Unfortunately the rubocop complaints are still there, I cannot get rid of them without a bigger refactoring of the code. Perhaps this can be raised as separate issue.

[pboling]

Looks good! Thanks for seeing it through 👍

[klippx]

Need to keep this branch open a while until a new version of the gem is released

[pboling]

Progress is slow on the next release, but it is coming!