search

assemblee-virtuelle / archipelago

Fostering interconnections between communities by creating synergies between their platforms
Apache License 2.0
14 stars 6 forks source link

[Minor] Handle list permissions correctly #154

Open mguihal opened 8 months ago

mguihal commented 8 months ago

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 :

Dépendances

⚠️ Cette PR est dépendante de la PR https://github.com/assemblee-virtuelle/semapps/pull/1211 sur Semapps.

srosset81 commented 7 months ago
  • 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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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