I reviewed the code and explanation in the beginning of this thread and I have the following comments:
- First, in case of a result.containsKey(ModelService.ERROR_MESSAGE) you are still returning "success". Does not make sense at all. - Also the comments are unnecessary and do not make sense in the same code block - The repetition of the return "success" with a validation if separating them is a sign of a code smell. - The comment you wrote: "Check it's the right tenant in case username and password are the same in different tenants. Not sure this is really useful in the case of external server, does not hurt anyway" is self-defeating. Never put in code you won't use. We have enough mess in the code base. - Calling LoginWorker.doBasicLogin(userLogin, request) after the if/else block does not make sense. The else block guarantees falling into the exception. This whole code block needs refactoring - A testing method needs to be provided in detail. For example I'm not sure how the Javascript portions will execute and when. So some provision of a clear testing mechanism is necessary. - Finally, I'm not sure this feature is needed at all. Why not just assign an LDAP server to take care of OFBiz and non-OFBiz in a single sign-on environment. I see too much code being written for something that might not be of great value and there are other better solutions out there. Using JWT tokens might come with problems [1] that need to be justified before we adopt it. [1] https://en.wikipedia.org/wiki/JSON_Web_Token#Vulnerabilities_and_criticism On Wed, Aug 22, 2018 at 5:22 PM Jacques Le Roux <jacques.le.r...@les7arts.com> wrote: > > Hi Jinghai, > > You can do if you want, but that's not possible in the context of the ASF. > > Also I'm for the ASL2 license as is, not to add a common clause. > > Of course if you pay me for that I would :D But I guess it's a joke, isn'it? > > Jacques > > > Le 22/08/2018 à 14:52, Shi Jinghai a écrit : > > Thank you Jacques! > > > > Perhaps we can build a cloud-ready (SAAS version) OFBiz and add such a > > common clause :) > > > > > > -----邮件原件----- > > 发件人: Jacques Le Roux [mailto:jacques.le.r...@les7arts.com] > > 发送时间: 2018年8月22日 12:11 > > 收件人: dev@ofbiz.apache.org > > 主题: Re: OFBIZ-10307: Navigate from a domain to another with automated > > signed in authentication > > > > Hi Jinghai, > > > > You maybe read it already, what did redislabs very recently might be > > affecting you > > > > https://redislabs.com/community/commons-clause/ > > > > Jacques > > > > > > Le 15/08/2018 à 14:37, Shi Jinghai a écrit : > >> Dear Jacques, > >> > >> On how to store the Tokens, as a token is a key, value is the UserLogin > >> entity and/or other info, a key-value db, Redis[1] is a good choice. Redis > >> is no.7 in db ranking in Aug 2018[2], becomes more and more popular. > >> Goldman Sachs invested Redis team in last year[3]. It's common view now in > >> China that Redis is better than any others including Gemfire of Pivotal, > >> the railway ticket system of China replaced its 3 Gemfire clusters with 3 > >> Redis clusters last year and then there are much less complains on how > >> difficulties to buy spring festival tickets. > >> > >> Mr. Dai Haipeng contributed a Redis component in Jira[4]. > >> > >> [1] https://redis.io/ > >> [2] https://db-engines.com/en/ranking > >> [3] > >> https://redislabs.com/press/redis-labs-secures-44-million-funding-led-goldman-sachs-private-capital-investing-strengthen-database-leadership/ > >> [4] https://issues.apache.org/jira/browse/OFBIZ-9829 > >> > >> BTW, I'll try to review the patches. > >> > >> Kind Regards, > >> > >> Shi Jinghai > >> > >> -----邮件原件----- > >> 发件人: Jacques Le Roux [mailto:jacques.le.r...@les7arts.com] > >> 发送时间: 2018年8月15日 15:09 > >> 收件人: dev@ofbiz.apache.org > >> 主题: OFBIZ-10307: Navigate from a domain to another with automated signed > >> in authentication > >> > >> Hi, > >> > >> Some time ago I created https://issues.apache.org/jira/browse/OFBIZ-10307. > >> > >> I asked for reviews but only Taher answered and he asked to know the goal > >> of this new feature. > >> > >> It was actually developed for a client who needed to get from one OFBiz > >> instance on a server (on a domain) to another OFBiz instance on another > >> server (on another domain) without having to sign up between the 2 while > >> keeping things secure. > >> > >> There could be many reasons why you want to split OFBiz application on > >> servers. In their case it was for performance issues. > >> > >> The technology used is as secure as possible. Like OAuth 2.0 it uses a > >> token but it does not need a middle authorization server (think to > >> two-factor > >> authentication) because it's only for OFBiz instances of the same version. > >> > >> To commit this work we need 1st to agree an commit the work done by Deepak > >> at OFBIZ-9833 "Token Based Authentication" that I use in my last patch. > >> > >> For me there is only one question outstanding: how to store the Token > >> secret. But this should not prevent us to commit Deepak's work. > >> > >> It's now a long time (9 months) since I started this work. And my last > >> patch is ready for a month. > >> > >> I crossed several issues which are now all resolved. So please review and > >> answer to this thread. > >> > >> Without negative comments well argumented I'll commit both OFBIZ-9833 and > >> OFBIZ-10307 in a week. You can always test and review later, we use RTC. > >> > >> Also a veto on a commit is always possible... Of course, as ever, a good > >> consensus is preferred. > >> > >> Let me know if you need more information about the goal. For the technical > >> details I think I already provided them the in OFBIZ-10307. > >> > >> Jacques > >> >
