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

@@ -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'])

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'
+class PhabricatorLister(SWHIndexingHttpLister):
+    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:
+            <instance>:
+              - username: <account>
+                password: <api-token>
+
+        """
+        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

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')

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)

[Phabricator Migration user]

Some references in the commit message have been migrated:

• T1809 is now #1809 (closed)