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.
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.
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.
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.
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.
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.
@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.
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.
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.
should we add # pragma: no cover here also?
There was a problem hiding this comment.
@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 |
alexpantyukhin
force-pushed
the
wip_add_jwt
branch
from
55fcc99
to
70c08f7
Compare
|
@jettify thank you! |
alexpantyukhin
force-pushed
the
wip_add_jwt
branch
from
70c08f7
to
05110f0
Compare
alexpantyukhin
force-pushed
the
wip_add_jwt
branch
from
58c253d
to
e32457b
Compare
|
I have no idea how to win the codecov :) |
|
@asvetlov thank you a lot! |
Fix #1