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

Async extra verifications #221

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Async extra verifications #221

wants to merge 7 commits into from

Conversation

jekel
Copy link

@jekel jekel commented Oct 30, 2021

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

ahopkins
ahopkins previously approved these changes Oct 30, 2021
@jekel
Copy link
Author

jekel commented Oct 31, 2021

Main problem with tests, is that in the new Sanic version - major part of responses body have been changed, type and params :(

ahopkins
ahopkins previously approved these changes Oct 31, 2021
@jekel
Copy link
Author

jekel commented Nov 1, 2021

All tests pass except test_auth_verify_invalid_token and test_config_with_cookie_domain, i dont know how to fix them, they dont depend on sanic version

@ahopkins
Copy link
Owner

ahopkins commented Nov 1, 2021

All tests pass except test_auth_verify_invalid_token and test_config_with_cookie_domain, i dont know how to fix them, they dont depend on sanic version

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)
Copy link
Author

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....

Copy link
Owner

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.

Copy link
Author

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

@jekel
Copy link
Author

jekel commented Nov 18, 2021

Any news?

@ahopkins
Copy link
Owner

Sorry. Thanks for pinging me. I'll add this to the list for the weekend.

@ahopkins
Copy link
Owner

Will get this released in upcoming release this week.

Copy link
Owner

@ahopkins ahopkins left a 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.

@jekel
Copy link
Author

jekel commented Dec 30, 2021

This needs some unit tests for the functionality it is trying to introduce.

done

@jekel
Copy link
Author

jekel commented Jan 20, 2022

@ahopkins any news? thanks

@ahopkins
Copy link
Owner

Releasing the PyJWT first. Then will come back to this one since it has a breaking change.

Comment on lines +187 to +189
self.instance.exception(exceptions.Unauthorized)(
self.responses.exception_response
)
Copy link
Owner

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.

@jekel
Copy link
Author

jekel commented Jul 3, 2022

@ahopkins if you have some suggestions or improvements - now i have free time to realise them ;)

@jekel
Copy link
Author

jekel commented Dec 9, 2022

@ahopkins let's do something with this pull request to finally merge it into main branch ;)
if you have any questions/suggestions i'm ready to resolve them

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.

None yet

2 participants