Oops, I meant to say:
For example, a user in a purchasing role should be able to create and
update purchase orders - so the CRUD permissions and the entities they
apply to are implied.
-Adrian
On 2/28/2012 9:37 AM, Adrian Crum wrote:
Another aspect that should be included in this discussion is user
roles versus party roles. They are not the same thing, but there are
permission checks that treat them as being the same thing.
I never fully understood the original intent of the ROLE versus ENTITY
permissions, so I skip them and use a different design pattern.
The "correct" ROLE permission design you mentioned sounds little
different from the basic CRUD permissions, and from my perspective,
they are confusing. What does ROLE_UPDATE mean? To me, a user in a
particular role should have implied permissions, so anything after
ROLE (_CREATE, _UPDATE, etc) are meaningless. For example, a user in
an accounts payable role should be able to create and update purchase
orders - so the CRUD permissions and the entities they apply to are
implied. Maybe that is why you see the permission check duplication:
If a user is in a certain role OR if they have a specific CRUD
permission, then allow access.
-Adrian
On 2/28/2012 8:49 AM, Jacopo Cappellato wrote:
On Feb 27, 2012, at 8:47 PM, Adrian Crum wrote:
I would welcome a discussion of wrong (or bad) patterns. Lately I
spend about half my development time fixing things that are done wrong.
-Adrian
Thank you Adrian, here is what I would like to address: I see a lot
of code that checks ROLE permissions in a way that make them useless
or redundant.
Before I describe the issue that I see I would like to summarize what
I know about ROLE based permissions, it would be useful to check if
we are all on the same page with these as well.
There are two main families of permissions (SecurityPermission) in
OFBiz and they are categorized based on a naming convention:
a) standard permissions are in the format<NAME>_<ACTION>; for
example: ACCOUNTING_VIEW, CATALOG_UPDATE, ...; a user with one these
permission can perform the ACTION on all data related to<NAME>
domain; for example a user with CATALOG_UPDATE can update any catalog
in the system
b) role permissions are in the format<NAME>_ROLE_<ACTION>; for
example: ACCOUNTING_ROLE_VIEW, CATALOG_ROLE_UPDATE, ...; a user with
one these permission can perform the ACTION on the data related
to<NAME> domain but only if the party (associated to the user) is
associated to (has a role) the data; the nature of the "association"
depends on the domain/context; for example a user with
CATALOG_ROLE_UPDATE can update only the catalogs in the system for
which the user is in the role of LTD_ADMIN (this information is
stored in ProductCategoryRole); however the "association" could be
more complex than a role record; for example we could have a special
ROLE permission that grants some action on some employee data to
users that are managers in the company division where the employees
work: the "association" here would be a combination of
PartyRole/PartyRelationship (several records, possibly on a
hierarchical tree); but the general rule is: if the user has a ROLE
permission then additional checks on data are needed before granting
rights to perform the operation on data and they depend on the
context and business rules
The bad pattern I see in the system is exemplified by the following
code snippet from setPaymentStatus service:
<check-permission permission="ACCOUNTING" action="_UPDATE">
<alt-permission permission="ACCOUNTING_ROLE" action="_UPDATE"/>
<fail-property resource="AccountingUiLabels"
property="AccountingPermissionError"/>
</check-permission>
A check like this returns true if the user has ACCOUNTING_UPDATE or
ACCOUNTING_ROLE_UPDATE; in this way the two permissions are equivalent.
For the same reason, the following permission services are not very
useful:
<simple-method method-name="basePermissionCheck"
short-description="Accounting component base permission logic">
<set field="primaryPermission" value="ACCOUNTING"/>
<call-simple-method method-name="genericBasePermissionCheck"
xml-resource="component://common/script/org/ofbiz/common/permission/CommonPermissionServices.xml"/>
</simple-method>
<simple-method method-name="basePlusRolePermissionCheck"
short-description="Accounting component base permission logic">
<set field="primaryPermission" value="ACCOUNTING"/>
<set field="altPermission" value="ACCOUNTING_ROLE"/>
<call-simple-method method-name="genericBasePermissionCheck"
xml-resource="component://common/script/org/ofbiz/common/permission/CommonPermissionServices.xml"/>
</simple-method>
in fact:
* basePermissionCheck returns true if the user has one of the base
ACCOUNTING CRUD+ADMIN permissions
* basePlusRolePermissionCheck returns true if user has one of the
base ACCOUNTING CRUD+ADMIN permissions OR if user has one of the base
ACCOUNTING_ROLE CRUD+ADMIN permissions
How should the two services be used properly? If we run only
basePlusRolePermissionCheck we will not know if we have to check if
the party/user is "associated" to the data because we do not know if
the user has an ACCOUNTING permission or only an ACCOUNTING_ROLE
permission.
In theory the two services should be used together in the following way:
1) run basePermissionCheck and if it returns true then grant right to
perform the action;
2) otherwise, run basePlusRolePermissionCheck and if it returns true
then check "association" data (but this,even inside of the ACCOUNTING
domain, may depend on different data structures for different
processes); if the data is available then grant right to perform the
action
At this point it would be more useful to have the following base
services:
* basePermissionCheck: same as above
* rolePermissionCheck (instead of basePlusRolePermissionCheck):
returns true if user has one of the base ACCOUNTING_ROLE CRUD+ADMIN
permissions
As a side note, for the same reason the methods
security.hasRolePermission(...) are useless if you do not pass the
roles (and no code does currently) and were all used in the wrong way.
The end result is that we have a lot of code that treats standard
permissions and ROLE permission as equivalent; this happens mostly
for two reasons:
A) bad implementation; for example see service
orderAdjustmentPermissionCheck)
B) incomplete implementation (the code to check the "association" is
still missing); for example see service acctgAgreementPermissionCheck
In my opinion we should fix #A and #B by returning "false" if the
user has a ROLE permission only but the code doesn't check for
"association" data and we will add a placeholder TODO comment as a
reminder: if the code is not implemented then the ROLE permission
should not work rather than (as it happens now) working as the
standard permission
I apologize for the long email, I look forward at your feedback.
Jacopo
