-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Async extra verifications #221
base: main
Are you sure you want to change the base?
Conversation
Main problem with tests, is that in the new Sanic version - major part of responses body have been changed, type and params :( |
All tests pass except |
I'll take a look 👀 |
@@ -390,6 +388,8 @@ async def _verify( | |||
if token: | |||
try: | |||
payload = await self._decode(token, verify=verify) | |||
if self._extra_verifications: | |||
await self._verify_extras(payload, request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahopkins i have moved extra verifications here, to not pass explicit Request to all methods inside.
main reason - is to be able to access application context on verifications, for ex. to make calls to DB from sqlalchemy session inside app.ctx....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to also check for verify
otherwise it is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
btw, i thought to make _extra_verifications
default to empty list like _custom_claims
, to remove extra checks like if _extra_verifications
to make code look cleaner... but not sure to place it in this MR or to make another one after
Any news? |
Sorry. Thanks for pinging me. I'll add this to the list for the weekend. |
Will get this released in upcoming release this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some unit tests for the functionality it is trying to introduce.
done |
@ahopkins any news? thanks |
Releasing the PyJWT first. Then will come back to this one since it has a breaking change. |
self.instance.exception(exceptions.Unauthorized)( | ||
self.responses.exception_response | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this better than my solution of changing line 179 from self.bp
to self.app
. Maybe I should change that back.
@ahopkins if you have some suggestions or improvements - now i have free time to realise them ;) |
…possible to use Request, added Request argument to _verify_extras tests fix for latest Sanic version
@ahopkins let's do something with this pull request to finally merge it into main branch ;) |
Goal
I had to make jwt token invalidation, and the best way i have found - is to make it inside extra verifications, but they were not async
it is also discussed here #219
New Features/Changes
Possibility to use sync/async functions for
extra_verifications