-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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
Validate auth0 token #725
Validate auth0 token #725
Conversation
…ction to our Controllers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @lgarofalo has more experience than me on JWTs and stuff, so he might know if there's a general strategy for the caching of JWKS that we should be following.
app/controllers/concerns/secured.rb
Outdated
|
||
module Secured | ||
extend ActiveSupport::Concern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could maybe wait for another PR, for when we actually integrate this into a route, but I think it'd be good to write a comment describing what this is and how to use it. Outside of this repo, I haven't done any serious Ruby on Rails development in a really, really long time, and I've never heard of a "Concern" before, so I guess it must have been added after I stopped doing Rails work.
Since you mix this in to the ApplicationController
, does this basically just mean that we just need to make sure #authorize
is called in a before:
block for any routes that we want to guard on authentication?
Also, not blocking this PR, but how will we insert more advanced authorization logic? To be precise, I'm going to use "authentication" and "authorization" in their more precision definitions, which is that "authentication" means "verify person is who they claim to be (i.e. they can log in to the user account)" and "authorization" means "verify person should have access to a particular resource". I believe that Auth0 gives us the authentication part, but, it's up to us to implement more specific authorization checks afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some basic comments in this file, which I will flesh out as we begin to use this utility. I haven't thought too much about how it will work with Authz checks. We may have to create a separate concern for that, which will draw upon the Roles/Groups tables we created some months ago
|
||
def self.jwks | ||
jwks_uri = URI("#{domain_url}.well-known/jwks.json") | ||
Net::HTTP.get_response jwks_uri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we should be caching? I'm a bit concerned about performance and reliability if every request to one of our auth-guarded routes also has to perform an HTTP request to Auth0 before being able to validate JWTs.
I only did a quick Google search on it, but I found this StackOverflow post that says that the JWKS keys are usually valid for months at a time. That StackOverflow post also says that the ordinary HTTP Cache Control headers on the HTTP response should let us know how long it should be valid for, so we could even build in a little cache expiration thing that first checks if our JWKS tokens are valid, and only if it isn't, then fetch a new one from Auth0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. I dug a bit into Auth0's docs, as well as community questions, and it seems like JWKS keys issued by this endpoint may not have an expiration date AT ALL. We could cache them indefinitely and/or for some arbitrary time, like a month? I think it is maybe a good idea to punt this for now and maybe it bundle it up with our user authorization implementation. I created a ticket for this in the meantime: #726
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes, this LGTM!
For the JWKS caching thing, it's probably something we want to do, but I suppose it doesn't need to block this PR.
lib/auth0_client.rb
Outdated
# rubocop:disable Lint/ShadowedException | ||
rescue JWT::DecodeError, JWT::VerificationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Rubocop error is because JWT::VerificationError
is a subclass of JWT::DecodeError
, which means that it's redundant to specify JWT::VerificationError
because JWT::DecodeError
will already handle it.
Maybe remove the redundant error, which should allow you to remove the rubocop:disable
comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh thanks! That was stumping me
I lifted most of this code from Auth0's Rails tutorial here. This can be merged in as is, given it isn't being used yet to lock down any of our endpoints. The Secured#authorize method can be included in any given controller as a
before_action
(with specific controller methods listed out as needed) to restrict access. Let me know if there's any feedback/requests.