-
Notifications
You must be signed in to change notification settings - Fork 670
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 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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
import gitlab.config | ||
import gitlab.const | ||
import gitlab.exceptions | ||
from gitlab import _backends, utils | ||
from gitlab import _backends, oauth, utils | ||
|
||
REDIRECT_MSG = ( | ||
"python-gitlab detected a {status_code} ({reason!r}) redirection. You must update " | ||
|
@@ -41,8 +41,6 @@ class Gitlab: | |
the value is a string, it is the path to a CA file used for | ||
certificate validation. | ||
timeout: Timeout to use for requests to the GitLab server. | ||
http_username: Username for HTTP authentication | ||
http_password: Password for HTTP authentication | ||
api_version: Gitlab API version to use (support for 4 only) | ||
pagination: Can be set to 'keyset' to use keyset pagination | ||
order_by: Set order_by globally | ||
|
@@ -51,6 +49,7 @@ class Gitlab: | |
or 52x responses. Defaults to False. | ||
keep_base_url: keep user-provided base URL for pagination if it | ||
differs from response headers | ||
oauth_credentials: Password credentials for authenticating via OAuth ROPC flow | ||
|
||
Keyword Args: | ||
requests.Session session: HTTP Requests Session | ||
|
@@ -64,8 +63,6 @@ def __init__( | |
oauth_token: Optional[str] = None, | ||
job_token: Optional[str] = None, | ||
ssl_verify: Union[bool, str] = True, | ||
http_username: Optional[str] = None, | ||
http_password: Optional[str] = None, | ||
timeout: Optional[float] = None, | ||
api_version: str = "4", | ||
per_page: Optional[int] = None, | ||
|
@@ -74,6 +71,8 @@ def __init__( | |
user_agent: str = gitlab.const.USER_AGENT, | ||
retry_transient_errors: bool = False, | ||
keep_base_url: bool = False, | ||
*, | ||
oauth_credentials: Optional[oauth.PasswordCredentials] = None, | ||
**kwargs: Any, | ||
) -> None: | ||
6D40 self._api_version = str(api_version) | ||
|
@@ -92,11 +91,9 @@ def __init__( | |
self.ssl_verify = ssl_verify | ||
|
||
self.private_token = private_token | ||
self.http_username = http_username | ||
self.http_password = http_password | ||
self.oauth_token = oauth_token | ||
self.job_token = job_token | ||
self._set_auth_info() | ||
self.oauth_credentials = oauth_credentials | ||
|
||
#: Create a session object for requests | ||
_backend: Type[_backends.DefaultBackend] = kwargs.pop( | ||
|
@@ -105,6 +102,7 @@ def __init__( | |
self._backend = _backend(**kwargs) | ||
self.session = self._backend.client | ||
|
||
self._set_auth_info() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
self.per_page = per_page | ||
self.pagination = pagination | ||
self.order_by = order_by | ||
|
@@ -271,8 +269,6 @@ def from_config( | |
job_token=config.job_token, | ||
ssl_verify=config.ssl_verify, | ||
timeout=config.timeout, | ||
http_username=config.http_username, | ||
http_password=config.http_password, | ||
api_version=config.api_version, | ||
per_page=config.per_page, | ||
pagination=config.pagination, | ||
|
@@ -471,41 +467,51 @@ def set_license(self, license: str, **kwargs: Any) -> Dict[str, Any]: | |
return result | ||
|
||
def _set_auth_info(self) -> None: | ||
tokens = [ | ||
token | ||
for token in [self.private_token, self.oauth_token, self.job_token] | ||
if token | ||
auth_types = [ | ||
auth | ||
for auth in [ | ||
self.private_token, | ||
self.oauth_token, | ||
self.oauth_credentials, | ||
self.job_token, | ||
] | ||
if auth | ||
] | ||
if len(tokens) > 1: | ||
if len(auth_types) > 1: | ||
raise ValueError( | ||
"Only one of private_token, oauth_token or job_token should " | ||
"be defined" | ||
) | ||
if (self.http_username and not self.http_password) or ( | ||
not self.http_username and self.http_password | ||
): | ||
raise ValueError("Both http_username and http_password should be defined") | ||
if tokens and self.http_username: | ||
raise ValueError( | ||
"Only one of token authentications or http " | ||
"authentication should be defined" | ||
"Only one of private_token, oauth_token, oauth_credentials" | ||
"or job_token should be defined" | ||
) | ||
|
||
self._auth: Optional[requests.auth.AuthBase] = None | ||
if self.private_token: | ||
self._auth = _backends.PrivateTokenAuth(self.private_token) | ||
return | ||
|
||
if self.oauth_token: | ||
self._auth = _backends.OAuthTokenAuth(self.oauth_token) | ||
return | ||
|
||
if self.oauth_credentials: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if a user provides both There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
post_data = { | ||
"grant_type": self.oauth_credentials.grant_type, | ||
"scope": self.oauth_credentials.scope, | ||
"username": self.oauth_credentials.username, | ||
"password": self.oauth_credentials.password, | ||
} | ||
response = self.http_post( | ||
f"{self._base_url}/oauth/token", post_data=post_data | ||
) | ||
if isinstance(response, dict): | ||
self.oauth_token = response["access_token"] | ||
else: | ||
self.oauth_token = response.json()["access_token"] | ||
self._auth = self.oauth_credentials.basic_auth | ||
return | ||
|
||
if self.job_token: | ||
self._auth = _backends.JobTokenAuth(self.job_token) | ||
|
||
if self.http_username and self.http_password: | ||
self._auth = requests.auth.HTTPBasicAuth( | ||
self.http_username, self.http_password | ||
) | ||
|
||
@staticmethod | ||
def enable_debug() -> None: | ||
import logging | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import dataclasses | ||
from typing import Optional | ||
|
||
|
||
@dataclasses.dataclass | ||
class PasswordCredentials: | ||
""" | ||
Resource owner password credentials modelled according to | ||
https://docs.gitlab.com/ee/api/oauth2.html#resource-owner-password-credentials-flow | ||
https://datatracker.ietf.org/doc/html/rfc6749#section-4-3. | ||
|
||
If the GitLab server has disabled the ROPC flow without client credentials, | ||
client_id and client_secret must be provided. | ||
""" | ||
|
||
username: str | ||
password: str | ||
grant_type: str = "password" | ||
scope: str = "api" | ||
client_id: Optional[str] = None | ||
client_secret: Optional[str] = None | ||
|
||
def __post_init__(self) -> None: | ||
basic_auth = (self.client_id, self.client_secret) | ||
|
||
if not any(basic_auth): | ||
self.basic_auth = None | ||
return | ||
|
||
if not all(basic_auth): | ||
raise TypeError("Both client_id and client_secret must be defined") | ||
|
||
self.basic_auth = basic_auth |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import pytest | ||
|
||
from gitlab.oauth import PasswordCredentials | ||
|
||
|
||
def test_password_credentials_without_password_raises(): | ||
with pytest.raises(TypeError, match="missing 1 required positional argument"): | ||
PasswordCredentials("username") | ||
|
||
|
||
def test_password_credentials_with_client_id_without_client_secret_raises(): | ||
with pytest.raises(TypeError, match="client_id and client_secret must be defined"): | ||
PasswordCredentials( | ||
"username", | ||
"password", | ||
client_id="abcdef123456", | ||
) | ||
|
||
|
||
def test_password_credentials_with_client_credentials_sets_basic_auth(): | ||
credentials = PasswordCredentials( | ||
"username", | ||
"password", | ||
client_id="abcdef123456", | ||
client_secret="123456abcdef", | ||
) | ||
assert credentials.basic_auth == ("abcdef123456", "123456abcdef") |
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 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.