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

fix(Token Expire) Show proper response if token expires #16

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

ronelvcabrera
Copy link

Give a json response for token expire.

https://github.com/newmatik/Newmatik/issues/1824

Screen Shot 2019-04-08 at 4 31 01 PM

@dottenbr
Copy link

dottenbr commented Apr 8, 2019

@ronelvcabrera why do you have the message in data and then again outside of it?

@ronelvcabrera
Copy link
Author

@dottenbr Just a readable message I guess. I can remove that no problem.

@ronelvcabrera
Copy link
Author

here is the new screenshot.

Screen Shot 2019-04-08 at 5 05 06 PM

Copy link

@kickapoo kickapoo left a comment

Choose a reason for hiding this comment

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

Just a small try because i dont like this

response = validate_jwt()
if response:
    ...

The reason that I dont like is that if you see the other validations they dont return anything. We need to keep the structure so in some point we PR this to the framework.

Please update the code

 except jwt.ExpiredSignatureError as e:
        raise e
``
to check the response. 

@ronelvcabrera
Copy link
Author

@kickapoo yes that is true. As you can see on the screenshot below, code is still 401 but no body. If your fine with that we can do that. We will only capture the http code of 401, which means expired token (this is not used on login, verifying login credentials). I have pushed the changes as well, kindly review. Thank you.

Screen Shot 2019-04-10 at 9 42 23 AM

@creamdory
Copy link

Merging this to our core. @ronelvcabrera send this as PR to frappe core.

@creamdory creamdory merged commit fa43d99 into newmatik:develop Apr 10, 2019
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.

4 participants