Merge pull request #3916 from hove-io/optimize_verif_instance_for_user

Optimize while fetching authorized coverages for an user

Author: Patrick Qian

source/jormungandr/jormungandr/authentication.py

@@ -281,6 +281,11 @@ def get_user(token, abort_if_no_token=True):
                 g.user.id = 0
         else:
             g.user = cache_get_user(token)
+            if g.user.type == 'no_access':
+                flask_restful.abort(
+                    401,
+                    message='Token absent in the database You can get one at http://www.navitia.io or contact your support if you’re using the opensource version of Navitia https://github.com/hove-io/navitia',
+                )
 
         return g.user
 

source/jormungandr/jormungandr/instance_manager.py

@@ -41,7 +41,6 @@
 from jormungandr.exceptions import ApiNotFound, RegionNotFound, DeadSocketException, InvalidArguments
 from jormungandr import authentication, cache, app
 from jormungandr.instance import Instance
-from jormungandr.utils import can_connect_to_database
 import gevent
 import os
 
@@ -154,7 +153,7 @@ def initialization(self):
     def _clear_cache(self):
         logging.getLogger(__name__).info('clear cache')
         try:
-            cache.delete_memoized(self._all_keys_of_id)
+            cache.delete_memoized(self._exists_id_in_instance)
         except:
             # if there is an error with cache, flask want to access to the app, this will fail at startup
             # with a "working outside of application context"
@@ -219,21 +218,19 @@ def stop(self):
         if not self.thread_event.is_set():
             self.thread_event.set()
 
-    def _filter_authorized_instances(self, instances, api):
-        if not instances:
-            return []
-        # get_user is cached hence access to database only once when cache expires.
-        user = authentication.get_user(token=authentication.get_token())
-        # has_access returns true if can_connect_to_database = false when cache expires for each coverage
-        valid_instances = [
-            i for i in instances if authentication.has_access(i.name, abort=False, user=user, api=api)
+    def _get_authorized_instances(self, user, api):
+        authorized_instances = [
+            i
+            for name, i in self.instances.items()
+            if authentication.has_access(name, abort=False, user=user, api=api)
         ]
-        if not valid_instances:
+
+        if not authorized_instances:
             context = 'User has no access to any instance'
             authentication.abort_request(user, context)
-        return valid_instances
+        return authorized_instances
 
-    def _find_coverage_by_object_id(self, object_id):
+    def _find_coverage_by_object_id_in_instances(self, instances, object_id):
         if object_id.count(";") == 1 or object_id[:6] == "coord:":
             if object_id.count(";") == 1:
                 lon, lat = object_id.split(";")
@@ -244,32 +241,33 @@ def _find_coverage_by_object_id(self, object_id):
                 flat = float(lat)
             except:
                 raise InvalidArguments(object_id)
-            return self._all_keys_of_coord(flon, flat)
-        return self._all_keys_of_id(object_id)
+            return self._all_keys_of_coord_in_instances(instances, flon, flat)
 
-    @cache.memoize(app.config[str('CACHE_CONFIGURATION')].get(str('TIMEOUT_PTOBJECTS'), None))
-    def _all_keys_of_id(self, object_id):
-        instances = []
-        futures = {}
-        for name, instance in self.instances.items():
-            futures[name] = gevent.spawn(instance.has_id, object_id)
-        for name, future in futures.items():
-            if future.get():
-                instances.append(name)
+        return self._all_keys_of_id_in_instances(instances, object_id)
 
-        if not instances:
+    def _all_keys_of_id_in_instances(self, instances, object_id):
+        valid_instances = []
+        for instance in instances:
+            if self._exists_id_in_instance(instance, object_id):
+                valid_instances.append(instance)
+        if not valid_instances:
             raise RegionNotFound(object_id=object_id)
-        return instances
 
-    def _all_keys_of_coord(self, lon, lat):
+        return valid_instances
+
+    @cache.memoize(app.config[str('CACHE_CONFIGURATION')].get(str('TIMEOUT_PTOBJECTS'), None))
+    def _exists_id_in_instance(self, instance, object_id):
+        return instance.has_id(object_id)
+
+    def _all_keys_of_coord_in_instances(self, instances, lon, lat):
         p = geometry.Point(lon, lat)
-        instances = [i.name for i in self.instances.values() if i.has_point(p)]
+        valid_instances = [i for i in instances if i.has_point(p)]
         logging.getLogger(__name__).debug(
-            "all_keys_of_coord(self, {}, {}) returns {}".format(lon, lat, instances)
+            "_all_keys_of_coord_in_instances(self, {}, {}) returns {}".format(lon, lat, instances)
         )
-        if not instances:
+        if not valid_instances:
             raise RegionNotFound(lon=lon, lat=lat)
-        return instances
+        return valid_instances
 
     def get_region(self, region_str=None, lon=None, lat=None, object_id=None, api='ALL'):
         return self.get_regions(region_str, lon, lat, object_id, api, only_one=True)
@@ -284,27 +282,34 @@ def get_regions(self, region_str=None, lon=None, lat=None, object_id=None, api='
             return [i.name for i in valid_instances]
 
     def get_instances(self, name=None, lon=None, lat=None, object_id=None, api='ALL'):
-        available_instances = []
+        if name and name not in self.instances:
+            raise RegionNotFound(region=name)
+
+        # Request without token or bad token makes a request exception and exits with a message
+        # get_user is cached hence access to database only once when cache expires.
+        user = authentication.get_user(token=authentication.get_token())
+
+        # fetch all the authorized instances (free + private) using cached function has_access()
+        authorized_instances = self._get_authorized_instances(user, api)
+        if not authorized_instances:
+            # user doesn't have access to any of the instances
+            context = 'User has no access to any instance'
+            authentication.abort_request(user=user, context=context)
+
+        # Filter instances among instances in authorized_instances
         if name:
-            if name in self.instances:
-                available_instances = [self.instances[name]]
+            valid_instances = [i for i in authorized_instances if i.name == name]
         elif lon and lat:
-            available_instances = [
-                self.instances[k] for k in self._all_keys_of_coord(lon, lat) if k in self.instances
-            ]
+            valid_instances = self._all_keys_of_coord_in_instances(authorized_instances, lon, lat)
         elif object_id:
-            instance_keys = self._find_coverage_by_object_id(object_id)
-            if instance_keys is None:
-                available_instances = []
-            else:
-                available_instances = [self.instances[k] for k in instance_keys if k in self.instances]
+            valid_instances = self._find_coverage_by_object_id_in_instances(authorized_instances, object_id)
         else:
-            available_instances = list(self.instances.values())
-        valid_instances = self._filter_authorized_instances(available_instances, api)
-        if available_instances and not valid_instances:
+            valid_instances = authorized_instances
+
+        if not valid_instances:
             # user doesn't have access to any of the instances
-            context = 'User does not have access to any of the instances'
-            authentication.abort_request(user=authentication.get_user(None), context=context)
+            context = "User has no access to any instance or instance doesn't exist"
+            authentication.abort_request(user=user, context=context)
         else:
             return valid_instances
 

source/jormungandr/jormungandr/tests/instance_manager_tests.py

@@ -62,11 +62,11 @@ def get_instances_test(manager):
         assert len(instances) == 1
         assert 'paris' == instances[0].name
 
-        assert manager.get_instances('foo') == []
-
 
 def get_instances_by_coord_test(manager, mocker):
-    mock = mocker.patch.object(manager, '_all_keys_of_coord', return_value=['paris'])
+    mock = mocker.patch.object(
+        manager, '_all_keys_of_coord_in_instances', return_value=[manager.instances['paris']]
+    )
     with app.test_request_context('/'):
         instances = manager.get_instances(lon=4, lat=3)
         assert len(instances) == 1
@@ -75,7 +75,7 @@ def get_instances_by_coord_test(manager, mocker):
 
 
 def get_instances_by_object_id_test(manager, mocker):
-    mock = mocker.patch.object(manager, '_all_keys_of_id', return_value=['pdl'])
+    mock = mocker.patch.object(manager, '_all_keys_of_id_in_instances', return_value=[manager.instances['pdl']])
     with app.test_request_context('/'):
         instances = manager.get_instances(object_id='sa:pdl')
         assert len(instances) == 1

source/jormungandr/tests/authentication_tests.py

@@ -251,9 +251,9 @@ def test_status_code(self):
             # stopA and stopB and in main routing test, all is ok
             ('/v1/journeys?from=stopA&to=stopB&datetime=20120614T080000', 200),
             # stop1 is in departure board -> KO
-            ('/v1/journeys?from=stopA&to=stop2&datetime=20120614T080000', 403),
+            ('/v1/journeys?from=stopA&to=stop2&datetime=20120614T080000', 404),
             # stop1 and stop2 are in departure board -> KO
-            ('/v1/journeys?from=stop1&to=stop2&datetime=20120614T080000', 403),
+            ('/v1/journeys?from=stop1&to=stop2&datetime=20120614T080000', 404),
         ]
 
         with user_set(app, FakeUserAuth, 'bob'):
@@ -262,7 +262,7 @@ def test_status_code(self):
 
     def test_unkown_region(self):
         """
-        the authentication process must not mess if the region is not found
+        the authentication process prevails even if the region is not found
         """
         with user_set(app, FakeUserAuth, 'bob'):
             r, status = self.query_no_assert('/v1/coverage/the_marvelous_unknown_region/stop_areas')
@@ -419,7 +419,7 @@ def test_journeys_for_tgv(self):
             response = self.query('/v1/journeys?from=stopA&to=stopB&datetime=20120614T080000')
             assert 'error' not in response
             _, status = self.query_no_assert('/v1/journeys?from=stop1&to=stop2&datetime=20120614T080000')
-            assert status == 403
+            assert status == 404
 
             _, status = self.query_no_assert(
                 '/v1/coverage/empty_routing_test/journeys?from=stop1&to=stop2&datetime=20120614T080000'
@@ -444,9 +444,9 @@ def test_journeys_for_bobette(self):
         """
         with user_set(app, FakeUserAuth, 'bobette'):
             response, status = self.query_no_assert('/v1/journeys?from=stopA&to=stopB&datetime=20120614T080000')
-            assert status == 403
+            assert status == 404
             response, status = self.query_no_assert('/v1/journeys?from=stop1&to=stop2&datetime=20120614T080000')
-            assert status == 403
+            assert status == 404
 
             response, status = self.query_no_assert(
                 '/v1/journeys?from={from_coord}&to={to_coord}&datetime={d}'.format(

source/jormungandr/tests/journey_common_tests.py

@@ -1472,10 +1472,11 @@ def test_no_region(self):
 
         assert status != 200, "the response should not be valid"
 
-        assert response['error']['id'] == "unknown_object"
-
-        error_regexp = re.compile('^No region available for the coordinates.*')
-        assert error_regexp.match(response['error']['message'])
+        assert 'message' in response
+        assert (
+            response['message']
+            == "You don't have the permission to access the requested resource. It is either read-protected or not readable by the server."
+        )
 
 
 @dataset({"basic_routing_test": {}})