Re: Proposal: Improved Web Application Authorization

[Adrian Crum <ad...@sandglass-software.com>]

Done in revision 1546891.

Adrian Crum
Sandglass Software
www.sandglass-software.com

On 10/14/2013 7:16 PM, 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?
>