Conversation
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 17 +1
Lines 420 445 +25
=========================================
+ Hits 420 445 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
matin
left a comment
matin
left a comment
There was a problem hiding this comment.
also missing some form of docs on how to use JWT auth. At a minimum, it should be in README.md
| try: | ||
| payload_encoded = self.jwt_token.split('.')[1] | ||
| payload = json.loads(base64.b64decode(f'{payload_encoded}==')) | ||
| # Get a new token if there's less than 5 mins for the actual | ||
| # to be expired | ||
| if dt.datetime.utcfromtimestamp( | ||
| payload['exp'] | ||
| ) - dt.datetime.utcnow() <= dt.timedelta(minutes=5): | ||
| raise Exception('Expired token') |
There was a problem hiding this comment.
try block should only include one line of code
| try: | ||
| payload_encoded = self.jwt_token.split('.')[1] | ||
| payload = json.loads(base64.b64decode(f'{payload_encoded}==')) | ||
| # Get a new token if there's less than 5 mins for the actual | ||
| # to be expired | ||
| if dt.datetime.utcfromtimestamp( | ||
| payload['exp'] | ||
| ) - dt.datetime.utcnow() <= dt.timedelta(minutes=5): | ||
| raise Exception('Expired token') |
There was a problem hiding this comment.
huh? why take this approach?
There was a problem hiding this comment.
Payload in JWT tokens contain exp in base64 which indicates the datetime when the token will be invalid. So I'm extracting that part of the token to validate it before I send the request, this way I save one request saying that the token is expired.
| payload['exp'] | ||
| ) - dt.datetime.utcnow() <= dt.timedelta(minutes=5): | ||
| raise Exception('Expired token') | ||
| except Exception: |
There was a problem hiding this comment.
There are few cases in life where you should catch Exception. Why are you doing it in this case?
There was a problem hiding this comment.
There're a few cases here where this part of the code can generate an Exception:
IndexErrorif the current token is malformedbinascii.Errorif the token can't be decoded with base64JSONDecodeErrorif the JSON is malformed- I added one if the token is expired
All of this exceptions can happen if authed responded with an invalid token or the current token is somehow manipulated (which there should not be a reason you would want to do it). In any case, the token should be generated again.
If I include only one line of code per try block, I'd ended up having something like:
try:
payload_encoded = self.jwt_token.split('.')[1]
except IndexError:
self.jwt_token = None
else:
try:
payload = json.loads(base64.b64decode(f'{payload_encoded}=='))
except (binascii.Error, JSONDecodeError):
self.jwt_token = None
else:
if dt.datetime.utcfromtimestamp(
payload['exp']
) - dt.datetime.utcnow() <= dt.timedelta(minutes=5):
self.jwt_token = NoneWhich I think is way more confusing. I didn't see it necessary to create a new Exception since it's only going to be used here but I could clean it up by specify all the possible exceptions:
try:
payload_encoded = self.jwt_token.split('.')[1]
payload = json.loads(base64.b64decode(f'{payload_encoded}=='))
# Get a new token if there's less than 5 mins for the actual
# to be expired
if dt.datetime.utcfromtimestamp(
payload['exp']
) - dt.datetime.utcnow() <= dt.timedelta(minutes=5):
raise CustomException('Expired token')
except (IndexError, binascii.Error, JSONDecodeError, CustomException):
self.jwt_token = None
matin
left a comment
matin
left a comment
There was a problem hiding this comment.
Only one minor change. Otherwise, looks good!
|
@ricardo8990 looks good. just need to rebase |
matin
left a comment
matin
left a comment
There was a problem hiding this comment.
@ricardo8990 can you release?
2 participants
Agrega la autenticación por JWT para usar en aplicaciones internas
closes #77