Guard against OIDC Provider configuration being stale #5296
Conversation
Fixes TOB-RGM-14
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5296 +/- ##
=======================================
Coverage 97.14% 97.15%
=======================================
Files 457 458 +1
Lines 9564 9581 +17
=======================================
+ Hits 9291 9308 +17
Misses 273 273 ☔ View full report in Codecov by Sentry. |
|
|
||
| def verify_jwt_time | ||
| now = Time.now.to_i | ||
| return if @jwt["nbf"] <= now && now < @jwt["exp"] |
There was a problem hiding this comment.
🤷♂️
| return if @jwt["nbf"] <= now && now < @jwt["exp"] | |
| return if (@jwt["nbf"]...@jwt["exp"]).include?(now) |
It creates a range so maybe slightly less efficient, your call.
| # TODO: delete &. after all providers have updated their configuration | ||
| return unless @provider.configuration_updated_at&.before?(1.day.ago) |
There was a problem hiding this comment.
Something about this is very hard to reason about for me. I think it almost reads like a double negative.
| # TODO: delete &. after all providers have updated their configuration | |
| return unless @provider.configuration_updated_at&.before?(1.day.ago) | |
| return if @provider.configuration_updated_at.nil? # TODO: remove this line after all providers have updated their configuration | |
| return if @provider.configuration_updated_at.after?(1.day.ago) |
| @@ -0,0 +1,5 @@ | |||
| class AddConfigurationUpdatedAtToOIDCProviders < ActiveRecord::Migration[7.1] | |||
| def change | |||
| add_column :oidc_providers, :configuration_updated_at, :timestamp | |||
There was a problem hiding this comment.
What if you set a default here of epoch just so we don't have to guard for nil? I'm wondering if new providers will ever be updated immediately anyway or will there be a time gap while they crash the check above?
There was a problem hiding this comment.
they'll all be updated within 30 minutes
| raise UnsupportedIssuer, "Provider is missing jwks" if @provider.jwks.blank? | ||
| # TODO: delete &. after all providers have updated their configuration | ||
| return unless @provider.configuration_updated_at&.before?(1.day.ago) | ||
| raise UnsupportedIssuer, "Configuration last updated too long ago: #{@provider.configuration_updated_at}" |
There was a problem hiding this comment.
When this error happens there's nothing the user can do about it, no?
Should we indicate this with a 503, in effect saying "it's not you it's me".
| %w[nbf iat exp].each do |claim| | ||
| raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(Integer) | ||
| end | ||
| %w[iss jti].each do |claim| | ||
| raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(String) | ||
| end |
There was a problem hiding this comment.
Nit: could extract to constant {iss: String, ...} which would allocate once and be a little simpler.
| end | ||
|
|
||
| def decode_jwt | ||
| @jwt = JSON::JWT.decode_compact_serialized(params.permit(:jwt).require(:jwt), jwt_key_or_secret) |
There was a problem hiding this comment.
Is :skip_verification handled by the JWT lib?
Fixes TOB-RGM-14