Sécurité: Empêcher un utilisateur d’accéder aux droits des prescripteurs habilités s’il effectue des démarches depuis une organisation non habilitée [GEN-2332]

[tonial]

🤔 Pourquoi ?

⚠️ Attention : il y a deux propositions dans cette PR : avec ou sans le dernier commit

1. request.user_is_authorized_prescriber (propriété sur l'objet request)
2. request.user.is_authorized_prescriber (propriété sur l'objet User)

J'attends qu'on ai choisi la solution retenue pour adapter les tests

Pour mieux prendre en compte les permissions des utilisateurs

J'ai mis toute l'équipe en relecteurs parce que c'est une property qu'on utilise à pas mal d'endroit et qui disparait.
À présent :

• soit c'est l'utilisateur de la requête et on peut dire s'il est actuellement sur une organisation habilitée
• soit c'est un utilisateur récupéré en base de donnée, et on ne peut que dire s'il est membre d'une organisation habilitée

J'ai trouvé plus simple de mettre la propriété sur le user pour éviter de passer l'objet request (notamment sur les @check_user) mais c'est ouvert à discussion : je pourrait mettre un request.user_is_authorized_prescriber à la place
On pourrait ajouter un post_generation sur la factory pour définir automatiquement is_authorized_prescriber plutôt que de le faire à la main à chaque fois, mais j'ai peut qu'on se retrouve à créer des bugs car on ne se rendrait pas compte qu'une fonction aura en argument un objet qui n'y a pas accès normalement.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

• Mettre à jour le CHANGELOG_breaking_changes.md ?
• Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

[notion-workspace]

☠️ Empêcher un utilisateur d’accéder aux droits des prescripteurs habilités s’il effectue des démarches depuis une organisation non habilitée

[xavfernandez]

Ça fait plaisir toutes ces requêtes en moins 👍

[rsebille]

Personnellement je suis pas super fan de simplifier un comportement en se basant sur des suppositions, si un jour elles changent alors tout devient faux et il y a peu de chance qu'on revienne sur ce bout de code, surtout si rien ne casse.

[tonial]

Cette property est actuellement fausse: si self.author.is_prescriber_with_authorized_org is False mais que self.author.is_prescriber is True alors ça veut dire qu'il était PH à l'époque du diag, et donc que geiq_allowance_amount() == 1400
Il ne faut donc pas regarder is_prescriber_with_authorized_org qui donne une vision aujourd'hui, et pas à l'époque.

La bonne approche est de stocker dans un champ le montant de l'aide, mais @xavfernandez a dit que ce n'était pas prioritaire, et qu'on pouvait faire comme ça en attendant.
Ça ne me dérange pas d'ajouter le montant sur l'objet, c'est assez rapide à faire dans une PR séparée.

[rsebille]

Pas directement lié aux changements de la PR, mais maintenant que ça utilise request on pourrais tout à fait faire des template filter et ainsi réduire un peu le boilerplate dans certaines vues.

[tonial]

Bonne idée !

[tonial]

Bon, je suis en train de regarder pour passer can_(view|edit)_personal_information en template filters, sauf que dans les template, on a des {{ job_application.job_seeker.get_full_name|mask_unless:can_view_personal_information }} ce qui fait qu'on doit mettre des {% with ... %} autour et c'est pas franchement mieux selon moi.
Je vais donc laisser de côté pour l'instant.

[vincentporte]

Bravo pour l'investigation Antoine !
Je comprends que si nous mettons la propriété sur User, nous devrions à chaque fois restester si user == request.user. Donc plutôt option 1.
Je comprends moins bien la question que tu soulèves sur les check_users, car la méthode reçoit déjà request.user.

[tonial]

Suite à la réunion on part sur request.from_authorized_prescriber

[celine-m-s]

Pourquoi avoir supprimé ce test ?

[tonial]

Parce qu'on ne peut plus appeler ce code, et que c'était redondant avec la suite du test (le post ne fait rien)

[celine-m-s]

Toutes ces requêtes en moins, ça fait bien plaisir !!! 🤩
J'ai tout relu mais je ne sais pas si tu as fini. N'hésite pas à redemander une relecture sinon.

[xavfernandez]

J'aurais bien vu de nouveaux checks du nouvel attribut dans les tests de TestItouCurrentOrganizationMiddleware

[xavfernandez]

Les deux derniers commits sont à inverser car beaucoup de tests échouent logiquement après le renommage de la méthode en is_prescriber_with_authorized_org_memberships alors que is_prescriber_with_authorized_org est encore bien utilisé.