feat(client): replace basic auth with OAuth ROPC flow #2422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
+ Coverage 92.16% 96.17% +4.00%
==========================================
Files 88 88
Lines 5708 5692 -16
==========================================
+ Hits 5261 5474 +213
+ Misses 447 218 -229
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7b1a20d to
90be806
Compare
|
|
||
| #: Create a session object for requests | ||
| http_backend: Type[http_backends.DefaultBackend] = kwargs.pop( | ||
| "http_backend", http_backends.DefaultBackend | ||
| ) | ||
| self.http_backend = http_backend(**kwargs) | ||
|
|
||
| self._set_auth_info() |
There was a problem hiding this comment.
We have to configure the backend first to be able to make requests in _set_auth_info() because that potentially makes a request to retrieve the OAuth token, so moving this down here.
There was a problem hiding this comment.
Would it make sense to use an auth class for tracking the data?
There was a problem hiding this comment.
Maybe, I'm not sure I fully get what you had in mind though :) but it might make this PR grow quite a bit, could you explain a bit what you had in mind and if it's more code we can maybe expand in a follow-up?
There was a problem hiding this comment.
It seems like client is a monolithic class. Moving auth properties and functions to an auth class would be easier to maintain. IMO, implementing the auth class should come first before switching to a different method.
|
@lmilbaum this should also help get rid of |
| @@ -75,6 +76,8 @@ def __init__( | |||
| user_agent: str = gitlab.const.USER_AGENT, | |||
| retry_transient_errors: bool = False, | |||
| keep_base_url: bool = False, | |||
| *, | |||
There was a problem hiding this comment.
@lmilbaum It means that all arguments following are required to be keyword-only arguments.
There was a problem hiding this comment.
@JohnVillalovos Thanks for the clarification. I wasn't aware of this Python feature. BTW, does it make sense to specify the oauth_credentials argument in the kwargs such that it doesn't affect the function signature?
There was a problem hiding this comment.
@lmilbaum I think that would lose some of the explicitness. That's actually why I added the * here, so it doesn't affect the users as much even if the signature changes a bit. Are you worried about the typing in the backends code?
There was a problem hiding this comment.
Personally, I don't like function signatures with a large amount of arguments. On the other hand, the explicitness argument is stronger.
| self.http_username, self.http_password | ||
| ) | ||
|
|
||
| if self.oauth_credentials: |
There was a problem hiding this comment.
What if a user provides both oauth_credentials and http_username and/or http_password?
There was a problem hiding this comment.
If it's only one of them it will already fail earlier. But if it's both it will ignore them and use oauth. I can make this more explicit as well :)
There was a problem hiding this comment.
IMO, this use case should be checked and an error should be thrown.
|
|
||
| #: Create a session object for requests | ||
| http_backend: Type[http_backends.DefaultBackend] = kwargs.pop( | ||
| "http_backend", http_backends.DefaultBackend | ||
| ) | ||
| self.http_backend = http_backend(**kwargs) | ||
|
|
||
| self._set_auth_info() |
There was a problem hiding this comment.
Would it make sense to use an auth class for tracking the data?
| @@ -75,6 +76,8 @@ def __init__( | |||
| user_agent: str = gitlab.const.USER_AGENT, | |||
| retry_transient_errors: bool = False, | |||
| keep_base_url: bool = False, | |||
| *, | |||
There was a problem hiding this comment.
@lmilbaum I think that would lose some of the explicitness. That's actually why I added the * here, so it doesn't affect the users as much even if the signature changes a bit. Are you worried about the typing in the backends code?
|
|
||
| #: Create a session object for requests | ||
| http_backend: Type[http_backends.DefaultBackend] = kwargs.pop( | ||
| "http_backend", http_backends.DefaultBackend | ||
| ) | ||
| self.http_backend = http_backend(**kwargs) | ||
|
|
||
| self._set_auth_info() |
There was a problem hiding this comment.
Maybe, I'm not sure I fully get what you had in mind though :) but it might make this PR grow quite a bit, could you explain a bit what you had in mind and if it's more code we can maybe expand in a follow-up?
| self.http_username, self.http_password | ||
| ) | ||
|
|
||
| if self.oauth_credentials: |
There was a problem hiding this comment.
If it's only one of them it will already fail earlier. But if it's both it will ignore them and use oauth. I can make this more explicit as well :)
90be806 to
be7745d
Compare
be7745d to
7e65adc
Compare
7e65adc to
3733872
Compare
Small step towards #1195, and also to get rid of the old username/password auth.
Also gets rid of the requests-specific
HTTPBasicAuth.