Skip to content

[Minor] Handle list permissions correctly#154

Closed
mguihal wants to merge 1 commit into
masterfrom
fix-HandleListPermissionsCorrectly
Closed

[Minor] Handle list permissions correctly#154
mguihal wants to merge 1 commit into
masterfrom
fix-HandleListPermissionsCorrectly

Conversation

@mguihal

@mguihal mguihal commented Nov 30, 2023

Copy link
Copy Markdown
Collaborator

Hello,

Contexte

Le besoin initial de cette PR est de pouvoir masquer des éléments du menu en fonction de si l'utilisateur est connecté ou non, ou selon s'il est dans un certain groupe ou non. Par exemple, la liste des personnes peut être une page sensible qu'il n'est pas nécessaire de laisser visible à tous les utilisateurs non-connectés. Ou par exemple pour laisser seulement les admins du site pouvoir modifier les thèmes, les rôles ou les statuts.

En investiguant, j'ai remarqué que les pages de liste en général n'avait pas de permissions associées.

Proposition

Je propose donc cette PR dans laquelle j'ai effectué deux modifications :

  • La première pour rajouter le check des permissions sur les pages List.

  • La seconde pour également masquer les ressources dont la liste n'est pas accessible du menu. La proposition ici est de requêter les permissions de chacun des éléments affichés du menu, pour savoir si on a les permissions de le voir ou pas. Ca fonctionne, mais du coup fait autant de requête acl au chargement du site, et ça ralentit l'affichage du menu... Ca m'embête un peu parce que c'est pas top niveau expérience utilisateur de ne pas avoir le menu instantanément disponible, et aussi parce que ça rajoute beaucoup de requêtes sur le réseau, mais je n'arrive pas à voir comment faire ça d'une meilleure façon. On sera de toutes façons toujours obligé de faire un appel au backend, la seule autre option serait de grouper en un seul call toutes les permissions demandées, mais ça nécessite un peu plus de travail. N'hésitez pas à me dire s'il y a une solution plus élégante à ce problème 🙏

Dépendances

⚠️ Cette PR est dépendante de la PR assemblee-virtuelle/semapps#1211 sur Semapps.

@srosset81

Copy link
Copy Markdown
Contributor
  • La seconde pour également masquer les ressources dont la liste n'est pas accessible du menu. La proposition ici est de requêter les permissions de chacun des éléments affichés du menu, pour savoir si on a les permissions de le voir ou pas. Ca fonctionne, mais du coup fait autant de requête acl au chargement du site, et ça ralentit l'affichage du menu... Ca m'embête un peu parce que c'est pas top niveau expérience utilisateur de ne pas avoir le menu instantanément disponible, et aussi parce que ça rajoute beaucoup de requêtes sur le réseau, mais je n'arrive pas à voir comment faire ça d'une meilleure façon. On sera de toutes façons toujours obligé de faire un appel au backend, la seule autre option serait de grouper en un seul call toutes les permissions demandées, mais ça nécessite un peu plus de travail. N'hésitez pas à me dire s'il y a une solution plus élégante à ce problème 🙏

Dans une des premières version d'Archipelago, j'avais aussi essayé de charger les permissions des ressources avant de les afficher dans le menu, mais j'avais trouvé que l'expérience utilisateur était vraiment trop dégradée. C'est surtout vous qui utilisez Archipelago, donc je ne bloquerai pas, mais perso sur mes projets je n'utiliserai pas cette fonctionnalité.

Pour améliorer les performances, la solution serait en effet de faire une seule requête SPARQL. Je crois qu'elle pourrait être assez rapide, et ensuite on serait capable d'afficher le menu d'un seul coup. A voir comment ça pourrait se faire côté React-Admin... mais en tout cas, il faudrait garder aussi la possibilité de faire les requêtes individuelles (comme maintenant) car il est fort possible que dans le futur, il faille que le semantic-data-provider puisse gérer des serveurs sans endpoint SPARQL (notamment si on veut lui permettre de se connecter à des Pods Solid).

@mguihal

mguihal commented Dec 7, 2023

Copy link
Copy Markdown
Collaborator Author

Dans une des premières version d'Archipelago, j'avais aussi essayé de charger les permissions des ressources avant de les afficher dans le menu, mais j'avais trouvé que l'expérience utilisateur était vraiment trop dégradée. C'est surtout vous qui utilisez Archipelago, donc je ne bloquerai pas, mais perso sur mes projets je n'utiliserai pas cette fonctionnalité.

Pour améliorer les performances, la solution serait en effet de faire une seule requête SPARQL. Je crois qu'elle pourrait être assez rapide, et ensuite on serait capable d'afficher le menu d'un seul coup. A voir comment ça pourrait se faire côté React-Admin... mais en tout cas, il faudrait garder aussi la possibilité de faire les requêtes individuelles (comme maintenant) car il est fort possible que dans le futur, il faille que le semantic-data-provider puisse gérer des serveurs sans endpoint SPARQL (notamment si on veut lui permettre de se connecter à des Pods Solid).

Je peux tenter de factoriser la requête des permissions en une seule requête, parce que ça me chagrine aussi cette expérience utilisateur un peu dégradée par l'affichage un peu différée du menu. Cependant je suis un peu mitigé par le fait de passer par le endpoint SPARQL et par faire une requête SPARQL côté frontend, car ça expose beaucoup de logique métier au niveau frontend alors qu'elle pourrait être côté backend (structure de la base, permissions pour requêter les objets, etc...). Est-ce que créer une nouvelle route pour requêter toutes les permissions des containers dans le service webacl (en le surchargeant ici dans Archipelago si la nécessité de cette route se résume à ce repo) peut être une possibilité ? Comme ça toute la logique de savoir comment requêter en SPARQL se ferait côté backend, et le frontend aurait juste à appeler une route http://<middleware>/_containersAcl par exemple.

@simonLouvet

Copy link
Copy Markdown
Contributor

Comme ça toute la logique de savoir comment requêter en SPARQL se ferait côté backend, et le frontend aurait juste à appeler une route http://<middleware>/_containersAcl par exemple.

Il y a déjà une API http://<middleware> qui retourne les containers. Elle pourrait également retourné les droits sur ces containers si le user est authentifié?

@srosset81

Copy link
Copy Markdown
Contributor

http://<middleware> ne retourne que les containers racines. Et ça me semblerait être une mauvaise idée d'ajouter ici des informations sur les permissions, alors que pour le moment tout l'API WebACL passe par http://<middleware>/_acl/<resource-our-container-path>.

En revanche, comme il est prévu d'implémenter les Type-Indexes de Solid, on pourrait envisager de ne retourner que les containers que l'utilisateur connecté a le droit de lire ? Je ne crois pas que ça fait partie de la spec (qui est en cours d'écriture) mais ça aurait du sens. Pour le moment il y a la notion de Unlisted Type Index, mais c'est plutôt pour les containers que seul le propriétaire du pod peut voir.

Dernière option possible à mon avis: adapter l'endpoint VOID pour qu'il ne retourne que les containers que l'utilisateur a le droit de voir (en fetchant cet endpoint avec le token de l'utilisateur). Je crois que ça avait été envisagé au début, mais on avait abandonné cette option, je ne sais plus pourquoi...

@simonLouvet

Copy link
Copy Markdown
Contributor

Je suis assez d'accord avec @srosset81 d'essayer de mutualiser les travaux avec VOID ou Type-Indexes.
Proposition : que @mguihal fasse comme il le propose pour archipelgo et ouvrir une issue pour semapps en reportant cette discussion et avec la spécification de sur VOID ou Type-Indexes

Base automatically changed from next to master February 20, 2024 14:32
@mguihal

mguihal commented Aug 19, 2024

Copy link
Copy Markdown
Collaborator Author

Hello,

Je me permets de clôturer cette PR.

  • Les modifications sur les permissions des pages Liste ont été copiés dans cette PR : [Minor] Migrate Layout folder to Typescript #185
  • Concernant les permissions du menu, je vais réfléchir à aborder le problème différemment. Le besoin n'est plus prioritaire en tout cas, une solution via customization du layout a été trouvé, avec un menu non affiché par défaut.

@mguihal mguihal closed this Aug 19, 2024
@srosset81 srosset81 deleted the fix-HandleListPermissionsCorrectly branch August 20, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants