You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2012/02/27 20:47:04 UTC
Re: svn commit: r1294291 - in /ofbiz/trunk/applications/order: script/org/ofbiz/order/order/OrderSimpleMethods.xml
servicedef/services.xml
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
On 2/27/2012 7:26 PM, jacopoc@apache.org wrote:
> Author: jacopoc
> Date: Mon Feb 27 19:26:23 2012
> New Revision: 1294291
>
> URL: http://svn.apache.org/viewvc?rev=1294291&view=rev
> Log:
> Fixed permission service for creation/update of order adjustment that was completely wrong (probably because it has been copied from another service from the Party component and then not properly completed): the end result was that the service always granted the rights to perform the task.
> As a side note, the service is still using incorrectly the _ROLE permission but this is a broader issue (a wrong pattern used everywhere in OFBiz) and for this I would like to fix it everywhere after discussion with the community.
>
> Modified:
> ofbiz/trunk/applications/order/script/org/ofbiz/order/order/OrderSimpleMethods.xml
> ofbiz/trunk/applications/order/servicedef/services.xml
>
> Modified: ofbiz/trunk/applications/order/script/org/ofbiz/order/order/OrderSimpleMethods.xml
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/script/org/ofbiz/order/order/OrderSimpleMethods.xml?rev=1294291&r1=1294290&r2=1294291&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/script/org/ofbiz/order/order/OrderSimpleMethods.xml (original)
> +++ ofbiz/trunk/applications/order/script/org/ofbiz/order/order/OrderSimpleMethods.xml Mon Feb 27 19:26:23 2012
> @@ -21,40 +21,28 @@ under the License.
> <simple-methods xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/simple-methods.xsd">
>
> -<!-- Returns hasPermission=true if userLogin partyId equals partyId parameter
> - Only the order owner should be able to cancel an item from Ecommerce
> - -->
> -<simple-method method-name="orderAdjustmentPermissionCheck" short-description="Party contact mech permission logic">
> -<if-empty field="parameters.partyId">
> -<set field="parameters.partyId" from-field="userLogin.partyId"/>
> -</if-empty>
> -<if-compare-field to-field="userLogin.partyId" field="parameters.partyId" operator="equals">
> -<set field="hasPermission" type="Boolean" value="true"/>
> -<field-to-result field="hasPermission"/>
> +<simple-method method-name="orderAdjustmentPermissionCheck" short-description="Permission service for the creation and editing of order adjustments">
> +<set field="primaryPermission" value="ORDERMGR"/>
> +<set field="altPermission" value="ORDERMGR_ROLE"/>
> +<set field="mainAction" from-field="parameters.mainAction"/>
> +<call-simple-method method-name="genericBasePermissionCheck" xml-resource="component://common/script/org/ofbiz/common/permission/CommonPermissionServices.xml"/>
> +<if-compare field="hasPermission" operator="not-equals" value="true">
> +<set field="resourceDescription" from-field="parameters.resourceDescription"/>
> +<if-empty field="resourceDescription">
> +<property-to-field resource="CommonUiLabels" property="CommonPermissionThisOperation" field="resourceDescription"/>
> +</if-empty>
> +<if-compare field="mainAction" value="CREATE" operator="equals">
> +<property-to-field resource="OrderErrorUiLabels" property="OrderSecurityErrorToRunCreateOrderAdjustement" field="failMessage"/>
> +</if-compare>
> +<if-compare field="mainAction" value="UPDATE" operator="equals">
> +<property-to-field resource="OrderErrorUiLabels" property="OrderSecurityErrorToRunAutoCreateOrderAdjustments" field="failMessage"/>
> +</if-compare>
> +<set field="hasPermission" type="Boolean" value="false"/>
> +<field-to-result field="failMessage"/>
> <else>
> -<set field="primaryPermission" value="ORDERMGR"/>
> -<set field="altPermission" value="ORDERMGR_ROLE"/>
> -<set field="mainAction" from-field="parameters.mainAction"/>
> -<call-simple-method method-name="genericBasePermissionCheck" xml-resource="component://common/script/org/ofbiz/common/permission/CommonPermissionServices.xml"/>
> -<if-compare field="hasPermission" operator="not-equals" value="true">
> -<set field="resourceDescription" from-field="parameters.resourceDescription"/>
> -<if-empty field="resourceDescription">
> -<property-to-field resource="CommonUiLabels" property="CommonPermissionThisOperation" field="resourceDescription"/>
> -</if-empty>
> -<if-compare field="mainAction" value="CREATE" operator="equals">
> -<property-to-field resource="OrderErrorUiLabels" property="OrderSecurityErrorToRunCreateOrderAdjustement" field="failMessage"/>
> -</if-compare>
> -<if-compare field="mainAction" value="UPDATE" operator="equals">
> -<property-to-field resource="OrderErrorUiLabels" property="OrderSecurityErrorToRunAutoCreateOrderAdjustments" field="failMessage"/>
> -</if-compare>
> -<set field="hasPermission" type="Boolean" value="false"/>
> -<field-to-result field="failMessage"/>
> -<else>
> -<field-to-result field="hasPermission"/>
> -</else>
> -</if-compare>
> +<field-to-result field="hasPermission"/>
> </else>
> -</if-compare-field>
> +</if-compare>
> </simple-method>
>
> <simple-method method-name="createOrderAdjustment" short-description="Create an OrderAdjustment">
>
> Modified: ofbiz/trunk/applications/order/servicedef/services.xml
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/servicedef/services.xml?rev=1294291&r1=1294290&r2=1294291&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/servicedef/services.xml (original)
> +++ ofbiz/trunk/applications/order/servicedef/services.xml Mon Feb 27 19:26:23 2012
> @@ -184,12 +184,8 @@ under the License.
>
> <service name="orderAdjustmentPermissionCheck" engine="simple"
> location="component://order/script/org/ofbiz/order/order/OrderSimpleMethods.xml" invoke="orderAdjustmentPermissionCheck">
> -<description>
> - Performs a party contact mech security check. The userLogin partyId must equal the partyId parameter.
> - Only the order owner should be able to cancel an item from Ecommerce.
> -</description>
> +<description>Permission service for the creation and editing of order adjustments</description>
> <implements service="permissionInterface"/>
> -<attribute name="partyId" type="String" mode="IN" optional="true"/>
> </service>
>
> <service name="createOrderAdjustment" default-entity-name="OrderAdjustment" engine="simple"
>
>
Re: Fixing bad patterns widely used to check role based permission
Posted by Adrian Crum <ad...@sandglass-software.com>.
On 2/28/2012 11:13 AM, Jacopo Cappellato wrote:
> On Feb 28, 2012, at 11:49 AM, Adrian Crum wrote:
>
>> 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.
> Yes, that is an example of a party role that is important to determine if the user is granted to perform an operation (together with the ROLE permission).
> But I was confused about the meaning of "user role" as there is not a direct relationship with OFBiz artifacts: the ingredients of the security framework in OFBiz are:
> * SecurityGroups: this is probably the closest concept to a "user role"
> * SecurityPermissions
> * *Role entities, i.e. party roles (FacilityRole, OrderRole etc...) but also other structures (like PartyRelationship etc...) can play a... "role" in this :-)
The lack of a direct relationship is the reason why I came up with my
own design. A user logs into (or is assigned to) an organization context
(an internal organization Party), and data access is controlled by
relating data to that party. That approach permits a user's role to
remain separate from party roles, plus it allows the user to log into
different organization contexts.
I suggested that approach years ago, but it never caught on.
Anyway, I understand the pattern you described and I agree the
differences between ROLE and ENTITY permissions need to be handled in a
consistent way.
-Adrian
Re: Fixing bad patterns widely used to check role based permission
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On Feb 28, 2012, at 11:49 AM, Adrian Crum wrote:
> 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.
Yes, that is an example of a party role that is important to determine if the user is granted to perform an operation (together with the ROLE permission).
But I was confused about the meaning of "user role" as there is not a direct relationship with OFBiz artifacts: the ingredients of the security framework in OFBiz are:
* SecurityGroups: this is probably the closest concept to a "user role"
* SecurityPermissions
* *Role entities, i.e. party roles (FacilityRole, OrderRole etc...) but also other structures (like PartyRelationship etc...) can play a... "role" in this :-)
Jacopo
Re: Fixing bad patterns widely used to check role based permission
Posted by Adrian Crum <ad...@sandglass-software.com>.
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
>>>
>>>
>>>
>>>
Re: Fixing bad patterns widely used to check role based permission
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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?
>
> 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.
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
>>
>>
>>
>>
>
Re: Fixing bad patterns widely used to check role based permission
Posted by Adrian Crum <ad...@sandglass-software.com>.
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
>>
>>
>>
>>
>
Re: Fixing bad patterns widely used to check role based permission
Posted by Adrian Crum <ad...@sandglass-software.com>.
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
>
>
>
>
Re: Fixing bad patterns widely used to check role based permission
Posted by Sascha Rodekamp <sa...@googlemail.com>.
Hi,
thanks for the detailed mail Jacopo. I agree with you. Maybe the API
is not immediately obvious. That might be the reason why there is an
enormous use of wrong/ redundant security checks.
IMHO it would be better to create a more restrictive and clear
security API (basePermissionCheck, rolePermissionCheck sounds good). I
like your suggestion and will think of any additional ideas.
Best Regards,
Sascha
2012/2/28 Jacopo Cappellato <ja...@hotwaxmedia.com>:
> 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
>
>
>
>
--
Sascha Rodekamp
Visit the new german OFBiz Blog: http://www.ofbiz.biz
Lynx-Consulting GmbH
Johanniskirchplatz 6
D-33615 Bielefeld
http://www.lynx.de
Fixing bad patterns widely used to check role based permission
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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