Lever des exceptions pundit plutôt qu’AR lorsqu’un agent essaie d’accéder à une orga étrangère

[adipasquale]

corrige #4383

Lorsqu’un agent accède à une URL d’une orga à laquelle il n’a pas accès :

1. on lève une exception ActiveRecord::RecordNotFound
2. l’agent voit une 404
3. on notifie Sentry

Cette PR propose de changer tout ce comportement :

1. lever une exception Pundit::NotAuthorizedError
2. rediriger vers la page précédente ou la home de l’agent avec un flash d’erreur "vous n’avez pas le droit"
3. ne pas notifier sentry

Il me semble que c’est un meilleur comportement pour les agents et que cela nous fera moins de bruit dans Sentry

Followup

• [ ] une autre PR similaire pour les contrôleurs agent de la partie "admin"

[adipasquale]

merci pour tes retours @victormours ! peut-être à discuter de vive voix aussi.

Je n’ai pas codé ce comportement de redirection 302 avec flash d’erreur, c’est celui par défaut qui se produit pour les Pundit::NotAuthorizedError cf Admin::AuthenticatedControllerConcern

Ce qui m’importe avec cette PR c’est effectivement de limiter le bruit sur Sentry.

Je n’ai pas d’avis fort sur ce qui devrait être le comportement dans ce cas :

1. cacher la vraie erreur avec une 404
2. exprimer clairement l’erreur de permissions que ce soit avec une 302 + flash ou autre chose

Mon avis faible va plutôt pour 2 :

• Il me semble que 2. est dans tous les cas une meilleure UX que 1. malgré que le flash ne soit pas lu, l’info correcte est présente tandis qu’avec 1. l’info est cachée ou alors on pourra au mieux écrire des messages plus flous comme "il se peut que le problème vienne d’un problème de permissions".
• la version 2. est un peu plus sécurisée pour le sniffing mais on parle ici uniquement des agents loggés ce qui limite la surface « d’attaque » et effectivement l’info des IDs n’est pas très secrète (voire publique ?)
• la quasi totalité des occurences semble être des redirections post sign in d’agents (potentiellement sur des ordis partagés) ou bien des liens venant de l’extérieur. On peut voir ça par ex ici dans sentry discover avec une colonne referer. il ne s’agit pas de liens cliqués dans l’appli qui pourraient sembler ne pas marcher si le flash n’est pas vu

@victormours Si tu as un avis fort pour 1 je peux changer cette PR dans cette direction

[adipasquale]

Discuté avec @victormours : en termes d’UX le comportement actuel 404 versus le changement de cette PR avec redirection et flash générique « vous n’avez pas le droit » se valent à peu près.

Or, le rétablissement du bruit des ActiveRecord::RecordNotFound avait justement été fait pour être alerté de problèmes d’UX améliorables. Cette PR va enlever ce bruit et on n’aura plus l’info qu’il faudrait améliorer cette UX.

C’est donc le moment de le faire :)

J’ai modifié le message du flash d’erreur pour ce cas précis :

Vous ne pouvez pas accéder à cette organisation

On peut itérer dessus assez facilement.

Au passage mes derniers commits :

• utilisent de manière plus native Pundit.authorize plutôt que d’instancier la policy directement. Cela permet de raiser exactement la bonne erreur pundit bien chargée avec les infos et d’utiliser le bon message d’erreur i18n
• corrige le nom de la spec sur la recherche d’usager coté agent dont j’ai modifié le comportement en effet de bord de cette PR

[adipasquale]

10 down 🚀