Add ID token validation

[joshcanhelp]

This PR adds ID token validation to the basic OmniAuth-Auth0 strategy. The main strategy was not altered in any way functionally except to add this validation (minor docs additions and minor formatting). The main work for this PR is in the new OmniAuth::Auth0::JWTValidator class.

This uses the Ruby JWT library and validates the following:

• Signature
• Expiration (30 sec leeway)
• Issuer
• Audience
• Not Before (30 sec leeway)

[machuga]

Options "class"? What does that mean here? Should it say object?

[machuga]

4 space indentation - should be 2

[machuga]

If you return {} from here, can the rest of the library execute? Or will this get caught in the else clause of decode and raise the JWT::VerificationError?

[machuga]

What would you name this line if you had to give it a name? What are we checking for?

[joshcanhelp]

If you return {} from here, can the rest of the library execute?

The part of this lib that uses it will fail on looking for an algorithm, which seems rational to me for an empty head.

What would you name this line if you had to give it a name?

Not sure what you're asking here.

What are we checking for?

Whether or not was have something to parse.

[machuga]

This line and the one above seem strange. A conditional may make more sense.

[joshcanhelp]

Can you rephrase? This allows a URI to be loaded without http:// or https:// and will convert it.

[machuga]

Sure! So here you're assigning to domain_url twice (you're also shadowing the method name itself which is confusing) and I get why, it just feels odd. However, I think moving this up to the constructor would make sense anyway. For instance:

def initialize(options)
        @issuer = "#{domain_url}/"
        @client_id = options.client_id
        @client_secret = options.client_secret
        @jwks = nil

        temp_domain = URI(options.domain)
        @domain = temp_domain.scheme? ? temp_domain : URI("https://#{options.domain}")
      end

Then this method can simply return @domain.to_s

[joshcanhelp]

@machuga - Good comment. I ended up removing this method and the @domain property since it was only used for @issuer.

[machuga]

You should be able to shorten this down to:

def jwks_key(key, kid)
  @jwks ||= fetch_jwks
  return nil if blank?(@jwks[:keys])
  @jwks[:keys].find { |key| jwk[:kid] == kid }
end

However, you're memoizing @jwks here in a method that isn't named jwks or directly related and that makes me a bit uncomfortable. Maybe it should fetch_jwks should be the location where this is memoized? Maybe the flow could be a bit different?

[joshcanhelp]

I agree RE: where this is memoized so I moved that to fetch_jwks. But I think you're misunderstanding what this is doing. The JWKS is like:

{
  keys [
    {
      "kid" : "kid_1",
      "key" : "thing_we_want"
    }, {
      "kid" : "kid_2",
      "key" : "thing_we_want"
    }
  ]
}

... in the general case so we need to iterate through the keys, check the kid for each, and return the value if we have a matching kid. Otherwise return nil. I simplified it a bit based on your feedback but what you're proposing won't do what we need here.

[machuga]

I'm mostly just being picky about the Ruby idioms in this situation!

[machuga]

Is this intended to be a public method?

[joshcanhelp]

It was but I think it should be private in this case.

[esarafianou]

LGTM from a security perspective

[machuga]

Last few requests and then I think we'll be good!

[machuga]

So what I was trying to point out earlier about this method name was that if you keep it as jwks you can then us this memoized form around the app, like above in jwks_key:

      def jwks_key(key, kid)
        return nil if blank?(jwks[:keys])
        jwks[:keys].each { |jwk| return jwk[key] if jwk[:kid] == kid }
      end

That way you're never concerned with when/if the fetching is done.

[machuga]

I think this method may look a bit more clear as:

def jwks_key(key, kid)
  return nil if blank?(jwks[:keys])
  matching_jwk = jwks[:keys].find { |jwk| jwk[:kid] == kid }
  matching_jwk[key] if matching_jwk
end

This way uses a clear method name/block to indicate "I'm looking for a certain key, then going to perform an operation with what I found"

[machuga]

A more idiomatic way of handling the hash assignment in this block may be:

credentials do
	# Not sure of the name `hash` here...what is its purpose?
	hash = {
		'token' => access_token.token,
		'expires' => true
	}

	if access_token.params
		hash.merge!({
			'id_token' => access_token.params['id_token'],
			'token_type' => access_token.params['token_type'],
			'refresh_token' => access_token.refresh_token,
		})
	end

	# Make sure the ID token can be verified and decoded.
	auth0_jwt = OmniAuth::Auth0::JWTValidator.new(options)
	fail!(:invalid_id_token) unless auth0_jwt.decode(hash['id_token']).length

	hash
end