Re: Fixing bad patterns widely used to check role based permission

[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