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
