If you associate 2 or more groups to an ir.rule, rules are not correctly applied

[Lorenzo Battistini]

Steps:
1- create new db with only 'base' module
2- create 2 groups: 'group1' and 'group2'
3- create 2 rules on res.partner:
    - 'rule1' with domain: [('name','=','rule1')] and groups: 'group1'
    - 'rule2' with domain: [('ref','=','rule2')] and groups: 'group1' and 'group2'
4- create user 'test' and associate to 'group1'
5- create 2 partners:
    - with name: 'rule1' and ref: 'rule2'
    - with name: 'test' and ref: 'rule2'
6- login with user 'test'
7- you'll see both of partners

This is wrong because since the user 'test' belongs to 'group1' and this group contains 2 rules, these rules must be combined with AND operator. So, user 'test' should see first partner only.
This happens because second rule and both 2 rules are combined with OR:
((rule1 AND rule2) OR rule2)
I suppose the problem to be connected with line 117 of ir_rule.py: http://bazaar.launchpad.net/~openerp/openobject-server/6.0/view/3404/bin/addons/base/ir/ir_rule.py#L115
Instead of adding every group of the rule, you should check whether the user belongs to the group that will be added

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Lorenzo,

The documentation explicitely states "If there are multiple rules on same object, then all of them are joined using OR operator". If I understand correctly, that is precisely what is happening in your case.

See http://doc.openerp.com/v6.0/book/8/8_20_Config/8_20_Config_accessRights.html

If I understand correctly, you can add the restriction of rule2 to rule1 and remove group1 from rule2 to get the desired results.

Cheers,
Stefan.

[Lorenzo Battistini (elbati)]

Hello Stefan,

actually, I think the statement "If there are multiple rules on same object, then all of them are joined using OR operator" to be misleading.
If it relates to global rules, they should be combined with AND (and so it is)

This is better explained here: http://bazaar.launchpad.net/~openerp/openobject-server/6.0/view/3404/bin/addons/base/ir/ir.xml#L1667

Note that we are talking about group-specific rules, so they (rules associated to the same group) should be combined with AND operator and with OR if user belonged to several groups (in this case, 'test' belongs to 'group1' only)

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Lorenzo,

This is a matter of semantics. If a rule is regarded as a method to *restrict* access rights, then you have a good point. If on the other hand, a rule is regarded as *granting* access right, then the current implementation is correct: in case of two different rules granting access to two sets of records to the same group, members of this group should indeed have access to the union of the two sets.

The documentation is in fact unclear on this semantic aspect, but with respect to the current implementation remark no. 2 on the form itself is simply wrong. It should say that group-specific rules are combined together with a logical OR operator instead.

For now, I would prefer for the documentation to be adapted to the code and not the other way around. But you can also ask the usability experts for their opinion on the subject: https://launchpad.net/~openerp-expert-ergonomy

Cheers,
Stefan.

[Lorenzo Battistini (elbati)]

For me, a rule has to be regarded in this way:
a test that must be passed in order to grant read|write|create|unlink access(es).
I can't see other ways to read rules...

> in case of two different rules granting access to two sets of records
> to the same group, members of this group should indeed have access
> to the union of the two sets

Actually, to the intersection of the two sets. OpenERP behaves so and it is correct for me.

> The documentation is in fact unclear on this semantic aspect, but
> with respect to the current implementation remark no. 2 on the
> form itself is simply wrong. It should say that group-specific rules
> are combined together with a logical OR operator instead.

As above, the remark is correct since "group-specific rules are combined together with a logical AND operator". Try it yourself

> For now, I would prefer for the documentation to be adapted to
> the code and not the other way around

Sure, the documentation should be corrected.
But the problem I raised is different, more particular. Ain't talking about the case of 2 rules associated to 1 group. I'm talking about the case of 2 rules associated to 1 group AND one of these rules associated to another group. The rule with 2 groups is INCORRECTLY combined with the others with an OR operator

[Stefan Rijnhart (Opener) (stefan-opener)]

The semantics are important when two rules interact. If a rule grants access, then the two different rules grant the user access to the two partners. If a rule restricts access, then the two restrictions joinly grant the user access to the second partner only.

Based on your results I had guessed the former. Looking at the code however makes it clear to me now that the record rules apply the latter (as supported by remark 2 on the form) and I can only subscribe to your analysis that the resulting domain contains a clause that should not be there because the keys in 'group_rule' are not filtered against the user's actual groups.

Thanks,
Stefan.

[Lorenzo Battistini (elbati)]

> The semantics are important when two rules interact. If a rule
> grants access, then the two different rules grant the user
> access to the two partners. If a rule restricts access, then the
> two restrictions joinly grant the user access to the second
> partner only.

Sure.
My previous statement about the meaning of rules was related to OpenERP rules only.

Regards

[Amit Parik (amit-parik)]

Hello Lorenzo,

I have checked your issue and I have totally agree with Stefan 's comment #1.

The operator working on record rule like this.

1) If the rule is global means there is no group added in ir.rules then rules will be combined with AND operator So the domain working with "AND" operator.

2) If the rule is not global means there is added group in ir.rules then rules will be combined with OR operator So the domain working with "OR" operator.

Please see the line number 129-134 on http://bazaar.launchpad.net/~openerp/openobject-server/6.0/view/3404/bin/addons/base/ir/ir_rule.py.

From your scenario you have added a groups in your rules So the rules are combined with the "OR" operator not with the "AND" operator.

So now all are working expected and this is not a bug.

That's why I am closing this bug.

Hope this will help for you.

Thanks for the understanding!

[Lorenzo Battistini (elbati)]

> From your scenario you have added a groups in your rules So
> the rules are combined with the "OR" operator not with the
> "AND" operator.

Incomplete.
In my scenario, 'rule2' has 2 groups: 'group1' and 'group2'.
But 'group1' has 2 rules: 'rule1' and 'rule2'. My user ('test') belongs to 'group1' only.

So, since "Group-specific rules are combined together with a logical AND operator", for user 'test', 'rule1' and 'rule2' should be combined with AND.
OR operator should be used if 'test' belonged to both 'group1' and 'group2'.

[Stefan Rijnhart (Opener) (stefan-opener)]

Amit,

Terribly sorry for the confusion, but if you only read my first comment then our whole discussion was for nothing. This is definitely a bug. Notwithstanding the various snippets of documentation and their interpretation, the behaviour of the system for user1 changes if you remove group2 from the rule, even though user1 is not in that group at all. That can't be right, can it?

The problem is clear: the keys in 'group_rule' are not filtered against the user's actual groups, so under certain circumstances rules of groups are applied to users who are not in those groups.

Regards,
Stefan.

[Amit Parik (amit-parik)]

Hello,

Thanks for the reporting!

[Stephane Wirtel (OpenERP) (stephane-openerp)]

There is an OPW request @openerp for this bug report.

Thank you

[Anup(SerpentCS) (anup-serpent)]

Hello Lorenzo,

   It has been fixed by revision 3425 <email address hidden> in the branch lp:~openerp-dev/openobject-server/6.0-opw-5692-ach.

   It will be merged soon.

Thanks.
.

[Stefan Rijnhart (Opener) (stefan-opener)]

Danger Anup!

Even though user1 now does see the first partner only, your solution is not correct. With the operator change, you have now completely turned the semantics of the rules upside down when multiple rules apply to different groups of the same user.

Ask yourself, do you use groups in OpenERP to grant permissions or to take them away? Before your changes, a user with more group memberships would have broader permissions. In your version, adding the user to more groups would further limit the user's permissions.

An example to illustrate the problem would be:

2- create 2 groups: 'group1' and 'group2'
3- create 2 rules on res.partner:
    - 'rule1' with domain: [('name','=','rule1')] and groups: 'group1'
    - 'rule2' with domain: [('ref','=','rule2')] and groups: 'group2'
4- create user 'test' and associate to 'group1' and 'group2'
5- create 2 partners:
    - with name: 'rule1' and ref: 'rule2'
    - with name: 'test' and ref: 'rule2'
6- login with user 'test'
7- you'll only see partner 'rule1'

You'll want to see both of partners instead.

The combined group rule in this example comes out as

     ['&', ('name', '=', 'rule1'), ('ref', '=', 'rule2')]

It needs to be

    ['|', ('name', '=', 'rule1'), ('ref', '=', 'rule2')]

Please undo, and start checking in line 118 whether the user is actually in that group before adding the group rule.

Cheers,
Stefan.

[Olivier Dony (Odoo) (odo-openerp)]

(edited copy of my comment on the merge proposal, for the record)
I agree with Stefan, the patch from comment #12 is not correct. The issue is not about changing the semantics of the combination of rules, it is only about being sure a user does not get rules applied from a group he does not actually belong to.

As Lorenzo explained in the bug report, at line 117 there is an iteration on the rule's groups, but obviously this could include groups the user does not belong to! So we just need to filter out the groups that are irrelevant to the user.

This is better explained with code, so here's an unverified, dumb patch to illustrate the desired result:

=== modified file 'bin/addons/base/ir/ir_rule.py'
--- bin/addons/base/ir/ir_rule.py 2011-03-02 11:08:16 +0000
+++ bin/addons/base/ir/ir_rule.py 2011-05-18 12:25:41 +0000
@@ -115,7 +115,9 @@
         if ids:
             for rule in self.browse(cr, uid, ids):
                 for group in rule.groups:
- group_rule.setdefault(group.id, []).append(rule.id)
+ # filter out irrelevant groups!
+ if uid in [u.id for u in group.users]:
+ group_rule.setdefault(group.id, []).append(rule.id)
                 if not rule.groups:
                   global_rules.append(rule.id)
             global_domain = self.domain_create(cr, uid, global_rules)

[Olivier Dony (Odoo) (odo-openerp)]

Sorry, LP screwed up the patch indentation in the copy/paste of previous comment, see the comments on https://code.launchpad.net/~openerp-dev/openobject-server/6.0-opw-5692-ach/+merge/61101 for a correctly indented one.

[Anup(SerpentCS) (anup-serpent)]

Hello Everyone,

   I have fixed the issue in the same branch.

Thanks.

[Anup(SerpentCS) (anup-serpent)]

Hello Lorenzo,

   It has been fixed by revision 3457 <email address hidden> in the stable v6.

Thanks.