I couldn't find the testing mechanism and the JIRA is filled with too
much information. Sharing a simple testing mechanism would help
On Wed, Aug 29, 2018 at 12:33 PM Jacques Le Roux
<jacques.le.r...@les7arts.com> wrote:
>
> Hi Taher,
>
> Before creating the last patch, I have reviewed the code a last time again,
> it's totally OK now. Did you test from
> https://localhost:8443/catalog/control/FindCatalog with the provided test
> patch?
>
> I'll now provide a simple and concise AsciiDoc documentation. Maybe with a
> graph, but that's not sure, I have to try...
>
> For the test part I'll wait for the user ML "OFBiz Sanity Test Automation"
> thread https://markmail.org/message/nxoyqxn7lqfupf55 to conclude before
> endeavouring in web tests
>
> I don't think it should stop to commit this work, which can be tested
> manually using the provided patches (though the one from example needs the
> commit to be done)
>
> Anyway I'll wait more feedbacks before committing, I'm not in a hurry and I
> know it's a sensitive feature.
>
> Jacques
>
>
> Le 27/08/2018 à 14:55, Taher Alkhateeb a écrit :
> > 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
> >>>>>>
>
