-
Notifications
You must be signed in to change notification settings - Fork 68
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
init for jwt identity #147
Conversation
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
=========================================
+ Coverage 94.97% 95.1% +0.13%
=========================================
Files 10 12 +2
Lines 517 572 +55
Branches 18 19 +1
=========================================
+ Hits 491 544 +53
- Misses 20 22 +2
Partials 6 6
Continue to review full report at Codecov.
|
I would like that the consumer has the ability to not install PyJWT if JWTIdentityPolicy is not used. But I'm not sure that this is critical. |
aiohttp_security/jwt_identity.py
Outdated
""" | ||
|
||
from .abc import AbstractIdentityPolicy | ||
import jwt |
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 think conditional import like https://github.com/aio-libs/aiohttp-session/blob/master/aiohttp_session/redis_storage.py#L1-L4 would be better.
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.
Nice idea! thanks!
aiohttp_security/jwt_identity.py
Outdated
def __init__(self, secret, algorithm=None, param_name=None): | ||
self.secret = secret | ||
self.algorithm = 'HS256' if algorithm is None else algorithm | ||
self.param_name = 'Authorization' if param_name is None else param_name |
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.
What is the use-case for custom header name?
Every extra argument has maintenance and mental burden. It forces users to learn and remember never used API names.
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.
Yes.. param_name
is overhead .
aiohttp_security/jwt_identity.py
Outdated
return identity['identity'] | ||
|
||
async def remember(self, request, response, identity, **kwargs): | ||
encoded_identity = jwt.encode({'identity': identity}, |
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 don't understand the purpose of the method.
remember()
/forget()
pair is used to save/clear HTTP cookies on session-based auth, here you do something very different.
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.
@asvetlov Yes, you are right: remember
is not totally appropriate name for this action. But if I correctly understand, jwt authentication approach doesn't imply for storing jwt token (excluding storing it on client side). For this action the most suitable name would be login
which provides jwt token in the result or 403 otherwise. But for now we should to implement AbstractIdentityPolicy
and use the same approach for eachone identity way.
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.
Let's keep remember()
/forget()
empty.
login
is a client method, the client should acquire JWT by making a custom HTTP request to some resource. After that he is responsible for providing Authorization
header properly.
@asvetlov review notes are fixed. |
aiohttp_security/jwt_identity.py
Outdated
try: | ||
import jwt | ||
except ImportError: | ||
jwt = None |
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.
Please add # pragma: no cover
here in order to shut up coverage check
aiohttp_security/jwt_identity.py
Outdated
return identity['identity'] | ||
|
||
async def remember(self, request, response, identity, **kwargs): | ||
pass |
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.
should we add # pragma: no cover
here also?
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.
@jettify Yeah :) it's fun that if mark remember
with # pragma: no cover
then flake8 doesn't allow to pass tests :)
Branch is a bit out of date, PR should be rebased |
55fcc99
to
70c08f7
Compare
@jettify thank you! |
70c08f7
to
05110f0
Compare
58c253d
to
e32457b
Compare
I have no idea how to win the codecov :) |
@asvetlov thank you a lot! |
Fix #1