Kr/fix expired token error (#3612)

* WIP raise JWT expired token error as a 401

* Update code for expired token

* Make error handling a little better

* Have more style

* Handle jwt decode errors

* Move JWT rescues,refactor more

* Move token logic to its own class, refactor

* Update other auth controllers

* Refactor error raising

* More refactor

* Make more stylish

app/controllers/application_controller.rb

@@ -87,6 +87,8 @@ def skip_sentry_exception_types
       case exception
       when Pundit::NotAuthorizedError
         Common::Exceptions::Forbidden.new(detail: 'User does not have access to the requested resource')
+      when Common::Exceptions::TokenValidationError
+        Common::Exceptions::Unauthorized.new(detail: exception.detail)
       when ActionController::ParameterMissing
         Common::Exceptions::ParameterMissing.new(exception.param)
       when ActionController::UnknownFormat

app/controllers/openid_application_controller.rb

@@ -15,10 +15,10 @@ class OpenidApplicationController < ApplicationController
   private
 
   def permit_scopes(scopes, actions: [])
-    return false unless token_payload
+    return false unless token.payload
 
     if actions.empty? || Array.wrap(actions).map(&:to_s).include?(action_name)
-      render_unauthorized if (Array.wrap(scopes) & token_payload['scp']).empty?
+      render_unauthorized if (Array.wrap(scopes) & token.payload['scp']).empty?
     end
   end
 
@@ -40,41 +40,22 @@ def token_from_request
     auth_request = request.authorization.to_s
     return unless auth_request[TOKEN_REGEX]
 
-    auth_request.sub(TOKEN_REGEX, '').gsub(/^"|"$/, '')
+    Token.new(auth_request.sub(TOKEN_REGEX, '').gsub(/^"|"$/, ''))
   end
 
   def establish_session
-    validate_token
-    ttl = token_payload['exp'] - Time.current.utc.to_i
-    profile = fetch_profile(token_identifiers.okta_uid)
-    user_identity = OpenidUserIdentity.build_from_okta_profile(uuid: token_identifiers.uuid, profile: profile, ttl: ttl)
+    ttl = token.payload['exp'] - Time.current.utc.to_i
+    profile = fetch_profile(token.identifiers.okta_uid)
+    user_identity = OpenidUserIdentity.build_from_okta_profile(uuid: token.identifiers.uuid, profile: profile, ttl: ttl)
     @current_user = OpenidUser.build_from_identity(identity: user_identity, ttl: ttl)
-    @session = build_session(token, token_identifiers.uuid, ttl)
+    @session = build_session(ttl)
     @session.save && user_identity.save && @current_user.save
-  rescue => e
-    Rails.logger.warn(e)
   end
 
   def token
     @token ||= token_from_request
   end
 
-  def token_payload
-    @token_payload ||= if token
-                         pubkey = expected_key(token)
-                         return if pubkey.blank?
-
-                         JWT.decode(token, pubkey, true, algorithm: 'RS256')[0]
-                       end
-  end
-
-  def validate_token
-    raise 'Validation error: no payload to validate' unless token_payload
-    raise 'Validation error: issuer' unless token_payload['iss'] == Settings.oidc.issuer
-    raise 'Validation error: audience' unless token_payload['aud'] == Settings.oidc.audience
-    raise 'Validation error: ttl' unless token_payload['exp'] >= Time.current.utc.to_i
-  end
-
   def fetch_profile(uid)
     profile_response = Okta::Service.new.user(uid)
     if profile_response.success?
@@ -86,30 +67,11 @@ def fetch_profile(uid)
     end
   end
 
-  def token_identifiers
-    # Here the `sub` field is the same value as the `uuid` field from the original upstream ID.me
-    # SAML response. We use this as the primary identifier of the user because, despite openid user
-    # records being controlled by okta, we want to remain consistent with the va.gov SSO process
-    # that consumes the SAML response directly, outside the openid flow.
-    # Example of an upstream uuid for the user: cfa32244569841a090ad9d2f0524cf38
-    # Example of an okta uid for the user: 00u2p9far4ihDAEX82p7
-    @token_identifiers ||= OpenStruct.new(
-      uuid: token_payload['sub'],
-      okta_uid: token_payload['uid']
-    )
-  end
-
-  def build_session(token, uuid, ttl)
-    session = Session.new(token: token, uuid: uuid)
+  def build_session(ttl)
+    session = Session.new(token: token.to_s, uuid: token.identifiers.uuid)
     session.expire(ttl)
     session
   end
 
-  def expected_key(token)
-    decoded_token = JWT.decode(token, nil, false, algorithm: 'RS256')
-    kid = decoded_token[1]['kid']
-    OIDC::KeyService.get_key(kid)
-  end
-
   attr_reader :current_user, :session, :scopes
 end

app/models/token.rb

@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+class Token
+  attr_reader :token_string
+
+  def initialize(token_string)
+    @token_string = token_string
+    validate_token
+  end
+
+  def to_s
+    @token_string
+  end
+
+  def payload
+    @payload ||= if @token_string
+                   pubkey = public_key
+                   return if pubkey.blank?
+
+                   JWT.decode(@token_string, pubkey, true, algorithm: 'RS256')[0]
+                 end
+  rescue JWT::ExpiredSignature => e
+    raise error_klass("Validation error: #{e.message}")
+  end
+
+  def public_key
+    decoded_token = JWT.decode(@token_string, nil, false, algorithm: 'RS256')
+    kid = decoded_token[1]['kid']
+    OIDC::KeyService.get_key(kid)
+  rescue JWT::DecodeError => e
+    raise error_klass("Validation error: #{e.message}")
+  end
+
+  def validate_token
+    raise error_klass('Validation error: no payload to validate') unless payload
+    raise error_klass('Validation error: issuer') unless valid_issuer?
+    raise error_klass('Validation error: audience') unless valid_audience?
+  end
+
+  def valid_issuer?
+    payload['iss'] == Settings.oidc.issuer
+  end
+
+  def valid_audience?
+    payload['aud'] == Settings.oidc.audience
+  end
+
+  def identifiers
+    # Here the `sub` field is the same value as the `uuid` field from the original upstream ID.me
+    # SAML response. We use this as the primary identifier of the user because, despite openid user
+    # records being controlled by okta, we want to remain consistent with the va.gov SSO process
+    # that consumes the SAML response directly, outside the openid flow.
+    # Example of an upstream uuid for the user: cfa32244569841a090ad9d2f0524cf38
+    # Example of an okta uid for the user: 00u2p9far4ihDAEX82p7
+    @identifiers ||= OpenStruct.new(
+      uuid: payload['sub'],
+      okta_uid: payload['uid']
+    )
+  end
+
+  def error_klass(error_detail_string)
+    Common::Exceptions::TokenValidationError.new(detail: error_detail_string)
+  end
+end

config/locales/exceptions.en.yml

@@ -177,6 +177,12 @@ en:
         detail: "%{detail}"
         code: 108
         status: 400
+      token_validation_error:
+        <<: *defaults
+        title: Token Validation Error
+        detail: "%{detail}"
+        code: 401
+        status: 401
 
 # EXTERNAL EXCEPTIONS
     # This is a Generic Error corresponding to backend services

lib/common/exceptions.rb

@@ -31,3 +31,4 @@
 require_dependency 'common/exceptions/external/bad_gateway'
 require_dependency 'common/exceptions/external/bing_service_error'
 require_dependency 'common/exceptions/internal/ambiguous_request'
+require_dependency 'common/exceptions/internal/token_validation_error'

lib/common/exceptions/internal/token_validation_error.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Common
+  module Exceptions
+    # Validation Error - an ActiveModel having validation errors, can be sent to this exception
+    class TokenValidationError < BaseError
+      attr_reader :detail
+
+      def initialize(options = {})
+        @detail = options[:detail]
+        @source = options[:source]
+      end
+
+      def errors
+        Array(SerializableError.new(i18n_data.merge(detail: @detail, source: @source)))
+      end
+    end
+  end
+end

modules/openid_auth/app/controllers/openid_auth/application_controller.rb

@@ -8,7 +8,7 @@ def validate_user
       raise Common::Exceptions::RecordNotFound, @current_user.uuid if @current_user.va_profile_status == 'NOT_FOUND'
       raise Common::Exceptions::BadGateway if @current_user.va_profile_status == 'SERVER_ERROR'
 
-      obscure_token = Session.obscure_token(token)
+      obscure_token = Session.obscure_token(token.to_s)
       Rails.logger.info("Logged in user with id #{@session.uuid}, token #{obscure_token}")
     end
   end

modules/openid_auth/app/controllers/openid_auth/v0/validation_controller.rb

@@ -17,7 +17,7 @@ def index
 
       def validated_payload
         # Ensure the token has an `va_identifiers` key.
-        payload_object = OpenStruct.new(token_payload.merge(va_identifiers: { icn: nil }))
+        payload_object = OpenStruct.new(token.payload.merge(va_identifiers: { icn: nil }))
 
         # Sometimes we'll already have an `icn` in the token. If we do, copy if down into `va_identifiers`
         # for consistency. Otherwise use the ICN value we used to look up the MVI attributes.