Skip to content
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

Allow customizing the JSON error response's message key #160

Merged
merged 1 commit into from
Jul 4, 2018
Merged

Allow customizing the JSON error response's message key #160

merged 1 commit into from
Jul 4, 2018

Conversation

mprahl
Copy link
Contributor

@mprahl mprahl commented Jun 23, 2018

I'd like to return JSON error responses in the format of

{
    "message": "Some error message"
}

instead of

{
    "msg": "Some error message"
}

This PR allows that to be customized via the JWT_ERROR_MESSAGE_KEY configuration option.

@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 1e2129e on mprahl:customize-error-msg-key into 182abbf on vimalloc:master.

@vimalloc
Copy link
Owner

The code looks good here. I want to think on this a bit before going forward, I'm not sure if adding the ability to change just the key here is really worth it, when there is already a built it way to change the values returned already. I'll get back to you soon 👍

@mprahl
Copy link
Contributor Author

mprahl commented Jun 30, 2018

@vimalloc is there a way to customize all messages? Based on looking at the docs, it looks like I'd have to customize every message type individually which is why I submitted this PR.

@vimalloc
Copy link
Owner

vimalloc commented Jul 4, 2018

True, there is not a way to customize all the message keys, but I do wonder what value that really brings to the extension. If you are just passing back the default error messages already, in the majority of cases the frontend and backend will need to agree on what the structure for how those error messages looks like already, so just changing the key and no other structure of the data doesn't make much sense to me. The frontend should be able to parse the JSON result and grab the message string and display it however it wants, without any changes needing to happen on the backend.

On the flip side, there is already a built in way to change the error messages to however you want built in to the application. By adding this additional way to change only the key, my worry is that a new user might be more confused on how to customize their errors (if they only saw this new key option and not the decorators that let them fully customize their error responses, for example).

@mprahl
Copy link
Contributor Author

mprahl commented Jul 4, 2018

@vimalloc the issue with forcing the msg key is that applications that use this extension are forced to use that key for errors not relating to JWT. This means an application that already uses a key such as message for their error messages would have to break their API and change it to msg to conform with the JWT errors.

@vimalloc
Copy link
Owner

vimalloc commented Jul 4, 2018

Alright, fair enough. Let me do a quick CR and we can see about getting this merged.

docs/options.rst Outdated
@@ -41,6 +41,8 @@ General Options:
Defaults to ``'user_claims'``.
``JWT_CLAIMS_IN_REFRESH_TOKEN`` If user claims should be included in refresh tokens.
Defaults to ``False``.
``JWT_ERROR_MESSAGE_KEY`` The key of the error message in a JSON error response.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add some text in here along the lines of when using the default error handlers?

@@ -255,6 +255,10 @@ def user_claims_in_refresh_token(self):
def exempt_methods(self):
return {"OPTIONS"}

@property
def error_msg_key(self):
return current_app.config.get('JWT_ERROR_MESSAGE_KEY', 'msg')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to stay consistent with the rest of the app here, could we remove this .get() and have it just access the JWT_ERROR_MESSAGE_KEY directly, and add this option as a app.config.setdefault in the JWTManager?

@mprahl
Copy link
Contributor Author

mprahl commented Jul 4, 2018

@vimalloc I addressed your comments. Please let me know if you need anything else.

@vimalloc
Copy link
Owner

vimalloc commented Jul 4, 2018

Looks good to me. I'll get a new release cut later today that includes these changes.

Thanks for contributing. 👍

@vimalloc vimalloc closed this Jul 4, 2018
@vimalloc vimalloc reopened this Jul 4, 2018
@vimalloc vimalloc merged commit 6364f28 into vimalloc:master Jul 4, 2018
@vimalloc
Copy link
Owner

So sorry about the delay here, ended up getting sick and then this totally slipped my mind. It has now been released in 3.11.0. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants