Closed christophweegen closed 6 years ago
Hi @christophweegen ,
thanks for taking your time trying to improve this gem. However, in general I think that maybe it is better to improve the documentation instead of adding more methods to the helper. I'll explain; almost everything you proposed can be done with just a method call in Warden::JWTAuth
. But I'll go step by step:
jwt_for(user) # returns valid JWT for given user
Warden::JWTAuth::UserEncoder.new.call(user)
jwt_for!(user) # returns a valid JWT for given user, but respects the full devise-jwt sign_in flow # by making a real sign_in request and return the associated JWT. # Possibly slower but useful for exercising `devise-jwt` revocation strategies # where JWT tracking gets included in the database
How would you go through the sign_in flow? Read later about auth_request
method.
revoke_jwt(jwt) # revokes the given JWT if any of the revocation strategies is used. # Actually a sign_out if you want to name it like that.
Warden::JWTAuth::TokenRevoker.new.call(jwt)
auth_headers!(headers, user) # Does the same as the already existing auth_headers methods, but like jwt_for! it also # respects the full sign_in flow for the same reasons mentioned.
Same than jwt_for!
.
auth_request(user, http_method, url, headers: {}, default: true) # Makes an authorized http request for given user, http_methods and url # with default headers = { 'Accept' => 'application/json', 'Content-Type' => >'application/json' }. # You can simply add additional headers to the default ones by providing 'headers' argument. # If you want to make up your headers yourself ( besides "Authorization" which is given by user ) # just set default: false, so default headers don't get included.
I also thought about something like that when creating the TestHelpers
module. However I reached the conclusion that it was better to just provide the minimal behavior (adding a JWT to given headers) for several reasons:
default
parameter tells us. At the end a lot of people would end up wrapping this helper module with its own method so that they don't have to provide the same arguments every time. At the end, it is the same that you'll end up doing with the single helper present right now.get
, post
, etc. For a framework it can be acceptable, but not for a gem. At the end, with just auth_headers
function what I want is to direct people towards a custom helper or, at least, to just use our gem for the minimal needed behavior.auth_request!(user, http_method, url, headers: {}, default: true) # Same as auth_request but again respects the full devise-jwt # sign_in flow for the same reasons mentioned above in the other bang methods.
Same than before.
global_sign_out(user) # revokes all valid tokens for given user which are tracked by the database. # Would probably be most useful/only be reasonable with whitelist strategy. # I imagine a use case if some attacker has gained access to JWT by XSS or similar # and you instantly want to revoke all tokens.
I think that could be nice as a class method for the whitelist strategy.
nuke_jwts_from orbit!
# Deletes all valid JWTs from database for all users.
# Same could be achieved by altering your secret key, but this method would be useful
# if you don't want / can't alter your secret key.
# I guess only useful for JTIMatcher/Whitelist strategy.
I think it is simpler to just change the secret key. You can still remove all records in the tables easily as any AR model. But maybe it could be nice to add methods to delete expired tokens for each strategy.
Hey thanks for your answer!
Yes i wasn't quite sure how much sense it would make, adding these methods. That's why i just proposed them for discussion. Since you're much more involved in the internals, you have a deeper understanding what's going on.
almost everything you proposed can be done with just a method call in Warden::JWTAuth. But I'll go step by step:
Great, to know where I have to search for methods. Didn't dig through the full codebase right now.
How would you go through the sign_in flow? I would create a valid user in Rspec and just make a
POST /sign_in
request with the user credentials in the helper method and extract the returned JWT. :-)Making the request can be very specific to an application, as the need to introduce default parameter tells us.
My intention behind the default headers was the thought that usually people would use devise-jwt
for an API and thus would use json as their default format in usually most cases.
For people that could go with the default call, maybe they will end up using it all across its tests. For a functionality that is so heavily used, I think it is a bad idea to use a third party method (our gem) and that it is much better to wrap it in a custom helper.
I thought since devise
is already providing its own helper methods, you usually use for almost all tests too, why not provide some more or less analogues to that in devise-jwt
with some good defaults, that almost anybody can use? If you need tweaking you can still do though..
There were already backward compatibility headaches out there with Rails updating its signature for methods like get, post, etc. For a framework it can be acceptable, but not for a gem. At the end, with just auth_headers function what I want is to direct people towards a custom helper or, at least, to just use our gem for the minimal needed behavior.
Does it always make sense to provide backward compatibility for too many old version? Most of them have already too much incompatibilites or even vulnerabilities on their own. I mean when does it really make sense to combine a current version of devise-jwt
with an old version of rails?. If you start from scratch you can directly use a current version, and if you have an old running app you maybe already have your auth implemented and can't change it this easily without the whole app crashing.
So you would maybe directly consider to update your app with a newer rails version while simultaneously implementing devise-jwt. You could also integrate some modern features this way too and maybe get some old vulnerabilities out of your app.
If your backwards compatibility is too strict, isn't it difficult too evolve your gem at all?
You could add new functionality and changing the minor version and if something is incompatible also the major version and make a hint in the docs. People who need older behaviour could just use an older version then.
I'm not sure about all of this, at least that's what I would do. Or do I see smth. wrong?
You have a broader overview over the topic, so you will know what's best..
Great, to know where I have to search for methods. Didn't dig through the full codebase right now.
That's my fault, surely it would be nice to point to the right direction from the README. I'll change it.
I thought since devise is already providing its own helper methods, you usually use for almost all tests too, why not provide some more or less analogues to that in devise-jwt with some good defaults, that almost anybody can use? If you need tweaking you can still do though..
But here it is different. devise
sign_in
helper just puts the user into the session, and then you can go on with standard rails testing calls, like get
or post
. In our case, we are talking about wrapping those calls inside a "vendor helper" that would be used in place. In a way, our auth_headers
is the equivalent of devise
sign_in
; it just provides the minimal functionality specific to our gem.
Does it always make sense to provide backward compatibility for too many old version?
It's not that I don't want to add auth_request
method for backward compatibility. Maybe you can say I don't want to add it for forward compatibility :) I'll try to explain it better.
Imagine we introduce a method auth_request
and people start using it everywhere in their tests. If someday we decide to change, for example, its signature, then all those tests will fail. In essence, I want to say that adding a method to the library that goes beyond the library purpose is a bad practice. A library must implement the minimal behavior that it wants to address.
I think that it is easier to understand if you look at it from a developer point of view. I'm developing a Rails application and I add a dependency to be used for authentication purposes. Adding a dependency has to be always a very conscious decision. I'm adding code that I don't control and that may change at any time (because of course I want to keep the dependency updated for security reasons). For that, and specially for broader impact libraries, it is always a good practice to wrap the library API in own modules. For example, if I'm going to use library's sign_in
method in 100 places, it is better if I wrap it in my own sign_in
method which just delegates to the vendor one. This way, if something changes the fix has to be done in just one place and not in a hundred places.
In our case, I'd like to point developers to the good direction. I don't want them to use a helper from here in 100 places. It is better if they are forced to implement its own (which will use auth_headers
under the hood). Or, at least, they will just use auth_headers
all around, but that's the minimal behavior we must implement from our end.
Hey @christophweegen ,
what do you think about that?
global_sign_out(user) # revokes all valid tokens for given user which are tracked by the database. # Would probably be most useful/only be reasonable with whitelist strategy. # I imagine a use case if some attacker has gained access to JWT by XSS or similar # and you instantly want to revoke all tokens.
I think that could be nice as a class method for the whitelist strategy.
nuke_jwts_from orbit! # Deletes all valid JWTs from database for all users. # Same could be achieved by altering your secret key, but this method would be useful # if you don't want / can't alter your secret key. # I guess only useful for JTIMatcher/Whitelist strategy.
I think it is simpler to just change the secret key. You can still remove all records in the tables easily as any AR model. But maybe it could be nice to add methods to delete expired tokens for each strategy.
Ara you interested in implementing it?
Hey @waiting-for-dev,
I'm sorry for my late reply. Had it on my to-do list all the time, but didn't find the time to answer adequately. Some busy days the last weeks..
Your objections make a lot of sense and I'm aware of them. Actually i was thinking about working out something with auth_request
, that's unprobable to change in future.
But i don't know now what and if you maybe want to change in future, that's why i want to specify how i would implement these things exactly. Worked out a first draft back then.
Actually it doesn't introduce anything new. Just a recombination of the already existing auth_headers
and standard rails testing HTTP calls ( something that's very unlikely to change in future, since HTTP will be quite some time around and rails/devise conventions for routes won't be changed in future too, i guess, since they are so deep buried in the philosophy of the framework ).
Didn't consider the following yet, so maybe some/all of the proposed can be refactored. This first draft just for clarifying what I had in mind.
That's my fault, surely it would be nice to point to the right direction from the README. I'll change it.
Here's the first draft, note that not all proposed methods are included yet, since I will first wait for your answer if you think it's a good idea to include them after reading this. I included rather verbose comments for convenience, which could then be extracted into the README in case.
module Devise
module JWT
module TestHelpers
# returns a JWT for user
def self.jwt_for(user)
headers = Devise::JWT::TestHelpers.auth_headers({}, user)
headers["Authorization"].split(' ')[1]
end
# Returns a JWT for user but respects the full devise sign_in flow.
# This is useful if you want to test the generation and revokation
# of JWTs if you chose one of the revokation strategies
# that track your JWTs in the database
def self.jwt_for!(user)
params = { email: user.email,
password: user.password }.to_json
headers = { "Content-Type" => "application/json" }
post user_session_path, params: params, headers: headers
response.headers["Authorization"].split(' ')[1]
end
# Provides authorization headers like Devise::JWT::TestHelpers.auth_headers
# but additionally respects the full devise sign_in flow, so it can be used
# to track JWTs generation/revokation in your database while testing.
# Generates a new JWT each time it's called, so you should probably want
# to cache it in a variable if you want to authenticate multiple requests
# with the same headers.
def self.auth_headers!(user)
headers = { 'Accept' => 'application/json', 'Content-Type' => 'application/json' }
jwt = Devise::JWT::TestHelpers.jwt_for!(user)
headers.merge("Authorization" => "Bearer #{jwt}")
end
# revokes a previously generated jwt if theres a revocation strategy present
def self.revoke_jwt(jwt)
headers = { "Content-Type" => "application/json" }
headers.merge("Authorization" => "Bearer #{jwt}")
delete destroy_user_session, headers: headers
end
# can be included in your tests if you like short methods.
# # `include Devise::JWT::TestHelpers::Wrappers`
module Wrappers
def jwt_for(user)
Devise::JWT::TestHelpers.jwt_for(user)
end
def jwt_for!(user)
Devise::JWT::TestHelpers.jwt_for!(user)
end
def auth_headers(headers, user)
Devise::JWT::TestHelpers.auth_headers(headers, user)
end
def auth_headers!(user)
Devise::JWT::TestHelpers.auth_headers!(user)
end
def revoke_jwt(jwt)
Devise::JWT::TestHelpers.revoke_jwt(jwt)
end
end
end
end
end
So you see there's only code involved, that's unlikely to change in future. I think rails won't change the way they implement requests/responses any time soon and in case i guess it would be quite easy to just update it on devise-jwt
side.
The only variable would be auth_headers
, but you control it and you could easily fix it, if maybe some of the underlying devise/warden stuff would change in future or if you want to change it somehow else, you could do it in a way that's backwards compatible. But I don't think there's much that has to be changed about smth. that's so common and basic like headers.
As you see i assumed that the user will follow some conventions about routes that go along with devise. But to customize that it would be very easy to add an optional request_path
argument into these methods.
And in the worst case the user would just have to override the provided methods with his own implementation if his test should break, which is just a little extra effort he would have to do anyway ( and in each project where he uses devise-jwt
) if we wouldn't provide these helpers.. ;-)
Additionally i appreciate gems that provide default functionality, even if you don't use it. But including the test helpers and a hint how they work and how you can customize them, is a nice thing to give developers ( especially rails beginners ) a little headstart while working on their projects, a thing i really enjoyed when i started rails ( and programming in general ).
I actually really like gems, which provide a quite detailed documentation, which acts more or less as a tutorial too. That's why I think it's a fantastic idea you had to make users of devise-jwt
read your great blog posts about jwt ( revocation ) first. I learned a lot by them too. So why not expand the rest of the documentation in a similar way ( where possible )?
To give a little background to you why i like this style of documentation: I worked 12 years full time as a guitar teacher and music tutor, that's why explaining things to people is maybe "in my DNA" and you're maybe wondering why my replies are often quite verbose too, so now you know why :-) Also i went through the struggle to learn rails and programming all by myself, so i just want to provide the information to beginners I wish i had when i was starting. :-) Just to share my philosophy that make me maybe consider some things different then maybe someone with a more technical background. :-) Additionally I started programming at all less than two years ago, so please have mercy with me if I still see some things a bit naive. :-)
Actually i strongly believe that extraordinary and thoroughly documentation is one of the motors of open source. It makes beginners start more easily and can often multiply the rate in which people grasp things and thus can push open source forward faster, rather then digging through thousand lines of source code first to understand how things work.
For beginners a thoroughly documentation is valuable lesson and tutorial material, while providing a quick refresher for more experienced developers at the same time. I made the experience, that it's always nice to skim over a good documentation to get into the gem's "mood", if you didn't work with it for quite a while, even if you already grasped it completely back then.
That's the reason why i chose devise-jwt
too. Additionally to the great functionality it provided a lot of valuable insights with your blog posts which are more a complete tutorial about jwt revocation, both features all the other related gems didn't provide.
The documentation is already very good as well. Easy to grasp and well structured. Actually a good documentation is always a hint for me that the author also invested time in smth. that's somewhat "optional", so it can give a first clue that she/he was conscientious with programming too. :-)
So following this philosophy i wanted to give some background about my motivations why i propose some things the way i do and you maybe better understand why i have one or another idea which seem unnecessary/unconventional at first. Since JWTs are still not so widespread in use, many people maybe still have questions about implementing devise-jwt
because it somewhat differs quite a bit from usual cookie sessions. According to my own experience while starting/learning about JWT in rails, this topic isn't well covered in depth if you want to have a more complete auth solution than just checking for current_user
. A gap, devise-jwt
can really fill, which i think has the potential for being the reference auth solution for JWT in Rails since nothing comparable already exist as far as i have discovered yet.
@waiting-for-dev referring https://github.com/waiting-for-dev/devise-jwt/issues/77#issuecomment-372744850
Of course. Would be a pleasure. Don't know when i will find time for it in the next days but I will provide a first draft as soon as possible.
I understand your point of view. In fact, there is an intense debate out there about simple vs easy. I agree that something easy flattens the entry path for a beginner, but you have to find an agreement between both if you think beyond. When you use something easy at expenses of introducing too much complexity or too much magic, my view is that you take a deceptive pill. At first it seems like a miraculous cure, but with the time you see plenty of secondary effects coming to the surface. I have seen too many broken designs because this exact issue.
One of the most common errors in programming is the failure to anticipate what can go wrong (or change) in the future. It is the source of uncountable broken abstractions. Looking behind, it isn't so long time ago that Rails 5 changed the signature of that precise methods.
At the end, as I said, I would consider that a bad practice. If we want to help beginners the best we can do is to point them in the good direction. Except for last two methods, I think that these are things that are much better tackled from the documentation.
Talking about the docs, I would be more than happy to improve them. I would leave the README specific to the gem behavior. For other related stuff, people are more than welcome to improve the wiki. For now, we already have a page about how to configure devise for APIs.
Of course. Would be a pleasure. Don't know when i will find time for it in the next days but I will provide a first draft as soon as possible.
Great, take your time. Just wanted to recap the state of each open issue.
Ok i guess you're right. Makes a lot of sense what you're saying. :-)
So should we make it this way?
devise-jwt
, which get linked in the testing part of the documentation. This way it won't break anything, beginners have a little introduction how they can test their devise-jwt
application and additionally my efforts weren't totally in vain. :-) ( Not that i care much about working on smth. that doesn't get used in the end the way it was intended. I rather would like to have them as a reference bundled with devise-jwt
for my future self too, so i don't have to dig through my projects source code every time i start a new project :-D )Other questions arise for the mentioned sign_out methods:
global_sign_out(user)
only makes sense for the whitelisted revocation strategy. JTIMatcher strategy has only one JWT to handle per user, so i guess there's already a method for that. Could you point out where it is located?
should i make global_sign_out(user)
and global_sign_out!(user)
, one which calls user.whitelisted_jwts.destroy_all
( handling callbacks ) and the other one user.whitelisted_jwts.delete_all
for deleting all JWTs of said user in a single DB call without callbacks for performance reasons?
For one user this wouldn't have such a big impact on performance i guess, but if you want to revoke all whitelisted JWTs for all users this should be a thing to be considered. I guess i would name these methods for consistency global_sign_out_users
and global_sign_out_users!
respectively.
For JTIMatcher strategy would it suffice for a global_sign_out_users
to set the jti
column on user
to an empty string with a single DB call to update_all
for performance reasons or would that make problems? nil
is not possible because of null: false
DB constraint. For including callbacks i will have to see what can be the best way.
I will add the two mentioned methods.
:+1:
The other proposed testing methods go into a wiki post about examples for testing devise-jwt, which get linked in the testing part of the documentation. This way it won't break anything, beginners have a little introduction how they can test their devise-jwt application and additionally my efforts weren't totally in vain. :-) ( Not that i care much about working on smth. that doesn't get used in the end the way it was intended. I rather would like to have them as a reference bundled with devise-jwt for my future self too, so i don't have to dig through my projects source code every time i start a new project :-D )
That is stuff 100% related with the gem, so for me it is ok to add it in the README.
About the auth_request
helper, I would add it as an example just after the sentence "Usually you will wrap this in your own test helper." that currently appears at the end of the testing section.
About handcrafting and revoking tokens manually, it is stuff not only related with testing. We could add another section "Handcrafting tokens" just before "Testing", and point from there to the API in Warden::JWTAuth
. It should be a very short section, just with an example of both calls. Maybe in the "Testing" section we could also point to the "Handcrafting tokens" section.
global_sign_out(user) only makes sense for the whitelisted revocation strategy. JTIMatcher strategy has only one JWT to handle per user, so i guess there's already a method for that. Could you point out where it is located?
should i make global_sign_out(user) and global_sign_out!(user), one which calls user.whitelisted_jwts.destroy_all ( handling callbacks ) and the other one user.whitelisted_jwts.delete_all for deleting all JWTs of said user in a single DB call without callbacks for performance reasons?
Just one, which calls delete_all
. We inform in the method documentation about that. Usually there are no callbacks in that model, but a user can easily call destroy_all
itself if needed.
For one user this wouldn't have such a big impact on performance i guess, but if you want to revoke all whitelisted JWTs for all users this should be a thing to be considered. I guess i would name these methods for consistency global_sign_out_users and global_sign_out_users! respectively.
I don't want to cover all scenarios and wrap Active Record API for each case. For that, a user can simply delete all records as in any other model.
For JTIMatcher strategy would it suffice for a global_sign_out_users to set the jti column on user to an empty string with a single DB call to update_all for performance reasons or would that make problems? nil is not possible because of null: false DB constraint. For including callbacks i will have to see what can be the best way.
It doesn't make too much sense in JTIMatcher
, we already have the revoke_jwt
method there, which is in one to one correspondence with a user (there is no several tokens for a single user, as in the whitelist).
When I talked about removing expired tokens, I mean removing the tokens that have the exp
claim before in time from now. It makes sense for blacklist and whitelist strategies.
Just one thing more, we should rename global_sign_out
to something else. We talk about revoking tokens, not signing out users. This is so because a token can be revoked in other scenarios besides signing out requests. For example, sometimes it make sense to revoke the token at each request and dispatch a new one. Maybe we could call it revoke_all_for_user
. The other one could be called revoke_expired
.
Good morning @waiting-for-dev :-)
I guess i misunderstood smth. in https://github.com/waiting-for-dev/devise-jwt/issues/77#issuecomment-372744850 somehow, now that i read it again, guess i didn't had much time back then and was tired while reading it :-D
Will reconsider things and get in touch again. :-)
Hey @christophweegen , going to close it because it seems quite dead. Anyway, feel free to submit your additions if you are still interested. I'd happily merge them :)
Hey @waiting-for-dev, sorry for the long inactivity. Didn't had much time the last months to work on it. Started a new job and so on. But still working/thinking about it.
What about putting it in a gem under the devise-jwt
namespace? So it's not included by default, since you're totally right about mixing up functionality, but can be added if one wishes to do so?
Thought about something like just including the module in the test helper from minitest/rspec then and you're good to go and have a bunch of multiple helper methods available?
Maybe we have some ideas for other extensions devise-jwt
could benefit from, so it could act as a namespace and there could evolve a little "ecosystem" around devise-jwt
, where others could contribute gems too. Just some thoughts that came to my mind, since as far as I know there's no complete modern token-based authentication for rails available right now.
Which name would you prefer to associate the test helpers gem with devise-jwt
since it won't run without devise-jwt
, so it would make sense to include that in the name? How about the module namespace? What about Devise::JWT::Extensions::TestHelpers
?
Hey @christophweegen ,
I really don't think that devise-jwt
has such a potentiality for extensions. It has a narrow scope, and if the need here is to add just a few testing helper methods, I guess we could add them right here. But, of course, if you think in a different way you are totally free to create another gem devise-jwt-whatever
:smile:
Hey @waiting-for-dev
I'm working on a project with
devise-jwt
, where I need some additional TestHelpers with general functionality, so i can extract them in a pull request if you would like to add some.Right now the testing could be more "streamlined" out of the box, at least that's what i would wish for ( and could need for sure all the time I'm working with
devise-jwt
).I don't know if it's your vision to implement some more. But I just want to suggest some methods, I'm working right now with. So in order to avoid work while extracting them for
devise-jwt
, I want to get some first feedback what you think about adding them.The usual very convenient sign_in/sign_out methods of
devise
won't work anymore so I thought about some substitutes that make testing withdevise-jwt
more easy and joyful :-)I just outline what have in mind:
First of all this will only be additions to the
Devise::JWT::TestHelpers
module, where also theauth_headers
method lives. Nothing gets patched or similar. So everything is backwards compatible and doesn't introduce new bugs regarding old functionality.So here are the methods I would like to introduce to
Devise::JWT::TestHelpers
and possibly an additionalDevise::JWT::TestHelpers::Shorthands
module you could include in RSpec for example, so you don't have to always use the full namespace for the methods below:Notice how methods go from finer grained to methods that subsequently introduce some default functionality to lighten/accelerate the developers work:
Some other methods i thought about, not only useful for tests, could be:
I'm not sure about naming the next one, but i guess the name is quite remarkable and makes quite clear what it does.. :-D
Maybe someone has another idea, for useful methods like these.
@waiting-for-dev if you like you can keep this issue open and we could collect some ideas if you think my suggestions are useful.
Thanks!