Produits Entr'ouvert » Passerelle

On va se retrouver sur le soucis de cookies partagés entre différentes requêtes alors que le souhait est uniquement d'avoir ça le long de la requête, il me semble; il faudrait plutôt voir pour qu'en début de endpoint un CookieJar dédié soit assigné, libéré à la fin.

Pas sûr de bien te suivre, je détaille mon cas d'usage.
Pour une requête arrivant à un endpoint du connecteur, je dois faire les requêtes suivantes vers planitech :

• première requête pour obtenir un token.
• une deuxième requête pour envoyer un hash de ce token + password. La réponse à cette requête positionne un cookie qui permet l'authentification pour les requêtes suivantes.
• troisième requête : je fais vraiment quelque chose avec l'API de planitech

Et peut-être une une quatrième requête comme la troisième selon le endpoint, je ne sais pas encore, à voir.

Bon je te relis mieux et je te comprends mieux, maintenant je réfléchis.

Bon, avant tout, l'origine de la suppression des cookies c'est une situation dans combo, qui peut-être ne s'appliquait pas à Passerelle. Et à découvrir ça, il me semble aussi que ce patch ne marcherait pas pour toi.

Sur l'idée, dans le connecteur, qu'on trouve des self.requests.get(...), mais ce self.requests, c'est :

    @property
    def requests(self):
        return passerelle.utils.Request(resource=self, logger=self.logger)

et donc, créé à chaque fois, ce qui fait cookiejar vide à chaque appel.

Ton test passe à côté de ça, en créant et en réutilisant un même objet request.

Ce que cela voudrait dire, c'est que le clear_cookies() n'était pas utile, mais aussi, que les cookies sont de toute façon absent et qu'il te faut trouver autre chose.

Et donc les choses faisant leur tour, pas le sens que j'imaginais mais quand même, ma suggestion du commentaire précédent se traduirait par quelque chose de ce genre :

diff --git a/passerelle/utils/__init__.py b/passerelle/utils/__init__.py
index 815fe79..604d06d 100644
--- a/passerelle/utils/__init__.py
+++ b/passerelle/utils/__init__.py
@@ -214,8 +214,9 @@ class Request(RequestSession):
         if 'timeout' not in kwargs:
             kwargs['timeout'] = settings.REQUESTS_TIMEOUT

-        # don't use persistent cookies
-        self.cookies.clear()
+        # persist cookie for the endpoint duration
+        if self.resource and hasattr(self.resource, 'cookiejar'):
+            self.cookies = self.resource.cookiejar

         response = super(Request, self).request(method, url, **kwargs)

diff --git a/passerelle/views.py b/passerelle/views.py
index 33b8ccc..3ebc229 100644
--- a/passerelle/views.py
+++ b/passerelle/views.py
@@ -365,6 +365,7 @@ class GenericEndpointView(GenericConnectorMixin, SingleObjectMixin, View):
             if result is not None:
                 return result

+        self.cookiejar = requests.cookies.cookiejar_from_dict({})
         result = self.endpoint(request, **params)
         if request.method == 'GET' and self.endpoint.endpoint_info.cache_duration:
             cache.set(cache_key, result, self.endpoint.endpoint_info.cache_duration)

Ce que cela voudrait dire, c'est que le clear_cookies() n'était pas utile, mais aussi, que les cookies sont de toute façon absent et qu'il te faut trouver autre chose.

clear_cookies clairement pas utile.
Sur #24404 dans combo la session est partagée :

diff --git a/combo/utils/requests_wrapper.py b/combo/utils/requests_wrapper.py
index 6dbde7d..52594b9 100644
--- a/combo/utils/requests_wrapper.py
+++ b/combo/utils/requests_wrapper.py
@@ -128,4 +128,5 @@ class Requests(RequestsSession):

         return response

+    
 requests = Requests()

Ici le fait que l'attribut requests soit une factory de session nous prémunit des problèmes de #24404.

En tenant compte de cette histoire de factory, mon patch marche marche bel et bien en procédant comme suit :

def _login(self):
        auth_url = urlparse.urljoin(self.url, 'auth')
        session = self.requests
        ...
        # faire ce qui doit être fait avec cette session pour obtenir un cookie
        .....
        return session

    @endpoint(methods=['get'], perm='can_access')
    def getplaceslist(self, request):
        session = self._login()
        response = session.get(urlparse.urljoin(self.url, 'getPlacesList'))
        data = mste.decode(response.json())
        return {'data': data}

Je comprends l'idée ton patch, mais je crois que ça marchera pas parce que dans passerelle/views.py::perform on pas accès à l'objet resource sur lequel il faudrait positionner le cookiejar.

Bref je pense que le bon patch c'est ça (testé en procédant comme décrit précédemment) :

diff --git a/passerelle/utils/__init__.py b/passerelle/utils/__init__.py
index 815fe79..766dd78 100644
--- a/passerelle/utils/__init__.py
+++ b/passerelle/utils/__init__.py
@@ -214,9 +214,6 @@ class Request(RequestSession):
         if 'timeout' not in kwargs:
             kwargs['timeout'] = settings.REQUESTS_TIMEOUT

-        # don't use persistent cookies
-        self.cookies.clear()

Je veux des cookies, j'utilise la même session. J'en veux pas j'en demande une nouvelle via l'attribut requests.
Non ?

En tenant compte de cette histoire de factory, mon patch marche marche bel et bien en procédant comme suit :

Mais c'est moche comme tout, ça suppose se souvenir que self.requests c'est pas juste un raccourci qui va bien (logging etc.) mais quelque chose de plus compliqué, avec un état, qu'on doit appeler mais conserver si on veut, etc.

Je comprends l'idée ton patch, mais je crois que ça marchera pas parce que dans passerelle/views.py::perform on pas accès à l'objet resource sur lequel il faudrait positionner le cookiejar.

Il fallait bien sûr y lire connector.cookiejar = ... et pas self.cookiejar = ... parce que oui, "resource" c'est "connector".

Frédéric Péters a écrit :

En tenant compte de cette histoire de factory, mon patch marche marche bel et bien en procédant comme suit :

Mais c'est moche comme tout, ça suppose se souvenir que self.requests c'est pas juste un raccourci qui va bien (logging etc.) mais quelque chose de plus compliqué, avec un état, qu'on doit appeler mais conserver si on veut, etc.

J'ai l'opinion inverse : je trouve que c'est moche comme tout d'essayer de re-implémenter nous même une notion de session dans un endpoint alors qu'on l'a déjà gratos et proprement dans une session requests qui est faite exactement pour ça.

L'attribut s'appelle requests, c'est une propriété, ce qui ne laisse pas non plus penser qu'à chaque appel il y aura un résultat différent.

D'un point de vue documentation, c'est simple et clair d'écrire « self.requests utilisé ici est un wrapper léger au-dessus du module Requests, qui ajoute une journalisation et une expiration automatique des appels. », j'aimerais pouvoir conserver ça.

Ma proposition le permet, tout en gagnant "les cookies persistent le long des différentes requêtes que peut faire l'endpoint".

À expliquer c'est "Si jamais vous avez besoin de persister les cookies le long des appels de votre endpoint, remplacez vos self.request. par des self.session." ? Je ne trouve pas ça terrible et je continue à penser que ma proposition fait le job sans rien demander à la documentation.

Avec quelques aménagements nécessaires.

Je suis surpris par la nécessité de :

        if getattr(self, 'cookies', None) and self.resource \
           and hasattr(self.resource, 'cookiejar'):
            self.resource.cookiejar = self.cookies

j'aurais imaginé requests utilisé le cookiejar qu'on lui donnait, pas l'écraser. Je vais lire un peu.

j'aurais imaginé requests utilisé le cookiejar qu'on lui donnait, pas l'écraser. Je vais lire un peu.

Et c'est donc httmock qui remplace l'attribut. Ces trois lignes sont uniquement utiles pour httmock. Ça m'ennuie pas mal mais je ne vais pas me battre là... Je propose juste d'être explicite sur les raisons, façon :

--- a/passerelle/utils/__init__.py
+++ b/passerelle/utils/__init__.py
@@ -214,14 +214,16 @@ class Request(RequestSession):
         if 'timeout' not in kwargs:
             kwargs['timeout'] = settings.REQUESTS_TIMEOUT

-        # persist cookie for the endpoint duration
         if self.resource and hasattr(self.resource, 'cookiejar'):
+            # use cookies that will last the whole endpoint duration
             self.cookies = self.resource.cookiejar

         response = super(Request, self).request(method, url, **kwargs)

-        if getattr(self, 'cookies', None) and self.resource \
-           and hasattr(self.resource, 'cookiejar'):
+        if self.resource and hasattr(self.resource, 'cookiejar'):
+            # make sure we get the new cookiejar; it is actually not necessary
+            # in normal operations but httmock will replace the cookiejar
+            # instead of filling it :/
             self.resource.cookiejar = self.cookies

         if method == 'GET' and cache_duration and (response.status_code // 100 == 2):

Frédéric Péters a écrit :

Et c'est donc httmock qui remplace l'attribut. Ces trois lignes sont uniquement utiles pour httmock. Ça m'ennuie pas mal mais je ne vais pas me battre là...

Merci, je pensais que c'était requests, sans avoir pris le temps de vérifier.
Je trouve ça naze aussi, je prends un moment pour voir si je peux pas tester autrement et virer ces lignes.

getattr(self, 'cookies', None)

J'avais dégagé ça de mon diff, je ne pense pas que c'est nécessaire. (et ce que je trouve particulièrement moche c'est le \ de fin de ligne)