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
