developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#836 closed bug (fixed)

jAcl2: problem when using rights on resources

Reported by: Julien Owned by: laurentj
Priority: high Milestone: Jelix 1.1.3
Component: jelix:acl Version: trunk
Severity: major Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

There's a problem when using the method jAcl2DbManager::setRightsOnGroup() because it will erase all rights for that group, including rights on resources, before adding new rights.

This particularly occurs in the jAcl2 admin module, now part of the master_admin module.

The jAcl2 admin module only allows setting "global" rights (ie without resource assignment) and that's good, as rights on resource should be set from foreign modules (setting rights for a particular page in a cms for example).

I suggest:

  • that jAcl2DbManager::setRightsOnGroup() should not erase rights with resources
  • that jAcl2 admin module should only deal with subjects with a locale key, so that we could have "invisible" rights subjects that could be used only with resources (because some subjects may only have sense with a resource). We could also add a 'global' boolean field in jacl2_subject if you think it's clearer.

What do you think about this ?

Any other idea to solve that bug ?

Attachments (2)

ticket836-doesnt-remove-existing-rights.patch (2.0 KB) - added by laurentj 11 years ago.
fix a bug on setRightsOnGroup
ticket836-doesnt-remove-existing-rights-v2.patch (2.0 KB) - added by foxmask 11 years ago.
ajout de <eq property="id_aclres" value="" /> qui fait l'affaire !

Download all attachments as: .zip

Change History (33)

comment:1 follow-up: Changed 11 years ago by foxmask

Hi,

On my side i've clone setRightsOnGroup for my needs here and then use group + ressource in the dao jacl2rights i've overloaded

Couldnt we just apply something like that too and modify the jacl2rights dao like that ?

        <method name="deleteByGroup" type="delete">
           <parameter name="group" />
           <parameter name="resources"  default=""/>
           <conditions >
              <eq property="id_aclgrp" expr="$group" />
              <eq property="id_aclres" expr="$resources" />
           </conditions>
        </method>

regads.

comment:2 in reply to: ↑ 1 Changed 11 years ago by Julien

  • Status changed from new to assigned

Replying to foxmask:

Couldnt we just apply something like that too and modify the jacl2rights dao like that ?

>         <method name="deleteByGroup" type="delete">
>            <parameter name="group" />
>            <parameter name="resources"  default=""/>
>            <conditions >
>               <eq property="id_aclgrp" expr="$group" />
>               <eq property="id_aclres" expr="$resources" />
>            </conditions>
>         </method>

That's what I thought about first, but the problem is then: how do you delete ALL rights, with or without resource, for a group (when the group is deleted for example) ?

I would prefer something like:

<method name="deleteGlobalsByGroup" type="delete">
    <parameter name="group" />
    <conditions >
        <eq property="id_aclgrp" expr="$group" />
        <eq property="id_aclres" value="" />
    </conditions>
</method>

and jAcl2DbManager::setRightsOnGroup() will use this new DAO method

Then we should find a good way to remove rights with resources from the admin module too.

I will write a patch for that, so we can discuss on it.

comment:3 follow-up: Changed 11 years ago by laurentj

that jAcl2DbManager::setRightsOnGroup() should not erase rights with resources

So, in the admin, you want to remove a specific right for a user. It remove it but in fact, on some resources, the user have still the right. This is a thing that you don't probably want. If you remove a right, you remove... the right.. Or else the admin module should be improved so the administrator can says if he want to remove all rights, or only general right.

when using the method jAcl2DbManager::setRightsOnGroup() because it will erase all rights for that group, including rights on resources, before adding new rights.

In fact, the setRightsOnGroups should verify if the new rights are already set or not. If an old right is not in the list of new rights, it should be deleted (even thus which have set with a resource). Else, don't remove the right, don't setup.

I admit that I removed all rights because it is a simpler solution and I would like a quick prototype. we have to improve it of course.

comment:4 in reply to: ↑ 3 Changed 11 years ago by Julien

Replying to laurentj:

that jAcl2DbManager::setRightsOnGroup() should not erase rights with resources

So, in the admin, you want to remove a specific right for a user. It remove it but in fact, on some resources, the user have still the right. This is a thing that you don't probably want.

yup, that's what I wanted to say by "find a good way to remove rights with resources"

If you remove a right, you remove... the right.. Or else the admin module should be improved so the administrator can says if he want to remove all rights, or only general right.

That's why I suggested separating global and "per resource" right subjects.

Examples of global subjects:

  • cms.articles.admin
  • cms.articles.create
  • cms.articles.edit
  • media_manager.admin
  • media_manager.folder.create

Examples of "per resource" subjects:

  • cms.articles.edit.resource
  • cms.articles.folder.create_subfolder

"cms.articles.folder.create_subfolder" only makes sense when associated with a resource.

to check if the article X can be edited, we should query jAcl2 for "cms.articles.edit" or "cms.articles.edit.resource" on resource X.

Only global rights could be managed from the admin module that exist today. We should improve it to remove all "per resource" rights that match a "per resource" subject.

We could also imagine that a single subject can be used for both "global" and "per resource" rights, but we should then provide a way to specify that a subject is only available "per resource". That way, we don't break compatibility.

I'll try to write a patch that way.

comment:5 Changed 11 years ago by foxmask

I'm waiting for this patch as I met this issue recently ;) ( without understanding what happened immediatly )

how i use jacl2 :

  • i use global subject all other the admin of the forum
  • i use the subject with "resources" all other the "public" part, to filter the acces of each resource (like forum/member list / search engine and so on)

how i config the rights :

  • I added rights to admin/moderator/member groups to a forumA (so to one resource)
  • I needed a new subject so I added a new one to the system and decide to add this new right to the admins group

Then, all the config done on the forumA went away, and none of the forum were accessible anymore ;-)

comment:6 Changed 11 years ago by foxmask

Julien,

do you start something about this ticket ?

i will be more quiet if it was fix as i'm near to release my forum which use this feature.

Otherwise i will try to find a temporary workarround to restore the lost rigts.

In the meantime, I already did a function that put default rights to forum.

regards.

comment:7 Changed 11 years ago by Julien

yup, working on it, but there isn't a quick way to solve this... I think we need to separate "global rights" and "per resource rights" ; and only global rights would be managed by the jacl2_db_admin module.

But this would break compatibility I think...

In the meantime, and this would be another discussion, I looked at other projects, to see how they manage ACLs. I think Zend_ACL is really powerful (allow/deny rules, group inheritence, ...) -> maybe we could rely on this in the future (a driver for jacl ? jacl3 ?). The only problem is that the whole acl rules are stored in an object, which is serializable, but may be very huge whith tons of users and rules... http://framework.zend.com/manual/en/zend.acl.html

comment:8 Changed 11 years ago by foxmask

ok thanks for this post.

i will follow this ;)

Changed 11 years ago by laurentj

fix a bug on setRightsOnGroup

comment:9 Changed 11 years ago by laurentj

I added a first patch which fixes an issue I talked about : when setting rights on a group, now it doesn't remove existing rights about subjects which are still valid.

For other issues, we need still to work on them.

comment:10 Changed 11 years ago by foxmask

Laurent :

there is a little mistake in your patch i've just tested.

in jAcl2DbManager.class.php, line 77, we should have :

$rightsToNotRemove[] = $sbj;

instead of

$rightsToNotRemove = $sbj;

because after the if count() the dao method is waiting for an array and receive here a string and then we produce an SQL error

[exception 403] Erreur dans la requte (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1(DELETE FROM hf_jacl2_rights WHERE id_aclgrp =1 AND id_aclsbj NOT IN ()))

regards

comment:11 Changed 11 years ago by foxmask

one last test i made : after submitting the form of rights of groups, i still lost my others rights i made on all others forums.

i digg the issue and keep you inform

comment:12 follow-up: Changed 11 years ago by foxmask

A detail, if that can help, on page that manages the rights of group, some rights are not checked as shown here

And I use right with resources later as as shown here

All this to try to illustrate the issue when using jAcl2DbManager::setRightsOnGroup().

regards.

comment:13 in reply to: ↑ 12 Changed 11 years ago by Julien

Replying to foxmask:

A detail, if that can help, on page that manages the rights of group, some rights are not checked as shown here

And I use right with resources later as as shown here

All this to try to illustrate the issue when using jAcl2DbManager::setRightsOnGroup().

Hello,

that's why I think the only way to solve this issue is to have the jAcl2db_admin module manage "global" (!= per resource) rights.

I think my patch will be ready this afternoon, we then may discuss about it.

I still think that, for the future, something like Zend_Acl may be really interesting (see my previous comment)

comment:14 Changed 11 years ago by foxmask

je la fais en franais ;)

comportement du framework :

  • sur la page de gestion des droits des groupes, tout droit qui n'est pas activ (coch), aura ses resources lies, limines.

Solution (temporaire) :

  • pour eviter cet tat de fait il faut par consquent cocher tous les droits dont on se servira plus tard (dans le code avec une resource rattache).

ainsi ce qui suit fonctionnera avec cette solution :

{if 'hfnu.post.edit', 'forum'.$id_forum}
...
{/if}

par contre ceci introduit une faille potentielle (sur l'applicationi pas sur le framework) si demain un developpeur se mettait crire

{if 'hfnu.post.edit'}
...
{/if}

car videment l'dition d'un post est li une resource (un forum).

j'espere que j'ai t assez clair dans mes propos ;)

cdt.

comment:15 follow-up: Changed 11 years ago by foxmask

(desole pour les accents qui ont saute, j'ai un editeur moisi avec mon nav en mode caractere qui soumet pas les car en utf8)

comment:16 in reply to: ↑ 15 Changed 11 years ago by Julien

Replying to foxmask:

(desole pour les accents qui ont saute, j'ai un editeur moisi avec mon nav en mode caractere qui soumet pas les car en utf8)

you should have written your comment in english, which has not accents ;)

your temporary solution is not good I think, because as you said, this could be a security hole in the app.

What I'm trying to do in the patch I will propose :

  • distinguish global and per resource rights (a bool field in the db)
  • only global rights are managed through jacl2_db_admin
  • local/per resource rights should only be set/unset by calling the jAcl2 API from foreign modules, with a custom admin UI, ...
  • jacl2_db_admin will provide a way to remove all "per resource" rules for a given subject/group

Is that OK for you ?

comment:17 Changed 11 years ago by foxmask

yes for me ; but what about the big Manitou ? :D

comment:18 Changed 11 years ago by laurentj

Bon en fait, je n'ai vraiment pas une idée claire de votre problème. je pense qu'on va continuer en français parce que là ça devient complètement flou. Merci de refaire une explication CLAIRE en français du problème.

I still think that, for the future, something like Zend_Acl may be really interestin

ouai enfin, si il fait de l'héritage de groupe, faut faire gaffe. ça peut être très lourd au niveau ressource. Je ne sais pas comment ils s'y prennent au niveau implémentation, mais je sais par expérience que c'est très gourmand. J'ai essayé de faire en sorte que jAcl soit le plus performant possible.

Et puis je vois pas très bien la différence. Leur resource/role correspond à sujet/user.

distinguish global and per resource rights (a bool field in the db)

????

Tu n'es pas en train de remettre en cause toute l'archi de jAcl ? De telle modifications seront alors pour le trunk, pas pour 1.1.x.

only global rights are managed through jacl2_db_admin

C'est plus ou moins le but aujourd'hui non ?

local/per resource rights should only be set/unset by calling the jAcl2 API from foreign modules, with a custom admin UI, ...

c'est le principe actuel.

jacl2_db_admin will provide a way to remove all "per resource" rules for a given subject/group

oui ça ok (si j'ai bien compris ta phrase)

comment:19 Changed 11 years ago by foxmask

le comportement actuel de jacl2_db_admin observé de mon coté est :

  • quand je modifie un droit pour un groupe A , tous les droits attribués à tous les groupes pour une ressource donnée sont supprimés.

exemple concret :

  • dans mon forum, j'autorise des actions selon le forum (ma ressource) sur lequel je suis, telles : creer un sujet, supprimer un sujet, editer un sujet etc...

ces droits sont hfnu.posts.create, hfnu.posts.delete, hfnu.posts.edit.

  • ces droits sont explicitement liés à une ressource forum (voir screenshot).
  • quand je suis sur la page de gestion des "droits des groupes", ces memes droits sont "décochés" voir screenshot, car je ne souhaite pas que ceux ci soient utilisables "globalement" à l'application.
  • j'ajoute un droit acl.group.create au group Anonymes
  • quand j'enregistre la page (qui appelle jAcl2DbManager::setRightsOnGroup dans la fonction saverights du controleur groups du module jacl2db_admin ). ma configuration faite sur le premier screenshot est entierement éffacée.

les droits sur mes ressources sont supprimés.

comment:20 Changed 11 years ago by laurentj

Ok, j'ai maintenant pigé. Je résume :

Le problème de la page de gestion des "droits des groupes" (même avec le patch que j'ai fourni), est que pour les droits non cochés, il efface tout les droits existants sur les ressources.

La solution que je vois:

  • l'action qui traite les données soumises de formulaire, devrait modifier les droits uniquement pour les cases à cochées qui ont été modifiées, (qu'elles aient été cochées ou décochées).
  • pour les cases que l'on décoche, il devrait y avoir une confirmation pour détruire les ressources associées ou pas.

À réfléchir maintenant comment on matérialise cette confirmation. Après soumission du formulaire, on pourrait afficher un deuxième formulaire qui propose pour chacun des sujet/groupe dont on a décoché les cases, deux boutons radios :

  • enlever uniquement le droit global (j'aime pas trop cette expression)
  • enlever aussi les droits sur les ressources attachées à ce sujet

Bien sûr, si il n'y a pas des droits sur les ressources pour les sujet/groupe concernés, on n'affiche pas les boutons. Et d'ailleurs, si il n'y a aucun droits sur des ressources sur aucun des sujets/groupes concerné, on n'affiche pas ce deuxième formulaire.

Je pense que les modifications sont donc à faire principalement dans le module jacl2_db_admin, et pas dans l'api (sauf mon patch qu'il faudrait inclure avec la correction indiquée par foxmask).

comment:21 Changed 11 years ago by Julien

tu m'as pris de cours foxmask, ton explication est la même ;)

la grande question à se poser est selon moi la suivante : est-ce que le même sujet peut avoir ou non une ressource associée ?

Par exemple, est-ce que l'on ferait :

  • cms.article.edit avec une ressource facultative
  • cms.article.all.edit (sans ressource) et cms.article.this.edit (avec une ressource obligatoire)

La première option est celle en place actuellement, avec le problème soulevé par foxmask.

Je rajouterai que lors d'un check avec un ressource, si le même sujet existe sans resource, le résultat doit être "autorisé" : si cms.article.edit est activé sans ressource pour mon groupe, et que je demande

if(jAcl2::check('cms.article.edit',42))

je dois être autorisé même si je n'ai pas de droit spécifique sur la ressource 42. Il ne faudrait pas devoir écrire

if(jAcl2::check('cms.article.edit',42) || jAcl2::check('cms.article.edit'))

surtout avec les plugins de template.

Bref, un sujet activé "sans ressource" devrait valider le droit globalement (sauf erreur ce n'est pas le cas actuellement)

la deuxième option casse la compatibilité car il faut distinguer les sujets avec et sans ressource possible, donc trunk seulement.

Seuls les sujets sans ressource seraient gérés par jacl2_db_admin, ce qui règle tous les problèmes évoqués.

Les sujets avec ressources sont à gérer directement à travers l'API via les modules concernés.

Autre inconvénient : il faut ré-écrire des trucs du genre :

if(jAcl2::check('cms.article.this.edit',42) || jAcl2::check('cms.article.all.edit'))

comment:22 Changed 11 years ago by Julien

à la lecture du comment de Laurent, je pense que c'est tout à fait ça qu'il faut faire. Je suis d'accord avec tout ça.

comment:23 Changed 11 years ago by laurentj

La première option est celle en place actuellement, avec le problème soulevé par foxmask.

Le problème soulévé par foxmask est en fait un problème au niveau de l'interface de gestion des droits, pas de l'api ou autre.

Pour moi garder le principe d'un sujet avec une ressource facultative n'est pas un problème, en tout cas au niveau du concept ou des API.

un sujet activé "sans ressource" devrait valider le droit globalement (sauf erreur ce n'est pas le cas actuellement)

Je ne sais plus si c'est le cas ou pas actuellement. À verifier. Au pire, on peut améliorer l'API : ajouter un boolean facultatif en troisième argument, qui indique si on veut que ça check aussi le droit sans ressource. true = ça check, false = ça check uniquement pour la ressource indiquée (false étant la valeur par défaut si c'est le comportement actuel). Ainsi pas d'incompatibilité avec le code existant, et chacun peut choisir le "mode" qu'il veut.

Toutefois, sur cette évolution d'API, c'est un sujet différent de celui du présent ticket, donc créer un nouveau ticket pour ça.

comment:24 Changed 11 years ago by Julien

  • Status changed from assigned to new

Unfortunately, I don't have enough time to work on this right now, so I'm leaving it out. Feel free to take this ticket.

comment:25 Changed 11 years ago by Julien

  • Owner Julien deleted

comment:26 Changed 11 years ago by foxmask

bonsoir,

j'ai trouvé la solution, qui effectivement n'a à voir qu'avec l'interface de gestion des droits des groupes.

j'ai patché le patch ;-)

Changed 11 years ago by foxmask

ajout de <eq property="id_aclres" value="" /> qui fait l'affaire !

comment:27 Changed 11 years ago by foxmask

  • review set to review?

comment:28 Changed 11 years ago by laurentj

  • Owner set to laurentj
  • review review? deleted

@foxmask: pas suffisant. Il faut aussi faire des modifications sur l'interface.

je vais travailler sur ce problème

comment:29 Changed 11 years ago by laurentj

  • Status changed from new to assigned

comment:30 Changed 11 years ago by laurentj

  • Resolution set to fixed
  • Status changed from assigned to closed

I landed a new interface to manage global rights and rights on resources. Please check if it's ok for you.

r1428

comment:31 Changed 11 years ago by foxmask

je confirme pour l'avoir teste, qu'en enregistrant les droits de groupes ou d'utilisateur, aucun des droits avec ressources ne disparait.

Note: See TracTickets for help on using tickets.