I did not look into all the details but this proposition sounds good to me.
I always wondered about OFBTOOLS and was not even aware about NONE.
I don't mind removing NONE (since it was undocumented) but it would be good to
have now a documentation about base-permission empty allows all accesses.
Of course having a ofbiz-component.xsd better documented would be a plus, but I
will not ask for the moon :)
Jacques
Ashish Vijaywargiya wrote:
> Hello Adrian,
>
> Your proposal looks good to me and yes it will make things much simpler.
>
> +1
>
>
> On Tuesday 15 October 2013 04:46 AM, Adrian Crum wrote:
>> Currently, the web application authorization is a jumbled up mess. I
>> would like to make things much simpler. The areas of improvement are
>> described below...
>>
>> 1. Configuring web application authorization is confusing:
>>
>> <webapp name="party"
>> title="Party"
>> ...
>> base-permission="OFBTOOLS,PARTYMGR"
>> mount-point="/partymgr"/>
>>
>> In order for a user to access the "party" web application, they must
>> have two permissions: one starting with "OFBTOOLS_" and ending with
>> "_VIEW", and another starting with "PARTYMGR_" and ending with
>> "_VIEW". The boolean AND of the base-permission list was a constant
>> source of frustration for new users - we were getting regular mail
>> about that - but adding more information to the schema seems to have
>> solved that confusion.
>>
>> The undocumented magic value "NONE" means the web application does not
>> require authorization.
>>
>> Even worse is how the implementation interprets the configured
>> permission list. If I create the permission "PARTY_MGR_VIEW" and
>> change the web application configuration to:
>>
>> <webapp name="party"
>> title="Party"
>> ...
>> base-permission="OFBTOOLS,PARTY_MGR"
>> mount-point="/partymgr"/>
>>
>> it doesn't work - because the implementation strips out the middle
>> bits of the permission string and checks for the permission
>> "PARTY_VIEW". This "feature" is confusing.
>>
>> I believe the original intent was to give a user implicit permission
>> to use a web application if they already had a similar permission. I
>> don't think that is a good design, and the need to add the "OFBTOOLS"
>> permission to every web application configuration proves that. It
>> would be better to EXPLICITLY grant a user permission to use (or
>> access) a web application:
>>
>>
>> <webapp name="party"
>> title="Party"
>> ...
>> access-permission="PARTY_MGR_VIEW"
>> mount-point="/partymgr"/>
>>
>> This will prevent the unintentional side effect of giving users access
>> to the web application when they have similar permissions that are
>> needed for invoking services, etc.
>>
>> If the access-permission attribute is empty or missing, then no
>> authorization is required to use the web application.
>>
>> 2. The web application authorization implementation has NO
>> encapsulation. The implementation is scattered everywhere. In
>> LoginWorker.java:
>>
>> ComponentConfig.WebappInfo info =
>> ComponentConfig.getWebAppInfo(serverId, contextPath);
>> if (info != null) {
>> for (String permission: info.getBasePermission()) {
>> if (!"NONE".equals(permission) &&
>> !security.hasEntityPermission(permission, "_VIEW", userLogin)) {
>> return false;
>> }
>> }
>> } else {
>> Debug.logInfo("No webapp configuration found for : " + serverId +
>> " / " + contextPath, module);
>> }
>>
>> The model of the webapp element is in the base component, and the code
>> (behavior) for that model is in the webapp component. That makes sense
>> from a separation-of-concerns perspective - the base component doesn't
>> "know" about web applications, so it merely generates a model of the
>> element, and the webapp component "knows" what to do with the model.
>>
>> But that ideal separation breaks down when the base component
>> manipulates the model contents instead of just creating the model.
>> That is why the webapp base-permission list doesn't behave as
>> expected. In ComponentConfig.java (in the base component):
>>
>> String basePermStr = element.getAttribute("base-permission");
>> if (UtilValidate.isNotEmpty(basePermStr)) {
>> this.basePermission = basePermStr.split(",");
>> } else {
>> this.basePermission = new String[] { "NONE" };
>> }
>> for (int i = 0; i < this.basePermission.length; i++) {
>> this.basePermission[i] = this.basePermission[i].trim();
>> if (this.basePermission[i].indexOf('_') != -1) {
>> this.basePermission[i] = this.basePermission[i].substring(0,
>> this.basePermission[i].indexOf('_'));
>> }
>> }
>>
>> Clearly, that code does not belong there. Instead, it belongs in
>> LoginWorker.java (in the webapp component).
>>
>> The code in LoginWorker.java is duplicated in every visual theme. In
>> Flat Grey's appbar.ftl:
>>
>> <#assign displayApps =
>> Static["org.ofbiz.base.component.ComponentConfig"].getAppBarWebInfos(ofbizServerName,
>> "main")>
>> <#assign displaySecondaryApps =
>> Static["org.ofbiz.base.component.ComponentConfig"].getAppBarWebInfos(ofbizServerName,
>> "secondary")>
>> ...
>> <#assign permissions = display.getBasePermission()>
>> <#list permissions as perm>
>> <#if (perm != "NONE" && !security.hasEntityPermission(perm, "_VIEW",
>> session))>
>> <#-- User must have ALL permissions in the base-permission list -->
>> <#assign permission = false>
>> </#if>
>> </#list>
>>
>> Why are we doing that??!!
>>
>> I propose we encapsulate all of the user web application authorization
>> code in LoginWorker.java. This will provide a central location to
>> determine if a user is authorized to access a web application, and it
>> will help make web application authorization behavior more consistent.
>>
>> So, the code fragment in Flat Grey's appbar.ftl becomes:
>>
>> <#assign displayApps =
>> Static["org.ofbiz.webapp.control.LoginWorker"].getAppBarWebInfos(userLogin,
>> ofbizServerName, "main")>
>> <#assign displaySecondaryApps =
>> Static["org.ofbiz.webapp.control.LoginWorker"].getAppBarWebInfos(userLogin,
>> ofbizServerName, "secondary")>
>>
>> The lists contain only the web applications the user is authorized to
>> access.
>>
>> What do you think?
