From 6d117059082f46b2a38b62293fcd16371e1728d1 Mon Sep 17 00:00:00 2001 From: "Antoine R. Dumont (@ardumont)" Date: Fri, 14 Jun 2019 18:10:42 +0200 Subject: [PATCH] phabricator.lister: Use credentials setup from configuration file Prior to this commit, this expected the api.token to be provided at task initialization. That behavior has been kept for cli purposes. It's no good for production purposes though (as this leaks the credentials in the scheduler db). So now, the credentials is fetched from the lister's configuration file as the other listers do. Another change is the authentication mechanism which is slighly different. It's not using a basic `auth` mechanism. It's expecting an `api.token` query parameter so the `request_params` is overriden to provide that. Related T1809 --- swh/lister/core/lister_transports.py | 21 ++++++++-- swh/lister/phabricator/lister.py | 47 +++++++++++++++++++--- swh/lister/phabricator/tasks.py | 8 ++-- swh/lister/phabricator/tests/test_tasks.py | 2 +- 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/swh/lister/core/lister_transports.py b/swh/lister/core/lister_transports.py index 75bc6ad..a2202bd 100644 --- a/swh/lister/core/lister_transports.py +++ b/swh/lister/core/lister_transports.py @@ -49,6 +49,21 @@ class SWHListerHttpTransport(abc.ABC): 'User-Agent': 'Software Heritage lister (%s)' % self.lister_version } + def request_instance_credentials(self): + """Returns dictionary of any credentials configuration needed by the + forge instance to list. + + Returns: + dict of credentials per instance lister or {} if none. + + """ + all_creds = self.config.get('credentials') + if not all_creds: + return {} + lister_creds = all_creds.get(self.LISTER_NAME, {}) + creds = lister_creds.get(self.instance, {}) + return creds + def request_uri(self, identifier): """Get the full request URI given the transport_request identifier. @@ -93,9 +108,9 @@ class SWHListerHttpTransport(abc.ABC): """ params = {} params['headers'] = self.request_headers() or {} - all_creds = self.config['credentials'] - lister_creds = all_creds.get(self.LISTER_NAME, {}) - creds = lister_creds.get(self.instance, {}) + creds = self.request_instance_credentials() + if not creds: + return params auth = random.choice(creds) if creds else None if auth: params['auth'] = (auth['username'], auth['password']) diff --git a/swh/lister/phabricator/lister.py b/swh/lister/phabricator/lister.py index c02103d..7009c10 100644 --- a/swh/lister/phabricator/lister.py +++ b/swh/lister/phabricator/lister.py @@ -2,33 +2,68 @@ # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information +import logging + import urllib.parse from swh.lister.core.indexing_lister import SWHIndexingHttpLister from swh.lister.phabricator.models import PhabricatorModel from collections import defaultdict +logger = logging.getLogger(__name__) + class PhabricatorLister(SWHIndexingHttpLister): - - PATH_TEMPLATE = '&order=oldest&attachments[uris]=1&after=%s' + PATH_TEMPLATE = '?order=oldest&attachments[uris]=1&after=%s' MODEL = PhabricatorModel LISTER_NAME = 'phabricator' - def __init__(self, forge_url, api_token, instance=None, + def __init__(self, forge_url, instance=None, api_token=None, override_config=None): if forge_url.endswith("/"): forge_url = forge_url[:-1] self.forge_url = forge_url - api_endpoint = ('api/diffusion.repository.' - 'search?api.token=%s') % api_token - api_baseurl = '%s/%s' % (forge_url, api_endpoint) + api_baseurl = '%s/api/diffusion.repository.search' % forge_url + self.api_token = api_token if not instance: instance = urllib.parse.urlparse(forge_url).hostname self.instance = instance super().__init__(api_baseurl=api_baseurl, override_config=override_config) + def _build_query_params(self, params, api_token): + """Build query params to include the forge's api token + + Returns: + updated params dict with 'params' entry. + + """ + params.update({'params': {'api.token': api_token}}) + return params + + def request_params(self, identifier): + """Override the default params behavior to retrieve the api token + + Credentials are stored as: + + credentials: + phabricator: + : + - username: + password: + + """ + params = {} + params['headers'] = self.request_headers() or {} + if self.api_token: + return self._build_query_params(params, self.api_token) + instance_creds = self.request_instance_credentials() + if not instance_creds: + raise ValueError( + 'Phabricator forge needs authentication credential to list.') + api_token = instance_creds[0]['password'] + return self._build_query_params(params, api_token) + def request_headers(self): """ (Override) Set requests headers to send when querying the diff --git a/swh/lister/phabricator/tasks.py b/swh/lister/phabricator/tasks.py index 3ebb981..ce37fa4 100644 --- a/swh/lister/phabricator/tasks.py +++ b/swh/lister/phabricator/tasks.py @@ -6,10 +6,10 @@ from swh.scheduler.celery_backend.config import app from swh.lister.phabricator.lister import PhabricatorLister -def new_lister(forge_url='https://forge.softwareheritage.org', api_token='', - instance='swh', **kw): - return PhabricatorLister(forge_url=forge_url, api_token=api_token, - instance=instance, **kw) +def new_lister(forge_url='https://forge.softwareheritage.org', instance='swh', + api_token=None, **kw): + return PhabricatorLister( + forge_url=forge_url, instance=instance, api_token=api_token, **kw) @app.task(name=__name__ + '.IncrementalPhabricatorLister') diff --git a/swh/lister/phabricator/tests/test_tasks.py b/swh/lister/phabricator/tests/test_tasks.py index a97196f..0aa27d6 100644 --- a/swh/lister/phabricator/tests/test_tasks.py +++ b/swh/lister/phabricator/tests/test_tasks.py @@ -24,7 +24,7 @@ def test_incremental(lister, swh_app, celery_session_worker): assert res.successful() lister.assert_called_once_with( - api_token='', forge_url='https://forge.softwareheritage.org', + api_token=None, forge_url='https://forge.softwareheritage.org', instance='swh') lister.db_last_index.assert_called_once_with() lister.run.assert_called_once_with(min_bound=42)