ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Le Roux <jacques.le.r...@les7arts.com>
Subject Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Date Tue, 27 Mar 2018 10:07:19 GMT


Le 27/03/2018 à 11:38, Taher Alkhateeb a écrit :
> On Tue, Mar 27, 2018 at 11:13 AM, Jacques Le Roux
> <jacques.le.roux@les7arts.com> wrote:
>> Hi Taher,
>>
>> That's good questions and I'll try to build a technical perspective here
>> answering them one by one and as possible in chronology.
>>
>> 1. Changes to cookies was introduced with OFBIZ-4959. Actually it was not
>> really a bug rather a clean-up. The autoLogin cookies were only used by the
>>     ecommerce component and maybe webpos. But all applications were creating
>> such cookies with a one year duration. They were useless until I needed
>>     them for the "Navigate from a domain to another with automated signed in
>> authentication" OFBIZ-10307 feature. But even if they were safe
>>     (httponly) then I needed them to be clean, not a one year duration (to be
>> as safe as possible, temporary cookies are better). So with your
>>     suggestion[1] (thanks) I introduced the keep-autologin-cookie <webapp>
>> attribute in ofbiz-component.xml. It's used to remove not kept cookies when
>>     login in or out. So those cookies are only kept during a session. Also a
>> cookie is created when an user jumps from one application to another.
>>     These cookies are used when navigating from a domain to another to
>> guarantee the safety of the user who jumps from a domain to another (unlike
>> my
>>     mistake of using a request parameter initially). Note that protected
>> cookies (httponly) are one of the safer ways to store information, js script
>>     can't use them[2].
> No one helped you in the design, there were issues in it which I
> objected to, you did not collaborate with us on the fix and just
> re-committed something else and when I objected you said you're
> welcome to fix it. I don't like your first solution at all (reverse
> dependency which I explained at length) and I am not comfortable with
> your second solution either.
What does make you uncomfortable?

>
>> 2. Http redirects, I'm not sure what you mean by that. I guess it's the part
>> which is in OFBIZ-10307 (following CORS standard) to allow an user to
>>     jump from a domain to another. For that I use Ajax to send a JWT token in
>> a HTTP header (as recommended by CORS).
> I mean disabling http URLs and their switch to https (port 8080 to
> 8443). That old behavior was removed by you in a commit again without
> community consensus or discussion, you just did it and you committed
> your work. Also, look at the JIRA you provided, I only see Jacques
> doing stuff in it.
After my explanations I thought it was OK, so I applied a lazy consensus

>
>> 3. Authentication, for that I use the checkExternalServerLogin pre-processor
>> in the same vein than checkExternalLoginKey, nothing extraordinary. Just
>>     that it check a JWT token in a HTTP header (as recommended by CORS)
>> rather than a request parameter. I must say that the devil is in the
>> technical
>>     details (of CORS) and I'll not explain that here.
> Again, you did not cooperate or consult with the community, and you
> touched a core component of the system on something which is complex
> and intrusive. There were questionable issues in the design here.
I have a patch waiting for review at https://issues.apache.org/jira/browse/OFBIZ-10307.
I'm working to have another domain to test my work before reverting the whole and submit a
new complete patch there
I don't want to break it all before having tested it on another domain. For now it works on
trunk demo and I can test it. It's temporary but I'd like 
to contribute this feature and let users able to test it on the trunk demo later.

>
>> I hope this helps, of course reviews are welcome.
> It seems unfortunately that our reviews are not welcome. You are not
> reverting and not cooperating.
What do you exactly want to revert and why? What are your propositions?

Jacques

>
>> Jacques
>>
>> [1] https://s.apache.org/qLGC
>>
>> [2]
>> https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage
>>
>>
>>
>> Le 23/03/2018 à 10:22, Taher Alkhateeb a écrit :
>>> I have issues with multiple decisions all around that same topic that
>>> never got community consensus. Changes to cookies, http redirects,
>>> authentication, and other commits that did not get a proper review
>>> from the community. Such major design decisions need proper review IMO
>>>
>>> On Fri, Mar 23, 2018 at 11:38 AM, Jacques Le Roux
>>> <jacques.le.roux@les7arts.com> wrote:
>>>> Le 23/03/2018 à 09:33, Jacques Le Roux a écrit :
>>>>> Le 23/03/2018 à 09:21, Jacopo Cappellato a écrit :
>>>>>> On Fri, Mar 23, 2018 at 8:36 AM, Jacques Le Roux <
>>>>>> jacques.le.roux@les7arts.com> wrote:
>>>>>>
>>>>>>> Did you try what I said?
>>>>>>>
>>>>>>> You can easily check by svn updating to r1819133 and removing
the
>>>>>>> wrapper
>>>>>>> in ContextFilter.java.
>>>>>>>
>>>>>>> Maybe we need to revert Tomcat SSO then?
>>>>>>
>>>>>> A thorough review of that feature is actually on my todo list since
>>>>>> some
>>>>>> time, after I have noticed some potential design issues.
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>> Thanks Jacopo,
>>>>>
>>>>> I'll also review ASAP since I seconded this feature
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>> BTW, forgot to say but the proposed feature at OFBIZ-10307 could be also
>>>> used locally (dropping the CORS part).
>>>> I tested it initially before crossing CORS (pun intended) and it works
>>>> perfectly. It's safe because, like JSESSION, it's build upon safe
>>>> AutoLogin
>>>> cookies
>>>> So we could use it instead of ExternalLoginKey or TomcatSSO. I did not
>>>> test
>>>> in a cluster environment though...
>>>>
>>>> Anyway just saying for now.
>>>>
>>>> Jacques
>>>>


Mime
View raw message