Additional claims #166
Additional claims #166
Conversation
| # Register the default error handler callback methods. These can be | ||
| # overridden with the appropriate loader decorators | ||
| self._user_claims_callback = default_user_claims_callback | ||
| self._additonal_claims_callback = default_additional_claims_callback |
| @property | ||
| def user_claims_key(self): | ||
| return current_app.config['JWT_USER_CLAIMS'] | ||
|
|
There was a problem hiding this comment.
I don't think this config option is necessary
| By default, we do not do any verification of the user claims. | ||
| """ | ||
| return True | ||
|
|
There was a problem hiding this comment.
Would prefer to get rid of this and catch the underlying pyjwt errors
| error message with a 400 status code | ||
| """ | ||
| return jsonify({'msg': 'User claims verification failed'}), 400 | ||
|
|
There was a problem hiding this comment.
Think we should get rid of this as well. Whatever callback handles the invalid token can handle if any additional claims do not validate as well.
| def handle_jwt_decode_error(e): | ||
| return self._invalid_token_callback(str(e)) | ||
|
|
||
| token_data[user_claims_key] = user_claims | ||
|
|
||
| # Make sure additional claims is a dict before merge | ||
| if additional_claims and isinstance(additional_claims, dict): |
There was a problem hiding this comment.
I think an error should be raised here instead of silently doing nothing if additional claims is not a dict.
| :param user_claims: Custom claims to include in this token. This data must | ||
| be json serializable | ||
| :param additional_claims: Custom claims to include in this token. Object | ||
| must be json serializable |
There was a problem hiding this comment.
Not only must it be json serializable, it must be a dictionary. Should update the wording here to match.
| :return: Dictionary containing contents of the JWT | ||
| """ | ||
| # The validation decorator for additional claims must evaluate these! | ||
| options = { |
There was a problem hiding this comment.
I don't like this, simply checking if these claims exist in the JWT is not enough to verify them. Instead I think we need a way to pass the expected verify options to the underlying pyjwt decode function.
| if user_claims_key not in data: | ||
| data[user_claims_key] = {} | ||
|
|
||
| if data['type'] != 'refresh' or config.user_claims_in_refresh_token: |
There was a problem hiding this comment.
Related to above, I think we shouldn't have this code path, and instead rely on the existing code path for verifying custom data in user_claims, and pyjwt.decode errors for standard JWT claims.
| [testenv] | ||
| commands = | ||
| coverage run --source flask_jwt_extended -m pytest tests/ | ||
| coverage run --source flask_jwt_extended -m pytest tests/ -vv |
|
So looking at this PR, most of my thoughts are stemming from the fact that I don't want to replace the |
|
Good points. I can see the benefits of that approach. |
|
@vimalloc. Do i understand it correctly that you want to limit the additional claims to spec specific ones? Like keeping the user claims as they are, and adding the possibility to have aud and iss, or should it be up to the user to define what goes in the additional claims and then only verify aud and iss in pyJWT if they are defined? |
|
Well... yes and no. In practice I think that is really what this feature should be used for, and when using this extension everything that doesn't fit as a default claim should be stuck in the So for the sake of this, I think allowing any claims to be put in here is a good thing, but in practice I don't want to have be encouraged to do so for custom stuff. This extension is very opinioned about what needs to be in it to get it to work, and I imagine most other JWT providers wont have the claims that this extension needs (and thus, users should be utilizing flask-jwt-simple instead). I think really the use case for this would be to create claims that other audiences can utilize. There are a lot of use cases though so obviously not a one size fits all kind of thing. |
3 participants
No description provided.