Feature/refactor auth flow #60
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
…ling This separation will be important since the developer login will use the login flow of the new login page. The alert banners are to help with debugging, though we will want some type error alerting for users, we should probably do this a bit nicer.
…h endpoint for creating callback_uri and adding to dev app just saving my progress to push to remote, this version is not working yet.
This allows us to use the the prisma client directly within our routers, this is a needed change to support batching queries
… a user's developer app I have added a method to replace the existing list of callback URIs with a new one. This may not have been the best way to do this and could cause performance issues or be a really nice way to DoS our database, we should review this. I have also added a way for users to view specific developer applications, this will allow viewing of callback URIs.
Placeholder so I can push and pull from remote
…eveloper login to use new auth flow, add env var for abandon auth internal application, remove unused environment variables I have refactored the login auth flow to accept a developer application ID and callback URI that are used in combination to redirect the user to the verified callback URI after a successful abandon auth login. The internal developer dashboard/index page utilizes this new auth flow and can act as an example.
I have gone through and added some additional checks that should have been included and that Pyright caught. This includes some better error handling. I also added a copied error response for the 403 on the /ui/login endpoint. This must be refactored so that the error is raised within the same check for dev_app existing and the callback URIs not being empty as it could be abused to determine valid application IDs, which may be undesirable--better safe than sorry.
This is to help standardize JWT claims across the project and allow for better documentation for what to expect on a token claim and for the return type of the JWT dependency.
… refactor endpoints for new aud claim The JWT should have scope and aud claims as per the existing issue for the auth rework. These will provide additional security and flexibility when using tokens created by abandonauth including the ability to distinguish abandon auth login tokens (for managing developer applications) from external identify tokens.
This value always needs to be set during normal use. We may want to handle this better in the future to allow first-time application startup.
This will make these unusable for now, but they will need to be refactored fully once the new auth flow is finalized.
… to allow space separated string We need to distinguish required scopes from a JWT to better protect abandon auth endpoints. This is to prevent an applicatio from hijacking a user's abandon auth account and getting permissions to modify a user account/developer accounts belonging to a user.
This endpoint should be able to safely exchange an identify token. Scope is handled by the JWT generator function and will add additional scopes based on whether or not the token was generated for the abandon auth UI/developer login.
We were allowing user inputs to inject any callback URI for an application. This is bad. I have changed the callback endpoint to validate the application ID and callback URI before using it in a redirect response. This will protect against unexpected hijacking of the auth flow.
…' header for exchange token
…refactor abandonauth dev login to use new flow.
…app router for new dependency and reorder conflicting endpoints, refactor index.py for new auth This new auth dependency is not completely necessary as it just subclasses from JWTBearer, but it acts as a convenience for configuring JWTBearer to be used with developer applications. This change requires that authenticating users are accompanied by the developer application that the authentication is intended for.
GDWR
approved these changes
Nov 4, 2023
|
|
||
| def _generate_jwt(user_id: str, long_lived: bool = False) -> str: | ||
|
|
||
| def _generate_jwt(user_id: str, application_id_aud: str, long_lived: bool = False) -> str: |
There was a problem hiding this comment.
is application_id_aud different to application_id?
There was a problem hiding this comment.
No, these values are identical. I felt it was a bit more clear to specify what the application ID is used for in this case though.
This variable having a default value was confusing since it implied the user was meant to type out the discord redirect rather than generate it.
This DTO had the incorrect typing of `list[str] | None` despite the callback_uris always returning either a list of strings or an empty list.
I can't imagine who added this, I vote it was Harris.
|
closes #58 |
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.
Completely redesign the auth flow of Abandon Auth to use AA UI and redirects to complete the authentication process.
This PR is a WIP and I am creating for visibility as there will be some very important changes that completely overhaul the auth flow.