Repository navigation
refactor: Move auth extractors into authentication module - #1547
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1547 +/- ##
==========================================
+ Coverage 95.69% 95.70% +0.01%
==========================================
Files 43 43
Lines 3226 3240 +14
==========================================
+ Hits 3087 3101 +14
Misses 139 139 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ZohebShaikh
left a comment
ZohebShaikh
left a comment
There was a problem hiding this comment.
Generally I think why write extra code when it is already been written by someone else ?
| """Get bearer token value from authorization header""" | ||
| auth = req.headers.get("Authorization") | ||
| scheme, param = get_authorization_scheme_param(auth) | ||
| if scheme.casefold() != "bearer": | ||
| return None | ||
| return param.strip() |
There was a problem hiding this comment.
This is a copy of this
authorization = request.headers.get("Authorization")
scheme, param = get_authorization_scheme_param(authorization)
if not authorization or scheme.lower() != "bearer":
if self.auto_error:
raise self.make_not_authenticated_error()
else:
return None # pragma: nocover
return param
Why not use the function already in fastAPI ?
After this PR you only use it at 1 more place.
A better thing to do would be to create oauth_scheme in main
oauth_scheme = OAuth2AuthorizationCodeBearer(
authorizationUrl="",
tokenUrl="",
refreshUrl=None,
)
and do depend(oauth_scheme) to get the unchecked_access_token
To a certain extend I get that you want authN related stuff to be in the authentication.py file
There was a problem hiding this comment.
Why not use the function already in fastAPI ?
Because passing in null/placeholder values that aren't needed makes the code more confusing. It also inserts auth details into the API schema because this dependency is always on the endpoints even though is optional.
After this PR you only use it at 1 more place
For now yes. This was paving the way for the authz changes that need access to the unvalidated access_token
A better thing to do would be to create oauth_scheme in main
I think if it's in main we end up with circular imports. main imports authz imports main. Having all access token handling in main makes it end up doing everything. As you said, having authn stuff in the authn module makes it clearer.
There was a problem hiding this comment.
This looks like a preference in doing things a certain way so will not block the merge, but having a link to the fastapi where it is being copied from and with some explanation will be a good thing
And split the auth check into two so that other methods can access the raw bearer token if required.
Splitting up the auth extractor method allows other extractors to access the raw token as well as easy access to the validated and decoded access token.