Project

General

Profile

Anomalie #2415

Les autorisations SPIP 3.0, oups ?

Added by Patrice Vanneufville almost 8 years ago. Updated over 7 years ago.

Status:
Fermé
Priority:
Immédiat
Assignee:
-
Category:
sécurité
Target version:
Start date:
11/14/2011
Due date:
% Done:

0%

Resolution:
Navigateur:

Description

Les autorisations SPIP 3.0 ont changé la donne depuis la rev #16301 (http://core.spip.org/projects/spip/repository/revisions/16301) qui passe le $type par la moulinette de@ objet_type()@, je ne sais pour quelle raison...

Du coup autoriser('configurer', 'plugins') amène à chercher une fonction autoriser_plugin_configurer() qui n'existe pas puisque le pluriel de $type est retiré...

Par ailleurs, est-ce que supprimer l'underscore est vraiment nécessaire ?
autoriser('configurer', 'mes_objets') va rechercher la fonction autoriser_mesobjet_configurer(), ce qui me parait évidemment totalement absurde.

Mes tests sont-ils partagés sous SPIP 3.0 ?
L'API des autorisations était déjà fragile amha, cette évolution est-elle pérenne ?

Associated revisions

Revision 18940 (diff)
Added by cedric@yterium.com over 7 years ago

Ferme #2415 : preciser les appels de autoriser qui ne mentionnent pas un objet en second argument

History

#1 Updated by cedric - almost 8 years ago

la raison première est que l'api autoriser s'applique aux objets :
autoriser('faire','type',$id)
en normalisant le type via objet_type on assure que toutes les fonctions génériques retomberont bien sur la bonne autorisation (voir les cas typiques des site/syndic forum/forums groupe_mot/groupes_mot/...).
De ce point de vue l'utilisation de objet_type est un bénéfice.

L'application de la fonction peut en effet poser problème dans le cas des autorisation 'détournée' qui ne s'appliquent pas à des objets (menu, configurer, et je ne sais pas si il y en a d'autre).
Pour menu on a deja une derogation (http://core.spip.org/projects/spip/repository/entry/spip/ecrire/inc/autoriser.php#L81), on peut envisager de l'appliquer aussi à configurer si c'est le seul autre cas ?

La suppression des _ dans le type permet également d'avoir des noms de fonction autoriser() plus lisible.

#2 Updated by Patrice Vanneufville almost 8 years ago

cedric, comme je l'écris dans le texte, il y a : autoriser('configurer', 'plugins'), d'où le caractère urgent de cette anomalie.

Dans autoriser('faire','type',$id), le 'type' est comme ça assez flou et peut servir à tout, pas seulement à des objets en base. Il sert déjà à tout de toute façon, et c'est parfait ainsi amha, ça 'universalise' l'API. Le 'type' sert également à imbriquer les niveaux d'autorisations suivant une certaine imbrication qui n'est pas mal, même si c'est loin d'être idéal pour une utilisation plus poussée.

A part ça, les underscores ne m'ont jamais posé de problème pour lire le nom d'une fonction.

Cette suppression du "s" m'a toujours paru, du point de vue de l'internationnalisation de SPIP et des pluriels irréguliers, un choix choquant. J'espérais que la table des alias allait tout résoudre, mais je me demande si on n'est pas encore au début du chemin...

#3 Updated by cedric - almost 8 years ago

Conserver les underscores créé une ambiguité potentielle sur le nom de la fonction et l'autorisation à laquelle elle s'applique. En les supprimant en évite ce risque.

Pour autoriser('configurer','plugins') on mappe en effet maintenant sur la fonction autoriser_plugin_configurer(). Il n'y a pas de trou de sécu côté core puisque seule autoriser_configurer() est implémentée.

Le type se refere bien a des objets en règle génerale. Et je demandais justement si les cas dérogeant étaient limités a menu/onglet/configurer ou si tu en as d'autres exemples ?

#4 Updated by Patrice Vanneufville almost 8 years ago

Mais il suffit de faire une recherche sur la zone et de lister toutes les fonctions autoriser_toto() ou tous les appels autoriser() ou autres balises #AUTORISER, pour s'apercevoir que beaucoup de plugins vont voir leurs autorisations non prises en compte sous SPIP 3 à cause de ce pluriel très bizarrement "singuliérisé".

Ce bug étant souvent silencieux, il me semble incroyable de s'engager désormais sur cette voie.

Depuis quand les autorisations sont-elle limitées aux objets ? Personnellement, je n'étais pas au courant, tout comme beaucoup d'autres auteurs de plugin.

Cette API prometteuse au départ est en train de s'étioler avec ce choix de rechercher des fonctions mises au singulier (l'underscore, à la limite, je pourrais m'y faire).

Je ne voudrais pas être lourd, mais SPIP a bien besoin de plus de clarté. Allez donc expliquer aux codeurs que autoriser('configurer','plugins') qui figure partout dans SPIP et sa galaxie, va chercher une fonction autoriser_plugin_configurer() !

Allez, je liste 2-3 choses vite fait :

- function autoriser_plugins_telecharger (STEP)
- function autoriser_groupemots_iconifier_dist (MOTS)
(ah oui c'est vrai, voici une exception...)
- function autoriser_voirrevisions_dist (REVISIONS)
(à la place de autoriser_revisions_voir_dist ?)
- function autoriser_statistiques_visites_menu_dist (STATISTIQUES)
(et non function autoriser_statistiquesvisites_menu_dist ?)
- function autoriser_orphelins_supprimer (MEDIATHEQUE)
- function autoriser_mes_fichiers_telecharger_dist (MES FICHIERS 2)
- function autoriser_seances_endroits_voir (SEANCES)

+ abomailmans, spip-lettres, spip-carto, SPIP-objets, admin_mot_cles, association, champs extras, contacts et organisations, etc., etc. !!

Je suis peut-être bête, mais là, le bon sens me dit que SPIP autorise d'une bien singulière manière !

#5 Updated by cam.lafit - almost 8 years ago

Bien le bonjour

Je n'avais pas d'eau à apporter au moulin, mais suite à discussion, je viens de me rappeler d'un cas que j'ai.
La gestion des autorisations de page depuis la squelette. Un exemple donné dans la discussion était :
#autoriser{voir, page-organigrammes}

Des s et des tirets la joie :)

#6 Updated by cedric - almost 8 years ago

je doute que autoriser{voir, page-organigrammes} ait jamais marché, puisque cela chercherai la fonction "autoriser_page-organigrammes_voir qui ne peut pas exister :)

#7 Updated by marcimat 🌈 almost 8 years ago

Cédric, ce que j'ai écrit était un exemple pour comprendre la situation possible de Camille. Effectivement, si tu as un squelettes ?page=photos, tu peux vouloir mettre dedans un #AUTORISER{voir,photos} ...

Je suis aussi perplexe par ces changements, certes utiles, mais qui donnent de nouveaux cheveux à retordre. Avant : a@utoriser(A, B) donnait une fonction @autoriser_B_A_dist() qui paraissait logique. Certes, le problème d'un souligné _ dans A ou B était source de bug potentielle. Cette correction est claire et logique à expliquer.

Mais appliquer objet_type() systématiquement sur B me fait vraiment bizarre. Enfin c'est bizarre surtout si l'objet n'existe pas.

#8 Updated by cedric - almost 8 years ago

regarde le code en v2.1 :
il y avait déjà une dérogation et un redressement pour le cas particulier des groupes de mots.
Dans le dev de la 3.0 il est apparu que l'objet 'site' necessitait aussi une derogation. D'ou la generalisation.

ton exemple AUTORISER{voir,photos} n'est pas bon non plus, car si jamais tu installe un plugin qui crée un objet photo tu vas melanger les autorisation de l'objet avec celles de ta page.

Donc il faudrait a minima un verbe discriminant 'voirpage'.
Mais cela veut dire qu'on a implicitement deux grandes familles d'autorisation : une sur les objets et une sur ... le reste (qui représente une proportion faible des appels a autoriser)

Comment les gere-t-on, les distingue-t-on ?

Peut être faut-il convenir d'un prefixe dans l'action ou le type qui permet de distinguer toutes les autorisations qui ne concernent pas les objets editoriaux ?
En tout état de cause le status quo par rapport à la v2.1 que demande Patrice ne me parait pas une bonne idée.

#9 Updated by marcimat 🌈 over 7 years ago

Cédric proposait sur IRC en en discutant l'autre jour qu'on pourrait définir un caractère de début de chaîne qui voudrait dire : ceci n'est pas un objet (en gros). Par exemple via :

autoriser('triturer','@photos') ou #AUTORISER{triturer,@photos}

Peut être à voir avec _ ou : ?

autoriser('triturer',':photos') ou #AUTORISER{triturer,:photos}
autoriser('triturer','_photos') ou #AUTORISER{triturer,_photos}

Est-ce que cette solution est un bon compromis ?

#10 Updated by marcimat 🌈 over 7 years ago

À ce propos, les noms d'autorisations des statistiques (http://zone.spip.org/trac/spip-zone/browser/_core_/plugins/statistiques/stats_autoriser.php) ne sont pas correctes actuellement en ?rev=56562

Un exemple :
autoriser_statistiques_referers_menu_dist doit devenir
- menu > onglet : (autoriser_statistiques_referers_onglet_dist)
- statistiques > stats : (autoriser_stats_referers_onglet_dist )
- espace > '' : (autoriser_statsreferers_onglet_dist )
- pluriel > singulier : (autoriser_statsreferer_onglet_dist )

Donc au final :
autoriser_statsreferer_onglet_dist

#11 Updated by Patrice Vanneufville over 7 years ago

Tout ceci ne me fait pas changer d'avis, ce n'est pas le nom des fonctions qu'il faut changer mais le traitement sur $type. Les règles de cette API marchent sur le tête amha, c'est bien trop complexe et source assurée de bugs. Il faudrait vraiment un fonctionnement simple, les autorisations c'est du sérieux.

L'idéal serait de pouvoir jongler facilement entre : autoriser($faire, $type='', $id=0, etc.) et autoriser($faire, $quoi='', $id=0, etc.). Et il y a beaucoup d'appels au sein du code de SPIP à cette API avec un $type qui n'est pas un objet !!

Afin d'éclaircir le code de SPIP, la fonction autoriser($faire, $quoi='', $id=0, etc.) se limiterait à faire un simple routage, et la fonction dédiée aux objets en base pourrait prendre un nouveau nom (singulier !) : autoriser_objet($faire, $quoi='', $id=0, etc.). De même pour autoriser_quoi($faire, $quoi='', $id=0, etc.). A voir si ces fonctions doivent être documentées, peut-être surchargeables, et directement utilisables dans les squelettes.

L'astuce de Marcimat avec le @ n'est pas géniale, mais pourrait faire l'affaire.

Autre piste dans le même ordre : si $id n'est pas un chiffre ou s'il est négatif (-1 par exemple), on peut éventuellement considérer que le $type (alias le $quoi) n'est pas un objet stocké en base et qu'il faut l'utiliser tel quel.

Enfin, quitte à révolutionner la chose, on pourrait :
- soit, au pire car un peu lourd, ajouter un paramètre à $opt : 'objet' => true/false
- soit, carrément mieux car évolutif, réécrire la fonction comme ceci : autoriser($tableau). Assurer la compatibilité avec SPIP 2.1 serait facile en testant le type de $tableau (string ou array) et en lançant le traitement de SPIP 2.1 : autoriser_old($faire, $quoi='', $id=0, etc.). Bref, dans ce cas, on pourrait implémenter facilement le même paramètre 'objet' => true/false, et éviter (enfin !) le passage par objet_type() ainsi que la suppression des underscores.

#12 Updated by marcimat 🌈 over 7 years ago

Bon... tiens, je vois dans mon commentaire que là aussi http://zone.spip.org/trac/spip-zone/browser/_plugins_/champs_extras/core/trunk/inc/cextras_autoriser.php#L94 (en 50696) je vais avoir un soucis par rapport à ce que je demandais de faire en SPIP 2.

j'appelle : autoriser('voirextra','auteur_prenom', $id_auteur);

«prenom» étant le nom du champ extra qui a été créé sur la table auteur.
S'il a un S ou non, ça va être pareil.

#13 Updated by cedric - over 7 years ago

il me semble que ce modele d'appel pour les champs extra est en lui-même peu robuste puisque tu ne peux pas faire la dire si ('voirextra','articles_syndic_oui') est le champ 'oui' sur la table 'articles_syndic' ou le champ 'syndic_oui' sur la table 'articles'.

Tu as donc toutes les chances d'une collision un jour ou l'autre.

Je préconiserai plutot le modèle d'appel

autoriser('voirextra_prenom','auteur',$id_auteur)

qui lève toute ambiguité possible et tout risque de collision.
Et cela montre bien que mettre n'importe quoi dans le $type de autoriser augmente les risques d'indetermination.

#14 Updated by marcimat 🌈 over 7 years ago

oui, je suis d'accord avec toi, d'ailleurs c'est le commentaire de la fonction qui n'était plus à jour car je sépare l'objet de la colonne par le caractère 0 maintenant article0prenom. Mais il y a certainement mieux oui.

ça donnerait quoi au bout du compte ?
autoriser('voirextra_prenom','auteur',$id_auteur)

-> autoriser_auteur_voirextra_prenom il semblerait. Ce qui me parait bien d'ailleurs (pour champs extras) :)

#15 Updated by marcimat 🌈 over 7 years ago

Bon, pour Champs Extras, c'est réglé par http://zone.spip.org/trac/spip-zone/changeset/56666

#16 Updated by cedric - over 7 years ago

r18896 implémente le '_' en premier caractère du $quoi pour distinguer que ce ne sont pas des $type. Les _ sont ensuite tous retirés pour construire le nom de la fonction.

#17 Updated by Patrice Vanneufville over 7 years ago

Bonjour,

Ce commit est incomplet, n'est-ce pas ?
Avec cette décision, il va falloir modifier bien des autorisations de SPIP, j'imagine que c'est prévu...

A la louche :

autoriser('configurer', 'plugins')
autoriser('detruire','statistiques')
autoriser('exporter','bookmarks')
autoriser('importer','bookmarks')
#AUTORISER{configurer,revisions}
#AUTORISER{configurer,plugins}
#AUTORISER{configurer,interactions}
etc., etc.

Et peut-être :

autoriser('exporter','sites')
#AUTORISER{creer,groupemots}

Pat

#18 Updated by cedric - over 7 years ago

  • Status changed from Nouveau to Fermé

Appliqué par commit r18940.

Also available in: Atom PDF