Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/permissions blurring #2571

Closed
wants to merge 17 commits into from

Conversation

mvergez
Copy link
Contributor

@mvergez mvergez commented May 25, 2023

Implémentation du floutage des données sensibles dans la synthèse en suivant les spécifications détaillées ici #2558

Closes #2558

@mvergez mvergez force-pushed the feat/permissions-blurring branch 2 times, most recently from 84bc884 to 269ce71 Compare August 2, 2023 12:21
@mvergez
Copy link
Contributor Author

mvergez commented Aug 9, 2023

Le seul test qui ne fonctionne pas (test_get_one_synthese_sensitive) pourra fonctionner une fois que la PR : PnX-SI/RefGeo#11 sera mergée car il dépend de la hiérarchisation des zones

@mvergez mvergez marked this pull request as ready for review August 18, 2023 14:00
@mvergez mvergez marked this pull request as draft August 18, 2023 14:00
@bouttier bouttier force-pushed the feat/permissions-blurring branch from 75c4aea to 8a59c42 Compare September 5, 2023 16:53
@mvergez mvergez force-pushed the feat/permissions-blurring branch from ec3d3d4 to 414b722 Compare November 6, 2023 08:36
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (85d6931) 78.57% compared to head (1a95cfa) 69.01%.

❗ Current head 1a95cfa differs from pull request most recent head 23bbdf2. Consider uploading reports for the commit 23bbdf2 to get more accurate results

Files Patch % Lines
...nature/core/gn_synthese/utils/query_select_sqla.py 87.50% 4 Missing ⚠️
backend/geonature/core/gn_synthese/models.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2571      +/-   ##
===========================================
- Coverage    78.57%   69.01%   -9.56%     
===========================================
  Files           84       86       +2     
  Lines         6907     7542     +635     
===========================================
- Hits          5427     5205     -222     
- Misses        1480     2337     +857     
Flag Coverage Δ
pytest 69.01% <96.42%> (-9.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvergez mvergez force-pushed the feat/permissions-blurring branch from 9c74f27 to 4d0c0db Compare November 8, 2023 15:33
@camillemonchicourt camillemonchicourt added this to the 2.14 milestone Nov 13, 2023
for perm in permissions:
if perm.has_other_filters_than("SCOPE", "SENSITIVITY"):
continue # unsupported filters
if perm.sensitivity_filter and self.nomenclature_sensitivity.cd_nomenclature != "0":
continue # sensitivity filter denied access, check next permission
if perm.sensitivity_filter and "nomenclature_sensitivity" not in unloaded_objs:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si nomenclature_sensitivity n’est pas chargé, on applique pas le floutage ? Ça me semble un problème. C’est dans quel cas que cette nomenclature n’est pas chargé ? Si trop lourd de rajouter cette jointure, il reste possible d’utiliser l’id_nomenclature de l’obs directement, en récupérant les nomenclatures à partir de leur code (avec du cache pour les récupérer une seule fois évidemment !).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La nomenclature_sensitivity n'est pas chargée lors des requêtes où la jointure n'est pas réalisée et un raiseload est mis, je trouve dommage d'ajouter une jointure juste pour que ça marche. Donc oui peut-être est-ce mieux de passer par les id_nomenclature. mais idem, c'est dommage de définir une fonction avec du cache juste pour ça...
Il faudrait trouver un moyen de voir si on est vraiment dans un contexte de floutage ou pas...

docs/images/blurring_query.svg Outdated Show resolved Hide resolved
l'observation à la maille de 10km par exemple.

Comme décrit ci-dessous, un paramètre en configuration a été ajoutée pour donner l'a possibilité d'exclure
toutes les données sensibles plutôt que de les flouter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donner un exemple typique de jeu de permissions : précis données perso/orga + floutée données de tous le monde.

backend/geonature/core/gn_synthese/routes.py Outdated Show resolved Hide resolved
backend/geonature/core/gn_synthese/routes.py Outdated Show resolved Hide resolved
backend/geonature/core/gn_synthese/routes.py Outdated Show resolved Hide resolved
backend/geonature/core/gn_synthese/routes.py Outdated Show resolved Hide resolved
# Careful: do not db.session.commit after these lines !!!
# Because we set manually the synthese attributes
synthese.the_geom_4326 = the_geom
synthese.areas = synthese_for_areas.areas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On ne peut pas utiliser directement synthese_for_areas en tant que synthese ? Ça éviterait cette modification de synthese (outre de commiter, l’auto-flush de sqlalchemy peut faire des surprises). Si pas possible d’utiliser synthese_for_areas directement, peut-être faire db.session.expunge(synthese) avant de le modifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui je suis d'accord avec toi, je vais regarder, je te tiens au courant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De ce que j'ai testé :

  • Renvoyer le synthese_for_areas fonctionne. Mais faut faire gaffe car ce n'est pas le même objet que plus haut car il n'y a pas toutes les options comme ici : https://github.com/PnX-SI/GeoNature/pull/2571/files/07f33f26036ced488e1f77159a3fa32c8ea517a6#diff-5fb8d19676b01cc7c0b77799d74e25c6a6ff510372180a633a2ecd1682c3c4d9R485 (on pourrait les stocker dans une variable et les réutiliser, à voir)
  • expunge ne fonctionne pas car cela renvoie l'erreur suivante, je ne comprends pas trop, j'ai l'impression qu'il essaie de loader les relationships (?) :
     sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Synthese at 0x7f95fc504400> is not bound to a Session; lazy load operation of attribute 'source' cannot proceed (Background on this error at: http://sqlalche.me/e/13/bhk3)
  • Possibilité d'ajouter : with DB.session.no_autoflush: pour être sûr mais je comprends qu'assigner une geom à un objet ORM c'est pas ouf...

@mvergez
Copy link
Contributor Author

mvergez commented Dec 4, 2023

En discutant avec @bouttier, cette PR nécessite 2 PR sur RefGeo avant d'être mergée :

Donc c'est normal que les actions de tests ne fonctionnent pas pour le moment...

@camillemonchicourt
Copy link
Member

OK, mais les tests ne passent pas non plus sur ces 2 PR ?
Enfin seulement sur SQLA 1.3, mais peut-être pas grave car on va passer sur SQLA 1.4 dans la prochaine version de GeoNature ?

@mvergez mvergez force-pushed the feat/permissions-blurring branch from 50fd60c to d0fd4f2 Compare December 14, 2023 09:25
@bouttier
Copy link
Contributor

bouttier commented Jan 5, 2024

Remplacé par #2687 qui reprend le travail initié ici

@bouttier bouttier closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants