- Notifications
You must be signed in to change notification settings - Fork673
feat(client): replace basic auth with OAuth ROPC flow#2422
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedDec 11, 2022 • edited by codecovbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by codecovbot
Uh oh!
There was an error while loading.Please reload this page.
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 to90be806Compare| #: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it make sense to use an auth class for tracking the data?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What does this* mean?
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.
@lmilbaum It means that all arguments following are required to be keyword-only arguments.
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.
@JohnVillalovos Thanks for the clarification. I wasn't aware of this Python feature. BTW, does it make sense to specify theoauth_credentials argument in thekwargs such that it doesn't affect the function signature?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if a user provides bothoauth_credentials andhttp_username and/orhttp_password?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it make sense to use an auth class for tracking the data?
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.
@lmilbaum I just realized I left my responses as Pending for weeks :( just clicking submit now :)
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tobe7745dComparebe7745d to7e65adcCompare7e65adc to3733872Compare
Small step towards#1195, and also to get rid of the old username/password auth.
Also gets rid of the requests-specific
HTTPBasicAuth.