-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implemented jwt token authentication for login #1574
Conversation
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.
Very nice code!
I think the only thing missing is modifying the introspect endpoint to deserialise the access token and check that it has not expired.
Also the tests are failing and may need to be looked at π
introspect depends on |
Since a temporary user is not a real user and is not present in a database, we need to create it in the database, so for that, we need to explicitly specify the fields like id and other fields like profile_img as None. |
I think test was failing because env file is not updated with variables like |
Thanks for the feedback - was reviewing on my phone again and missed the login_required link π We already have user svcfmtm in the db - could we use that for the temp user possibly? I'll update the env vars for the tests π |
should we use username "svcfmtm" also or just the id ? |
Probably good to just hardcode |
This is pretty much good to go, but I will wait to merge until we finalise the 2024.3.1 release. |
In addition to the above @Sujanadh and I just discussed the possibility of using JWT refresh tokens to keep the token alive. If the user is inactive for many days it will expire, but if accessed regularly the token will refresh. Seems like a good idea to me π |
β¦oles OrgManagers
Did a bit of additional refactoring and finished off the roles! Still need to:
|
* feat: implemented jwt token authentication for login * fix: added audience while decoding * feat: added pyjwt , updated user id to existing svcfmtm osmid * feat: change username to svcfmtm * feat: implemented refresh token and updated dependency lock file * fix: return user data from refresh endpoint * changed img_url -> picture, retrieve id from sub field * feat: exception handling if osm deserialization fails * build: relock deps after pyjwt added * refactor: remove auth key path variables, require passing string values * docs: update install docs with info for gen priv/pub keys * docs: update all docs for AUTH_PUBLIC_KEY vars * build: add AUTH_PUBLIC_KEY vars to gen-env.sh script * refactor: exclude rsa keys from debug printed settings * fix(backend): improve logic for /me endpoint upsert & return ProjectRoles OrgManagers * fix(backend): return int type for AuthUser.id * refactor: create auth_schemas and replace all imports * refactor: pass user_id only to task_crud functions * test: update auth user creation for fixtures * test: guarantee randomness with uuid4 over randint * fix(backend): create_project existing project check must be scalar * fix(backend): conform to nominatim usage policy referer header * test: rename var for clarity --------- Co-authored-by: spwoodcock <sam.woodcock@protonmail.com>
What type of PR is this? (check all applicable)
Related Issue
Describe this PR
This PR adds new configuration to generate jwt authentication token for temporary login as well as to encapsulate the osm login for consistency. For token generation, the "RS256" algorithm is used, whereas rsa keys are generated for encoding and decoding the token.
Screenshots
N/A
Alternative Approaches Considered
Did you attempt any other approaches that are not documented in code?
Review Guide
Notes for the reviewer. How to test this change?
Checklist before requesting a review
[optional] What gif best describes this PR or how it makes you feel?