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