Correctif pour les tokens envoyés dans les sms aux responsables #3183
Conversation
|
Vu les tests qui échouent, je vais tenter une autre approche |
victormours
force-pushed
the
fix-responsable-sms-token
branch
from
2eb47a2
to
c702ac9
Compare
There was a problem hiding this comment.
Merci @victormours pour le fix et pour les tests 🙌 !
Je t'ai laissé un commentaire mais rien de bloquant.
Du coup pour ces cas-là (utilisateurs créés par les agents sans email), le token joue un rôle essentiel, lui seul permet à l'utilisateur de voir/modifier ses rdvs.
C'est probablement acceptable pour l'instant, mais à plus long terme on pourrait penser à une solution plus robuste, en faisant par exemple un matching sur le numéro de tel quand l'utilisateur s'inscrit sur la plateforme.
| @@ -50,7 +50,7 @@ def delete_token_from_session_and_redirect(error_msg) | |||
| end | |||
|
|
|||
| def invited_user | |||
| user_by_token || rdv_user_by_token&.user | |||
| user_by_token || rdv_user_by_token&.user&.user_to_notify | |||
There was a problem hiding this comment.
Ça me perturbe un peu de faire appel à une notion de notifications ici.
Mais c'est probablement juste le nom de la méthode, on pourrait peut-être avoir un alias du genre responsible_or_self, et/ou ajouter un commentaire ici précisant qu'il n'y a que le responsable qui peut voir et modifier le rdv.
There was a problem hiding this comment.
je crois que ce qui est particulier ici c'est que le user qui est notifié doit correspondre au user qu'on authentifie, donc c'est une bonne chose qu'on rende cette correspondance explicite en faisant appel à une notion de notification
victormours
force-pushed
the
fix-responsable-sms-token
branch
from
bb04cee
to
2ec7574
Compare
victormours
force-pushed
the
fix-responsable-sms-token
branch
from
2ec7574
to
c9b2280
Compare
Les sms envoyés aux responsables n'avaient pas de token. Voir https://sentry.incubateur.net/organizations/betagouv/issues/15798/?environment=production&query=is%3Aunresolved (une cinquantaine d'occurrences par jour)
Pour la spec, j'ai pas vraiment trouvé de test qui faisait vraiment des vérifications sur les sms au niveau intégration, donc je me suis positionné à ce niveau, vu qu'on est vraiment sur du bug difficile à voir autrement qu'en intégration.
Le fix est de s'assurer qu'on génère bien un token pour le user qui est notifié, et pas le user qui participe (et de faire la vérification correspondante au moment de l'authentification).
En regardant les tests de plus près, je me suis rendu compte que c'était pas évident de tester que le bon lien était envoyé, et qu'une bonne modification à apporter pour éviter ce genre de bug à l'avenir c'est de rendre le token obligatoire dans le lien envoyé par sms. En plus ça économise quelques caractères dans le sms.