Hello Pierre, Thanks for providing all the details for tenant functionality. Today Arun and I had a good discussion on all the points that you shared here. Arun will provide the concluded details here.
-- Kind Regards, Ashish Vijaywargiya On Thu, Jan 29, 2015 at 7:19 AM, Pierre Smits <[email protected]> wrote: > 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 >
