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
