Don’t generate different inst-fs urls every time
Closes: CORE-2913 Right now, in canvas, every inst-fs url we generate will be unique. This means that, for example, every time you go to your dashboard, You will have to redownload all the course images for your all your dashcards. This is because we include Time.now in the jwt token we add as a query string parameter on the url, and since time is always changing, so will that token in the url. This fix says: if tokens are valid for 24 hours, every time we generate a url, pick a consistent time between now and 12 hours ago and say the token was created at that time. And we make sure that the times used are evenly distributed across that 12 hour window so there is never a thundering herd of invalidations. (my example uses 24 and 12 hours because the default expiration time is 24 hours, but the logic is the same if you say it expires in is 2 hours or 24 hours, it just makes sure that there is at least half of the availability time left before it expires) Note: this fixes the canvas side so we actually use the same url, we still need to fix things on the inst-fs side so that they send a “304 not modified” when you send a request to a url that you already have in your browser cache. Test plan: * once this is applied, in an environment that is set up to use inst-fs: * as a user enrolled in at least one course that uses an inst-fs image as it’s dashcard image * load the dashboard * a few seconds later, load it again * both requests should have used the same url to that image Change-Id: Id059874ac3e7b51646f28028faad3cb8eda2a816 Reviewed-on: https://gerrit.instructure.com/192174 Tested-by: Jenkins QA-Review: Brent Burgoyne <bburgoyne@instructure.com> QA-Review: Rob Orton <rob@instructure.com> Product-Review: Brent Burgoyne <bburgoyne@instructure.com> Product-Review: Rob Orton <rob@instructure.com> Reviewed-by: Rob Orton <rob@instructure.com> Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
This commit is contained in:
parent
1ce4492b8c
commit
4eb697d6c7
|
@ -183,15 +183,56 @@ module InstFS
|
|||
"/thumbnails/#{attachment.instfs_uuid}"
|
||||
end
|
||||
|
||||
def service_jwt(claims, expires_in)
|
||||
Canvas::Security.create_jwt(claims, expires_in.from_now, self.jwt_secret, :HS512)
|
||||
# `expires_at` can be either a Time or an ActiveSupport::Duration
|
||||
def service_jwt(claims, expires_at)
|
||||
expires_at = expires_at.from_now if expires_at.respond_to?(:from_now)
|
||||
Canvas::Security.create_jwt(claims, expires_at, self.jwt_secret, :HS512)
|
||||
end
|
||||
|
||||
# floor_to rounds `number` down to a multiple of the chosen step.
|
||||
def floor_to(number, step)
|
||||
whole, remainder = number.divmod(step)
|
||||
whole * step
|
||||
end
|
||||
|
||||
# If we just say every token was created at Time.now, since that token
|
||||
# is included in the url, every time we make a url it will be a new url and no browser
|
||||
# will never be able to get it from their cache. Which means, for example: every time you
|
||||
# load your dash cards you will download all new thumbnails instead of using one from
|
||||
# your browser cache. That's not what we want to do.
|
||||
#
|
||||
# But we also don't want to just have them all expire at the same time because then we'd
|
||||
# get a thundering herd at the end of that cache window.
|
||||
#
|
||||
# So what we do is have all tokens for a certain resource say they were signed at same
|
||||
# time within a 12 hour window. that way you're browser will be able to cache it for at
|
||||
# least 12 hours and up to 24. And instead of picking something like the beginning of
|
||||
# the day or hour, we use a random offset that is evenly distributed throughout the
|
||||
# cache window. (this example uses 24 and 12 hours because the default expiration time is
|
||||
# 24 hours, but the logic is the same if you say expires_in is 2 hours or 24 hours, it
|
||||
# just makes sure that there is at least half of the availibilty time left before it expires)
|
||||
def consistent_iat(resource, expires_in)
|
||||
now = Time.now.utc.to_i
|
||||
window = expires_in.to_i / 2
|
||||
beginning_of_cache_window = floor_to(now, window)
|
||||
this_resources_random_offset = resource.hash % window
|
||||
if (beginning_of_cache_window + this_resources_random_offset) > now
|
||||
# step back a window if adding the random offset would put us into the future
|
||||
beginning_of_cache_window -= window
|
||||
end
|
||||
beginning_of_cache_window + this_resources_random_offset
|
||||
end
|
||||
|
||||
def access_jwt(resource, options={})
|
||||
expires_in = Setting.get('instfs.access_jwt.expiration_hours', '24').to_i.hours
|
||||
expires_in = options[:expires_in] || expires_in
|
||||
expires_in = options[:expires_in] || Setting.get('instfs.access_jwt.expiration_hours', '24').to_i.hours
|
||||
if (expires_in >= 1.hour.to_i) && Setting.get('instfs.access_jwt.use_consistent_iat', 'true') == "true"
|
||||
iat = consistent_iat(resource, expires_in)
|
||||
else
|
||||
iat = Time.now.utc.to_i
|
||||
end
|
||||
|
||||
claims = {
|
||||
iat: Time.now.utc.to_i,
|
||||
iat: iat,
|
||||
user_id: options[:user]&.global_id&.to_s,
|
||||
resource: resource,
|
||||
host: options[:oauth_host]
|
||||
|
@ -200,7 +241,7 @@ module InstFS
|
|||
claims[:acting_as_user_id] = options[:acting_as].global_id.to_s
|
||||
end
|
||||
amend_claims_for_access_token(claims, options[:access_token], options[:root_account])
|
||||
service_jwt(claims, expires_in)
|
||||
service_jwt(claims, Time.zone.at(iat) + expires_in)
|
||||
end
|
||||
|
||||
def upload_jwt(user:, acting_as:, access_token:, root_account:, capture_url:, capture_params:)
|
||||
|
|
|
@ -107,6 +107,22 @@ describe InstFS do
|
|||
end
|
||||
end
|
||||
|
||||
it "generates the same url within a cache window of time so it's not unique every time" do
|
||||
url1 = InstFS.authenticated_url(@attachment)
|
||||
url2 = InstFS.authenticated_url(@attachment)
|
||||
expect(url1).to eq(url2)
|
||||
|
||||
Timecop.freeze(1.day.from_now) do
|
||||
url3 = InstFS.authenticated_url(@attachment)
|
||||
expect(url1).to_not eq(url3)
|
||||
|
||||
first_token = url1.split(/token=/).last
|
||||
expect(->{
|
||||
Canvas::Security.decode_jwt(first_token, [ @secret ])
|
||||
}).to raise_error(Canvas::Security::TokenExpired)
|
||||
end
|
||||
end
|
||||
|
||||
describe "jwt claims" do
|
||||
def claims_for(options={})
|
||||
url = InstFS.authenticated_url(@attachment, options)
|
||||
|
@ -114,6 +130,22 @@ describe InstFS do
|
|||
Canvas::Security.decode_jwt(token, [ @secret ])
|
||||
end
|
||||
|
||||
it "no matter what time it is, the token has no less than 12 hours of validity left and never more than 24" do
|
||||
24.times do |i|
|
||||
Timecop.freeze(i.hours.from_now) do
|
||||
claims = claims_for()
|
||||
now = Time.zone.now
|
||||
exp = Time.zone.at(claims['exp'])
|
||||
expect(exp).to be > now + 12.hours
|
||||
expect(exp).to be < now + 24.hours
|
||||
|
||||
iat = Time.zone.at(claims['iat'])
|
||||
expect(iat).to be <= now
|
||||
expect(iat).to be > now - 12.hours
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it "includes global user_id claim in the token if user provided" do
|
||||
user = user_model
|
||||
claims = claims_for(user: user)
|
||||
|
|
Loading…
Reference in New Issue