-
-
Notifications
You must be signed in to change notification settings - Fork 27
start using fastapi-auth0 #155
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
base: main
Are you sure you want to change the base?
Conversation
devsjc
left a comment
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.
A few suggestions. I'm not really sure what advantage this brings over the previous method, sure its saves us a few lines of code for another dependency, but do we gain any functionality with that? In fact, it seems we lose access to the sub key which is actually used in other parts of the code - so those parts would need updating also.
| # Lets setup the auths | ||
| # 'auth' can be imported into the route if we want to limit a route by scopes | ||
| auth = DummyAuth() | ||
| if (os.getenv("AUTH0_DOMAIN") is not None) and (os.getenv("AUTH0_AUDIENCE") is not None): | ||
| auth = Auth0(api_audience=os.getenv("AUTH0_AUDIENCE"), domain=os.getenv("AUTH0_DOMAIN")) |
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.
I've tried to keep environment variables limited only to the top level. How come you've changed the access pattern? Seems like you could just have the previous Auth0 class be replaced entirely with this imported one, and remove the remove the call entirely from the DummyAuth and make it instead inherit from the fastapi_auth0.Auth0 class (as you sort of have already with the get_user method override). Then the code in main would be something like
# Override dependencies according to configuration
auth_instance: fastapi_auth0.Auth0
match (conf.get_string("auth0.domain"), conf.get_string("auth0.audience")):
case (_, "") | ("", _):
auth_instance = auth.DummyAuth()
log.warning("disabled authentication. NOT recommended for production")
case (domain, audience):
auth_instance = fastapi_auth0.Auth0(domain=domain, api_audience=audience)
server.dependency_overrides[auth.get_user] = auth_instance.get_user
You could even make the abstract auth class with a get_user method in the auth middleware that defines the auth_instance type.
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.
Ill try again, (I think i tried this but somehting wasnt working, but Ill try again as I do like what youve done with env vars there)
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.
I know remember, it was tricky to get the "Autherization" green box up on the swagger UI. This is done via fastapis Security. But ill try and get this working
| @server.get("/check_authentication", tags=["API Information"]) | ||
| def check_authentication(user: Annotated[Auth0User, Security(get_user)]) -> dict: | ||
| """Check if the user is authenticated.""" | ||
| return {"authenticated": True, "user": user} |
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.
Ive added this route,
|
hey @peterdudfield, haven't looked into the code/revisions on this but @devsjc left me a tag when he wasn't feeling well I think. |
Yea, the library is only one file, so I could just copy that over. |
|
Ive moved this back to draft, as if we do this, we need to update the routes too |
Pull Request
Description
How Has This Been Tested?
Checklist: