LG-7836 Automatically refresh USPS auth token when necessary

[eileen-nava]

🎫 Ticket

Automatically refresh USPS auth token when necessary.

🛠 Summary of changes

• Check the USPS auth token before every request to the USPS API. If the token is expired, refresh it.
• The above change necessitated a lot of spec set-up changes.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Deploy feature branch to Joy sandbox
• Open a Rails console in the Joy sandbox. I used this command: aws-vault exec sandbox-power -- bin/ssm-instance -d rails-c asg-joy-idp
• Ensure that the usps mock fallback is off. IdentityConfig.store.usps_mock_fallback should equal false.
• Create an account, fail the attempt to verify online and get sent to in-person verification.
• On the PO Search page, search for an address. Then, check on how long the USPS token has to expire. Rails.cache.redis.ttl(:usps_ippaas_api_auth_token)
• Once Rails.cache.redis.ttl(:usps_ippaas_api_auth_token) shows that the token is expired, proceed through the IPP flow.

👀 Screenshots

I didn't include screenshots because this isn't a user-facing change.

[zachmargolis]

Can we pull the ttl once? Just in case there's some weird condition that changes between calls?

Also can we move the 0.5 magic number into a constant?

Suggested change

	if Rails.cache.redis.ttl(AUTH_TOKEN_CACHE_KEY) != -2 && Rails.cache.redis.ttl(AUTH_TOKEN_CACHE_KEY) <= 0.5 # rubocop:disable Layout/LineLength
	retrieve_token!
	key_ttl = Rails.cache.redis.ttl(AUTH_TOKEN_CACHE_KEY)
	if key_ttl != -2 && key_ttl <= 0.5
	retrieve_token!

[eileen-nava]

Good idea. 👍🏻

[NavaTim]

Also what do you think about bumping the 0.5 seconds to maybe 5 seconds?

[eileen-nava]

@NavaTim Why 5 seconds? I'm fine with increasing the buffer, though I am curious about why you chose that number.

[NavaTim]

I'm specifically thinking here about an amount of time approximating the upper end of what it'd take to serve an average request (maybe 75th percentile or higher) - including backend calls to DB, cache, and a potentially slow USPS API. Part of the goal is to prevent requests from failing midway through because the token wasn't updated early enough. We would also need to add a little buffer for the token update process itself.

[eileen-nava]

Makes sense, thank you. 🙏🏻

[zachmargolis]

we repeat this bit a lot in these specs. Would it make sense to either move it up to a top level context (have it once) or make a helper method like use_redis_cache?

[eileen-nava]

Yes, I'll look into this. 👍🏻

[zachmargolis]

Quick question, would Faraday still return the same error if we had something like to_return(status: 403)

Suggested change

	).to_raise(Faraday::ForbiddenError)
	).to_return(status: 403, body: '')

[eileen-nava]

It's slightly different. Here's what I saw, using binding.pry, for the raise option.

 #<WebMock::RequestStub:0x000000011abda978
 @request_pattern=
  #<WebMock::RequestPattern:0x000000011abda928
   @body_pattern=nil,
   @headers_pattern=nil,
   @method_pattern=#<WebMock::MethodPattern:0x000000011abda8d8 @pattern=:post>,
   @uri_pattern=
    #<WebMock::URIRegexpPattern:0x000000011abda888
     @pattern=/\/ivs-ippaas-api\/IPPRest\/resources\/rest\/requestEnrollmentCode/,
     @query_params=nil>,
   @with_block=nil>,
 @responses_sequences=
  [#<WebMock::ResponsesSequence:0x000000011abda5b8
    @current_position=0,
    @responses=
     [#<WebMock::Response:0x000000011abda798
       @body=nil,
       @exception=#<Faraday::ForbiddenError #<Faraday::ForbiddenError: Exception from WebMock>>,
       @headers=nil,
       @should_timeout=nil,
       @status=nil>],
    @times_to_repeat=1>]>

Here's what I see with the return option.

#<WebMock::RequestStub:0x000000010e80a840
 @request_pattern=
  #<WebMock::RequestPattern:0x000000010e80a7f0
   @body_pattern=nil,
   @headers_pattern=nil,
   @method_pattern=#<WebMock::MethodPattern:0x000000010e80a7a0 @pattern=:post>,
   @uri_pattern=
    #<WebMock::URIRegexpPattern:0x000000010e80a750
     @pattern=/\/ivs-ippaas-api\/IPPRest\/resources\/rest\/requestEnrollmentCode/,
     @query_params=nil>,
   @with_block=nil>,
 @responses_sequences=
  [#<WebMock::ResponsesSequence:0x000000010e80a4d0
    @current_position=0,
    @responses=
     [#<WebMock::Response:0x000000010e80a660
       @body="",
       @exception=nil,
       @headers=nil,
       @should_timeout=nil,
       @status=[403, ""]>],
    @times_to_repeat=1>]>

Changing the stub does not change test behavior.

[zachmargolis]

I have a slight preference for having the mock server return a response, and letting the adapter translate + throw that, but since it doesn't change the tests, its prob fine

[eileen-nava]

@NavaTim I couldn't leave an inline comment because I didn't change this area of the code, but I wanted to address your feedback on this portion of proofer.rb in the retrieve_token! method

      Rails.cache.write(AUTH_TOKEN_CACHE_KEY, token, expires_at: expires_at)
      # If using a redis cache we have to manually set the expires_at. This is because we aren't
      # using a dedicated Redis cache and instead are just using our existing Redis server with
      # mixed usage patterns. Without this cache entries don't expire.
      # More at https://api.rubyonrails.org/classes/ActiveSupport/Cache/RedisCacheStore.html
      Rails.cache.try(:redis)&.expireat(AUTH_TOKEN_CACHE_KEY, expires_at.to_i)

I remember you suggested consolidating this into one call, rather than two calls to redis, because you were concerned that if one call passes and one call fails, we'd have an entry without expireat set on it. I tried to consolidate this into one call using the method below:
Rails.cache.try(:redis)&.setex(AUTH_TOKEN_CACHE_KEY, expires_at.to_i, token)

but when I did this the ttl for the key then equaled -1, which indicates that the redis value doesn’t have an associated expiration time.
As a result, I went back to using two commands. I think (as indicated by the comments in the code) because we’re using a redis cache we might have to do this in two commands, since we have to manually set the expireat. I’m interested to hear your thoughts on this. If there's a way to consolidate the two commands into one and I'm missing it, please let me know.

[NavaTim]

@NavaTim I couldn't leave an inline comment because I didn't change this area of the code, but I wanted to address your feedback on this portion of proofer.rb in the retrieve_token! method

Read more...

      Rails.cache.write(AUTH_TOKEN_CACHE_KEY, token, expires_at: expires_at)
      # If using a redis cache we have to manually set the expires_at. This is because we aren't
      # using a dedicated Redis cache and instead are just using our existing Redis server with
      # mixed usage patterns. Without this cache entries don't expire.
      # More at https://api.rubyonrails.org/classes/ActiveSupport/Cache/RedisCacheStore.html
      Rails.cache.try(:redis)&.expireat(AUTH_TOKEN_CACHE_KEY, expires_at.to_i)

I remember you suggested consolidating this into one call, rather than two calls to redis, because you were concerned that if one call passes and one call fails, we'd have an entry without expireat set on it. I tried to consolidate this into one call using the method below: Rails.cache.try(:redis)&.setex(AUTH_TOKEN_CACHE_KEY, expires_at.to_i, token)

but when I did this the ttl for the key then equaled -1, which indicates that the redis value doesn’t have an associated expiration time. As a result, I went back to using two commands. I think (as indicated by the comments in the code) because we’re using a redis cache we might have to do this in two commands, since we have to manually set the expireat. I’m interested to hear your thoughts on this. If there's a way to consolidate the two commands into one and I'm missing it, please let me know.

The important part here is ensuring that the token properly expires and gets renewed regardless of how many separate Redis calls we use. If we store the expiry as part of the value itself (in addition to, or instead of, the expiry on the entry) then we can address the scenario: What if the second call fails?

Furthermore if we directly use the expiry value (unlike the 403 fallback strategy), then we can anticipate that the token may expire by the time we use it and therefore avert the 403 altogether.

To be clear - with this approach we would pass in a hash/object instead of token to the original write method call. When we retrieve the value back from Redis, we would check the expiry value contained in that same object before attempting to use the token. This process doesn't require any use of Redis's built-in expiry (though it can be supplemented with that expiry).

[NavaTim]

@eileen-nava I took a closer look at the logic, and considering that the result of a failed second call would be repeating the token fetch up to a few times (since Redis would return -1 for the TTL)... it's probably fine for that to happen & your implementation is probably ok as-is (excepting the comments from Zach's review & my comment about fetching the token at 5 seconds instead of 0.5 seconds).

[NavaTim]

While the test does verify that the appropriate method calls happen, it seems like without and_call_original that this may not be returning a token. Additionally, it would seem that the test is not actually checking that the cached token is used.

[eileen-nava]

Unfortunately, I can't add and_call_original here because it is only available on a partial double, and cache is a pure test double.

[NavaTim]

The test does appear to adequately verify that the cache was updated, which seems like the major part of this story. That said, I recommend adding the validation that the token is being used appropriately.

[zachmargolis]

LGTM

[zachmargolis]

I have a slight preference for having the mock server return a response, and letting the adapter translate + throw that, but since it doesn't change the tests, its prob fine