I already found problems in this code to give me pause and to show
that it is not properly studied and designed. If you want to continue
the effort, then I recommend reviewing your code thoroughly, providing
a clear testing mechanism (especially the web scripts), and ask for
reviews and tests.
On Sun, Aug 26, 2018 at 12:33 PM Jacques Le Roux
<jacques.le.r...@les7arts.com> wrote:
>
> Hi Taher,
>
> Thanks for your review, much appreciated.
>
>
> Le 22/08/2018 à 18:42, Taher Alkhateeb a écrit :
> > 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.
> You are right I should have used "error".
> I did not notice (actually forgot a last own review after much variations in
> code) because it's an event preprocessor and no response is
> expected/handled.
> Not to bad, in case of error just an EventHandlerException was not thrown.
>
> > - Also the comments are unnecessary and do not make sense in the same code
> > block
> You mean "// Something unexpected happened here", or ?
>
> > - 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.
> I must say I initially copied that from
> ExternalLoginKeysManager::checkExternalLoginKey and adapted it to the
> context. It's roughly the same code.
> I let it there because I was then unsure. But when you think about it, there
> is no reason to not make this check. So I'll simply remove the comment. I
> should have put a TODO.
>
> BTW this is out of subject and I'll start a new thread about it after reading
> https://www.linkedin.com/pulse/architecture-constraints-end-multi-tenancy-gregor-hohpe/
> The subject will be: "Should we keep the multi-tenants feature in OFBiz."
> I though think the tenants in OFBiz are still useful when you only needs
> dozens or maybe even few hundreds tenants (begin to be a lot of DBs!).
> But I faced that with a startup which wanted to handle thousands, if not
> millions (actually they failed), of tenants, obviously OFBiz can't do that.
>
> > - 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
> You are totally right, I have uploaded a new patch where I put the call of
> LoginWorker.doBasicLogin(userLogin, request);
> in the positive part of the if and used the same block pattern than above
> where there is a warning log then returning an error.
> I was so focused on the other parts of the mechanism, especially to secure
> it, than I forgot to review the Java part.
> My bad, thank you again for your review.
>
> > - 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.
> Yep, right, I'll document all the mechanism and how to use it. Pierre even
> proposed to make a graph in AsciiDoc, not sure I'll be able to, by I'll try.
> Actually it's only a matter of following what should be in the example
> component, see last OFBIZ-10307-test from example.patch.
> I thought it would be enough for devs. I understand it's not, a bit of clear
> documentation is always welcomed, and I often advocate for it.
>
> > - 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.
> Because you don't need to have (install, maintain, etc.) a LDAP server, or
> whatnot (CAS, Oauth2, SAML, you name it) if you use this code, it's ready
> OOTB.
>
> > I see too much code being written for something
> > that might not be of great value and there are other better solutions
> > out there.
> Which ones do you think about? And why they are better at doing this specific
> feature?
>
> > Using JWT tokens might come with problems [1] that need to
> > be justified before we adopt it.
> That's very interesting, let's check, after reading
> https://connect2id.com/products/nimbus-jose-jwt/vulnerabilities
>
> 1. Never let the JWT header alone drive verification
> * Not our case, we have the loginUserId ciphered in and checked. The
> crucial point was how to send the loginUserId securely.
> 2. Know the algorithms
> * This part is handled by JWTManager class. We have begun to discuss
> where to store the secret key at https://s.apache.org/IiE0
> 3. Use an appropriate key size
> * This part is handled by JWTManager class, which uses HMAC with
> SHA-512. And that's OK reading the article Wikipedia refers to.|
> |
>
> |Jacques|
>
> >
> > [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
> >>>>
>
