search

NASA-IMPACT / HLS-Transfer-Catalog

Data Catalog to manage HLS S2 transfers from ESA to NASA
Apache License 2.0
2 stars 1 forks source link

Feature/jwt authentication implementation #4

Closed ub0005 closed 2 years ago

NISH1001 commented 2 years ago

Thanks @ub0005 . I will also test this and comment if we need other changes...

NISH1001 commented 2 years ago

@ub0005 Can you please add jwt to requirements.txt with proper versioning...

NISH1001 commented 2 years ago

Tangent: @ub0005 Can you also please install pre-commit in your virtualenv and run pre-commit install at project's root directory. There are a few pre-commit hooks that standardize the codebase. For instance, the code linting is done through black.

NISH1001 commented 2 years ago

@ub0005 I we need to add trailing slash to /login -> /login/

NISH1001 commented 2 years ago

We need to fix the expiration bug. And we're good to go. Thanks @ub0005 for this auth feature...

ub0005 commented 2 years ago

Fixed JWT token expiration issue.

NISH1001 commented 2 years ago

@ub0005 I think the new change breaks the upload API (will pull tomorrow and test) as you have actually removed the from datetime import datetime import. See: https://github.com/NASA-IMPACT/HLS-Transfer-Catalog/blob/feature/JWT-Authentication-Implementation/src/app.py#L134=

Here we actually make use of datetime.now() to generate a timestamp.

Possible mitigitation: Make the import the way it was and just use datetime.utcnow() in the auth part

This might sound confusing initially while using datetime, but datetime itself has a submodule named datetime. So, if we're doing import datetime, we need to do datetime.datetime.now() or we could do from datettime import datetime and just use datetime.now().

Rule of thumb: Before removing imports, please do cross-check on whether that removal hampers other parts of the code...

ub0005 commented 2 years ago

@NISH1001 Yeah, that was my mistake. I was in the assumption that I have imported the whole DateTime module so nothing will break. Going forward will take care of these confusing things. Thanks for pointing it out and I have reverted that import statement and tested all the endpoints. All the endpoints working fine for me after this fix.

NISH1001 commented 2 years ago

@ub0005 Thanks Uday. Yeah. I understand. Initially, datetime module seems confusing...

NISH1001 commented 2 years ago

@ub0005 Also another suggestion is to have these integrations to your IDE/editor:

This will help in your python workflow too. I think for VS code it might needs some settings change (not sure...)