From 4917b9bf9629f078ba4b04ed4e905aaa27e925fb Mon Sep 17 00:00:00 2001 From: James Date: Mon, 7 Oct 2024 10:23:03 +0200 Subject: [PATCH] Fix/rest (#17083) * fixing Rest layer * wip * fix test * caching localdb creds and capabilities --- conan/cli/commands/remote.py | 2 +- conans/__init__.py | 1 - conans/client/rest/auth_manager.py | 83 ++++++------- conans/client/rest/client_routes.py | 3 - conans/client/rest/remote_credentials.py | 8 +- conans/client/rest/rest_client.py | 48 +++----- conans/client/rest/rest_client_v2.py | 81 ++++--------- conans/model/rest_routes.py | 1 - test/integration/remote/auth_test.py | 9 +- test/integration/remote/rest_api_test.py | 7 +- test/integration/remote/token_refresh_test.py | 109 ------------------ 11 files changed, 82 insertions(+), 270 deletions(-) delete mode 100644 test/integration/remote/token_refresh_test.py diff --git a/conan/cli/commands/remote.py b/conan/cli/commands/remote.py index b1737fe06c1..e297b1881bd 100644 --- a/conan/cli/commands/remote.py +++ b/conan/cli/commands/remote.py @@ -204,7 +204,7 @@ def remote_login(conan_api, parser, subparser, *args): if args.username is not None and args.password is not None: user, password = args.username, args.password else: - user, password = creds.auth(r, args.username) + user, password, _ = creds.auth(r, args.username) if args.username is not None and args.username != user: raise ConanException(f"User '{args.username}' doesn't match user '{user}' in " f"credentials.json or environment variables") diff --git a/conans/__init__.py b/conans/__init__.py index 095880329e8..fcb99451816 100644 --- a/conans/__init__.py +++ b/conans/__init__.py @@ -1,5 +1,4 @@ CHECKSUM_DEPLOY = "checksum_deploy" # Only when v2 REVISIONS = "revisions" # Only when enabled in config, not by default look at server_launcher.py -OAUTH_TOKEN = "oauth_token" __version__ = '2.9.0-dev' diff --git a/conans/client/rest/auth_manager.py b/conans/client/rest/auth_manager.py index 8320bb86c77..e241e9f8b91 100644 --- a/conans/client/rest/auth_manager.py +++ b/conans/client/rest/auth_manager.py @@ -11,9 +11,6 @@ get_conan with the new token. """ -import hashlib -from uuid import getnode as get_mac - from conan.api.output import ConanOutput from conans.client.rest.remote_credentials import RemoteCredentials from conans.client.rest.rest_client import RestApiClient @@ -22,22 +19,38 @@ LOGIN_RETRIES = 3 +class RemoteCreds: + def __init__(self, localdb): + self._localdb = localdb + + def get(self, remote): + creds = getattr(remote, "_creds", None) + if creds is None: + user, token, _ = self._localdb.get_login(remote.url) + creds = user, token + setattr(remote, "_creds", creds) + return creds + + def set(self, remote, user, token): + setattr(remote, "_creds", (user, token)) + self._localdb.store(user, token, None, remote.url) + + class ConanApiAuthManager: def __init__(self, requester, cache_folder, localdb, global_conf): self._requester = requester - self._localdb = localdb + self._creds = RemoteCreds(localdb) self._global_conf = global_conf self._cache_folder = cache_folder - self._cached_capabilities = {} # common to all RestApiClient def call_rest_api_method(self, remote, method_name, *args, **kwargs): """Handles AuthenticationException and request user to input a user and a password""" - user, token, refresh_token = self._localdb.get_login(remote.url) - rest_client = self._get_rest_client(remote) + user, token = self._creds.get(remote) + rest_client = RestApiClient(remote, token, self._requester, self._global_conf) if method_name == "authenticate": - return self._authenticate(remote, *args, **kwargs) + return self._authenticate(rest_client, remote, *args, **kwargs) try: ret = getattr(rest_client, method_name)(*args, **kwargs) @@ -51,15 +64,8 @@ def call_rest_api_method(self, remote, method_name, *args, **kwargs): # Anonymous is not enough, ask for a user ConanOutput().info('Please log in to "%s" to perform this action. ' 'Execute "conan remote login" command.' % remote.name) - return self._retry_with_new_token(user, remote, method_name, *args, **kwargs) - elif token and refresh_token: - # If we have a refresh token try to refresh the access token - try: - self._authenticate(remote, user, None) - except AuthenticationException: - # logger.info("Cannot refresh the token, cleaning and retrying: {}".format(exc)) - self._clear_user_tokens_in_db(user, remote) - return self.call_rest_api_method(remote, method_name, *args, **kwargs) + if self._get_credentials_and_authenticate(rest_client, user, remote): + return self.call_rest_api_method(remote, method_name, *args, **kwargs) else: # Token expired or not valid, so clean the token and repeat the call # (will be anonymous call but exporting who is calling) @@ -67,59 +73,40 @@ def call_rest_api_method(self, remote, method_name, *args, **kwargs): self._clear_user_tokens_in_db(user, remote) return self.call_rest_api_method(remote, method_name, *args, **kwargs) - def _retry_with_new_token(self, user, remote, method_name, *args, **kwargs): + def _get_credentials_and_authenticate(self, rest_client, user, remote): """Try LOGIN_RETRIES to obtain a password from user input for which we can get a valid token from api_client. If a token is returned, credentials are stored in localdb and rest method is called""" + creds = RemoteCredentials(self._cache_folder, self._global_conf) for _ in range(LOGIN_RETRIES): - creds = RemoteCredentials(self._cache_folder, self._global_conf) - input_user, input_password = creds.auth(remote) + input_user, input_password, interactive = creds.auth(remote) try: - self._authenticate(remote, input_user, input_password) + self._authenticate(rest_client, remote, input_user, input_password) except AuthenticationException: out = ConanOutput() if user is None: out.error('Wrong user or password', error_type="exception") else: out.error(f'Wrong password for user "{user}"', error_type="exception") + if not interactive: + raise AuthenticationException(f"Authentication error in remote '{remote.name}'") else: - return self.call_rest_api_method(remote, method_name, *args, **kwargs) - + return True raise AuthenticationException("Too many failed login attempts, bye!") - def _get_rest_client(self, remote): - username, token, refresh_token = self._localdb.get_login(remote.url) - custom_headers = {'X-Client-Anonymous-Id': self._get_mac_digest(), - 'X-Client-Id': str(username or "")} - return RestApiClient(remote, token, refresh_token, custom_headers, self._requester, - self._global_conf, self._cached_capabilities) - def _clear_user_tokens_in_db(self, user, remote): try: - self._localdb.store(user, token=None, refresh_token=None, remote_url=remote.url) + self._creds.set(remote, user, token=None) except Exception as e: out = ConanOutput() - out.error('Your credentials could not be stored in local cache\n', error_type="exception") + out.error('Your credentials could not be stored in local cache', error_type="exception") out.debug(str(e) + '\n') - @staticmethod - def _get_mac_digest(): - sha1 = hashlib.sha1() - sha1.update(str(get_mac()).encode()) - return str(sha1.hexdigest()) - - def _authenticate(self, remote, user, password): - rest_client = self._get_rest_client(remote) - if user is None: # The user is already in DB, just need the password - prev_user = self._localdb.get_username(remote.url) - if prev_user is None: - raise ConanException("User for remote '%s' is not defined" % remote.name) - else: - user = prev_user + def _authenticate(self, rest_client, remote, user, password): try: - token, refresh_token = rest_client.authenticate(user, password) + token = rest_client.authenticate(user, password) except UnicodeDecodeError: raise ConanException("Password contains not allowed symbols") # Store result in DB - self._localdb.store(user, token, refresh_token, remote.url) + self._creds.set(remote, user, token) diff --git a/conans/client/rest/client_routes.py b/conans/client/rest/client_routes.py index ab1846b51cb..8b60962e478 100644 --- a/conans/client/rest/client_routes.py +++ b/conans/client/rest/client_routes.py @@ -49,9 +49,6 @@ def search_packages(self, ref): url = _format_ref(route, ref) return self.base_url + url - def oauth_authenticate(self): - return self.base_url + self.routes.oauth_authenticate - def common_authenticate(self): return self.base_url + self.routes.common_authenticate diff --git a/conans/client/rest/remote_credentials.py b/conans/client/rest/remote_credentials.py index 701af57766e..ad001a5767f 100644 --- a/conans/client/rest/remote_credentials.py +++ b/conans/client/rest/remote_credentials.py @@ -43,13 +43,13 @@ def auth(self, remote, user=None): msg = scoped_traceback(msg, e, scope="/extensions/plugins") raise ConanException(msg) if plugin_user and plugin_password: - return plugin_user, plugin_password + return plugin_user, plugin_password, False # Then prioritize the cache "credentials.json" file creds = self._urls.get(remote.name) if creds is not None: try: - return creds["user"], creds["password"] + return creds["user"], creds["password"], False except KeyError as e: raise ConanException(f"Authentication error, wrong credentials.json: {e}") @@ -58,12 +58,12 @@ def auth(self, remote, user=None): if env_passwd is not None: if env_user is None: raise ConanException("Found password in env-var, but not defined user") - return env_user, env_passwd + return env_user, env_passwd, False # If not found, then interactive prompt ui = UserInput(self._global_conf.get("core:non_interactive", check_type=bool)) input_user, input_password = ui.request_login(remote.name, user) - return input_user, input_password + return input_user, input_password, True @staticmethod def _get_env(remote, user): diff --git a/conans/client/rest/rest_client.py b/conans/client/rest/rest_client.py index 76eade345fc..e2dcf1714ef 100644 --- a/conans/client/rest/rest_client.py +++ b/conans/client/rest/rest_client.py @@ -1,6 +1,6 @@ -from conans import CHECKSUM_DEPLOY, REVISIONS, OAUTH_TOKEN +from conans import CHECKSUM_DEPLOY, REVISIONS from conans.client.rest.rest_client_v2 import RestV2Methods -from conans.errors import AuthenticationException, ConanException +from conans.errors import ConanException class RestApiClient: @@ -8,29 +8,22 @@ class RestApiClient: Rest Api Client for handle remote. """ - def __init__(self, remote, token, refresh_token, custom_headers, requester, - config, cached_capabilities): - - # Set to instance + def __init__(self, remote, token, requester, config): self._token = token - self._refresh_token = refresh_token self._remote_url = remote.url - self._custom_headers = custom_headers self._requester = requester - self._verify_ssl = remote.verify_ssl self._config = config - - # This dict is shared for all the instances of RestApiClient - self._cached_capabilities = cached_capabilities + self._remote = remote def _capable(self, capability, user=None, password=None): - capabilities = self._cached_capabilities.get(self._remote_url) + # Caching of capabilities per-remote + capabilities = getattr(self._remote, "_capabilities", None) if capabilities is None: - tmp = RestV2Methods(self._remote_url, self._token, self._custom_headers, + tmp = RestV2Methods(self._remote_url, self._token, self._requester, self._config, self._verify_ssl) capabilities = tmp.server_capabilities(user, password) - self._cached_capabilities[self._remote_url] = capabilities + setattr(self._remote, "_capabilities", capabilities) return capability in capabilities def _get_api(self): @@ -41,7 +34,7 @@ def _get_api(self): "Conan 2.0 is no longer compatible with " "remotes that don't accept revisions.") checksum_deploy = self._capable(CHECKSUM_DEPLOY) - return RestV2Methods(self._remote_url, self._token, self._custom_headers, + return RestV2Methods(self._remote_url, self._token, self._requester, self._config, self._verify_ssl, checksum_deploy) @@ -61,26 +54,11 @@ def upload_package(self, pref, files_to_upload): return self._get_api().upload_package(pref, files_to_upload) def authenticate(self, user, password): - api_v2 = RestV2Methods(self._remote_url, self._token, self._custom_headers, + # BYPASS capabilities, in case v1/ping is protected + api_v2 = RestV2Methods(self._remote_url, self._token, self._requester, self._config, self._verify_ssl) - - if self._refresh_token and self._token: - token, refresh_token = api_v2.refresh_token(self._token, self._refresh_token) - else: - try: - # Check capabilities can raise also 401 until the new Artifactory is released - oauth_capable = self._capable(OAUTH_TOKEN, user, password) - except AuthenticationException: - oauth_capable = False - - if oauth_capable: - # Artifactory >= 6.13.X - token, refresh_token = api_v2.authenticate_oauth(user, password) - else: - token = api_v2.authenticate(user, password) - refresh_token = None - - return token, refresh_token + token = api_v2.authenticate(user, password) + return token def check_credentials(self): return self._get_api().check_credentials() diff --git a/conans/client/rest/rest_client_v2.py b/conans/client/rest/rest_client_v2.py index 21d27634ad8..b5b74255e68 100644 --- a/conans/client/rest/rest_client_v2.py +++ b/conans/client/rest/rest_client_v2.py @@ -1,9 +1,11 @@ import copy import fnmatch +import hashlib import json import os from requests.auth import AuthBase, HTTPBasicAuth +from uuid import getnode as get_mac from conan.api.output import ConanOutput @@ -24,11 +26,11 @@ class JWTAuth(AuthBase): """Attaches JWT Authentication to the given Request object.""" def __init__(self, token): - self.token = token + self.bearer = "Bearer %s" % str(token) if token else None def __call__(self, request): - if self.token: - request.headers['Authorization'] = "Bearer %s" % str(self.token) + if self.bearer: + request.headers['Authorization'] = self.bearer return request @@ -47,21 +49,28 @@ def get_exception_from_error(error_code): return None +def _get_mac_digest(): # To avoid re-hashing all the time the same mac + cached = getattr(_get_mac_digest, "_cached_value", None) + if cached is not None: + return cached + sha1 = hashlib.sha1() + sha1.update(str(get_mac()).encode()) + cached = str(sha1.hexdigest()) + _get_mac_digest._cached_value = cached + return cached + + class RestV2Methods: - def __init__(self, remote_url, token, custom_headers, requester, config, verify_ssl, - checksum_deploy=False): - self.token = token + def __init__(self, remote_url, token, requester, config, verify_ssl, checksum_deploy=False): self.remote_url = remote_url - self.custom_headers = custom_headers + self.custom_headers = {'X-Client-Anonymous-Id': _get_mac_digest()} self.requester = requester self._config = config self.verify_ssl = verify_ssl self._checksum_deploy = checksum_deploy - - @property - def auth(self): - return JWTAuth(self.token) + self.router = ClientV2Router(self.remote_url.rstrip("/")) + self.auth = JWTAuth(token) @staticmethod def _check_error_response(ret): @@ -85,52 +94,6 @@ def authenticate(self, user, password): self._check_error_response(ret) return ret.content.decode() - def authenticate_oauth(self, user, password): - """Sends user + password to get: - - A json with an access_token and a refresh token (if supported in the remote) - Artifactory >= 6.13.X - """ - url = self.router.oauth_authenticate() - auth = HTTPBasicAuth(user, password) - headers = {} - headers.update(self.custom_headers) - headers["Content-type"] = "application/x-www-form-urlencoded" - # logger.debug("REST: Authenticating with OAUTH: %s" % url) - ret = self.requester.post(url, auth=auth, headers=headers, verify=self.verify_ssl) - self._check_error_response(ret) - - data = ret.json() - access_token = data["access_token"] - refresh_token = data["refresh_token"] - # logger.debug("REST: Obtained refresh and access tokens") - return access_token, refresh_token - - def refresh_token(self, token, refresh_token): - """Sends access_token and the refresh_token to get a pair of - access_token and refresh token - - Artifactory >= 6.13.X - """ - url = self.router.oauth_authenticate() - # logger.debug("REST: Refreshing Token: %s" % url) - headers = {} - headers.update(self.custom_headers) - headers["Content-type"] = "application/x-www-form-urlencoded" - payload = {'access_token': token, 'refresh_token': refresh_token, - 'grant_type': 'refresh_token'} - ret = self.requester.post(url, headers=headers, verify=self.verify_ssl, data=payload) - self._check_error_response(ret) - - data = ret.json() - if "access_token" not in data: - # logger.debug("REST: unexpected data from server: {}".format(data)) - raise ConanException("Error refreshing the token") - - new_access_token = data["access_token"] - new_refresh_token = data["refresh_token"] - # logger.debug("REST: Obtained new refresh and access tokens") - return new_access_token, new_refresh_token - def check_credentials(self): """If token is not valid will raise AuthenticationException. User will be asked for new user/pass""" @@ -240,10 +203,6 @@ def search_packages(self, ref): package_infos = self._get_json(url) return package_infos - @property - def router(self): - return ClientV2Router(self.remote_url.rstrip("/")) - def _get_file_list_json(self, url): data = self._get_json(url) # Discarding (.keys()) still empty metadata for files diff --git a/conans/model/rest_routes.py b/conans/model/rest_routes.py index f3537d26f5f..7dab1f0779a 100644 --- a/conans/model/rest_routes.py +++ b/conans/model/rest_routes.py @@ -2,7 +2,6 @@ class RestRoutes(object): ping = "ping" common_search = "conans/search" common_authenticate = "users/authenticate" - oauth_authenticate = "users/token" common_check_credentials = "users/check_credentials" def __init__(self): diff --git a/test/integration/remote/auth_test.py b/test/integration/remote/auth_test.py index 8f2890cabb1..be9f6d5f87a 100644 --- a/test/integration/remote/auth_test.py +++ b/test/integration/remote/auth_test.py @@ -140,6 +140,7 @@ def test_authorize_disabled_remote(self): tc.run("remote login default pepe -p pepepass") self.assertIn("Changed user of remote 'default' from 'None' (anonymous) to 'pepe' (authenticated)", tc.out) + class AuthenticationTest(unittest.TestCase): def test_unauthorized_during_capabilities(self): @@ -158,17 +159,17 @@ def get(url, **kwargs): elif "ping" in url: resp_basic_auth.headers = {"Content-Type": "application/json", "X-Conan-Server-Capabilities": "revisions"} - token = getattr(kwargs["auth"], "token", None) + bearer = getattr(kwargs["auth"], "bearer", None) password = getattr(kwargs["auth"], "password", None) - if token and token != "TOKEN": + if bearer and bearer != "Bearer TOKEN": raise Exception("Bad JWT Token") - if not token and not password: + if not bearer and not password: raise AuthenticationException( "I'm an Artifactory without anonymous access that " "requires authentication for the ping endpoint and " "I don't return the capabilities") elif "search" in url: - if kwargs["auth"].token != "TOKEN": + if kwargs["auth"].bearer != "Bearer TOKEN": raise Exception("Bad JWT Token") resp_basic_auth._content = b'{"results": []}' resp_basic_auth.headers = {"Content-Type": "application/json"} diff --git a/test/integration/remote/rest_api_test.py b/test/integration/remote/rest_api_test.py index 5566bbbfbc8..cbb87dcd161 100644 --- a/test/integration/remote/rest_api_test.py +++ b/test/integration/remote/rest_api_test.py @@ -49,10 +49,11 @@ def setUpClass(cls): cls.auth_manager = ConanApiAuthManager(requester, temp_folder(), localdb, config) cls.remote = Remote("myremote", "http://127.0.0.1:%s" % str(cls.server.port), True, True) - cls.auth_manager._authenticate(cls.remote, user="private_user", + cls.api = RestApiClient(cls.remote, localdb.access_token, requester, config) + cls.auth_manager._authenticate(cls.api, cls.remote, user="private_user", password="private_pass") - cls.api = RestApiClient(cls.remote, localdb.access_token, localdb.refresh_token, - {}, requester, config, {}) + # Need to define again with new token + cls.api = RestApiClient(cls.remote, localdb.access_token, requester, config) @classmethod def tearDownClass(cls): diff --git a/test/integration/remote/token_refresh_test.py b/test/integration/remote/token_refresh_test.py deleted file mode 100644 index b1ef3ce03d6..00000000000 --- a/test/integration/remote/token_refresh_test.py +++ /dev/null @@ -1,109 +0,0 @@ -import unittest - -import mock - -from conan.api.model import Remote -from conans.client.rest.auth_manager import ConanApiAuthManager -from conans.model.conf import ConfDefinition -from conans.model.recipe_ref import RecipeReference -from conan.test.utils.mocks import LocalDBMock -from conan.test.utils.test_files import temp_folder - -common_headers = {"X-Conan-Server-Capabilities": "oauth_token,revisions", - "Content-Type": "application/json"} - - -class ResponseOK(object): - def __init__(self): - self.ok = True - self.headers = common_headers - self.status_code = 200 - self.content = b'' - - -class ResponseDownloadURLs(object): - def __init__(self): - self.ok = True - self.headers = common_headers - self.status_code = 200 - self.content = b'{"files": {}}' - - -class ResponseWithTokenMock(object): - def __init__(self, token): - self.token = token - self.ok = True - self.headers = common_headers - self.status_code = 200 - self.content = '' - - def json(self): - return {"access_token": self.token, "refresh_token": "refresh_token"} - - -class ResponseAuthenticationRequired(object): - def __init__(self): - self.ok = False - self.headers = common_headers - self.status_code = 401 - self.content = b'Login needed' - - -class RequesterWithTokenMock(object): - - def get(self, url, **kwargs): - if not kwargs["auth"].token or kwargs["auth"].token == "expired": - return ResponseAuthenticationRequired() - if url.endswith("files"): - return ResponseDownloadURLs() - elif url.endswith("users/authenticate"): - raise Exception("This endpoint should't be called when oauth supported") - - def post(self, url, **kwargs): - """If the call is to refresh we return "refreshed_access_token" otherwise we return - "access_token" - """ - if url.endswith("users/token"): - if kwargs.get("data") and kwargs.get("data").get("grant_type") == "refresh_token": - return ResponseWithTokenMock("refreshed_access_token") - return ResponseWithTokenMock("access_token") - else: - raise Exception("This endpoint should't be reached") - - -class TestTokenRefresh(unittest.TestCase): - # MISSING MOCKS - - def setUp(self): - requester = RequesterWithTokenMock() - config = ConfDefinition() - self.localdb = LocalDBMock() - self.auth_manager = ConanApiAuthManager(requester, temp_folder(), self.localdb, config) - self.remote = Remote("myremote", "myurl", True, True) - self.ref = RecipeReference.loads("lib/1.0@conan/stable#myreciperev") - - def test_auth_with_token(self): - """Test that if the capability is there, then we use the new endpoint""" - with mock.patch("conans.client.rest.remote_credentials.UserInput.request_login", - return_value=("myuser", "mypassword")): - - self.auth_manager.call_rest_api_method(self.remote, "get_recipe", self.ref, ".", - metadata=None, only_metadata=False) - self.assertEqual(self.localdb.user, "myuser") - self.assertEqual(self.localdb.access_token, "access_token") - self.assertEqual(self.localdb.refresh_token, "refresh_token") - - def test_refresh_with_token(self): - """The mock will raise 401 for a token value "expired" so it will try to refresh - and only if the refresh endpoint is called, the value will be "refreshed_access_token" - """ - with mock.patch("conans.client.rest.remote_credentials.UserInput.request_login", - return_value=("myuser", "mypassword")): - self.localdb.access_token = "expired" - self.localdb.refresh_token = "refresh_token" - - self.auth_manager.call_rest_api_method(self.remote, "get_recipe", self.ref, ".", - metadata=None, only_metadata=False) - self.assertEqual(self.localdb.user, "myuser") - self.assertEqual(self.localdb.access_token, "refreshed_access_token") - self.assertEqual(self.localdb.refresh_token, "refresh_token")