This is a review regarding the effect of recent changes committed through r1645310 (patch provided through issue OFBIZ-5898) and the patch provided through OFBIZ-5986.
*References:* - OFBIZ-5898 Tenant should run with specified domain name. Front store should support mulit tenancy feature (see: https://issues.apache.org/jira/browse/OFBIZ-5898) - OFBIZ commit r1643510 (see http://svn.apache.org/r1643510) - OFBIZ-5986 Error on accessing new tenant (see https://issues.apache.org/jira/browse/OFBIZ-5986) *Background* OFBiz facilitates the single tenant and multiple tenants setups. These setups are: 1. Single tenancy setup, supporting the master having a single internal organisation or multiple internal organisations 2. Multi tenancy setup, supporting each tenant (including the master) having a single internal organisation or multiple Ad 1. This is enforced through following property in /framework/common/config/general.properties and utilises the datasets in databases ofbiz, ofbizolap and ofbiztenant (the master databases) in the RDBMS: mulititenant=N Multiple internal organisations is facilitated through the OFBiz Setup application/component. Ad 2. This is enforced through following property in /framework/common/config/general.properties: mulititenant=Y Multiple internal organisations per tenant is facilitated through the same application as mentioned in ad 1. The effect of 'mulitenant=Y' is that the tenantId field is shown on the login screen and 1. that the tenantId can be valid (checked against records in the Tenant entity in master database ofbiztenant of the RDBMS) 2. that the user can be authenticated against the permission settings in the tenant database (ofbiz-<tenantId>) and can utilise the functions in an applications to create, augment and delete records in all the tenant databases in the RDBMS (ofbiz-<tenantId> and ofbizolap-<tenantId). A user logging in at a tenant (the tenantId field has a value in the login screen) can never access the data of the master databases (ofbiz, ofbizolap and ofbiztenant). *Re: issue OFBIZ-5898 Tenant should run with specified domain name. Front store should support mulit tenancy feature* This subject of the issue suggest two things, namely: - a perceived bug: in OFBiz it isn't possible to run a tenant within a specific domain, and - an improvement: store front should support multi tenant feature Subsequently, following aspects are lined up in the description to address the subject of the issue: 1. Enable multi tenancy for a store front application 2. Tenant should run with specified domain name 3. User should be able to provide the domain name during tenant creation 4. Configuration details should be tenant specific *Re: perceptions and findings regarding the issue, the patch provided and subsequent commit r1645310* This issue is about having the functionality to be able to associate multiple domains (sites) to a tenant in stead of just one domain, as I understand it, as well as being about having per tenant configuration settings. I will address the issue, the patch (the second - revised - one submitted on Dec 6th at 07.47) and subsequent commit per aspect outlined in the description of the issue. *aspect 1. Enable multi tenancy for a store front application* As I see this, it is about having an application (the store front) work for both anonymous users and authenticated users given the existence of multiple tenants. The creator of the issue perceives that OFBiz does not have the capability to deliver functionalities in an application to support anonymous users. The applications in the feature portfolio intended for anonymous users are: 1. The E-commerce application - application in the Special Purpose stack 2. The MyPortal application - application in the Special Purpose stack. This is about the first, the E-commerce application, whereby the creator of issue believes (as I see it) that OFBiz doesn't have the capabilities to deliver functionalities to anonymous users given the multi tenancy setup and thus having tenant specific data being delivered to a anonymous user of a particular tenant. Because after all, how does OFBiz retrieve and deliver tenant specific data to an anonymous user when the tenant is not identified? Hence the introduction of aspect 2 of the issue which I will address later. First a little explanation of how the delegator works regarding single and multi tenancy setups. In a single tenancy setup (multitenant=N) the delegator is always 'default'. This 'default' comes from the setting in the web.xml configuration file in any and all applications in OFBiz. See following example: *<context-param>* *<param-name>entityDelegatorName</param-name>* *<param-value>default</param-value>* *<description>The Name of the Entity Delegator to use, defined in entityengine.xml</description>* *</context-param>* This ensures that OFBiz can work with the application without regard of tenantId and whether any user is logged in or not. It is more about OFBiz being able to work with the application than a user, namely being able to work with data from the master databases (ofbiz, ofbizolap and ofbiztenant). In a multi tenancy setup (multitenant=Y) the delegator gets, when a user logs in at the login screen and provides the tenantId, extended with a separator (the # sign) and the tenantId, resulting in 'default#<tenantId' as can be seen in the logs at various registrations to have the user to be able to work with functions of applications in relation to data of the tenant. So, how does OFBiz enable access to the application, retrieve and deliver tenant specific data for the anonymous users of an application given a specific tenant? Well, this is done by changing the configuration setting for the delegator in the web.xml of the application that should deliver the tenant specific data. When the setting in web.xml of an application is changed to *<context-param>* *<param-name>entityDelegatorName</param-name>* *<param-value>default#<tenantId></param-value>* *<description>The Name of the Entity Delegator to use, defined in entityengine.xml</description>* *</context-param>* the application/component will deliver data of the tenant again to authenticated/authorised users when they provide the tenantId in the login screen, but also to anonymous users (who don't provide the tenantId). The specific entityDelegatorName tells OFBiz from which associated database it should retrieve data from and write data to. It essentially overrides the generic function regarding the 'default' way of working. This also ensures application can't be accessed and data can't be retrieved from or written to by authenticated users of a different tenant (different tenantId) or even users authenticated against the master. So regarding aspect 1 of the issue, OFbiz is covered and working as intended and no change should be needed. *Aspect 2: Tenant should run with specified domain name* Prior to the issue, only 1 domain name could be registered in a record in the Tenant entity through the interfaces of WEBTools. Apart from that OFBiz has the capabilities to define site URIs in the content application, so that ofbiz can work with not only multiple domains ( www.example.com vs www.rexample.org), but also with multiple sites within one domain (site1.example.com vs site2.example.com). Given the nature of OFBiz this works for both the single tenancy setup and the multi tenancy setup. Again, for anonymous and authenticated users. This I mentioned in a comment in the issue. See: https://issues.apache.org/jira/browse/OFBIZ-5898?focusedCommentId=14235398&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14235398 But, as I surmise, the creator of the issue was not aware of this functionality in the content application (and apparently chose to ignore the comment in the issue about content functionality visavis domains/web sites in the issue) and thus meant the aspect to be: have multi domain functionality in the tenant configuration (at master level). This aspect seems to be the main reason that the patch addresses. *Aspect 3: User should be able to provide domain name at tenant creation.* Prior to the issue, the OFBiz administrator (the user) could not set the domain name at creation of the tenant. Also, the OFBiz administrator could only set the one domain name through functionalities in the WEBTOOLS application by editing existing tenant records in the Tenant entity. Again I surmise, as no additional explanation is given within the context of the issue by the creator, that he sought to improve the process by providing the ability/option for setting the domain name in the tenant creation functionality (ant target). This aspect seems to be the second reason that the patch addresses. Aspect 4. Configuration details should be tenant specific. In order to have a tenant specific aspect to work properly, configurations settings should be applicable per tenant, and if a configuration setting is not provided for a specific tenant the default configuration settings (in the properties files of various applications) should/must be used. This aspect is not addressed by the patch. In fact, in a comment in the issue, the creator stated that a new issue were to be created (and has been created) to address this specifically). See https://issues.apache.org/jira/browse/OFBIZ-5902 *Effects of the patch on how OFBiz works after the commit* The commit has the following effect on how OFBiz is working: - Creating a tenant - the new method of creating a tenant (./ant create-tenant) now included setting the domain name of the tenant. This results in setting the tenant id and the domain name in entity TenantDomainName. As setting the domain name is optional when executing ./ant create-tenant, this should not be executed when the domain name is left blank. However, setting the domain name is always executed, and as the domainName is (part of) the primary key of the entity TenantDomainName it will be automatically generated (gets sequenced Id) when left blank. - Changing how urlServletHelper is working. Prior to the change the domain name was checked against the records in entity Tenant (single domain setup). With the change the domain name is now checked against the records in entity TenantDomainName (multiple domain setup). - Changing how ContextFilter is working. Prior to the change the domain name was checked against the records in entity Tenant (single domain setup). With the change the domain name is now checked against the records in entity TenantDomainName (multiple domain setup). - Changing how LoginWorker is working. Prior to the change the login was checked against availability set through the login screen or (when not available) the setting in the param-name 'entityDelegatorName' in the web.xml of the component. With the change an additional check is included and the cleanup of the delegator (from '*default**#<tenantId>'* back to 'default') is removed. Re: domainName setting (aspect 3) Having this setting of the domain name on the moment of the creation of the tenant via ./ant create-tenant (the commit) seems to be a regression in functionality,as it always creates a tenant domain (using a sequenced Id for the domainName field in entity TenantDomainName when left empty), in stead of working with the optionality (the choice not to set the domain name) Re: context-param param-name='entityDelegatorName' setting It seems that the commit has led to a more relaxed level of security regarding access to tenant specific applications/components. By setting the context-param for the entityDelegatorName in the web.xml file of the component, there is the extra security layer that the application/component is made available only to users within the defined delegator (*default**#<tenantId>*). This ensures that it can't be accessed by users of other tenants (apart from accidentally having the combination (userId-passWord-tenantId) right on login. That also excludes users of the master setup to be able to access the application/component, inherent to the expectation that userIds in the master database (ofbiz) don't exist in the db of the tenant (ofbiz_<tenantId>), irrespectively of having the SecurityPermissionSeedData of the tenant specific application and associations between userId and the security permissions of that application in the tables of the master database. Now it doesn't work that way anymore. Having tested against trunk and having security permissions of a tenant specific database in the master database combined with a userId associated to those security permissions, a user accessing the master environment (meaning: without providing the tenantId) sees the name of the tenant specific application in the top menu and can access that application. This is wrong. I did not test the functionality against another domain (the intended happy flow) than the default (localhost). I surmise that when the patch appeared in the issue only the code and the happy flow was reviewed and tested by the committer, no more thoughts were given regarding non-happy flows, how the changes affects how OFBiz works with respect to backend functionality or the comment regarding OFBiz content application and domains/web sites. As an example, the use-case regarding the removal of the delegator cleanup was neither motivated in the description of the issue as a need, nor questioned when the patch was committed. *Re: OFBIZ-5986 Error on accessing new tenant* This issue appeared after incorporation of commit r1643510 and seems similar to issue OFBIZ-5447 (see https://issues.apache.org/jira/browse/OFBIZ-5447). Strangely, OFBIZ-5447 was closed by the reviewing committer approximately an hour before he created OFBIZ-5986.But that is another issue. Shortly thereafter a patch was provided, which has not been reviewed till today. *The patch* - Changes how ContextFilter is working by adding additional checks for when a tenantId is not provided, like in the case of logging in at the master. - Removing changes introduced at r1643510 regarding, about the check when the delegatorName isn't equal to the baseDelegatorName (in effect: *default**#<tenantId>* !- default ) and the actions for when they aren't the same. Testing the patch against a backend applications in domain localhost yielded the following results: 1. While logging in at the master a tenant specific application (with context-param param-name='entityDelegatorName' having value *default* *#<tenantId>*) still shows the tenant specific application when the seed and demo data of that app is in the master database and the user can access the application. Effectively, the contex-param isn't factored in security-wise. 2. While logging in at the tenant (by providing the tenantId at the login screen) at the url for the tenant specific application ( https://localhost:8443/tb5896/control/login) led to an NPE. 3. Logging in at the tenant at the url for a application that is not tenant specific context-param param-name='entityDelegatorName' having value *default*) yields success. 4. Switching from the non tenant specific application to the tenant specific application led to the presentation of the login screen. This should not have happened as the user is already logged in at the tenant. After having provided the credentials of the tenant user again the application can be accessed. *Suggestions:* Given that the commit has led to unwanted effects (as indicated in the user ml, see http://ofbiz.markmail.org/message/22bmphzfhkprhrcm?q=Multiple+root+apps+%28tenant%29 and http://ofbiz.markmail.org/message/7pdfttecmqx6ygio?q=Unable+to+connect+tenant+component+with+tenant+database) and the creation of the second issue (OFBIZ-5986), I suggest that we reopen both issues (5898 and 5986) and redo the process and rework the solution. The reason for this is the following: Issues and their subjects/titles are incorporated in release notes, and when a user needs to evaluate a release (for upgrade or new implementation) he will look first at those release notes to be able to assess the applicability of the release regarding his requirements. If such an issue subject is unclear to him, he will look at the description of such issue. At first he will disregard the comments. OFBIZ-5898 has both an ambiguous title and aspects in the description that aren't addressed by the patch, but are in one or more other issues. E.g. aspect 4. And the commit breaks existing functionality. In stead of leaving the commit in and resolve the effects in other issues (like OFBIZ-5896) and creating more confusion for evaluators of the next release, we can reworking title and description of OFBIZ-5898 to we improve the clarity and applicability of the issue and the focus of the solution and redo the solution. That way we can determine issue OFBIZ-5986 is still warranted and can address the comment regarding OFBIZ content application visavis domains/web sites and the comments and findings stated above. Best regards, Pierre Smits *ORRTIZ.COM <http://www.orrtiz.com>* Services & Solutions for Cloud- Based Manufacturing, Professional Services and Retail & Trade http://www.orrtiz.com
