OFBIZ-10307: Navigate from a domain to another with automated signed in authentication

[Jacques Le Roux]

This was done

Jacques


Le 10/09/2018 à 11:13, Jacques Le Roux a écrit :

> Seems nobody is against renaming so I'll create a new last patches and attach them.
>
> Jacques
>
>
> Le 30/08/2018 à 16:27, Jacques Le Roux a écrit :
>> Hi Nicolas,
>>
>> Thanks for your feedback, much appreciated. I know it's not something easy to review and even less to test.
>>
>> About the names, I see no problems to change them. I used them to try to clarify what they do. Actually for the js functions I initially used names
>> close to what you suggest. And for checkExternalServerLogin I was inspired by checkExternalLoginKey.
>>
>> I'll be happy to change them if everybody agree about that. They are short and explicit enough in the context indeed.
>>
>> Note though that Deepak created the JWTManager class in order to use JWTs for more than an use with userLogin.
>>
>> Jacques
>>
>>
>> Le 30/08/2018 à 13:58, Nicolas Malin a écrit :
>>> Globally (after Taher remarks :) ) I found the code clear. Instead of my comment about the jwt generation [1] on issue OFBIZ-9833 [2] I haven't
>>> other technical remarks that can help to improve this.
>>>
>>> Now on naming vision I suggest simple naming like this :
>>>
>>> setUserLoginIdInJwtToken -> loadJWT()
>>> sendUserLoginIdInJwtToken -> sendJWT()
>>> checkExternalServerLogin -> checkJWTLogin()
>>>
>>> Because I didn't see why we call a token without userLogin and jwt always talk about a token (Jsin Web Token) not necessary to redound it
>>>
>>> Nicolas
>>>
>>> [1]
>>> https://issues.apache.org/jira/browse/OFBIZ-9833?focusedCommentId=16594702&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16594702
>>> [2] https://issues.apache.org/jira/browse/OFBIZ-9833
>>> On 29/08/2018 12:37, Jacques Le Roux wrote:
>>>> I'll soon provide an AsciiDoc for that. I have to think about its location, any ideas?
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 29/08/2018 à 12:06, Taher Alkhateeb a écrit :
>>>>> 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
>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]> 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
>>>>>>>>> <[hidden email]> 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:[hidden email]]
>>>>>>>>>>> 发送时间: 2018年8月22日 12:11
>>>>>>>>>>> 收件人: [hidden email]
>>>>>>>>>>> 主题: 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:[hidden email]]
>>>>>>>>>>>> 发送时间: 2018年8月15日 15:09
>>>>>>>>>>>> 收件人: [hidden email]
>>>>>>>>>>>> 主题: 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
>>>>>>>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>