Repository: CS-SI/eodag (python)

Issue:
prevent authentication during search preparation
Following https://github.com/CS-SI/eodag/pull/753, authentication is performed for all providers needing auth for search in `_prepare_search()`.

Only auth plugin should be passed as argument, and authentication should be performed before search for the given provider.

Pull request description:
fix: authenticate only when performing search
Authenticate against the provider directly during the `_do_search` method. The `_prepare_search` has been refactored to not include anything related to authentication.

Fix #801

Patch:
diff --git a/eodag/api/core.py b/eodag/api/core.py
index 9b394f4a..bff4f2cc 100644
--- a/eodag/api/core.py
+++ b/eodag/api/core.py
@@ -20,6 +20,7 @@ import os
 import re
 import shutil
 from operator import itemgetter
+from typing import List
 
 import geojson
 import pkg_resources
@@ -45,6 +46,7 @@ from eodag.config import (
 )
 from eodag.plugins.download.base import DEFAULT_DOWNLOAD_TIMEOUT, DEFAULT_DOWNLOAD_WAIT
 from eodag.plugins.manager import PluginManager
+from eodag.plugins.search.base import Search
 from eodag.utils import (
     GENERIC_PRODUCT_TYPE,
     MockResponse,
@@ -891,7 +893,7 @@ class EODataAccessGateway(object):
             return a list as a result of their processing. This requirement is
             enforced here.
         """
-        search_kwargs = self._prepare_search(
+        search_plugins, search_kwargs = self._prepare_search(
             start=start,
             end=end,
             geom=geom,
@@ -899,15 +901,13 @@ class EODataAccessGateway(object):
             provider=provider,
             **kwargs,
         )
-        search_plugins = search_kwargs.pop("search_plugins", [])
+
         if search_kwargs.get("id"):
             # adds minimal pagination to be able to check only 1 product is returned
             search_kwargs.update(
                 page=1,
                 items_per_page=2,
             )
-            # remove auth from search_kwargs as a loop over providers will be performed
-            search_kwargs.pop("auth", None)
             return self._search_by_id(
                 search_kwargs.pop("id"), provider=provider, **search_kwargs
             )
@@ -981,10 +981,9 @@ class EODataAccessGateway(object):
                   matching the criteria
         :rtype: Iterator[:class:`~eodag.api.search_result.SearchResult`]
         """
-        search_kwargs = self._prepare_search(
+        search_plugins, search_kwargs = self._prepare_search(
             start=start, end=end, geom=geom, locations=locations, **kwargs
         )
-        search_plugins = search_kwargs.pop("search_plugins")
         for i, search_plugin in enumerate(search_plugins):
             try:
                 return self.search_iter_page_plugin(
@@ -1198,10 +1197,9 @@ class EODataAccessGateway(object):
                 )
                 self.fetch_product_types_list()
 
-        search_kwargs = self._prepare_search(
+        search_plugins, search_kwargs = self._prepare_search(
             start=start, end=end, geom=geom, locations=locations, **kwargs
         )
-        search_plugins = search_kwargs.get("search_plugins", [])
         for i, search_plugin in enumerate(search_plugins):
             if items_per_page is None:
                 items_per_page = search_plugin.config.pagination.get(
@@ -1279,13 +1277,8 @@ class EODataAccessGateway(object):
                 "Searching product with id '%s' on provider: %s", uid, plugin.provider
             )
             logger.debug("Using plugin class for search: %s", plugin.__class__.__name__)
-            auth_plugin = self._plugins_manager.get_auth_plugin(plugin.provider)
-            if getattr(plugin.config, "need_auth", False) and callable(
-                getattr(auth_plugin, "authenticate", None)
-            ):
-                plugin.auth = auth_plugin.authenticate()
             plugin.clear()
-            results, _ = self._do_search(plugin, auth=auth_plugin, id=uid, **kwargs)
+            results, _ = self._do_search(plugin, id=uid, **kwargs)
             if len(results) == 1:
                 if not results[0].product_type:
                     # guess product type from properties
@@ -1311,8 +1304,7 @@ class EODataAccessGateway(object):
     def _prepare_search(
         self, start=None, end=None, geom=None, locations=None, provider=None, **kwargs
     ):
-        """Internal method to prepare the search kwargs and get the search
-        and auth plugins.
+        """Internal method to prepare the search kwargs and get the search plugins.
 
         Product query:
           * By id (plus optional 'provider')
@@ -1375,7 +1367,7 @@ class EODataAccessGateway(object):
                         "No product type could be guessed with provided arguments"
                     )
                 else:
-                    return kwargs
+                    return [], kwargs
 
         kwargs["productType"] = product_type
         if start is not None:
@@ -1412,7 +1404,7 @@ class EODataAccessGateway(object):
             )
             self.fetch_product_types_list()
 
-        search_plugins = []
+        search_plugins: List[Search] = []
         for plugin in self._plugins_manager.get_search_plugins(
             product_type=product_type
         ):
@@ -1442,7 +1434,6 @@ class EODataAccessGateway(object):
             )
         # Add product_types_config to plugin config. This dict contains product
         # type metadata that will also be stored in each product's properties.
-        auth_plugins = {}
         for search_plugin in search_plugins:
             try:
                 search_plugin.config.product_type_config = dict(
@@ -1466,23 +1457,7 @@ class EODataAccessGateway(object):
             # Remove the ID since this is equal to productType.
             search_plugin.config.product_type_config.pop("ID", None)
 
-            auth_plugin = self._plugins_manager.get_auth_plugin(search_plugin.provider)
-            auth_plugins[search_plugin.provider] = auth_plugin
-
-            # append auth to search plugin if needed
-            if getattr(search_plugin.config, "need_auth", False) and callable(
-                getattr(auth_plugin, "authenticate", None)
-            ):
-                try:
-                    search_plugin.auth = auth_plugin.authenticate()
-                except RuntimeError:
-                    logger.error(
-                        "could not authenticatate at provider %s",
-                        search_plugin.provider,
-                    )
-                    search_plugin.auth = None
-
-        return dict(search_plugins=search_plugins, auth=auth_plugins, **kwargs)
+        return search_plugins, kwargs
 
     def _do_search(self, search_plugin, count=True, raise_errors=False, **kwargs):
         """Internal method that performs a search on a given provider.
@@ -1514,22 +1489,18 @@ class EODataAccessGateway(object):
                 max_items_per_page,
             )
 
+        need_auth = getattr(search_plugin.config, "need_auth", False)
+        auth_plugin = self._plugins_manager.get_auth_plugin(search_plugin.provider)
+        can_authenticate = callable(getattr(auth_plugin, "authenticate", None))
+
         results = SearchResult([])
         total_results = 0
 
         try:
-            if "auth" in kwargs:
-                if isinstance(kwargs["auth"], dict):
-                    auth = kwargs["auth"][search_plugin.provider]
-                else:
-                    auth = kwargs["auth"]
-                kwargs.pop("auth")
-            else:
-                auth = None
+            if need_auth and auth_plugin and can_authenticate:
+                search_plugin.auth = auth_plugin.authenticate()
 
-            if "search_plugins" in kwargs:
-                kwargs.pop("search_plugins")
-            res, nb_res = search_plugin.query(count=count, auth=auth, **kwargs)
+            res, nb_res = search_plugin.query(count=count, auth=auth_plugin, **kwargs)
 
             # Only do the pagination computations when it makes sense. For example,
             # for a search by id, we can reasonably guess that the provider will return
@@ -1615,7 +1586,7 @@ class EODataAccessGateway(object):
                     download_plugin = self._plugins_manager.get_download_plugin(
                         eo_product
                     )
-                    eo_product.register_downloader(download_plugin, auth)
+                    eo_product.register_downloader(download_plugin, auth_plugin)
 
             results.extend(res)
             total_results = None if nb_res is None else total_results + nb_res
diff --git a/eodag/plugins/manager.py b/eodag/plugins/manager.py
index 93884728..2adf341f 100644
--- a/eodag/plugins/manager.py
+++ b/eodag/plugins/manager.py
@@ -18,6 +18,7 @@
 import logging
 from operator import attrgetter
 from pathlib import Path
+from typing import Optional
 
 import pkg_resources
 
@@ -200,7 +201,7 @@ class PluginManager(object):
             plugin = self._build_plugin(product.provider, plugin_conf.api, Api)
             return plugin
 
-    def get_auth_plugin(self, provider):
+    def get_auth_plugin(self, provider: str) -> Optional[Authentication]:
         """Build and return the authentication plugin for the given product_type and
         provider
 


Test patch:
diff --git a/tests/units/test_core.py b/tests/units/test_core.py
index c3b4347e..03323865 100644
--- a/tests/units/test_core.py
+++ b/tests/units/test_core.py
@@ -1188,12 +1188,12 @@ class TestCoreSearch(TestCoreBase):
     )
     def test__prepare_search_no_parameters(self, mock_fetch_product_types_list):
         """_prepare_search must create some kwargs even when no parameter has been provided"""
-        prepared_search = self.dag._prepare_search()
+        _, prepared_search = self.dag._prepare_search()
         expected = {
             "geometry": None,
             "productType": None,
         }
-        expected = set(["geometry", "productType", "auth", "search_plugins"])
+        expected = set(["geometry", "productType"])
         self.assertSetEqual(expected, set(prepared_search))
 
     @mock.patch(
@@ -1205,7 +1205,7 @@ class TestCoreSearch(TestCoreBase):
             "start": "2020-01-01",
             "end": "2020-02-01",
         }
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertEqual(prepared_search["startTimeFromAscendingNode"], base["start"])
         self.assertEqual(
             prepared_search["completionTimeFromAscendingNode"], base["end"]
@@ -1219,22 +1219,22 @@ class TestCoreSearch(TestCoreBase):
         # The default way to provide a geom is through the 'geom' argument.
         base = {"geom": (0, 50, 2, 52)}
         # "geom": "POLYGON ((1 43, 1 44, 2 44, 2 43, 1 43))"
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
         # 'box' and 'bbox' are supported for backwards compatibility,
         # The priority is geom > bbox > box
         base = {"box": (0, 50, 2, 52)}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
         base = {"bbox": (0, 50, 2, 52)}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
         base = {
             "geom": "POLYGON ((1 43, 1 44, 2 44, 2 43, 1 43))",
             "bbox": (0, 50, 2, 52),
             "box": (0, 50, 1, 51),
         }
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
         self.assertNotIn("bbox", prepared_search)
         self.assertNotIn("bbox", prepared_search)
@@ -1249,19 +1249,19 @@ class TestCoreSearch(TestCoreBase):
         # as regular kwargs. The new and recommended way to provide
         # them is through the 'locations' parameter.
         base = {"locations": {"country": "FRA"}}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
 
         # TODO: Remove this when support for locations kwarg is dropped.
         base = {"country": "FRA"}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertIn("geometry", prepared_search)
         self.assertNotIn("country", prepared_search)
 
     def test__prepare_search_product_type_provided(self):
         """_prepare_search must handle when a product type is given"""
         base = {"productType": "S2_MSI_L1C"}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertEqual(prepared_search["productType"], base["productType"])
 
     def test__prepare_search_product_type_guess_it(self):
@@ -1273,7 +1273,7 @@ class TestCoreSearch(TestCoreBase):
             platform="SENTINEL2",
             platformSerialIdentifier="S2A",
         )
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertEqual(prepared_search["productType"], "S2_MSI_L1C")
 
     def test__prepare_search_remove_guess_kwargs(self):
@@ -1285,13 +1285,13 @@ class TestCoreSearch(TestCoreBase):
             platform="SENTINEL2",
             platformSerialIdentifier="S2A",
         )
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertEqual(len(base.keys() & prepared_search.keys()), 0)
 
     def test__prepare_search_with_id(self):
         """_prepare_search must handle a search by id"""
         base = {"id": "dummy-id", "provider": "creodias"}
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         expected = {"id": "dummy-id"}
         self.assertDictEqual(expected, prepared_search)
 
@@ -1301,7 +1301,7 @@ class TestCoreSearch(TestCoreBase):
             "productType": "S2_MSI_L1C",
             "cloudCover": 10,
         }
-        prepared_search = self.dag._prepare_search(**base)
+        _, prepared_search = self.dag._prepare_search(**base)
         self.assertEqual(prepared_search["productType"], base["productType"])
         self.assertEqual(prepared_search["cloudCover"], base["cloudCover"])
 
@@ -1311,14 +1311,12 @@ class TestCoreSearch(TestCoreBase):
         try:
             self.dag.set_preferred_provider("peps")
             base = {"productType": "S2_MSI_L1C"}
-            prepared_search = self.dag._prepare_search(**base)
+            search_plugins, _ = self.dag._prepare_search(**base)
             # Just check that the title has been set correctly. There are more (e.g.
             # abstract, platform, etc.) but this is sufficient to check that the
             # product_type_config dict has been created and populated.
             self.assertEqual(
-                prepared_search["search_plugins"][0].config.product_type_config[
-                    "title"
-                ],
+                search_plugins[0].config.product_type_config["title"],
                 "SENTINEL2 Level-1C",
             )
         finally:
@@ -1335,38 +1333,35 @@ class TestCoreSearch(TestCoreBase):
         try:
             self.dag.set_preferred_provider("peps")
             base = {"productType": "product_unknown_to_eodag"}
-            prepared_search = self.dag._prepare_search(**base)
+            search_plugins, _ = self.dag._prepare_search(**base)
             # product_type_config is still created if the product is not known to eodag
             # however it contains no data.
             self.assertIsNone(
-                prepared_search["search_plugins"][0].config.product_type_config[
-                    "title"
-                ],
+                search_plugins[0].config.product_type_config["title"],
             )
         finally:
             self.dag.set_preferred_provider(prev_fav_provider)
 
     def test__prepare_search_peps_plugins_product_available(self):
-        """_prepare_search must return the search and auth plugins when productType is defined"""
+        """_prepare_search must return the search plugins when productType is defined"""
         prev_fav_provider = self.dag.get_preferred_provider()[0]
         try:
             self.dag.set_preferred_provider("peps")
             base = {"productType": "S2_MSI_L1C"}
-            prepared_search = self.dag._prepare_search(**base)
-            self.assertEqual(prepared_search["search_plugins"][0].provider, "peps")
-            self.assertEqual(prepared_search["auth"]["peps"].provider, "peps")
+            search_plugins, _ = self.dag._prepare_search(**base)
+            self.assertEqual(search_plugins[0].provider, "peps")
         finally:
             self.dag.set_preferred_provider(prev_fav_provider)
 
     def test__prepare_search_no_plugins_when_search_by_id(self):
         """_prepare_search must not return the search and auth plugins for a search by id"""
         base = {"id": "some_id", "provider": "some_provider"}
-        prepared_search = self.dag._prepare_search(**base)
-        self.assertNotIn("search_plugins", prepared_search)
+        search_plugins, prepared_search = self.dag._prepare_search(**base)
+        self.assertListEqual(search_plugins, [])
         self.assertNotIn("auth", prepared_search)
 
     def test__prepare_search_peps_plugins_product_not_available(self):
-        """_prepare_search can use another set of search and auth plugins than the ones of the preferred provider"""
+        """_prepare_search can use another search plugin than the preferred one"""
         # Document a special behaviour whereby the search and auth plugins don't
         # correspond to the preferred one. This occurs whenever the searched product
         # isn't available for the preferred provider but is made available by  another
@@ -1376,9 +1371,8 @@ class TestCoreSearch(TestCoreBase):
         try:
             self.dag.set_preferred_provider("theia")
             base = {"productType": "S2_MSI_L1C"}
-            prepared_search = self.dag._prepare_search(**base)
-            self.assertEqual(prepared_search["search_plugins"][0].provider, "peps")
-            self.assertEqual(prepared_search["auth"]["peps"].provider, "peps")
+            search_plugins, _ = self.dag._prepare_search(**base)
+            self.assertEqual(search_plugins[0].provider, "peps")
         finally:
             self.dag.set_preferred_provider(prev_fav_provider)
 
@@ -1420,7 +1414,6 @@ class TestCoreSearch(TestCoreBase):
         mock__do_search.assert_called_once_with(
             self.dag,
             mock_get_search_plugins.return_value[0],
-            auth=mock_get_auth_plugin.return_value,
             id="foo",
             productType="bar",
         )
@@ -1668,7 +1661,7 @@ class TestCoreSearch(TestCoreBase):
             pagination = {}
 
         search_plugin.config = DummyConfig()
-        prepare_seach.return_value = dict(search_plugin=search_plugin)
+        prepare_seach.return_value = ([search_plugin], {})
         page_iterator = self.dag.search_iter_page_plugin(
             items_per_page=2, search_plugin=search_plugin
         )
@@ -1695,7 +1688,7 @@ class TestCoreSearch(TestCoreBase):
             pagination = {}
 
         search_plugin.config = DummyConfig()
-        prepare_seach.return_value = dict(search_plugin=search_plugin)
+        prepare_seach.return_value = ([search_plugin], {})
         page_iterator = self.dag.search_iter_page_plugin(
             items_per_page=2, search_plugin=search_plugin
         )
@@ -1720,7 +1713,7 @@ class TestCoreSearch(TestCoreBase):
             pagination = {}
 
         search_plugin.config = DummyConfig()
-        prepare_seach.return_value = dict(search_plugin=search_plugin)
+        prepare_seach.return_value = ([search_plugin], {})
         page_iterator = self.dag.search_iter_page_plugin(
             items_per_page=2, search_plugin=search_plugin
         )
@@ -1736,7 +1729,7 @@ class TestCoreSearch(TestCoreBase):
         """search_iter_page must propagate errors"""
         search_plugin.provider = "peps"
         search_plugin.query.side_effect = AttributeError()
-        prepare_seach.return_value = dict(search_plugin=search_plugin)
+        prepare_seach.return_value = ([search_plugin], {})
         page_iterator = self.dag.search_iter_page_plugin()
         with self.assertRaises(AttributeError):
             next(page_iterator)
@@ -1806,13 +1799,13 @@ class TestCoreSearch(TestCoreBase):
 
         search_plugin.config = DummyConfig()
 
-        # Infinite generator her because passing directly the dict to
+        # Infinite generator here because passing directly the dict to
         # prepare_search.return_value (or side_effect) didn't work. One function
         # would do a dict.pop("search_plugin") that would remove the item from the
         # mocked return value. Later calls would then break
         def yield_search_plugin():
             while True:
-                yield {"search_plugins": [search_plugin]}
+                yield ([search_plugin], {})
 
         prepare_seach.side_effect = yield_search_plugin()
         all_results = self.dag.search_all(items_per_page=2)