misc: add {% temporary_access_url %} for temporary access to formdata (#82088) #768

Merged
fpeters merged 1 commits from wip/82088-temporary-formdata-url into main 2023-10-31 12:34:38 +01:00
Owner
No description provided.
fpeters force-pushed wip/82088-temporary-formdata-url from b7875818e3 to 63c92f3728 2023-10-07 10:29:30 +02:00 Compare
fpeters force-pushed wip/82088-temporary-formdata-url from 63c92f3728 to 03c61f0dab 2023-10-07 10:46:37 +02:00 Compare
fpeters changed title from WIP: misc: add {% temporary_access_url %} for temporary access to formdata (#82088) to misc: add {% temporary_access_url %} for temporary access to formdata (#82088) 2023-10-07 11:23:25 +02:00
fpeters force-pushed wip/82088-temporary-formdata-url from 03c61f0dab to 7743372254 2023-10-07 11:56:44 +02:00 Compare
fpeters force-pushed wip/82088-temporary-formdata-url from 7743372254 to 227b8fe1e8 2023-10-07 12:03:49 +02:00 Compare
fpeters force-pushed wip/82088-temporary-formdata-url from 227b8fe1e8 to f14c4f03fb 2023-10-07 12:19:44 +02:00 Compare
fpeters reviewed 2023-10-07 14:15:52 +02:00
@ -101,3 +101,3 @@
assert resp.json['err'] == 0
assert resp.json['url'] == 'http://example.net/test/%s/' % formdata.id
assert resp.json['load_url'] == 'http://example.net/code/%s/load' % code.id
assert get_app(pub).get(resp.json['load_url']).location == formdata.get_url()
Author
Owner

L'API de code de suivi retourne désormais une URL d'accès temporaire, on vérifie qu'en y accédant on est bien envoyé vers la bonne demande.

L'API de code de suivi retourne désormais une URL d'accès temporaire, on vérifie qu'en y accédant on est bien envoyé vers la bonne demande.
@ -1658,7 +1658,6 @@ def test_form_draft_with_file(pub):
resp = login(get_app(pub), username='foo', password='foo').get('/')
resp.forms[0]['code'] = tracking_code
resp = resp.forms[0].submit()
assert resp.location == 'http://example.net/code/%s/load' % tracking_code
Author
Owner

À pas mal d'endroits dans les tests il y avait un utilisation ou vérification de /code/XXX/load, c'est modifié.

À pas mal d'endroits dans les tests il y avait un utilisation ou vérification de /code/XXX/load, c'est modifié.
@ -317,1 +318,3 @@
resp = get_app(pub).get('http://example.net/code/%s/load' % tracking_code)
resp = get_app(pub).get('/')
resp.forms[0]['code'] = tracking_code
resp = resp.forms[0].submit()
Author
Owner

Il y avait un petit raccourci ici à aller directement sur l'URL de code de suivi, c'est modifié pour passer par le formulaire de code de suivi (présent nativement sur la page d'accueil, mais qu'on ne voit généralement pas dans Publik).

Il y avait un petit raccourci ici à aller directement sur l'URL de code de suivi, c'est modifié pour passer par le formulaire de code de suivi (présent nativement sur la page d'accueil, mais qu'on ne voit généralement pas dans Publik).
@ -352,6 +360,19 @@ def test_form_tracking_code_rate_limit(pub, freezer):
# and ok again
get_app(pub).get('/code/ABC/load', status=404)
# ditto with POST to /code/
Author
Owner

Avec les changements dans le code il devient utile de faire du rate limiting ici aussi (avant c'était toujours une redirection, maintenant ça va envoyer directement une 404 en cas de code de suivi pas trouvé, sans rate limiting ça permettrait donc de passer sur toutes les combinaisons possibles).

Avec les changements dans le code il devient utile de faire du rate limiting ici aussi (avant c'était toujours une redirection, maintenant ça va envoyer directement une 404 en cas de code de suivi pas trouvé, sans rate limiting ça permettrait donc de passer sur toutes les combinaisons possibles).
@ -737,0 +763,4 @@
formdata = formdef.data_class().select()[0]
resp = get_app(pub).get(f'/code/{tracking_code}/load')
assert resp.location == formdata.get_url()
Author
Owner

C'est encore autorisé pour le moment.

C'est encore autorisé pour le moment.
@ -737,0 +768,4 @@
pub.load_site_options()
if not pub.site_options.has_section('options'):
pub.site_options.add_section('options')
pub.site_options.set('options', 'allow-tracking-code-in-url', 'false')
Author
Owner

Mais il y a déjà un feature flag pour retirer ça.

Mais il y a déjà un feature flag pour retirer ça.
@ -737,0 +774,4 @@
get_app(pub).get(f'/code/{tracking_code}/load', status=403)
def test_temporary_access_url(pub, freezer):
Author
Owner

Les tests exhaustifs sur la nouvelle URL avec long jeton.

Les tests exhaustifs sur la nouvelle URL avec long jeton.
wcs/api.py Outdated
@ -1242,3 +1242,3 @@
'err': 0,
'url': formdata.get_url(),
'load_url': get_publisher().get_frontoffice_url() + '/code/%s/load' % component,
'load_url': formdata.get_temporary_access_url(duration=300),
Author
Owner

L'API de code de suivi (utilisée depuis combo) renvoie donc désormais une URL d'accès temporaire.

La documentation prévient depuis longtemps :

dans l’attribut load_url une adresse permettant de charger la demande sur la seule foi de l’accès. Il est important d’utiliser cette adresse et de ne pas essayer de la construire manuellement avec le code de suivi car elle peut évoluer. Pour cette même raison elle devrait être utilisée immédiatement, sans être stockée.

L'API de code de suivi (utilisée depuis combo) renvoie donc désormais une URL d'accès temporaire. La documentation prévient depuis longtemps : > dans l’attribut load_url une adresse permettant de charger la demande sur la seule foi de l’accès. Il est important d’utiliser cette adresse et de ne pas essayer de la construire manuellement avec le code de suivi car elle peut évoluer. Pour cette même raison elle devrait être utilisée immédiatement, sans être stockée.
wcs/formdata.py Outdated
@ -845,0 +849,4 @@
token.context = {
'form_slug': self.formdef.slug,
'form_type': self.formdef.xml_root_node,
'form_number_raw': self.id,
Author
Owner

Dans l'absolu ça permettrait de référencer une fiche mais il n'y a a priori pas d'utilité.

Dans l'absolu ça permettrait de référencer une fiche mais il n'y a a priori pas d'utilité.
@ -158,2 +158,3 @@
def load(self):
@classmethod
def get_formdata_from_code(cls, code):
Author
Owner

La récupération d'un formdata depuis un code de suivi est déplacée dans cette méthode de classe, pour pouvoir ensuite être également exploitée par /code/load

La récupération d'un formdata depuis un code de suivi est déplacée dans cette méthode de classe, pour pouvoir ensuite être également exploitée par /code/load
@ -185,2 +203,4 @@
raise errors.AccessForbiddenError()
if not bypass_checks:
if formdata.formdef.enable_tracking_codes is False:
Author
Owner

Le bypass_checks permet de faire une URL d'accès temporaire sur une démarche sans code de suivi activé.

Le bypass_checks permet de faire une URL d'accès temporaire sur une démarche sans code de suivi activé.
@ -214,2 +214,2 @@
bad_content = False
if form.is_submitted() and not form.has_errors():
if not bypass_checks:
verify_fields = []
Author
Owner

Et le bypass_checks zappe aussi la partie où on demande le contenu d'un champ existant.

C'est la situation du besoin d'Anaïs, pouvoir en cours de saisie faire un lien vers la connexion pour inviter l'usager à se connecter, et revenir où il était, ça serait nul au retour de demander à l'usager ce qu'il vient juste de saisir.

À part ça le code ici ne change pas, il est juste indenté.

Et le bypass_checks zappe aussi la partie où on demande le contenu d'un champ existant. C'est la situation du besoin d'Anaïs, pouvoir en cours de saisie faire un lien vers la connexion pour inviter l'usager à se connecter, et revenir où il était, ça serait nul au retour de demander à l'usager ce qu'il vient juste de saisir. À part ça le code ici ne change pas, il est juste indenté.
@ -246,2 +273,4 @@
def load(self):
if get_request().get_method() != 'POST':
raise MethodNotAllowedError(allowed_methods=['POST', 'PUT'])
Author
Owner

Le formulaire en page d'accueil (évoqué plus haut) amenait sur /code/load?code=XXX, ici c'est modifié pour accepter uniquement du POST. (parce que pourquoi interdire /code/XXX/load si /code/load?code=XXX permet pareil).

Le formulaire en page d'accueil (évoqué plus haut) amenait sur /code/load?code=XXX, ici c'est modifié pour accepter uniquement du POST. (parce que pourquoi interdire /code/XXX/load si /code/load?code=XXX permet pareil).
@ -2072,3 +2102,3 @@
r += htmltext('<h3>%s</h3>') % _('Tracking code')
r += htmltext('<form action="/code/load">')
r += htmltext('<form action="/code/load" method="POST">')
r += htmltext('<input size="12" name="code" placeholder="%s"/>') % _('ex: RPQDFVCD')
Author
Owner

On passe donc en POST.

On passe donc en POST.
@ -447,6 +447,7 @@ class QommonPublisher(Publisher):
'unused-files-behaviour': 'remove',
'relatable-hosts': '',
'disabled-fields': 'ranked-items, table, table-select, tablerows',
'allow-tracking-code-in-url': 'true',
Author
Owner

Pour le moment on accepte encore, ça sera changé.

Pour le moment on accepte encore, ça sera changé.
@ -1192,0 +1198,4 @@
# {% temporary_access_url %}
# parameters:
# * days/hours/minutes/seconds, to set duration of access (by default, 5 minutes)
# * bypass_checks, to give direct access, without checking if tracking code is enabled or
Author
Owner

On arrive enfin au template tag évoqué dans le ticket.

On arrive enfin au template tag évoqué dans le ticket.
@ -1192,0 +1215,4 @@
for amount, unit in ((days, 86400), (hours, 3600), (minutes, 60), (seconds, 1)):
duration += (unlazy(amount) or 0) * unit
if not duration:
duration = 300 # 5 minutes
Author
Owner

Par défaut 5 minutes.

Par défaut 5 minutes.
@ -1192,0 +1217,4 @@
if not duration:
duration = 300 # 5 minutes
duration = min(duration, 10 * 86400) # maximum duration is 10 days.
Author
Owner

Et jamais plus de 10 jours.

Et jamais plus de 10 jours.
fpeters force-pushed wip/82088-temporary-formdata-url from f14c4f03fb to edd7867221 2023-10-24 18:16:40 +02:00 Compare
fpeters reviewed 2023-10-24 18:17:14 +02:00
@ -1192,0 +1197,4 @@
):
# {% temporary_access_url %}
# parameters:
# * days/hours/minutes/seconds, to set duration of access (by default, 30 minutes)
Author
Owner

Valeur par défaut à 30 minutes pour suivre le commentaire https://dev.entrouvert.org/issues/82088#note-9 (qui n'était pas une relecture).

Valeur par défaut à 30 minutes pour suivre le commentaire https://dev.entrouvert.org/issues/82088#note-9 (qui n'était pas une relecture).
fpeters force-pushed wip/82088-temporary-formdata-url from edd7867221 to 0feb35a6e8 2023-10-24 18:35:08 +02:00 Compare
tnoel approved these changes 2023-10-31 11:59:31 +01:00
tnoel left a comment
Owner

J'ai posé deux commentaires anecdotiques. Pour le reste, il me semble que tu as pensé à tout.

J'ai posé deux commentaires anecdotiques. Pour le reste, il me semble que tu as pensé à tout.
@ -737,0 +805,4 @@
# valid token
token.context = {'form_slug': formdef.slug, 'form_number_raw': formdata.id}
token.store()
get_app(pub).get(f'/code/{token.id}/load', status=302)
Owner

Je m'attendais éventuellement à un test du « location » reçu, mais 302 est déjà un signe que ça marche, on peut effectivement s'en contenter.

Je m'attendais éventuellement à un test du « location » reçu, mais 302 est déjà un signe que ça marche, on peut effectivement s'en contenter.
Author
Owner

Ok j'ajoute :

@@ -805,7 +805,8 @@ def test_temporary_access_url(pub, freezer):
     # valid token
     token.context = {'form_slug': formdef.slug, 'form_number_raw': formdata.id}
     token.store()
-    get_app(pub).get(f'/code/{token.id}/load', status=302)
+    resp = get_app(pub).get(f'/code/{token.id}/load', status=302)
+    assert resp.location == formdata.get_url()
Ok j'ajoute : ``` @@ -805,7 +805,8 @@ def test_temporary_access_url(pub, freezer): # valid token token.context = {'form_slug': formdef.slug, 'form_number_raw': formdata.id} token.store() - get_app(pub).get(f'/code/{token.id}/load', status=302) + resp = get_app(pub).get(f'/code/{token.id}/load', status=302) + assert resp.location == formdata.get_url() ```
wcs/api.py Outdated
@ -1242,3 +1242,3 @@
'err': 0,
'url': formdata.get_url(),
'load_url': get_publisher().get_frontoffice_url() + '/code/%s/load' % component,
'load_url': formdata.get_temporary_access_url(duration=300),
Owner

Ces 300 secondes sont dans l'idée que le load_url va être appelé très rapidement après ? (par combo, typiquement) ? 300 secondes ça parait presque un peu long dans ce cas... Je mettrais éventuellement un petit commentaire ici pour rappeler sommairement l'usage de ce load_url

Ces 300 secondes sont dans l'idée que le load_url va être appelé très rapidement après ? (par combo, typiquement) ? 300 secondes ça parait presque un peu long dans ce cas... Je mettrais éventuellement un petit commentaire ici pour rappeler sommairement l'usage de ce load_url
Author
Owner

Commentaire pas des masses inspirés, et durée réduite à 30 secondes parce que ça doit être bien suffisant depuis combo.

+        # return load_url with a temporary access URL so the caller can directly
+        # redirect the user to the formdata.
         data = {
             'err': 0,
             'url': formdata.get_url(),
-            'load_url': formdata.get_temporary_access_url(duration=300),
+            'load_url': formdata.get_temporary_access_url(duration=30),
         }

(combo fait ceci :

            response = requests.get('/api/code/' + quote(code), remote_service=wcs_site, log_errors=False)
            if response.status_code == 200 and response.json().get('err') == 0:
                return response.json().get('load_url')

)

Commentaire pas des masses inspirés, et durée réduite à 30 secondes parce que ça doit être bien suffisant depuis combo. ``` + # return load_url with a temporary access URL so the caller can directly + # redirect the user to the formdata. data = { 'err': 0, 'url': formdata.get_url(), - 'load_url': formdata.get_temporary_access_url(duration=300), + 'load_url': formdata.get_temporary_access_url(duration=30), } ``` (combo fait ceci : ``` response = requests.get('/api/code/' + quote(code), remote_service=wcs_site, log_errors=False) if response.status_code == 200 and response.json().get('err') == 0: return response.json().get('load_url') ``` )
fpeters force-pushed wip/82088-temporary-formdata-url from 0feb35a6e8 to 8371c66c3e 2023-10-31 12:09:01 +01:00 Compare
fpeters merged commit 87beba821c into main 2023-10-31 12:34:38 +01:00
fpeters deleted branch wip/82088-temporary-formdata-url 2023-10-31 12:34:38 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/wcs#768
No description provided.