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
feat(client): replace basic auth with OAuth ROPC flow #2422
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
- Coverage 96.37% 96.17% -0.21%
==========================================
Files 88 88
Lines 5690 5692 +2
==========================================
- Hits 5484 5474 -10
- Misses 206 218 +12
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.
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 the oauth_credentials
argument in the kwargs
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 both oauth_credentials
and http_username
and/or http_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
to
be7745d
Compare
be7745d
to
7e65adc
Compare
Small step towards #1195, and also to get rid of the old username/password auth.
Also gets rid of the requests-specific
HTTPBasicAuth
.