Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++
https://thalhammer.github.io/jwt-cpp/
MIT License
864 stars 235 forks source link

Define a "jwt base class" for exceptions #252

Open prince-chrismc opened 1 year ago

prince-chrismc commented 1 year ago

closes #215

prince-chrismc commented 1 year ago

There's actually a lot of these left

image

prince-chrismc commented 1 year ago

I think we need a more clear goal for this one. I was not expecting it to be soo big 🙈

On the server side if you only do this

try { 
     process_jwt(token);
catch (jwt_exception e) {
     //... handling 
}

How can you know to return 401 vs 403 😕 there's a difference for the token being malformed and not granting permission 🤔

Thalhammer commented 1 year ago

there's a difference for the token being malformed and not granting permission

Wouldn't this normally be handled in two different places ? First you check if the token is a valid token (i.e. not malformed, not expired, etc.) then you check if the token (which you can now trust) actually allows access to the resource at hand. At least in my code bases this doesn't even happen in the same place, the first check is inside a middleware whereas the second is in the individual handler.

Normally the difference between multiple error groups is handled by std::error_category, if the token is malformed you'd get signature_verification_error_category() as the category, whereas a expired token (or one missing claims,etc) would give you a token_verification_error_category() error. So your handling (assuming you check the resource access inside the verifier) would look something like this:

try { 
     process_jwt(token);
catch (jwt_exception e) {
    if(e.code().category() == signature_verification_error_category())
         return 401;
    if(e.code().category() == token_verification_error_category())
         return 403;
    return 500;
}

You could also dig down into the actual error code, e.g. to distinguish token_expired and audience_missmatch.

Regardless, the fact that all the exception types now inherit std::exception twice definitly feels wrong and will almost certainly cause issues. What we could do is have a jwt::exception inherit std::system_error (and thus std::runtime_error and std::exception) and then inherit something like invalid_json_exception from jwt::exception so you can still catch those individually instead of checking the error category if you want.

prince-chrismc commented 1 year ago

Tomorrow while I travel, I will look at this again... I agree this isn't up to par. Theres 130+ exceptions that are from the STL... some are for sure traits but others I have no clue...

Once we know what the exceptions are doing we can figure out a few user stories.

Come up with a plan to improve this.

Thalhammer commented 1 year ago

Theres 130+ exceptions that are from the STL

You're not really supposed to use any of them though except for the 9 in <stdexcept> and std::exception.

prince-chrismc commented 1 year ago

I mean we are using them a lot 🤷 despite the other ways we are also using

prince-chrismc commented 1 year ago

Excluding the traits... there's 5 left

image