REST API cookie authentication and refactor #250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request introduces 7 alerts when merging c43aceb into d45425a - view on LGTM.com new alerts:
|
…ually anymore as it is automatically generated at runtime and then managed by fastapi_jwt_auth
…ally by the jwt package + comment out the authorization tests as it isn't clear if authorization is needed for now
…eded anymore with the introduction of refresh tokens, which provide a much safer alternative
giffels
requested changes
Aug 23, 2022
giffels
previously approved these changes
Aug 23, 2022
mschnepf
reviewed
Aug 24, 2022
mschnepf
approved these changes
Aug 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
The authentication method was changed to cookie authentication with jwt tokens.
The login router was completly changed and moved to /user.
OAuth scopes are included in the jwt token but scopes aren't implemented yet. This is why the check_authorization function was commented out instead of beeing removed. This is also why some unused imports aren't cleaned up yet.
I removed the secret key from the service class and moved it into the Settings class for the fastapi jwt package.
I also did some minor cleanup.
Reasoning
The package was used to increase security, decrease complexity and for ease of use.
The key is generated at runtime to decrease the security risk of accidently publishing it with the config file, decreasing the it's lifespan and enabling the possibilty to regenerate it during runtime in the future.
TODO