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 <[email protected]>: > 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
