Skip to content

LG-7918: Use Rails.cache for USPS IPPaaS auth tokens #7294

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.

Sign up for GitHub

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

Closed
sheldon-b wants to merge 5 commits into from
Closed

LG-7918: Use Rails.cache for USPS IPPaaS auth tokens #7294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cad73a4
Use Rails.cache for USPS IPPaaS auth tokens
Nov 4, 2022
5cffcf4
Simplify and clarify a few specs
Nov 4, 2022
f5497a6
changelog: Improvements, In-person proofing, Use Rails cache for USPS…
Nov 4, 2022
c5d2a96
Use token type during authorization
Nov 4, 2022
64b9707
Return the authorization value correctly
Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions app/services/usps_in_person_proofing/proofer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module UspsInPersonProofing
class Proofer
mattr_reader :token, :token_expires_at
API_TOKEN_CACHE_KEY = :usps_ippaas_api_token

# Makes HTTP request to get nearby in-person proofing facilities
# Requires address, city, state and zip code.
Expand Down Expand Up @@ -102,17 +102,28 @@ def request_enrollment_code(unique_id)
end

# Makes a request to retrieve a new OAuth token
# and modifies self to store the token and when
# and updates the cached token value
# it expires (15 minutes).
# @return [String] the token
# @return [String] Auth token
def retrieve_token!
body = request_token
@@token_expires_at = Time.zone.now + body['expires_in']
@@token = "#{body['token_type']} #{body['access_token']}"
token, expires_in, token_type = request_token.fetch_values(
'access_token',
'expires_in',
'token_type',
)
authz = "#{token_type} #{token}"
Rails.cache.write(
API_TOKEN_CACHE_KEY, authz,
expires_at: Time.zone.now + expires_in
)
authz
Comment on lines +114 to +119
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an average/expected expiration for these tokens? Wondering if we could pick a fixed duration and use the block Rails.cache.fetch(..., expires_in) { } syntax and simplify a bunch of this code

Copy link
Copy Markdown
Contributor Author

@sheldon-b sheldon-b Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected is 15 minutes but it feels weird to hardcode it when we get the actual value in the API response. Agreed it'd be more concise. Mixed feelings about it

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make our version of the token expire slightly before the value provided in the response, maybe by a few seconds.

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. to reduce the probability that the token will expire between fetching it from cache & attempting to use it.

Copy link
Copy Markdown
Contributor Author

@sheldon-b sheldon-b Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had that idea but Andrew convinced me that it's a half-measure and we should just fix it properly by adding in a retry if the token has expired. I created this ticket for it. Original PR thread

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not both?

Copy link
Copy Markdown
Contributor Author

@sheldon-b sheldon-b Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it removes the urgency of fixing it better. I think we'd want to do both, just not the one right now

end

def token_valid?
token.present? && token_expires_at.present? && token_expires_at.future?
# Checks the cache for an unexpired token and returns it. If the cache has expired, retrieves
# a new token and returns it
# @return [String] Auth token
def token
Rails.cache.read(API_TOKEN_CACHE_KEY) || retrieve_token!
end
Comment on lines +125 to 127
Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Rails cache include an in-memory cache or does it go straight to Redis? If it's the latter, then I would suggest keeping the class/module field.

e.g. in the use case of a large volume in the the USPS proofing job, this could mean hundreds of unnecessary trips to Redis.

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would still need to be mindful of the TTL though, as well as the possibility of another server writing to the Redis cache.

Copy link
Copy Markdown
Contributor Author

@sheldon-b sheldon-b Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be interesting to see how load testing goes with all of the requests to redis. I agree it could end up being an issue under very high load and doing two layers of caching (eg class variable and Rails.cache) may make a lot of sense here

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that it was a primary goal of this work to have it be stored in a Redis cache, so that the same USPS token would be reused across IdP instances.

The Redis cost is definitely an interesting factor I hadn't given much thought to previously, but then we'd still want to weigh it against the additional cost of roundtrips to USPS for IdP-instance-specific tokens.

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention would be to use the same token across instances, but that token could be cached in-memory - i.e. in a manner that respects the original TTL of the token.

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I see what you're saying now -- still use Redis, but also having a local copy to avoid repeatedly calling to Redis while the token is assumed to be valid? (what @sheldon-b was referring to in "two layers of caching")

Sounds like it could be a good idea, but also likely to be tricky to implement 😅

Copy link
Copy Markdown
Contributor

@NavaTim NavaTim Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I'm talking about.

Pseudocode would look like:

If local cache exists and TTL has not passed:
  Use local version
Else:
  Get Redis value

  If Redis value is missing:
    Authenticate w/ USPS
    SET Redis value w/ NX and PX options
    Get Redis value  

  Locally cache and use Redis value

The TTL should be identical between the local cache and Redis, but both should be very slightly before the real expiry of the token.


private
Expand Down Expand Up @@ -144,8 +155,6 @@ def faraday
#
# Returns the same value returned by that block of code.
def dynamic_headers
retrieve_token! unless token_valid?

{
'Authorization' => token,
'RequestID' => request_id,
Expand Down
11 changes: 4 additions & 7 deletions spec/services/arcgis_api/geocoder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,12 @@
end

describe '#retrieve_token!' do
it 'sets token and token_expires_at' do
it 'caches the token' do
stub_generate_token_response
subject.retrieve_token!
token = subject.retrieve_token!

expect(subject.token).to be_present
expect(subject).not_to receive(:request_token)
expect(subject.token).to eq(token)
end

it 'calls the endpoint with the expected params' do
Expand Down Expand Up @@ -121,10 +122,6 @@
end

it 'implicitly refreshes the token when expired' do
root_url = 'http://my.root.url'
allow(IdentityConfig.store).to receive(:arcgis_api_root_url).
and_return(root_url)

stub_generate_token_response(expires_at: 1.hour.from_now.to_i, token: 'token1')
stub_request_suggestions
subject.suggest('100 Main')
Expand Down
80 changes: 74 additions & 6 deletions spec/services/usps_in_person_proofing/proofer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,18 @@
let(:subject) { UspsInPersonProofing::Proofer.new }

describe '#retrieve_token!' do
it 'sets token and token_expires_at' do
stub_request_token
subject.retrieve_token!

expect(subject.token).to be_present
expect(subject.token_expires_at).to be_present
let(:applicant) do
double(
'applicant',
address: Faker::Address.street_address,
city: Faker::Address.city,
state: Faker::Address.state_abbr,
zip_code: Faker::Address.zip_code,
first_name: Faker::Name.first_name,
last_name: Faker::Name.last_name,
email: Faker::Internet.safe_email,
unique_id: '123456789',
)
end

it 'calls the endpoint with the expected params' do
Expand Down Expand Up @@ -50,6 +56,68 @@
},
)
end

it 'caches the token' do
stub_request_token
token = subject.retrieve_token!

expect(subject).not_to receive(:request_token)
expect(subject.token).to eq(token)
end

it 'reuses the cached token on subsequent requests' do
stub_request_token
stub_request_enroll
stub_request_enroll
stub_request_enroll

subject.request_enroll(applicant)
subject.request_enroll(applicant)
subject.request_enroll(applicant)
expect(WebMock).to have_requested(:post, %r{/oauth/authenticate}).once
end

it 'implicitly refreshes the token when expired' do
stub_request_token(expires_in: 1.hour.to_i, access_token: 'token1')
stub_request_enroll
subject.request_enroll(applicant)

travel 2.hours

stub_request_token(access_token: 'token2')
stub_request_enroll
subject.request_enroll(applicant)

expect(WebMock).to have_requested(:post, %r{/oauth/authenticate}).twice
expect(WebMock).to have_requested(
:post,
%r{/ivs-ippaas-api/IPPRest/resources/rest/optInIPPApplicant},
).
with(headers: { 'Authorization' => 'Bearer token1' }).once
expect(WebMock).to have_requested(
:post,
%r{/ivs-ippaas-api/IPPRest/resources/rest/optInIPPApplicant},
).
with(headers: { 'Authorization' => 'Bearer token2' }).once
end

it 'reuses the cached token across instances' do
stub_request_token(access_token: 'token1')
stub_request_enroll
stub_request_enroll

other_instance = UspsInPersonProofing::Proofer.new

subject.request_enroll(applicant)
other_instance.request_enroll(applicant)

expect(WebMock).to have_requested(:post, %r{/oauth/authenticate}).once
expect(WebMock).to have_requested(
:post,
%r{/ivs-ippaas-api/IPPRest/resources/rest/optInIPPApplicant},
).
with(headers: { 'Authorization' => 'Bearer token1' }).twice
end
end

def check_facility(facility)
Expand Down
9 changes: 7 additions & 2 deletions spec/support/usps_ipp_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
module UspsIppHelper
def stub_request_token
def stub_request_token(access_token: nil, expires_in: nil)
# Overwrite fixture values if values are specified
defaults = JSON.parse(UspsInPersonProofing::Mock::Fixtures.request_token_response)
body = defaults.merge(
{ access_token: access_token, expires_in: expires_in }.compact,
)
stub_request(:post, %r{/oauth/authenticate}).to_return(
status: 200,
body: UspsInPersonProofing::Mock::Fixtures.request_token_response,
body: body.to_json,
headers: { 'content-type' => 'application/json' },
)
end
Expand Down