Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml

[Scott Gray-3]

Yeah, I said exactly that elsewhere in the same email.

Regards
Scott

On 26 March 2018 at 21:17, Jacques Le Roux <[hidden email]>
wrote:

> Le 24/03/2018 à 18:49, Scott Gray a écrit :
>
>> I'm also not even sure if our custom ObjectType methods for checking this
>> type of thing are necessary any more with so many new java versions since
>> it was decided these were needed for performance reasons.
>>
> We would need some profiling before making any decision, don't you think
> so?
>
> Jacques
>
>

[Paul Foxworthy]

Hi Jacques,

On 27 March 2018 at 08:16, Jacques Le Roux <[hidden email]>
wrote:

> What makes you think that Tomcat SSO depends on servlet4preview?
>

Only your words

"So when James introduced Tomcat SSO and optionally passed a
javax.servlet.http.HttpServletRequest to the userLogin service it did not
break.
But when I removed HttpServletRequestWrapper from ContextFilter it popped
up".

In the the analysis I did for https://issues.apache.org/jira
> /browse/OFBIZ-10304 I only found that using Tomcat 8.5 (hence
> servlet4preview) we no longer can pass a standard HttpServletRequest  or
> HttpServletResponse with current code. Did you find something else?


No.

 If we now say OFBiz requires Servlet 4.0 and move to Tomcat 9, could we
>> then use the
>> standard HttpServletRequest?
>>
> Yes, that would remove the problem and is IMO the best solution.
>

I hadn't read through OFBIZ-9833 until this morning. My understanding is
now:

- Tomcat SSO is a red herring. It can be implemented with
HttpServletRequest. As you say, it doesn't need Servlet 4 or the
servlet4preview
package.

- HttpServletRequestWrapper implements HttpServletRequest anyway, so
whether we use it or not shouldn't affect services that want
HttpServletRequest.

- The crux of the problem is a one-generation type check in the OFBiz
service input checking, which uses Class.getInterfaces(). If a class
implements a derived interface, the service type checking
doesn't detect that an object of that class is compatible with the base
interface of the derived one.

One-generation type checking is not foolproof, but probably faster than
using recursion to search for base interfaces. OFBiz has been happily
running for years without a multi-generation type check. I like Scott's
idea: for the tiny fraction of services that accept HttpServletRequest,
define the type as Object with a custom validation method. We can probably
revert to HttpServletRequest with Tomcat 9, but that is a bigger and more
disruptive change.

Cheers

Paul Foxworthy

--
Coherent Software Australia Pty Ltd
PO Box 2773
Cheltenham Vic 3192
Australia

Phone: [phone] <+61%203%209585%206788>
Web: http://www.coherentsoftware.com.au/
Email: [hidden email]

[Jacques Le Roux]

Le 25/03/2018 à 03:11, Scott Gray a écrit :
> That would solve the problem for now until the service engine is improved
> and tested or the tomcat SSO login design is improved.
How is the tomcat SSO login design related here? I see only Tomcat 8.5. Of course the tomcat SSO login design may be improved but I don't see how it
could resolve the issue at hand.

Jacques

[Jacques Le Roux]

Hi Paul,

Inline...

Le 27/03/2018 à 03:42, Paul Foxworthy a écrit :
> I hadn't read through OFBIZ-9833 until this morning. My understanding is
> now:
>
> - Tomcat SSO is a red herring. It can be implemented with
> HttpServletRequest. As you say, it doesn't need Servlet 4 or the
> servlet4preview
> package.
Yes

>
> - HttpServletRequestWrapper implements HttpServletRequest anyway, so
> whether we use it or not shouldn't affect services that want
> HttpServletRequest.
Yes

> - The crux of the problem is a one-generation type check in the OFBiz
> service input checking, which uses Class.getInterfaces(). If a class
> implements a derived interface, the service type checking
> doesn't detect that an object of that class is compatible with the base
> interface of the derived one.
Yes

> One-generation type checking is not foolproof, but probably faster than
> using recursion to search for base interfaces. OFBiz has been happily
> running for years without a multi-generation type check. I like Scott's
> idea: for the tiny fraction of services that accept HttpServletRequest,
> define the type as Object with a custom validation method. We can probably
> revert to HttpServletRequest with Tomcat 9, but that is a bigger and more
> disruptive change.
This could be a temporary workaround but we eventually want to update to Tomcat 9 anyway because we don't know which other damages servlet4preview,
which is a temporary incomplete thing, can do.

Jacques

>
> Cheers
>
> Paul Foxworthy
>

[Jacques Le Roux]

I don't exactly find it, but I'll take your word. Anyway it makes absolutely sense, not a point to discuss more. At least I'll not.

Jacques


Le 27/03/2018 à 02:07, Scott Gray a écrit :

> Yeah, I said exactly that elsewhere in the same email.
>
> Regards
> Scott
>
> On 26 March 2018 at 21:17, Jacques Le Roux <[hidden email]>
> wrote:
>
>> Le 24/03/2018 à 18:49, Scott Gray a écrit :
>>
>>> I'm also not even sure if our custom ObjectType methods for checking this
>>> type of thing are necessary any more with so many new java versions since
>>> it was decided these were needed for performance reasons.
>>>
>> We would need some profiling before making any decision, don't you think
>> so?
>>
>> Jacques
>>
>>

[Jacques Le Roux]

BTW Paul, I forgot to thank you for your effort

Yesterday I thought about explaining the whole picture with this problem and especially how I came there.

It's now obvious to me but I understand that a summary would help everybody. I remember Taher was expressing his frustration about that.

Jacques


Le 27/03/2018 à 08:10, Jacques Le Roux a écrit :

> Hi Paul,
>
> Inline...
>
> Le 27/03/2018 à 03:42, Paul Foxworthy a écrit :
>> I hadn't read through OFBIZ-9833 until this morning. My understanding is
>> now:
>>
>> - Tomcat SSO is a red herring. It can be implemented with
>> HttpServletRequest. As you say, it doesn't need Servlet 4 or the
>> servlet4preview
>> package.
> Yes
>
>>
>> - HttpServletRequestWrapper implements HttpServletRequest anyway, so
>> whether we use it or not shouldn't affect services that want
>> HttpServletRequest.
> Yes
>
>> - The crux of the problem is a one-generation type check in the OFBiz
>> service input checking, which uses Class.getInterfaces(). If a class
>> implements a derived interface, the service type checking
>> doesn't detect that an object of that class is compatible with the base
>> interface of the derived one.
> Yes
>
>> One-generation type checking is not foolproof, but probably faster than
>> using recursion to search for base interfaces. OFBiz has been happily
>> running for years without a multi-generation type check. I like Scott's
>> idea: for the tiny fraction of services that accept HttpServletRequest,
>> define the type as Object with a custom validation method. We can probably
>> revert to HttpServletRequest with Tomcat 9, but that is a bigger and more
>> disruptive change.
> This could be a temporary workaround but we eventually want to update to Tomcat 9 anyway because we don't know which other damages servlet4preview,
> which is a temporary incomplete thing, can do.
>
> Jacques
>
>>
>> Cheers
>>
>> Paul Foxworthy
>>
>
>

[Jacques Le Roux]

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].
 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).
 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.

I hope this helps, of course reviews are welcome.

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
> <[hidden email]> 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 <
>>>> [hidden email]> 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
>>

[taher]

On Tue, Mar 27, 2018 at 11:13 AM, Jacques Le Roux
<[hidden email]> 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.

> 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.

> 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 hope this helps, of course reviews are welcome.

It seems unfortunately that our reviews are not welcome. You are not
reverting and not cooperating.

>
> 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
>> <[hidden email]> 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 <
>>>>> [hidden email]> 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
>>>
>

[Jacques Le Roux]

Le 27/03/2018 à 11:38, Taher Alkhateeb a écrit :

> On Tue, Mar 27, 2018 at 11:13 AM, Jacques Le Roux
> <[hidden email]> 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
>>> <[hidden email]> 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 <
>>>>>> [hidden email]> 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
>>>>