On 2/28/2012 10:33 AM, Jacopo Cappellato wrote:
Hi Adrian,
please see inline:
On Feb 28, 2012, at 10: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.
Could you please provide an example of a user role? Are you talking about "ROLE
permissions" or (as I suspect) about SecurityGroups?
An example is your description of the CATALOGADMIN_LTD security group.
The permission check is based on a party role, not a user role. Party
roles and user roles are not 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 names are actually confusing but if you review the code in the security
component and also some of the older code that is using them (see
createProduct) you will see that:
* ENTITY permissions (that I have named them "standard" permissions in my last post) are
*not* specifically mapped to "entities" and so they are not strictly CRUD permissions on
database tables;
* ROLE permissions are less powerful; if a user has<NAME>_<ACTION> then it has always the rights to run the<ACTION>
in the<NAME> context; on the other hand, if a user has<NAME>_ROLE_<ACTION> then it can run the<ACTION> in
the<NAME> context only if the party associated to the user has a "role" in that context; the main difference is that
ROLE permissions are not enough alone to grant rights to perform a task
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?
It means that the user is granted to UPDATE data but limited to the data
associated (in some way that depends on the process/data model) to the user's
party.
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.
In I am not misunderstanding completely what you are saying then I would
consider the following mappings:
* a SecurityGroup represents what you have named a "user role";
* all the SecurityPermission records associated to the SecurityGroup fulfill the
requirement: "a user in a particular role should have implied permissions"
* what you have described above doesn't require a ROLE permission (in the OFBiz original
terminology); a ROLE permission would only be needed if, continuing your example, the
"user in the accounts payable role" should only be limited to a specific
division in the system; in order to implement this you would setup:
a) a SecurityGroup with name "APADMIN" for users that can manage AP on all
divisions
b) a SecurityGroup with name "APADMIN_LTD" for users that can manage AP only
on divisions where they are assigned to
c) the SecurityPermission records associated to the APADMIN group will be all
the ENTITY permissions (standard permissions) needed to perform all the tasks
that the user will perform
d) the SecurityPermission records associated to the APADMIN_LTD group will be all the ROLE
permissions required; then the security services will check if the user is associated to a Party
that is an "employee" (or "accountant" etc...) of the given division before
granting rights to run the operation
You will find a decently good implementation of this pattern in the product
component where the CATALOGADMIN and CATALOGADMIN_LTD security groups are
created:
* CATALOGADMIN contains (ENTITY) permission to edit all catalogs in the system
* CATALOGADMIN_LTD contains (ROLE) permissions to edit only some limited
catalogs (the ones in which the party associated to the user has a
ProductCategoryRole)
I am not saying that the way the system was designed and implemented is
superior to what you are describing, and I am wide open to discuss a
refactoring; but before we start this process I would like to make sure we are
on the same page: if you will review the existing framework code you should see
that it is inline with what I am saying; then we will discuss the limitations
and how to fix them.
But if we read and interpret the existing security framework in different ways without
understanding each other and we try to "fix" the code in OFBiz accordingly to
our understanding then we will end up with a system that is not consistent.
I wasn't suggesting an alternative system. I was describing a use case.
Thanks,
Jacopo
-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
