You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Michael Brohl <mi...@ecomify.de> on 2018/03/22 15:22:38 UTC
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Mmhhh, Jacques,
I think this is problematic because it ties a special implementation for
Tomcat to the service. I didn't see this anywhere else.
The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
unclear and I don't get the purpose of this change.
Can you please explain more clearly which problem this changes solves
and why we'll need org.apache.catalina.connector.RequestFacade as the type?
Thanks,
Michael
Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
> Author: jleroux
> Date: Wed Mar 21 20:53:41 2018
> New Revision: 1827439
>
> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
> Log:
> Fixed: The "request" attribute type of the userLogin service is wrong
> (OFBIZ-10304)
>
> It should have been org.apache.catalina.connector.RequestFacade"
> instead of javax.servlet.http.HttpServletRequest see the Jira for details
>
> Modified:
> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>
> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml?rev=1827439&r1=1827438&r2=1827439&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml Wed Mar 21 20:53:41 2018
> @@ -379,7 +379,7 @@ under the License.
> <service name="userLogin" engine="java" location="org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
> <description>Used to Automatically Authenticate a username/password; create a UserLogin object</description>
> <implements service="authenticationInterface"/>
> - <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest" optional="true"/>
> + <attribute name="request" mode="IN" type="org.apache.catalina.connector.RequestFacade" optional="true"/>
> </service>
> <service name="createUserLogin" engine="java" auth="false"
> location="org.apache.ofbiz.common.login.LoginServices" invoke="createUserLogin">
>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
See my answer to Scott
Jacques
Le 23/03/2018 à 07:00, Taher Alkhateeb a écrit :
> Not only is this commit unnecessary, but also breaks one of the best
> features in Object Oriented Programming -- polymorphism -- for no value
> that I can see anywhere at all.
>
> Also by saying we depend fully on Tomcat and we should write code that
> further enforces that idea instead of making our architecture more modular
> is a real bad idea IMO.
>
> -1
>
>
> On Fri, Mar 23, 2018, 5:40 AM Scott Gray <sc...@hotwaxsystems.com>
> wrote:
>
>> Something else must be wrong Jacques, I can't understand what you're saying
>> in the ticket description or in your reply above but I do know the
>> following:
>> OFBiz is perfectly capable of knowing that
>> org.apache.catalina.connector.RequestFacade
>> implements the javax.servlet.http.HttpServletRequest and it should pass
>> type validation without errors.
>>
>> Since we know that, this commit becomes unnecessary.
>>
>> Regards
>> Scott
>>
>> On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
>> wrote:
>>
>>> Michael,
>>>
>>> I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>
>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>> committed
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>> pathrev=1819133
>>>
>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>> James
>>> would not have been allowed to commit
>>>
>>> <attribute name="request" mode="IN"
>> type="javax.servlet.http.HttpServletRequest"
>>> optional="true"/>
>>>
>>> Because it's then rejected, because then the userLogin service actually
>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>> implements HttpServletRequest.)
>>>
>>> You can easily check by svn updating to r1819133 and removing the wrapper
>>> in ContextFilter.java.
>>>
>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>> tools
>>> like Jetty.
>>>
>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows.
>> So
>>> I think it's safe to use a RequestFacade there.
>>>
>>> If an user feels the need to use another webapp servers or Jetty, I think
>>> changing that would not be the most of the worries (the rest being much
>>> more frightening, I know I did it once with Geronimo).
>>>
>>> Since the check is done at the service level, it's hard to do otherwise
>>> But I must say I did not dig too much and it's maybe possible, we can
>>> discuss that...
>>>
>>> Jacques
>>>
>>>
>>>
>>>
>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>
>>>> Mmhhh, Jacques,
>>>>
>>>> I think this is problematic because it ties a special implementation for
>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>
>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
>>>> unclear and I don't get the purpose of this change.
>>>>
>>>> Can you please explain more clearly which problem this changes solves
>> and
>>>> why we'll need org.apache.catalina.connector.RequestFacade as the type?
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>
>>>>> Author: jleroux
>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>> New Revision: 1827439
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>> Log:
>>>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>>>> (OFBIZ-10304)
>>>>>
>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>> details
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>>
>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>> ices.xml
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>> &r2=1827439&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> ---
>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>> (original)
>>>>> +++
>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>> Wed Mar 21 20:53:41 2018
>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>> <service name="userLogin" engine="java" location="
>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>> <description>Used to Automatically Authenticate a
>>>>> username/password; create a UserLogin object</description>
>>>>> <implements service="authenticationInterface"/>
>>>>> - <attribute name="request" mode="IN"
>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>> + <attribute name="request" mode="IN"
>>>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>>>> </service>
>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>> invoke="createUserLogin">
>>>>>
>>>>>
>>>>>
>>>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Taher Alkhateeb <sl...@gmail.com>.
Not only is this commit unnecessary, but also breaks one of the best
features in Object Oriented Programming -- polymorphism -- for no value
that I can see anywhere at all.
Also by saying we depend fully on Tomcat and we should write code that
further enforces that idea instead of making our architecture more modular
is a real bad idea IMO.
-1
On Fri, Mar 23, 2018, 5:40 AM Scott Gray <sc...@hotwaxsystems.com>
wrote:
> Something else must be wrong Jacques, I can't understand what you're saying
> in the ticket description or in your reply above but I do know the
> following:
> OFBiz is perfectly capable of knowing that
> org.apache.catalina.connector.RequestFacade
> implements the javax.servlet.http.HttpServletRequest and it should pass
> type validation without errors.
>
> Since we know that, this commit becomes unnecessary.
>
> Regards
> Scott
>
> On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
> wrote:
>
> > Michael,
> >
> > I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> > mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
> > l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
> >
> > Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
> > committed
> > http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> > mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
> > pathrev=1819133
> >
> > If I had not committed the wrapper in ContextFilter.java beforehand,
> James
> > would not have been allowed to commit
> >
> > <attribute name="request" mode="IN"
> type="javax.servlet.http.HttpServletRequest"
> > optional="true"/>
> >
> > Because it's then rejected, because then the userLogin service actually
> > receive an org.apache.catalina.connector.RequestFacade (RequestFacade
> > implements HttpServletRequest.)
> >
> > You can easily check by svn updating to r1819133 and removing the wrapper
> > in ContextFilter.java.
> >
> > You are right that it ties OFBiz to Tomcat. We took this decision
> > sometimes ago and we no longer support other webapp servers OOTB, nor
> tools
> > like Jetty.
> >
> > So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows.
> So
> > I think it's safe to use a RequestFacade there.
> >
> > If an user feels the need to use another webapp servers or Jetty, I think
> > changing that would not be the most of the worries (the rest being much
> > more frightening, I know I did it once with Geronimo).
> >
> > Since the check is done at the service level, it's hard to do otherwise
> > But I must say I did not dig too much and it's maybe possible, we can
> > discuss that...
> >
> > Jacques
> >
> >
> >
> >
> > Le 22/03/2018 à 16:22, Michael Brohl a écrit :
> >
> >> Mmhhh, Jacques,
> >>
> >> I think this is problematic because it ties a special implementation for
> >> Tomcat to the service. I didn't see this anywhere else.
> >>
> >> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
> >> unclear and I don't get the purpose of this change.
> >>
> >> Can you please explain more clearly which problem this changes solves
> and
> >> why we'll need org.apache.catalina.connector.RequestFacade as the type?
> >>
> >> Thanks,
> >>
> >> Michael
> >>
> >>
> >> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
> >>
> >>> Author: jleroux
> >>> Date: Wed Mar 21 20:53:41 2018
> >>> New Revision: 1827439
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
> >>> Log:
> >>> Fixed: The "request" attribute type of the userLogin service is wrong
> >>> (OFBIZ-10304)
> >>>
> >>> It should have been org.apache.catalina.connector.RequestFacade"
> >>> instead of javax.servlet.http.HttpServletRequest see the Jira for
> >>> details
> >>>
> >>> Modified:
> >>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
> >>>
> >>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
> >>> ices.xml
> >>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> >>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
> >>> &r2=1827439&view=diff
> >>> ============================================================
> >>> ==================
> >>> ---
> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
> >>> (original)
> >>> +++
> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
> >>> Wed Mar 21 20:53:41 2018
> >>> @@ -379,7 +379,7 @@ under the License.
> >>> <service name="userLogin" engine="java" location="
> >>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
> >>> <description>Used to Automatically Authenticate a
> >>> username/password; create a UserLogin object</description>
> >>> <implements service="authenticationInterface"/>
> >>> - <attribute name="request" mode="IN"
> >>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
> >>> + <attribute name="request" mode="IN"
> >>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
> >>> </service>
> >>> <service name="createUserLogin" engine="java" auth="false"
> >>> location="org.apache.ofbiz.common.login.LoginServices"
> >>> invoke="createUserLogin">
> >>>
> >>>
> >>>
> >>
> >>
> >
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 27/03/2018 à 11:38, Taher Alkhateeb a écrit :
> On Tue, Mar 27, 2018 at 11:13 AM, Jacques Le Roux
> <ja...@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
>>> <ja...@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
>>>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Taher Alkhateeb <sl...@gmail.com>.
On Tue, Mar 27, 2018 at 11:13 AM, Jacques Le Roux
<ja...@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.
> 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
>> <ja...@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
>>>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
> <ja...@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
>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Taher Alkhateeb <sl...@gmail.com>.
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
<ja...@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
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
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
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Michael,
AFAIK, Tomcat SSO does not directly relates to this issue. For me it was just a catalyseur which allowed me to find the servlet4preview issue due to
Tomcat 8.5 usage. And I can say it was not easy.
This said all reviews of https://issues.apache.org/jira/browse/OFBIZ-10047 are welcome. I agree we must be more careful about discussing things in dev
ML before putting in important new feature. This is something we agree about long ago, but tend to forget.
Like people asking user questions on dev ML (w/o not even being subscribed to this ML :/ OK I needed this rant). So yes, we need to continue to
educate ourselves and our users, for the good of the community, and it's not always easy.
Jacques
Le 25/03/2018 à 11:49, Michael Brohl a écrit :
> Hi Paul,
>
> I agree, that's what I tried to express in my comments on https://issues.apache.org/jira/browse/OFBIZ-10047:
>
> "7. Someone should check this solution from an architectural view. I appreciate the efforts but I am also in doubt if we should put this feature
> into the new release. It's very fresh, deals with a very central functionality and should be field tested more."
>
> I really do think that we should have another process when we introduce this kind of new functionality, make use of new third party API etc..
>
> You and Scott made valuable points in this discussion and if we would have had some kind of voting for this, they might be brought up earlier. The
> Jira discussions just slip through if there is no discussion on the dev list.
>
> Thanks,
>
> Michael
>
>
> Am 25.03.18 um 03:02 schrieb Paul Foxworthy:
>> Hi all,
>>
>> The servlet4preview package in Tomcat 8.5 "provides early access to some of
>> the features of Servlet 4.0" (
>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
>> ).
>>
>> Does use of this package make OFBiz dependent on a preview version, and if
>> so is that a good idea? Should features that use some preview (like support
>> for Tomcat SSO, it appears) be contained in a branch or plugin until the
>> API has been stabilised?
>>
>> Tomcat 9 was released in January, with support for Servlet 4.0. If we now
>> say OFBiz requires Servlet 4.0 and move to Tomcat 9, could we then use the
>> standard HttpServletRequest?
>>
>> Thanks
>>
>> Paul Foxworthy
>>
>>
>> On 25 March 2018 at 04:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
>>
>>> So to summarize your long email ;-)
>>>
>>> "The service engine has a limitation in that it only checks the interfaces
>>> directly implemented by the object being tested. Tomcat's RequestFacade
>>> doesn't directly implement javax.servlet.http.HttpServletRequest so it
>>> fails to pass the type validation."
>>>
>>> On the surface your suggested fix looks fine to me, my only concern being
>>> that we may need to do some performance testing. For every service that
>>> specifies an interface we'll now be checking the full type hierarchy which
>>> I imagine will be more expensive but I'm not sure how expensive.
>>>
>>> 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.
>>>
>>> Regards
>>> Scott
>>>
>>> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
>>> wrote:
>>>
>>>> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>>>>
>>>>> I don't need to try anything, I *know* that the service engine is
>>> supposed
>>>>> to accept a concrete class of an interface if the interface is specified
>>>>> as
>>>>> the attribute type.
>>>>>
>>>>> Either the service engine is broken by not accepting concrete
>>>>> implementations, or the bug report is incorrect.
>>>>>
>>>> Neither, it's "unfortunate", and a bit complicated.
>>>>
>>>> Tomcat servlet4preview was introduced with Tomcat 8.5
>>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>>> lina/servlet4preview/package-summary.html
>>>>
>>>> Before James introduced Tomcat SSO, we had only one service passing a
>>>> javax.servlet.http.HttpServletRequest to a service:
>>> payPalCheckoutUpdate.
>>>> AFAIK, we have actually always passed a org.apache.catalina.connector.
>>> RequestFacade
>>>> to services when asking for javax.servlet.http.HttpServletRequest in
>>>> services definition.
>>>> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.
>>> HttpServletRequest
>>>> indirectly through org.apache.catalina.servlet4pr
>>>> eview.http.HttpServletRequest
>>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>>> lina/connector/RequestFacade.html
>>>>
>>>> Since we have no tests on the payPalCheckoutUpdate service we did not
>>> spot
>>>> that Tomcat servlet4preview was introduced with Tomcat 8.5
>>>>
>>>> The classes check is done by the deeper interfaceOf() method of the
>>>> ObjectType class using Class.getInterfaces()
>>>> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
>>>> html#getInterfaces--
>>>> Class.getInterfaces() does not recurse and stops at one level up. So in
>>>> case of RequestFacade it returns only servlet4preview.http.
>>> HttpServletRequest
>>>> and not javax.servlet.http.HttpServletRequest
>>>> So when interfaceOf() compares the Classes it fails.
>>>>
>>>> What happened with my introduction of the HttpServletRequestWrapper in
>>>> ContextFilter is it hid the RequestFacade because
>>> HttpServletRequestWrapper
>>>> implements javax.servlet.http.HttpServletRequest
>>>> https://docs.oracle.com/javaee/7/api/javax/servlet/http/
>>>> HttpServletRequest.html
>>>>
>>>> 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
>>>>
>>>> Summary: it's unfortunate because we have no tests on the
>>>> payPalCheckoutUpdate service.
>>>> Because I temporarily introduced HttpServletRequestWrapper James was able
>>>> to pass a javax.servlet.http.HttpServletRequest, like in
>>>> payPalCheckoutUpdate.
>>>> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
>>>> discovered that we had a problem with Tomcat 8.5.
>>>>
>>>> I propose this fix which uses org.apache.commons.lang3.
>>> ClassUtils.getAllInterfaces()
>>>> and works
>>>> http://commons.apache.org/proper/commons-lang/javadocs/api-
>>>> release/src-html/org/apache/commons/lang3/ClassUtils.html
>>>>
>>>> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
>>>> ctType.java
>>>> ===================================================================
>>>> --- framework/base/src/main/java/org/apache/ofbiz/base/util/
>>> ObjectType.java
>>>> (révision 1827594)
>>>> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/
>>> ObjectType.java
>>>> (copie de travail)
>>>> @@ -263,7 +263,7 @@
>>>> */
>>>> public static boolean interfaceOf(Class<?> objectClass, Class<?>
>>>> interfaceClass) {
>>>> while (objectClass != null) {
>>>> - Class<?>[] ifaces = objectClass.getInterfaces();
>>>> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
>>>> Utils.getAllInterfaces(objectClass);
>>>>
>>>> for (Class<?> iface: ifaces) {
>>>> if (iface == interfaceClass) {
>>>> Index: framework/common/servicedef/services.xml
>>>> ===================================================================
>>>> --- framework/common/servicedef/services.xml (révision 1827594)
>>>> +++ framework/common/servicedef/services.xml (copie de travail)
>>>> @@ -379,7 +379,7 @@
>>>> <service name="userLogin" engine="java" location="
>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>> <description>Used to Automatically Authenticate a
>>>> username/password; create a UserLogin object</description>
>>>> <implements service="authenticationInterface"/>
>>>> - <attribute name="request" mode="IN" type="org.apache.catalina.
>>> connector.RequestFacade"
>>>> optional="true"/>
>>>> + <attribute name="request" mode="IN" type="javax.servlet.http.
>>> HttpServletRequest"
>>>> optional="true"/>
>>>> </service>
>>>> <service name="createUserLogin" engine="java" auth="false"
>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>> invoke="createUserLogin">
>>>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>>
>>>>> Regards
>>>>> Scott
>>>>>
>>>>> On 23 March 2018 at 07:36, 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?
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>>
>>>>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>>>>
>>>>>> Something else must be wrong Jacques, I can't understand what you're
>>>>>>> saying
>>>>>>> in the ticket description or in your reply above but I do know the
>>>>>>> following:
>>>>>>> OFBiz is perfectly capable of knowing that
>>>>>>> org.apache.catalina.connector.RequestFacade
>>>>>>> implements the javax.servlet.http.HttpServletRequest and it should
>>> pass
>>>>>>> type validation without errors.
>>>>>>>
>>>>>>> Since we know that, this commit becomes unnecessary.
>>>>>>>
>>>>>>> Regards
>>>>>>> Scott
>>>>>>>
>>>>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>>>>> jacques.le.roux@les7arts.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Michael,
>>>>>>>
>>>>>>>> I commited http://svn.apache.org/viewvc/o
>>>>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>>>>
>>>>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>>>>> committed
>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>>>>> pathrev=1819133
>>>>>>>>
>>>>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>>>>> James
>>>>>>>> would not have been allowed to commit
>>>>>>>>
>>>>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>>>>> ervletRequest"
>>>>>>>> optional="true"/>
>>>>>>>>
>>>>>>>> Because it's then rejected, because then the userLogin service
>>> actually
>>>>>>>> receive an org.apache.catalina.connector.RequestFacade
>>> (RequestFacade
>>>>>>>> implements HttpServletRequest.)
>>>>>>>>
>>>>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>>>>> wrapper
>>>>>>>> in ContextFilter.java.
>>>>>>>>
>>>>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>>>>> tools
>>>>>>>> like Jetty.
>>>>>>>>
>>>>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
>>> shows.
>>>>>>>> So
>>>>>>>> I think it's safe to use a RequestFacade there.
>>>>>>>>
>>>>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>>>>> think
>>>>>>>> changing that would not be the most of the worries (the rest being
>>> much
>>>>>>>> more frightening, I know I did it once with Geronimo).
>>>>>>>>
>>>>>>>> Since the check is done at the service level, it's hard to do
>>> otherwise
>>>>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>>>>> discuss that...
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>>>>
>>>>>>>> Mmhhh, Jacques,
>>>>>>>>
>>>>>>>>> I think this is problematic because it ties a special implementation
>>>>>>>>> for
>>>>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>>>>
>>>>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>>>>> bit
>>>>>>>>> unclear and I don't get the purpose of this change.
>>>>>>>>>
>>>>>>>>> Can you please explain more clearly which problem this changes
>>> solves
>>>>>>>>> and
>>>>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>>>>> type?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Michael
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>>>>
>>>>>>>>> Author: jleroux
>>>>>>>>>
>>>>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>>>>> New Revision: 1827439
>>>>>>>>>>
>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>>>>> Log:
>>>>>>>>>> Fixed: The "request" attribute type of the userLogin service is
>>> wrong
>>>>>>>>>> (OFBIZ-10304)
>>>>>>>>>>
>>>>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>>>>> details
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/
>>> services.xml
>>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>>>>> amework/common/servicedef/serv
>>>>>>>>>> ices.xml
>>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>>>>> &r2=1827439&view=diff
>>>>>>>>>> ============================================================
>>>>>>>>>> ==================
>>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>>> ices.xml
>>>>>>>>>> (original)
>>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>>> ices.xml
>>>>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>>>>> username/password; create a UserLogin object</description>
>>>>>>>>>> <implements service="authenticationInterface"/>
>>>>>>>>>> - <attribute name="request" mode="IN"
>>>>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>>>>> + <attribute name="request" mode="IN"
>>>>>>>>>> type="org.apache.catalina.connector.RequestFacade"
>>> optional="true"/>
>>>>>>>>>> </service>
>>>>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>>>>> invoke="createUserLogin">
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>
>>
>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Michael Brohl <mi...@ecomify.de>.
Hi Paul,
I agree, that's what I tried to express in my comments on
https://issues.apache.org/jira/browse/OFBIZ-10047:
"7. Someone should check this solution from an architectural view. I
appreciate the efforts but I am also in doubt if we should put this
feature into the new release. It's very fresh, deals with a very central
functionality and should be field tested more."
I really do think that we should have another process when we introduce
this kind of new functionality, make use of new third party API etc..
You and Scott made valuable points in this discussion and if we would
have had some kind of voting for this, they might be brought up earlier.
The Jira discussions just slip through if there is no discussion on the
dev list.
Thanks,
Michael
Am 25.03.18 um 03:02 schrieb Paul Foxworthy:
> Hi all,
>
> The servlet4preview package in Tomcat 8.5 "provides early access to some of
> the features of Servlet 4.0" (
> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
> ).
>
> Does use of this package make OFBiz dependent on a preview version, and if
> so is that a good idea? Should features that use some preview (like support
> for Tomcat SSO, it appears) be contained in a branch or plugin until the
> API has been stabilised?
>
> Tomcat 9 was released in January, with support for Servlet 4.0. If we now
> say OFBiz requires Servlet 4.0 and move to Tomcat 9, could we then use the
> standard HttpServletRequest?
>
> Thanks
>
> Paul Foxworthy
>
>
> On 25 March 2018 at 04:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
>
>> So to summarize your long email ;-)
>>
>> "The service engine has a limitation in that it only checks the interfaces
>> directly implemented by the object being tested. Tomcat's RequestFacade
>> doesn't directly implement javax.servlet.http.HttpServletRequest so it
>> fails to pass the type validation."
>>
>> On the surface your suggested fix looks fine to me, my only concern being
>> that we may need to do some performance testing. For every service that
>> specifies an interface we'll now be checking the full type hierarchy which
>> I imagine will be more expensive but I'm not sure how expensive.
>>
>> 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.
>>
>> Regards
>> Scott
>>
>> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
>> wrote:
>>
>>> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>>>
>>>> I don't need to try anything, I *know* that the service engine is
>> supposed
>>>> to accept a concrete class of an interface if the interface is specified
>>>> as
>>>> the attribute type.
>>>>
>>>> Either the service engine is broken by not accepting concrete
>>>> implementations, or the bug report is incorrect.
>>>>
>>> Neither, it's "unfortunate", and a bit complicated.
>>>
>>> Tomcat servlet4preview was introduced with Tomcat 8.5
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/servlet4preview/package-summary.html
>>>
>>> Before James introduced Tomcat SSO, we had only one service passing a
>>> javax.servlet.http.HttpServletRequest to a service:
>> payPalCheckoutUpdate.
>>> AFAIK, we have actually always passed a org.apache.catalina.connector.
>> RequestFacade
>>> to services when asking for javax.servlet.http.HttpServletRequest in
>>> services definition.
>>> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.
>> HttpServletRequest
>>> indirectly through org.apache.catalina.servlet4pr
>>> eview.http.HttpServletRequest
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/connector/RequestFacade.html
>>>
>>> Since we have no tests on the payPalCheckoutUpdate service we did not
>> spot
>>> that Tomcat servlet4preview was introduced with Tomcat 8.5
>>>
>>> The classes check is done by the deeper interfaceOf() method of the
>>> ObjectType class using Class.getInterfaces()
>>> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
>>> html#getInterfaces--
>>> Class.getInterfaces() does not recurse and stops at one level up. So in
>>> case of RequestFacade it returns only servlet4preview.http.
>> HttpServletRequest
>>> and not javax.servlet.http.HttpServletRequest
>>> So when interfaceOf() compares the Classes it fails.
>>>
>>> What happened with my introduction of the HttpServletRequestWrapper in
>>> ContextFilter is it hid the RequestFacade because
>> HttpServletRequestWrapper
>>> implements javax.servlet.http.HttpServletRequest
>>> https://docs.oracle.com/javaee/7/api/javax/servlet/http/
>>> HttpServletRequest.html
>>>
>>> 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
>>>
>>> Summary: it's unfortunate because we have no tests on the
>>> payPalCheckoutUpdate service.
>>> Because I temporarily introduced HttpServletRequestWrapper James was able
>>> to pass a javax.servlet.http.HttpServletRequest, like in
>>> payPalCheckoutUpdate.
>>> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
>>> discovered that we had a problem with Tomcat 8.5.
>>>
>>> I propose this fix which uses org.apache.commons.lang3.
>> ClassUtils.getAllInterfaces()
>>> and works
>>> http://commons.apache.org/proper/commons-lang/javadocs/api-
>>> release/src-html/org/apache/commons/lang3/ClassUtils.html
>>>
>>> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
>>> ctType.java
>>> ===================================================================
>>> --- framework/base/src/main/java/org/apache/ofbiz/base/util/
>> ObjectType.java
>>> (révision 1827594)
>>> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/
>> ObjectType.java
>>> (copie de travail)
>>> @@ -263,7 +263,7 @@
>>> */
>>> public static boolean interfaceOf(Class<?> objectClass, Class<?>
>>> interfaceClass) {
>>> while (objectClass != null) {
>>> - Class<?>[] ifaces = objectClass.getInterfaces();
>>> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
>>> Utils.getAllInterfaces(objectClass);
>>>
>>> for (Class<?> iface: ifaces) {
>>> if (iface == interfaceClass) {
>>> Index: framework/common/servicedef/services.xml
>>> ===================================================================
>>> --- framework/common/servicedef/services.xml (révision 1827594)
>>> +++ framework/common/servicedef/services.xml (copie de travail)
>>> @@ -379,7 +379,7 @@
>>> <service name="userLogin" engine="java" location="
>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>> <description>Used to Automatically Authenticate a
>>> username/password; create a UserLogin object</description>
>>> <implements service="authenticationInterface"/>
>>> - <attribute name="request" mode="IN" type="org.apache.catalina.
>> connector.RequestFacade"
>>> optional="true"/>
>>> + <attribute name="request" mode="IN" type="javax.servlet.http.
>> HttpServletRequest"
>>> optional="true"/>
>>> </service>
>>> <service name="createUserLogin" engine="java" auth="false"
>>> location="org.apache.ofbiz.common.login.LoginServices"
>>> invoke="createUserLogin">
>>>
>>> Jacques
>>>
>>>
>>>
>>>
>>>> Regards
>>>> Scott
>>>>
>>>> On 23 March 2018 at 07:36, 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?
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>>>
>>>>> Something else must be wrong Jacques, I can't understand what you're
>>>>>> saying
>>>>>> in the ticket description or in your reply above but I do know the
>>>>>> following:
>>>>>> OFBiz is perfectly capable of knowing that
>>>>>> org.apache.catalina.connector.RequestFacade
>>>>>> implements the javax.servlet.http.HttpServletRequest and it should
>> pass
>>>>>> type validation without errors.
>>>>>>
>>>>>> Since we know that, this commit becomes unnecessary.
>>>>>>
>>>>>> Regards
>>>>>> Scott
>>>>>>
>>>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>>>> jacques.le.roux@les7arts.com>
>>>>>> wrote:
>>>>>>
>>>>>> Michael,
>>>>>>
>>>>>>> I commited http://svn.apache.org/viewvc/o
>>>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>>>
>>>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>>>> committed
>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>>>> pathrev=1819133
>>>>>>>
>>>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>>>> James
>>>>>>> would not have been allowed to commit
>>>>>>>
>>>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>>>> ervletRequest"
>>>>>>> optional="true"/>
>>>>>>>
>>>>>>> Because it's then rejected, because then the userLogin service
>> actually
>>>>>>> receive an org.apache.catalina.connector.RequestFacade
>> (RequestFacade
>>>>>>> implements HttpServletRequest.)
>>>>>>>
>>>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>>>> wrapper
>>>>>>> in ContextFilter.java.
>>>>>>>
>>>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>>>> tools
>>>>>>> like Jetty.
>>>>>>>
>>>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
>> shows.
>>>>>>> So
>>>>>>> I think it's safe to use a RequestFacade there.
>>>>>>>
>>>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>>>> think
>>>>>>> changing that would not be the most of the worries (the rest being
>> much
>>>>>>> more frightening, I know I did it once with Geronimo).
>>>>>>>
>>>>>>> Since the check is done at the service level, it's hard to do
>> otherwise
>>>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>>>> discuss that...
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>>>
>>>>>>> Mmhhh, Jacques,
>>>>>>>
>>>>>>>> I think this is problematic because it ties a special implementation
>>>>>>>> for
>>>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>>>
>>>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>>>> bit
>>>>>>>> unclear and I don't get the purpose of this change.
>>>>>>>>
>>>>>>>> Can you please explain more clearly which problem this changes
>> solves
>>>>>>>> and
>>>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>>>> type?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Michael
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>>>
>>>>>>>> Author: jleroux
>>>>>>>>
>>>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>>>> New Revision: 1827439
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>>>> Log:
>>>>>>>>> Fixed: The "request" attribute type of the userLogin service is
>> wrong
>>>>>>>>> (OFBIZ-10304)
>>>>>>>>>
>>>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>>>> details
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/
>> services.xml
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>>>> amework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>>>> &r2=1827439&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>>>> username/password; create a UserLogin object</description>
>>>>>>>>> <implements service="authenticationInterface"/>
>>>>>>>>> - <attribute name="request" mode="IN"
>>>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>>>> + <attribute name="request" mode="IN"
>>>>>>>>> type="org.apache.catalina.connector.RequestFacade"
>> optional="true"/>
>>>>>>>>> </service>
>>>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>>>> invoke="createUserLogin">
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
>>
>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Paul Foxworthy <pa...@cohsoft.com.au>.
Hi Jacques,
On 27 March 2018 at 08:16, Jacques Le Roux <ja...@les7arts.com>
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: +61 3 9585 6788 <+61%203%209585%206788>
Web: http://www.coherentsoftware.com.au/
Email: info@coherentsoftware.com.au
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Paul, All,
inline...
Le 25/03/2018 à 03:02, Paul Foxworthy a écrit :
> Hi all,
>
> The servlet4preview package in Tomcat 8.5 "provides early access to some of
> the features of Servlet 4.0" (
> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
> ).
>
> Does use of this package make OFBiz dependent on a preview version, and if
> so is that a good idea? Should features that use some preview (like support
> for Tomcat SSO, it appears) be contained in a branch or plugin until the
> API has been stabilised?
What makes you think that Tomcat SSO depends on servlet4preview?
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?
> Tomcat 9 was released in January, with support for Servlet 4.0. 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.
But after upgrading to Tomcat 8.5 at https://issues.apache.org/jira/browse/OFBIZ-9813 (we did not notice the possible issue with servlet4preview, I
must say it's not obvious in Tomcat 8.5 changelog: no warning) Michael tried to update to Tomcat 9 twice w/o success:
* https://issues.apache.org/jira/browse/OFBIZ-10026
* https://issues.apache.org/jira/browse/OFBIZ-10036
So, I think we need now to continue the work initiated by Michael to definitely close the related dependent issues (by using Tomcat 9):
* OFBIZ-10304 that made the problem to appear. Actually it was already existing for all services which were using standard HttpServletRequest or
HttpServletResponse as IN or OUT or IN/OUT attributes. Not much OOTB, but we don't know for users... Note that with OFBIZ-9813 R17.12 uses Tomcat
8.5. So it's not only trunk and we need to consider this a bug and update R17.12 as well.
* https://issues.apache.org/jira/browse/OFBIZ-10047 which IMO does not depend on Tomcat 8.5 but made me research and open OFBIZ-10304 after I could
not revert the HttpServletRequestWrapper at http://svn.apache.org/viewvc?view=revision&revision=1827441
* https://issues.apache.org/jira/browse/OFBIZ-10307 were I need to revert my previous work committed for
https://issues.apache.org/jira/browse/OFBIZ-9833 which was partially wrong.
I purposely put the 1st appearance of a link inside the text. I think it's the easiest way to follow a thought put in an email.
Jacques
>
> Thanks
>
> Paul Foxworthy
>
>
> On 25 March 2018 at 04:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
>
>> So to summarize your long email ;-)
>>
>> "The service engine has a limitation in that it only checks the interfaces
>> directly implemented by the object being tested. Tomcat's RequestFacade
>> doesn't directly implement javax.servlet.http.HttpServletRequest so it
>> fails to pass the type validation."
>>
>> On the surface your suggested fix looks fine to me, my only concern being
>> that we may need to do some performance testing. For every service that
>> specifies an interface we'll now be checking the full type hierarchy which
>> I imagine will be more expensive but I'm not sure how expensive.
>>
>> 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.
>>
>> Regards
>> Scott
>>
>> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
>> wrote:
>>
>>> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>>>
>>>> I don't need to try anything, I *know* that the service engine is
>> supposed
>>>> to accept a concrete class of an interface if the interface is specified
>>>> as
>>>> the attribute type.
>>>>
>>>> Either the service engine is broken by not accepting concrete
>>>> implementations, or the bug report is incorrect.
>>>>
>>> Neither, it's "unfortunate", and a bit complicated.
>>>
>>> Tomcat servlet4preview was introduced with Tomcat 8.5
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/servlet4preview/package-summary.html
>>>
>>> Before James introduced Tomcat SSO, we had only one service passing a
>>> javax.servlet.http.HttpServletRequest to a service:
>> payPalCheckoutUpdate.
>>> AFAIK, we have actually always passed a org.apache.catalina.connector.
>> RequestFacade
>>> to services when asking for javax.servlet.http.HttpServletRequest in
>>> services definition.
>>> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.
>> HttpServletRequest
>>> indirectly through org.apache.catalina.servlet4pr
>>> eview.http.HttpServletRequest
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/connector/RequestFacade.html
>>>
>>> Since we have no tests on the payPalCheckoutUpdate service we did not
>> spot
>>> that Tomcat servlet4preview was introduced with Tomcat 8.5
>>>
>>> The classes check is done by the deeper interfaceOf() method of the
>>> ObjectType class using Class.getInterfaces()
>>> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
>>> html#getInterfaces--
>>> Class.getInterfaces() does not recurse and stops at one level up. So in
>>> case of RequestFacade it returns only servlet4preview.http.
>> HttpServletRequest
>>> and not javax.servlet.http.HttpServletRequest
>>> So when interfaceOf() compares the Classes it fails.
>>>
>>> What happened with my introduction of the HttpServletRequestWrapper in
>>> ContextFilter is it hid the RequestFacade because
>> HttpServletRequestWrapper
>>> implements javax.servlet.http.HttpServletRequest
>>> https://docs.oracle.com/javaee/7/api/javax/servlet/http/
>>> HttpServletRequest.html
>>>
>>> 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
>>>
>>> Summary: it's unfortunate because we have no tests on the
>>> payPalCheckoutUpdate service.
>>> Because I temporarily introduced HttpServletRequestWrapper James was able
>>> to pass a javax.servlet.http.HttpServletRequest, like in
>>> payPalCheckoutUpdate.
>>> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
>>> discovered that we had a problem with Tomcat 8.5.
>>>
>>> I propose this fix which uses org.apache.commons.lang3.
>> ClassUtils.getAllInterfaces()
>>> and works
>>> http://commons.apache.org/proper/commons-lang/javadocs/api-
>>> release/src-html/org/apache/commons/lang3/ClassUtils.html
>>>
>>> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
>>> ctType.java
>>> ===================================================================
>>> --- framework/base/src/main/java/org/apache/ofbiz/base/util/
>> ObjectType.java
>>> (révision 1827594)
>>> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/
>> ObjectType.java
>>> (copie de travail)
>>> @@ -263,7 +263,7 @@
>>> */
>>> public static boolean interfaceOf(Class<?> objectClass, Class<?>
>>> interfaceClass) {
>>> while (objectClass != null) {
>>> - Class<?>[] ifaces = objectClass.getInterfaces();
>>> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
>>> Utils.getAllInterfaces(objectClass);
>>>
>>> for (Class<?> iface: ifaces) {
>>> if (iface == interfaceClass) {
>>> Index: framework/common/servicedef/services.xml
>>> ===================================================================
>>> --- framework/common/servicedef/services.xml (révision 1827594)
>>> +++ framework/common/servicedef/services.xml (copie de travail)
>>> @@ -379,7 +379,7 @@
>>> <service name="userLogin" engine="java" location="
>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>> <description>Used to Automatically Authenticate a
>>> username/password; create a UserLogin object</description>
>>> <implements service="authenticationInterface"/>
>>> - <attribute name="request" mode="IN" type="org.apache.catalina.
>> connector.RequestFacade"
>>> optional="true"/>
>>> + <attribute name="request" mode="IN" type="javax.servlet.http.
>> HttpServletRequest"
>>> optional="true"/>
>>> </service>
>>> <service name="createUserLogin" engine="java" auth="false"
>>> location="org.apache.ofbiz.common.login.LoginServices"
>>> invoke="createUserLogin">
>>>
>>> Jacques
>>>
>>>
>>>
>>>
>>>> Regards
>>>> Scott
>>>>
>>>> On 23 March 2018 at 07:36, 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?
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>>>
>>>>> Something else must be wrong Jacques, I can't understand what you're
>>>>>> saying
>>>>>> in the ticket description or in your reply above but I do know the
>>>>>> following:
>>>>>> OFBiz is perfectly capable of knowing that
>>>>>> org.apache.catalina.connector.RequestFacade
>>>>>> implements the javax.servlet.http.HttpServletRequest and it should
>> pass
>>>>>> type validation without errors.
>>>>>>
>>>>>> Since we know that, this commit becomes unnecessary.
>>>>>>
>>>>>> Regards
>>>>>> Scott
>>>>>>
>>>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>>>> jacques.le.roux@les7arts.com>
>>>>>> wrote:
>>>>>>
>>>>>> Michael,
>>>>>>
>>>>>>> I commited http://svn.apache.org/viewvc/o
>>>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>>>
>>>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>>>> committed
>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>>>> pathrev=1819133
>>>>>>>
>>>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>>>> James
>>>>>>> would not have been allowed to commit
>>>>>>>
>>>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>>>> ervletRequest"
>>>>>>> optional="true"/>
>>>>>>>
>>>>>>> Because it's then rejected, because then the userLogin service
>> actually
>>>>>>> receive an org.apache.catalina.connector.RequestFacade
>> (RequestFacade
>>>>>>> implements HttpServletRequest.)
>>>>>>>
>>>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>>>> wrapper
>>>>>>> in ContextFilter.java.
>>>>>>>
>>>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>>>> tools
>>>>>>> like Jetty.
>>>>>>>
>>>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
>> shows.
>>>>>>> So
>>>>>>> I think it's safe to use a RequestFacade there.
>>>>>>>
>>>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>>>> think
>>>>>>> changing that would not be the most of the worries (the rest being
>> much
>>>>>>> more frightening, I know I did it once with Geronimo).
>>>>>>>
>>>>>>> Since the check is done at the service level, it's hard to do
>> otherwise
>>>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>>>> discuss that...
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>>>
>>>>>>> Mmhhh, Jacques,
>>>>>>>
>>>>>>>> I think this is problematic because it ties a special implementation
>>>>>>>> for
>>>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>>>
>>>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>>>> bit
>>>>>>>> unclear and I don't get the purpose of this change.
>>>>>>>>
>>>>>>>> Can you please explain more clearly which problem this changes
>> solves
>>>>>>>> and
>>>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>>>> type?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Michael
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>>>
>>>>>>>> Author: jleroux
>>>>>>>>
>>>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>>>> New Revision: 1827439
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>>>> Log:
>>>>>>>>> Fixed: The "request" attribute type of the userLogin service is
>> wrong
>>>>>>>>> (OFBIZ-10304)
>>>>>>>>>
>>>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>>>> details
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/
>> services.xml
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>>>> amework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>>>> &r2=1827439&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>>>> username/password; create a UserLogin object</description>
>>>>>>>>> <implements service="authenticationInterface"/>
>>>>>>>>> - <attribute name="request" mode="IN"
>>>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>>>> + <attribute name="request" mode="IN"
>>>>>>>>> type="org.apache.catalina.connector.RequestFacade"
>> optional="true"/>
>>>>>>>>> </service>
>>>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>>>> invoke="createUserLogin">
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>
>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Paul Foxworthy <pa...@cohsoft.com.au>.
Hi all,
The servlet4preview package in Tomcat 8.5 "provides early access to some of
the features of Servlet 4.0" (
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
).
Does use of this package make OFBiz dependent on a preview version, and if
so is that a good idea? Should features that use some preview (like support
for Tomcat SSO, it appears) be contained in a branch or plugin until the
API has been stabilised?
Tomcat 9 was released in January, with support for Servlet 4.0. If we now
say OFBiz requires Servlet 4.0 and move to Tomcat 9, could we then use the
standard HttpServletRequest?
Thanks
Paul Foxworthy
On 25 March 2018 at 04:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
> So to summarize your long email ;-)
>
> "The service engine has a limitation in that it only checks the interfaces
> directly implemented by the object being tested. Tomcat's RequestFacade
> doesn't directly implement javax.servlet.http.HttpServletRequest so it
> fails to pass the type validation."
>
> On the surface your suggested fix looks fine to me, my only concern being
> that we may need to do some performance testing. For every service that
> specifies an interface we'll now be checking the full type hierarchy which
> I imagine will be more expensive but I'm not sure how expensive.
>
> 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.
>
> Regards
> Scott
>
> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
> wrote:
>
> > Le 23/03/2018 à 17:09, Scott Gray a écrit :
> >
> >> I don't need to try anything, I *know* that the service engine is
> supposed
> >> to accept a concrete class of an interface if the interface is specified
> >> as
> >> the attribute type.
> >>
> >> Either the service engine is broken by not accepting concrete
> >> implementations, or the bug report is incorrect.
> >>
> > Neither, it's "unfortunate", and a bit complicated.
> >
> > Tomcat servlet4preview was introduced with Tomcat 8.5
> > https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
> > lina/servlet4preview/package-summary.html
> >
> > Before James introduced Tomcat SSO, we had only one service passing a
> > javax.servlet.http.HttpServletRequest to a service:
> payPalCheckoutUpdate.
> >
> > AFAIK, we have actually always passed a org.apache.catalina.connector.
> RequestFacade
> > to services when asking for javax.servlet.http.HttpServletRequest in
> > services definition.
> > Since Tomcat 8.5 RequestFacade implements javax.servlet.http.
> HttpServletRequest
> > indirectly through org.apache.catalina.servlet4pr
> > eview.http.HttpServletRequest
> > https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
> > lina/connector/RequestFacade.html
> >
> > Since we have no tests on the payPalCheckoutUpdate service we did not
> spot
> > that Tomcat servlet4preview was introduced with Tomcat 8.5
> >
> > The classes check is done by the deeper interfaceOf() method of the
> > ObjectType class using Class.getInterfaces()
> > https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
> > html#getInterfaces--
> > Class.getInterfaces() does not recurse and stops at one level up. So in
> > case of RequestFacade it returns only servlet4preview.http.
> HttpServletRequest
> > and not javax.servlet.http.HttpServletRequest
> > So when interfaceOf() compares the Classes it fails.
> >
> > What happened with my introduction of the HttpServletRequestWrapper in
> > ContextFilter is it hid the RequestFacade because
> HttpServletRequestWrapper
> > implements javax.servlet.http.HttpServletRequest
> > https://docs.oracle.com/javaee/7/api/javax/servlet/http/
> > HttpServletRequest.html
> >
> > 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
> >
> > Summary: it's unfortunate because we have no tests on the
> > payPalCheckoutUpdate service.
> > Because I temporarily introduced HttpServletRequestWrapper James was able
> > to pass a javax.servlet.http.HttpServletRequest, like in
> > payPalCheckoutUpdate.
> > When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
> > discovered that we had a problem with Tomcat 8.5.
> >
> > I propose this fix which uses org.apache.commons.lang3.
> ClassUtils.getAllInterfaces()
> > and works
> > http://commons.apache.org/proper/commons-lang/javadocs/api-
> > release/src-html/org/apache/commons/lang3/ClassUtils.html
> >
> > Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
> > ctType.java
> > ===================================================================
> > --- framework/base/src/main/java/org/apache/ofbiz/base/util/
> ObjectType.java
> > (révision 1827594)
> > +++ framework/base/src/main/java/org/apache/ofbiz/base/util/
> ObjectType.java
> > (copie de travail)
> > @@ -263,7 +263,7 @@
> > */
> > public static boolean interfaceOf(Class<?> objectClass, Class<?>
> > interfaceClass) {
> > while (objectClass != null) {
> > - Class<?>[] ifaces = objectClass.getInterfaces();
> > + List<Class<?>> ifaces = org.apache.commons.lang3.Class
> > Utils.getAllInterfaces(objectClass);
> >
> > for (Class<?> iface: ifaces) {
> > if (iface == interfaceClass) {
> > Index: framework/common/servicedef/services.xml
> > ===================================================================
> > --- framework/common/servicedef/services.xml (révision 1827594)
> > +++ framework/common/servicedef/services.xml (copie de travail)
> > @@ -379,7 +379,7 @@
> > <service name="userLogin" engine="java" location="
> > org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
> > <description>Used to Automatically Authenticate a
> > username/password; create a UserLogin object</description>
> > <implements service="authenticationInterface"/>
> > - <attribute name="request" mode="IN" type="org.apache.catalina.
> connector.RequestFacade"
> > optional="true"/>
> > + <attribute name="request" mode="IN" type="javax.servlet.http.
> HttpServletRequest"
> > optional="true"/>
> > </service>
> > <service name="createUserLogin" engine="java" auth="false"
> > location="org.apache.ofbiz.common.login.LoginServices"
> > invoke="createUserLogin">
> >
> > Jacques
> >
> >
> >
> >
> >> Regards
> >> Scott
> >>
> >> On 23 March 2018 at 07:36, 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?
> >>>
> >>> Jacques
> >>>
> >>>
> >>>
> >>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
> >>>
> >>> Something else must be wrong Jacques, I can't understand what you're
> >>>> saying
> >>>> in the ticket description or in your reply above but I do know the
> >>>> following:
> >>>> OFBiz is perfectly capable of knowing that
> >>>> org.apache.catalina.connector.RequestFacade
> >>>> implements the javax.servlet.http.HttpServletRequest and it should
> pass
> >>>> type validation without errors.
> >>>>
> >>>> Since we know that, this commit becomes unnecessary.
> >>>>
> >>>> Regards
> >>>> Scott
> >>>>
> >>>> On 22 March 2018 at 16:06, Jacques Le Roux <
> >>>> jacques.le.roux@les7arts.com>
> >>>> wrote:
> >>>>
> >>>> Michael,
> >>>>
> >>>>> I commited http://svn.apache.org/viewvc/o
> >>>>> fbiz/ofbiz-framework/trunk/fra
> >>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
> >>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
> >>>>>
> >>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
> >>>>> committed
> >>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> >>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
> >>>>> pathrev=1819133
> >>>>>
> >>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
> >>>>> James
> >>>>> would not have been allowed to commit
> >>>>>
> >>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
> >>>>> ervletRequest"
> >>>>> optional="true"/>
> >>>>>
> >>>>> Because it's then rejected, because then the userLogin service
> actually
> >>>>> receive an org.apache.catalina.connector.RequestFacade
> (RequestFacade
> >>>>> implements HttpServletRequest.)
> >>>>>
> >>>>> You can easily check by svn updating to r1819133 and removing the
> >>>>> wrapper
> >>>>> in ContextFilter.java.
> >>>>>
> >>>>> You are right that it ties OFBiz to Tomcat. We took this decision
> >>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
> >>>>> tools
> >>>>> like Jetty.
> >>>>>
> >>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
> shows.
> >>>>> So
> >>>>> I think it's safe to use a RequestFacade there.
> >>>>>
> >>>>> If an user feels the need to use another webapp servers or Jetty, I
> >>>>> think
> >>>>> changing that would not be the most of the worries (the rest being
> much
> >>>>> more frightening, I know I did it once with Geronimo).
> >>>>>
> >>>>> Since the check is done at the service level, it's hard to do
> otherwise
> >>>>> But I must say I did not dig too much and it's maybe possible, we can
> >>>>> discuss that...
> >>>>>
> >>>>> Jacques
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
> >>>>>
> >>>>> Mmhhh, Jacques,
> >>>>>
> >>>>>> I think this is problematic because it ties a special implementation
> >>>>>> for
> >>>>>> Tomcat to the service. I didn't see this anywhere else.
> >>>>>>
> >>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
> >>>>>> bit
> >>>>>> unclear and I don't get the purpose of this change.
> >>>>>>
> >>>>>> Can you please explain more clearly which problem this changes
> solves
> >>>>>> and
> >>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
> >>>>>> type?
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Michael
> >>>>>>
> >>>>>>
> >>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
> >>>>>>
> >>>>>> Author: jleroux
> >>>>>>
> >>>>>>> Date: Wed Mar 21 20:53:41 2018
> >>>>>>> New Revision: 1827439
> >>>>>>>
> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
> >>>>>>> Log:
> >>>>>>> Fixed: The "request" attribute type of the userLogin service is
> wrong
> >>>>>>> (OFBIZ-10304)
> >>>>>>>
> >>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
> >>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
> >>>>>>> details
> >>>>>>>
> >>>>>>> Modified:
> >>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/
> services.xml
> >>>>>>>
> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
> >>>>>>> amework/common/servicedef/serv
> >>>>>>> ices.xml
> >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> >>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
> >>>>>>> &r2=1827439&view=diff
> >>>>>>> ============================================================
> >>>>>>> ==================
> >>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
> >>>>>>> ices.xml
> >>>>>>> (original)
> >>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
> >>>>>>> ices.xml
> >>>>>>> Wed Mar 21 20:53:41 2018
> >>>>>>> @@ -379,7 +379,7 @@ under the License.
> >>>>>>> <service name="userLogin" engine="java" location="
> >>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
> >>>>>>> <description>Used to Automatically Authenticate a
> >>>>>>> username/password; create a UserLogin object</description>
> >>>>>>> <implements service="authenticationInterface"/>
> >>>>>>> - <attribute name="request" mode="IN"
> >>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
> >>>>>>> + <attribute name="request" mode="IN"
> >>>>>>> type="org.apache.catalina.connector.RequestFacade"
> optional="true"/>
> >>>>>>> </service>
> >>>>>>> <service name="createUserLogin" engine="java" auth="false"
> >>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
> >>>>>>> invoke="createUserLogin">
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >
>
--
Coherent Software Australia Pty Ltd
PO Box 2773
Cheltenham Vic 3192
Australia
Phone: +61 3 9585 6788
Web: http://www.coherentsoftware.com.au/
Email: info@coherentsoftware.com.au
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
That's indeed another option, but I'd prefer to update to Tomcat 9, to definitely get ride of servlet4preview and problems it brings
Jacques
Le 25/03/2018 à 03:11, Scott Gray a écrit :
> Another option if we don't want to have to care about this service engine
> limitation right now is the type-validation child element of attribute.
>
> Just specify a class/method that takes the HttpServletRequest as an Object
> parameter and then perform the "getAllInterfaces()" test inside there.
> e.g.
> UtilValidate.isHttpServletRequest(Object request) {
> // do the new test here
> }
>
> In the service definition:
> <attribute name="request" mode="IN"
> type="javax.servlet.http.HttpServletRequest"
> optional="true">
> <type-validate class="org.apache.ofbiz.base.util.UtilValidate"
> method="isHttpServletRequest"/>
> </attribute>
>
> That would solve the problem for now until the service engine is improved
> and tested or the tomcat SSO login design is improved.
>
> Regards
> Scott
>
> On 24 March 2018 at 17:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
>
>> So to summarize your long email ;-)
>>
>> "The service engine has a limitation in that it only checks the interfaces
>> directly implemented by the object being tested. Tomcat's RequestFacade
>> doesn't directly implement javax.servlet.http.HttpServletRequest so it
>> fails to pass the type validation."
>>
>> On the surface your suggested fix looks fine to me, my only concern being
>> that we may need to do some performance testing. For every service that
>> specifies an interface we'll now be checking the full type hierarchy which
>> I imagine will be more expensive but I'm not sure how expensive.
>>
>> 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.
>>
>> Regards
>> Scott
>>
>> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
>> wrote:
>>
>>> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>>>
>>>> I don't need to try anything, I *know* that the service engine is
>>>> supposed
>>>> to accept a concrete class of an interface if the interface is specified
>>>> as
>>>> the attribute type.
>>>>
>>>> Either the service engine is broken by not accepting concrete
>>>> implementations, or the bug report is incorrect.
>>>>
>>> Neither, it's "unfortunate", and a bit complicated.
>>>
>>> Tomcat servlet4preview was introduced with Tomcat 8.5
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/servlet4preview/package-summary.html
>>>
>>> Before James introduced Tomcat SSO, we had only one service passing a
>>> javax.servlet.http.HttpServletRequest to a service: payPalCheckoutUpdate.
>>>
>>> AFAIK, we have actually always passed a org.apache.catalina.connector.RequestFacade
>>> to services when asking for javax.servlet.http.HttpServletRequest in
>>> services definition.
>>> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.HttpServletRequest
>>> indirectly through org.apache.catalina.servlet4pr
>>> eview.http.HttpServletRequest
>>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>>> lina/connector/RequestFacade.html
>>>
>>> Since we have no tests on the payPalCheckoutUpdate service we did not
>>> spot that Tomcat servlet4preview was introduced with Tomcat 8.5
>>>
>>> The classes check is done by the deeper interfaceOf() method of the
>>> ObjectType class using Class.getInterfaces()
>>> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.ht
>>> ml#getInterfaces--
>>> Class.getInterfaces() does not recurse and stops at one level up. So in
>>> case of RequestFacade it returns only servlet4preview.http.HttpServletRequest
>>> and not javax.servlet.http.HttpServletRequest
>>> So when interfaceOf() compares the Classes it fails.
>>>
>>> What happened with my introduction of the HttpServletRequestWrapper in
>>> ContextFilter is it hid the RequestFacade because HttpServletRequestWrapper
>>> implements javax.servlet.http.HttpServletRequest
>>> https://docs.oracle.com/javaee/7/api/javax/servlet/http/Http
>>> ServletRequest.html
>>>
>>> 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
>>>
>>> Summary: it's unfortunate because we have no tests on the
>>> payPalCheckoutUpdate service.
>>> Because I temporarily introduced HttpServletRequestWrapper James was able
>>> to pass a javax.servlet.http.HttpServletRequest, like in
>>> payPalCheckoutUpdate.
>>> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
>>> discovered that we had a problem with Tomcat 8.5.
>>>
>>> I propose this fix which uses org.apache.commons.lang3.ClassUtils.getAllInterfaces()
>>> and works
>>> http://commons.apache.org/proper/commons-lang/javadocs/api-r
>>> elease/src-html/org/apache/commons/lang3/ClassUtils.html
>>>
>>> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
>>> ctType.java
>>> ===================================================================
>>> --- framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
>>> (révision 1827594)
>>> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
>>> (copie de travail)
>>> @@ -263,7 +263,7 @@
>>> */
>>> public static boolean interfaceOf(Class<?> objectClass, Class<?>
>>> interfaceClass) {
>>> while (objectClass != null) {
>>> - Class<?>[] ifaces = objectClass.getInterfaces();
>>> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
>>> Utils.getAllInterfaces(objectClass);
>>>
>>> for (Class<?> iface: ifaces) {
>>> if (iface == interfaceClass) {
>>> Index: framework/common/servicedef/services.xml
>>> ===================================================================
>>> --- framework/common/servicedef/services.xml (révision 1827594)
>>> +++ framework/common/servicedef/services.xml (copie de travail)
>>> @@ -379,7 +379,7 @@
>>> <service name="userLogin" engine="java" location="
>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>> <description>Used to Automatically Authenticate a
>>> username/password; create a UserLogin object</description>
>>> <implements service="authenticationInterface"/>
>>> - <attribute name="request" mode="IN"
>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>> + <attribute name="request" mode="IN"
>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>> </service>
>>> <service name="createUserLogin" engine="java" auth="false"
>>> location="org.apache.ofbiz.common.login.LoginServices"
>>> invoke="createUserLogin">
>>>
>>> Jacques
>>>
>>>
>>>
>>>
>>>> Regards
>>>> Scott
>>>>
>>>> On 23 March 2018 at 07:36, 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?
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>>>
>>>>> Something else must be wrong Jacques, I can't understand what you're
>>>>>> saying
>>>>>> in the ticket description or in your reply above but I do know the
>>>>>> following:
>>>>>> OFBiz is perfectly capable of knowing that
>>>>>> org.apache.catalina.connector.RequestFacade
>>>>>> implements the javax.servlet.http.HttpServletRequest and it should
>>>>>> pass
>>>>>> type validation without errors.
>>>>>>
>>>>>> Since we know that, this commit becomes unnecessary.
>>>>>>
>>>>>> Regards
>>>>>> Scott
>>>>>>
>>>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>>>> jacques.le.roux@les7arts.com>
>>>>>> wrote:
>>>>>>
>>>>>> Michael,
>>>>>>
>>>>>>> I commited http://svn.apache.org/viewvc/o
>>>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>>>
>>>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>>>> committed
>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>>>> pathrev=1819133
>>>>>>>
>>>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>>>> James
>>>>>>> would not have been allowed to commit
>>>>>>>
>>>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>>>> ervletRequest"
>>>>>>> optional="true"/>
>>>>>>>
>>>>>>> Because it's then rejected, because then the userLogin service
>>>>>>> actually
>>>>>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>>>>>> implements HttpServletRequest.)
>>>>>>>
>>>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>>>> wrapper
>>>>>>> in ContextFilter.java.
>>>>>>>
>>>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>>>> tools
>>>>>>> like Jetty.
>>>>>>>
>>>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
>>>>>>> shows.
>>>>>>> So
>>>>>>> I think it's safe to use a RequestFacade there.
>>>>>>>
>>>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>>>> think
>>>>>>> changing that would not be the most of the worries (the rest being
>>>>>>> much
>>>>>>> more frightening, I know I did it once with Geronimo).
>>>>>>>
>>>>>>> Since the check is done at the service level, it's hard to do
>>>>>>> otherwise
>>>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>>>> discuss that...
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>>>
>>>>>>> Mmhhh, Jacques,
>>>>>>>
>>>>>>>> I think this is problematic because it ties a special implementation
>>>>>>>> for
>>>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>>>
>>>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>>>> bit
>>>>>>>> unclear and I don't get the purpose of this change.
>>>>>>>>
>>>>>>>> Can you please explain more clearly which problem this changes solves
>>>>>>>> and
>>>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>>>> type?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Michael
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>>>
>>>>>>>> Author: jleroux
>>>>>>>>
>>>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>>>> New Revision: 1827439
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>>>> Log:
>>>>>>>>> Fixed: The "request" attribute type of the userLogin service is
>>>>>>>>> wrong
>>>>>>>>> (OFBIZ-10304)
>>>>>>>>>
>>>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>>>> details
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>>
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>>>> amework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>>>> &r2=1827439&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>>> ices.xml
>>>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>>>> username/password; create a UserLogin object</description>
>>>>>>>>> <implements service="authenticationInterface"/>
>>>>>>>>> - <attribute name="request" mode="IN"
>>>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>>>> + <attribute name="request" mode="IN"
>>>>>>>>> type="org.apache.catalina.connector.RequestFacade"
>>>>>>>>> optional="true"/>
>>>>>>>>> </service>
>>>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>>>> invoke="createUserLogin">
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Scott Gray <sc...@hotwaxsystems.com>.
Another option if we don't want to have to care about this service engine
limitation right now is the type-validation child element of attribute.
Just specify a class/method that takes the HttpServletRequest as an Object
parameter and then perform the "getAllInterfaces()" test inside there.
e.g.
UtilValidate.isHttpServletRequest(Object request) {
// do the new test here
}
In the service definition:
<attribute name="request" mode="IN"
type="javax.servlet.http.HttpServletRequest"
optional="true">
<type-validate class="org.apache.ofbiz.base.util.UtilValidate"
method="isHttpServletRequest"/>
</attribute>
That would solve the problem for now until the service engine is improved
and tested or the tomcat SSO login design is improved.
Regards
Scott
On 24 March 2018 at 17:49, Scott Gray <sc...@hotwaxsystems.com> wrote:
> So to summarize your long email ;-)
>
> "The service engine has a limitation in that it only checks the interfaces
> directly implemented by the object being tested. Tomcat's RequestFacade
> doesn't directly implement javax.servlet.http.HttpServletRequest so it
> fails to pass the type validation."
>
> On the surface your suggested fix looks fine to me, my only concern being
> that we may need to do some performance testing. For every service that
> specifies an interface we'll now be checking the full type hierarchy which
> I imagine will be more expensive but I'm not sure how expensive.
>
> 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.
>
> Regards
> Scott
>
> On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
> wrote:
>
>> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>>
>>> I don't need to try anything, I *know* that the service engine is
>>> supposed
>>> to accept a concrete class of an interface if the interface is specified
>>> as
>>> the attribute type.
>>>
>>> Either the service engine is broken by not accepting concrete
>>> implementations, or the bug report is incorrect.
>>>
>> Neither, it's "unfortunate", and a bit complicated.
>>
>> Tomcat servlet4preview was introduced with Tomcat 8.5
>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>> lina/servlet4preview/package-summary.html
>>
>> Before James introduced Tomcat SSO, we had only one service passing a
>> javax.servlet.http.HttpServletRequest to a service: payPalCheckoutUpdate.
>>
>> AFAIK, we have actually always passed a org.apache.catalina.connector.RequestFacade
>> to services when asking for javax.servlet.http.HttpServletRequest in
>> services definition.
>> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.HttpServletRequest
>> indirectly through org.apache.catalina.servlet4pr
>> eview.http.HttpServletRequest
>> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
>> lina/connector/RequestFacade.html
>>
>> Since we have no tests on the payPalCheckoutUpdate service we did not
>> spot that Tomcat servlet4preview was introduced with Tomcat 8.5
>>
>> The classes check is done by the deeper interfaceOf() method of the
>> ObjectType class using Class.getInterfaces()
>> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.ht
>> ml#getInterfaces--
>> Class.getInterfaces() does not recurse and stops at one level up. So in
>> case of RequestFacade it returns only servlet4preview.http.HttpServletRequest
>> and not javax.servlet.http.HttpServletRequest
>> So when interfaceOf() compares the Classes it fails.
>>
>> What happened with my introduction of the HttpServletRequestWrapper in
>> ContextFilter is it hid the RequestFacade because HttpServletRequestWrapper
>> implements javax.servlet.http.HttpServletRequest
>> https://docs.oracle.com/javaee/7/api/javax/servlet/http/Http
>> ServletRequest.html
>>
>> 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
>>
>> Summary: it's unfortunate because we have no tests on the
>> payPalCheckoutUpdate service.
>> Because I temporarily introduced HttpServletRequestWrapper James was able
>> to pass a javax.servlet.http.HttpServletRequest, like in
>> payPalCheckoutUpdate.
>> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
>> discovered that we had a problem with Tomcat 8.5.
>>
>> I propose this fix which uses org.apache.commons.lang3.ClassUtils.getAllInterfaces()
>> and works
>> http://commons.apache.org/proper/commons-lang/javadocs/api-r
>> elease/src-html/org/apache/commons/lang3/ClassUtils.html
>>
>> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
>> ctType.java
>> ===================================================================
>> --- framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
>> (révision 1827594)
>> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
>> (copie de travail)
>> @@ -263,7 +263,7 @@
>> */
>> public static boolean interfaceOf(Class<?> objectClass, Class<?>
>> interfaceClass) {
>> while (objectClass != null) {
>> - Class<?>[] ifaces = objectClass.getInterfaces();
>> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
>> Utils.getAllInterfaces(objectClass);
>>
>> for (Class<?> iface: ifaces) {
>> if (iface == interfaceClass) {
>> Index: framework/common/servicedef/services.xml
>> ===================================================================
>> --- framework/common/servicedef/services.xml (révision 1827594)
>> +++ framework/common/servicedef/services.xml (copie de travail)
>> @@ -379,7 +379,7 @@
>> <service name="userLogin" engine="java" location="
>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>> <description>Used to Automatically Authenticate a
>> username/password; create a UserLogin object</description>
>> <implements service="authenticationInterface"/>
>> - <attribute name="request" mode="IN"
>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>> + <attribute name="request" mode="IN"
>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>> </service>
>> <service name="createUserLogin" engine="java" auth="false"
>> location="org.apache.ofbiz.common.login.LoginServices"
>> invoke="createUserLogin">
>>
>> Jacques
>>
>>
>>
>>
>>> Regards
>>> Scott
>>>
>>> On 23 March 2018 at 07:36, 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?
>>>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>>
>>>> Something else must be wrong Jacques, I can't understand what you're
>>>>> saying
>>>>> in the ticket description or in your reply above but I do know the
>>>>> following:
>>>>> OFBiz is perfectly capable of knowing that
>>>>> org.apache.catalina.connector.RequestFacade
>>>>> implements the javax.servlet.http.HttpServletRequest and it should
>>>>> pass
>>>>> type validation without errors.
>>>>>
>>>>> Since we know that, this commit becomes unnecessary.
>>>>>
>>>>> Regards
>>>>> Scott
>>>>>
>>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>>> jacques.le.roux@les7arts.com>
>>>>> wrote:
>>>>>
>>>>> Michael,
>>>>>
>>>>>> I commited http://svn.apache.org/viewvc/o
>>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>>
>>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>>> committed
>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>>> pathrev=1819133
>>>>>>
>>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>>> James
>>>>>> would not have been allowed to commit
>>>>>>
>>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>>> ervletRequest"
>>>>>> optional="true"/>
>>>>>>
>>>>>> Because it's then rejected, because then the userLogin service
>>>>>> actually
>>>>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>>>>> implements HttpServletRequest.)
>>>>>>
>>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>>> wrapper
>>>>>> in ContextFilter.java.
>>>>>>
>>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>>> tools
>>>>>> like Jetty.
>>>>>>
>>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file
>>>>>> shows.
>>>>>> So
>>>>>> I think it's safe to use a RequestFacade there.
>>>>>>
>>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>>> think
>>>>>> changing that would not be the most of the worries (the rest being
>>>>>> much
>>>>>> more frightening, I know I did it once with Geronimo).
>>>>>>
>>>>>> Since the check is done at the service level, it's hard to do
>>>>>> otherwise
>>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>>> discuss that...
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>>
>>>>>> Mmhhh, Jacques,
>>>>>>
>>>>>>> I think this is problematic because it ties a special implementation
>>>>>>> for
>>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>>
>>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>>> bit
>>>>>>> unclear and I don't get the purpose of this change.
>>>>>>>
>>>>>>> Can you please explain more clearly which problem this changes solves
>>>>>>> and
>>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>>> type?
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Michael
>>>>>>>
>>>>>>>
>>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>>
>>>>>>> Author: jleroux
>>>>>>>
>>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>>> New Revision: 1827439
>>>>>>>>
>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>>> Log:
>>>>>>>> Fixed: The "request" attribute type of the userLogin service is
>>>>>>>> wrong
>>>>>>>> (OFBIZ-10304)
>>>>>>>>
>>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>>> details
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>> ices.xml
>>>>>>>>
>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>>> amework/common/servicedef/serv
>>>>>>>> ices.xml
>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>>> &r2=1827439&view=diff
>>>>>>>> ============================================================
>>>>>>>> ==================
>>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>> ices.xml
>>>>>>>> (original)
>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>>> ices.xml
>>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>>> username/password; create a UserLogin object</description>
>>>>>>>> <implements service="authenticationInterface"/>
>>>>>>>> - <attribute name="request" mode="IN"
>>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>>> + <attribute name="request" mode="IN"
>>>>>>>> type="org.apache.catalina.connector.RequestFacade"
>>>>>>>> optional="true"/>
>>>>>>>> </service>
>>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>>> invoke="createUserLogin">
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <ja...@les7arts.com>
> 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
>>
>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Scott Gray <sc...@hotwaxsystems.com>.
Yeah, I said exactly that elsewhere in the same email.
Regards
Scott
On 26 March 2018 at 21:17, Jacques Le Roux <ja...@les7arts.com>
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
>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Scott Gray <sc...@hotwaxsystems.com>.
So to summarize your long email ;-)
"The service engine has a limitation in that it only checks the interfaces
directly implemented by the object being tested. Tomcat's RequestFacade
doesn't directly implement javax.servlet.http.HttpServletRequest so it
fails to pass the type validation."
On the surface your suggested fix looks fine to me, my only concern being
that we may need to do some performance testing. For every service that
specifies an interface we'll now be checking the full type hierarchy which
I imagine will be more expensive but I'm not sure how expensive.
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.
Regards
Scott
On 24 March 2018 at 10:59, Jacques Le Roux <ja...@les7arts.com>
wrote:
> Le 23/03/2018 à 17:09, Scott Gray a écrit :
>
>> I don't need to try anything, I *know* that the service engine is supposed
>> to accept a concrete class of an interface if the interface is specified
>> as
>> the attribute type.
>>
>> Either the service engine is broken by not accepting concrete
>> implementations, or the bug report is incorrect.
>>
> Neither, it's "unfortunate", and a bit complicated.
>
> Tomcat servlet4preview was introduced with Tomcat 8.5
> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
> lina/servlet4preview/package-summary.html
>
> Before James introduced Tomcat SSO, we had only one service passing a
> javax.servlet.http.HttpServletRequest to a service: payPalCheckoutUpdate.
>
> AFAIK, we have actually always passed a org.apache.catalina.connector.RequestFacade
> to services when asking for javax.servlet.http.HttpServletRequest in
> services definition.
> Since Tomcat 8.5 RequestFacade implements javax.servlet.http.HttpServletRequest
> indirectly through org.apache.catalina.servlet4pr
> eview.http.HttpServletRequest
> https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
> lina/connector/RequestFacade.html
>
> Since we have no tests on the payPalCheckoutUpdate service we did not spot
> that Tomcat servlet4preview was introduced with Tomcat 8.5
>
> The classes check is done by the deeper interfaceOf() method of the
> ObjectType class using Class.getInterfaces()
> https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
> html#getInterfaces--
> Class.getInterfaces() does not recurse and stops at one level up. So in
> case of RequestFacade it returns only servlet4preview.http.HttpServletRequest
> and not javax.servlet.http.HttpServletRequest
> So when interfaceOf() compares the Classes it fails.
>
> What happened with my introduction of the HttpServletRequestWrapper in
> ContextFilter is it hid the RequestFacade because HttpServletRequestWrapper
> implements javax.servlet.http.HttpServletRequest
> https://docs.oracle.com/javaee/7/api/javax/servlet/http/
> HttpServletRequest.html
>
> 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
>
> Summary: it's unfortunate because we have no tests on the
> payPalCheckoutUpdate service.
> Because I temporarily introduced HttpServletRequestWrapper James was able
> to pass a javax.servlet.http.HttpServletRequest, like in
> payPalCheckoutUpdate.
> When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
> discovered that we had a problem with Tomcat 8.5.
>
> I propose this fix which uses org.apache.commons.lang3.ClassUtils.getAllInterfaces()
> and works
> http://commons.apache.org/proper/commons-lang/javadocs/api-
> release/src-html/org/apache/commons/lang3/ClassUtils.html
>
> Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
> ctType.java
> ===================================================================
> --- framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
> (révision 1827594)
> +++ framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
> (copie de travail)
> @@ -263,7 +263,7 @@
> */
> public static boolean interfaceOf(Class<?> objectClass, Class<?>
> interfaceClass) {
> while (objectClass != null) {
> - Class<?>[] ifaces = objectClass.getInterfaces();
> + List<Class<?>> ifaces = org.apache.commons.lang3.Class
> Utils.getAllInterfaces(objectClass);
>
> for (Class<?> iface: ifaces) {
> if (iface == interfaceClass) {
> Index: framework/common/servicedef/services.xml
> ===================================================================
> --- framework/common/servicedef/services.xml (révision 1827594)
> +++ framework/common/servicedef/services.xml (copie de travail)
> @@ -379,7 +379,7 @@
> <service name="userLogin" engine="java" location="
> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
> <description>Used to Automatically Authenticate a
> username/password; create a UserLogin object</description>
> <implements service="authenticationInterface"/>
> - <attribute name="request" mode="IN" type="org.apache.catalina.connector.RequestFacade"
> optional="true"/>
> + <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest"
> optional="true"/>
> </service>
> <service name="createUserLogin" engine="java" auth="false"
> location="org.apache.ofbiz.common.login.LoginServices"
> invoke="createUserLogin">
>
> Jacques
>
>
>
>
>> Regards
>> Scott
>>
>> On 23 March 2018 at 07:36, Jacques Le Roux <ja...@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?
>>>
>>> Jacques
>>>
>>>
>>>
>>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>>
>>> Something else must be wrong Jacques, I can't understand what you're
>>>> saying
>>>> in the ticket description or in your reply above but I do know the
>>>> following:
>>>> OFBiz is perfectly capable of knowing that
>>>> org.apache.catalina.connector.RequestFacade
>>>> implements the javax.servlet.http.HttpServletRequest and it should pass
>>>> type validation without errors.
>>>>
>>>> Since we know that, this commit becomes unnecessary.
>>>>
>>>> Regards
>>>> Scott
>>>>
>>>> On 22 March 2018 at 16:06, Jacques Le Roux <
>>>> jacques.le.roux@les7arts.com>
>>>> wrote:
>>>>
>>>> Michael,
>>>>
>>>>> I commited http://svn.apache.org/viewvc/o
>>>>> fbiz/ofbiz-framework/trunk/fra
>>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>>
>>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>>> committed
>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>>> pathrev=1819133
>>>>>
>>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>>> James
>>>>> would not have been allowed to commit
>>>>>
>>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>>> ervletRequest"
>>>>> optional="true"/>
>>>>>
>>>>> Because it's then rejected, because then the userLogin service actually
>>>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>>>> implements HttpServletRequest.)
>>>>>
>>>>> You can easily check by svn updating to r1819133 and removing the
>>>>> wrapper
>>>>> in ContextFilter.java.
>>>>>
>>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>>> tools
>>>>> like Jetty.
>>>>>
>>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows.
>>>>> So
>>>>> I think it's safe to use a RequestFacade there.
>>>>>
>>>>> If an user feels the need to use another webapp servers or Jetty, I
>>>>> think
>>>>> changing that would not be the most of the worries (the rest being much
>>>>> more frightening, I know I did it once with Geronimo).
>>>>>
>>>>> Since the check is done at the service level, it's hard to do otherwise
>>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>>> discuss that...
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>>
>>>>> Mmhhh, Jacques,
>>>>>
>>>>>> I think this is problematic because it ties a special implementation
>>>>>> for
>>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>>
>>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
>>>>>> bit
>>>>>> unclear and I don't get the purpose of this change.
>>>>>>
>>>>>> Can you please explain more clearly which problem this changes solves
>>>>>> and
>>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the
>>>>>> type?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Michael
>>>>>>
>>>>>>
>>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>>
>>>>>> Author: jleroux
>>>>>>
>>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>>> New Revision: 1827439
>>>>>>>
>>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>>> Log:
>>>>>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>>>>>> (OFBIZ-10304)
>>>>>>>
>>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>>> details
>>>>>>>
>>>>>>> Modified:
>>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>>>>
>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr
>>>>>>> amework/common/servicedef/serv
>>>>>>> ices.xml
>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>>> &r2=1827439&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>> ices.xml
>>>>>>> (original)
>>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>>> ices.xml
>>>>>>> Wed Mar 21 20:53:41 2018
>>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>>> <service name="userLogin" engine="java" location="
>>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>>> <description>Used to Automatically Authenticate a
>>>>>>> username/password; create a UserLogin object</description>
>>>>>>> <implements service="authenticationInterface"/>
>>>>>>> - <attribute name="request" mode="IN"
>>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>>> + <attribute name="request" mode="IN"
>>>>>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>>>>>> </service>
>>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>>> invoke="createUserLogin">
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 23/03/2018 à 17:09, Scott Gray a écrit :
> I don't need to try anything, I *know* that the service engine is supposed
> to accept a concrete class of an interface if the interface is specified as
> the attribute type.
>
> Either the service engine is broken by not accepting concrete
> implementations, or the bug report is incorrect.
Neither, it's "unfortunate", and a bit complicated.
Tomcat servlet4preview was introduced with Tomcat 8.5
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
Before James introduced Tomcat SSO, we had only one service passing a javax.servlet.http.HttpServletRequest to a service: payPalCheckoutUpdate.
AFAIK, we have actually always passed a org.apache.catalina.connector.RequestFacade to services when asking for javax.servlet.http.HttpServletRequest
in services definition.
Since Tomcat 8.5 RequestFacade implements javax.servlet.http.HttpServletRequest indirectly through
org.apache.catalina.servlet4preview.http.HttpServletRequest
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/connector/RequestFacade.html
Since we have no tests on the payPalCheckoutUpdate service we did not spot that Tomcat servlet4preview was introduced with Tomcat 8.5
The classes check is done by the deeper interfaceOf() method of the ObjectType class using Class.getInterfaces()
https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getInterfaces--
Class.getInterfaces() does not recurse and stops at one level up. So in case of RequestFacade it returns only servlet4preview.http.HttpServletRequest
and not javax.servlet.http.HttpServletRequest
So when interfaceOf() compares the Classes it fails.
What happened with my introduction of the HttpServletRequestWrapper in ContextFilter is it hid the RequestFacade because HttpServletRequestWrapper
implements javax.servlet.http.HttpServletRequest
https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html
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
Summary: it's unfortunate because we have no tests on the payPalCheckoutUpdate service.
Because I temporarily introduced HttpServletRequestWrapper James was able to pass a javax.servlet.http.HttpServletRequest, like in payPalCheckoutUpdate.
When I reverted (removed HttpServletRequestWrapper from ContextFilter) I discovered that we had a problem with Tomcat 8.5.
I propose this fix which uses org.apache.commons.lang3.ClassUtils.getAllInterfaces() and works
http://commons.apache.org/proper/commons-lang/javadocs/api-release/src-html/org/apache/commons/lang3/ClassUtils.html
Index: framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java
===================================================================
--- framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java (révision 1827594)
+++ framework/base/src/main/java/org/apache/ofbiz/base/util/ObjectType.java (copie de travail)
@@ -263,7 +263,7 @@
*/
public static boolean interfaceOf(Class<?> objectClass, Class<?> interfaceClass) {
while (objectClass != null) {
- Class<?>[] ifaces = objectClass.getInterfaces();
+ List<Class<?>> ifaces = org.apache.commons.lang3.ClassUtils.getAllInterfaces(objectClass);
for (Class<?> iface: ifaces) {
if (iface == interfaceClass) {
Index: framework/common/servicedef/services.xml
===================================================================
--- framework/common/servicedef/services.xml (révision 1827594)
+++ framework/common/servicedef/services.xml (copie de travail)
@@ -379,7 +379,7 @@
<service name="userLogin" engine="java" location="org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
<description>Used to Automatically Authenticate a username/password; create a UserLogin object</description>
<implements service="authenticationInterface"/>
- <attribute name="request" mode="IN" type="org.apache.catalina.connector.RequestFacade" optional="true"/>
+ <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest" optional="true"/>
</service>
<service name="createUserLogin" engine="java" auth="false"
location="org.apache.ofbiz.common.login.LoginServices" invoke="createUserLogin">
Jacques
>
> Regards
> Scott
>
> On 23 March 2018 at 07:36, Jacques Le Roux <ja...@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?
>>
>> Jacques
>>
>>
>>
>> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>>
>>> Something else must be wrong Jacques, I can't understand what you're
>>> saying
>>> in the ticket description or in your reply above but I do know the
>>> following:
>>> OFBiz is perfectly capable of knowing that
>>> org.apache.catalina.connector.RequestFacade
>>> implements the javax.servlet.http.HttpServletRequest and it should pass
>>> type validation without errors.
>>>
>>> Since we know that, this commit becomes unnecessary.
>>>
>>> Regards
>>> Scott
>>>
>>> On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
>>> wrote:
>>>
>>> Michael,
>>>> I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>>
>>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>>> committed
>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>>> pathrev=1819133
>>>>
>>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>>> James
>>>> would not have been allowed to commit
>>>>
>>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>>> ervletRequest"
>>>> optional="true"/>
>>>>
>>>> Because it's then rejected, because then the userLogin service actually
>>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>>> implements HttpServletRequest.)
>>>>
>>>> You can easily check by svn updating to r1819133 and removing the wrapper
>>>> in ContextFilter.java.
>>>>
>>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>>> tools
>>>> like Jetty.
>>>>
>>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows.
>>>> So
>>>> I think it's safe to use a RequestFacade there.
>>>>
>>>> If an user feels the need to use another webapp servers or Jetty, I think
>>>> changing that would not be the most of the worries (the rest being much
>>>> more frightening, I know I did it once with Geronimo).
>>>>
>>>> Since the check is done at the service level, it's hard to do otherwise
>>>> But I must say I did not dig too much and it's maybe possible, we can
>>>> discuss that...
>>>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>>
>>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>>
>>>> Mmhhh, Jacques,
>>>>> I think this is problematic because it ties a special implementation for
>>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>>
>>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
>>>>> unclear and I don't get the purpose of this change.
>>>>>
>>>>> Can you please explain more clearly which problem this changes solves
>>>>> and
>>>>> why we'll need org.apache.catalina.connector.RequestFacade as the type?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>>
>>>>> Author: jleroux
>>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>>> New Revision: 1827439
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>>> Log:
>>>>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>>>>> (OFBIZ-10304)
>>>>>>
>>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>>> details
>>>>>>
>>>>>> Modified:
>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>> ices.xml
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>>> &r2=1827439&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>> ices.xml
>>>>>> (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>>> ices.xml
>>>>>> Wed Mar 21 20:53:41 2018
>>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>>> <service name="userLogin" engine="java" location="
>>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>>> <description>Used to Automatically Authenticate a
>>>>>> username/password; create a UserLogin object</description>
>>>>>> <implements service="authenticationInterface"/>
>>>>>> - <attribute name="request" mode="IN"
>>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>>> + <attribute name="request" mode="IN"
>>>>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>>>>> </service>
>>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>>> invoke="createUserLogin">
>>>>>>
>>>>>>
>>>>>>
>>>>>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Scott Gray <sc...@hotwaxsystems.com>.
I don't need to try anything, I *know* that the service engine is supposed
to accept a concrete class of an interface if the interface is specified as
the attribute type.
Either the service engine is broken by not accepting concrete
implementations, or the bug report is incorrect.
Regards
Scott
On 23 March 2018 at 07:36, Jacques Le Roux <ja...@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?
>
> Jacques
>
>
>
> Le 23/03/2018 à 03:39, Scott Gray a écrit :
>
>> Something else must be wrong Jacques, I can't understand what you're
>> saying
>> in the ticket description or in your reply above but I do know the
>> following:
>> OFBiz is perfectly capable of knowing that
>> org.apache.catalina.connector.RequestFacade
>> implements the javax.servlet.http.HttpServletRequest and it should pass
>> type validation without errors.
>>
>> Since we know that, this commit becomes unnecessary.
>>
>> Regards
>> Scott
>>
>> On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
>> wrote:
>>
>> Michael,
>>>
>>> I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>>
>>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>>> committed
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>>> pathrev=1819133
>>>
>>> If I had not committed the wrapper in ContextFilter.java beforehand,
>>> James
>>> would not have been allowed to commit
>>>
>>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpS
>>> ervletRequest"
>>> optional="true"/>
>>>
>>> Because it's then rejected, because then the userLogin service actually
>>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>>> implements HttpServletRequest.)
>>>
>>> You can easily check by svn updating to r1819133 and removing the wrapper
>>> in ContextFilter.java.
>>>
>>> You are right that it ties OFBiz to Tomcat. We took this decision
>>> sometimes ago and we no longer support other webapp servers OOTB, nor
>>> tools
>>> like Jetty.
>>>
>>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows.
>>> So
>>> I think it's safe to use a RequestFacade there.
>>>
>>> If an user feels the need to use another webapp servers or Jetty, I think
>>> changing that would not be the most of the worries (the rest being much
>>> more frightening, I know I did it once with Geronimo).
>>>
>>> Since the check is done at the service level, it's hard to do otherwise
>>> But I must say I did not dig too much and it's maybe possible, we can
>>> discuss that...
>>>
>>> Jacques
>>>
>>>
>>>
>>>
>>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>>
>>> Mmhhh, Jacques,
>>>>
>>>> I think this is problematic because it ties a special implementation for
>>>> Tomcat to the service. I didn't see this anywhere else.
>>>>
>>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
>>>> unclear and I don't get the purpose of this change.
>>>>
>>>> Can you please explain more clearly which problem this changes solves
>>>> and
>>>> why we'll need org.apache.catalina.connector.RequestFacade as the type?
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>>
>>>> Author: jleroux
>>>>> Date: Wed Mar 21 20:53:41 2018
>>>>> New Revision: 1827439
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>>> Log:
>>>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>>>> (OFBIZ-10304)
>>>>>
>>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>>> details
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>>
>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>> ices.xml
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>>> &r2=1827439&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>> ices.xml
>>>>> (original)
>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>>> ices.xml
>>>>> Wed Mar 21 20:53:41 2018
>>>>> @@ -379,7 +379,7 @@ under the License.
>>>>> <service name="userLogin" engine="java" location="
>>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>>> <description>Used to Automatically Authenticate a
>>>>> username/password; create a UserLogin object</description>
>>>>> <implements service="authenticationInterface"/>
>>>>> - <attribute name="request" mode="IN"
>>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>>> + <attribute name="request" mode="IN"
>>>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>>>> </service>
>>>>> <service name="createUserLogin" engine="java" auth="false"
>>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>>> invoke="createUserLogin">
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
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?
Jacques
Le 23/03/2018 à 03:39, Scott Gray a écrit :
> Something else must be wrong Jacques, I can't understand what you're saying
> in the ticket description or in your reply above but I do know the
> following:
> OFBiz is perfectly capable of knowing that
> org.apache.catalina.connector.RequestFacade
> implements the javax.servlet.http.HttpServletRequest and it should pass
> type validation without errors.
>
> Since we know that, this commit becomes unnecessary.
>
> Regards
> Scott
>
> On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
> wrote:
>
>> Michael,
>>
>> I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
>> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>>
>> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
>> committed
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
>> pathrev=1819133
>>
>> If I had not committed the wrapper in ContextFilter.java beforehand, James
>> would not have been allowed to commit
>>
>> <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest"
>> optional="true"/>
>>
>> Because it's then rejected, because then the userLogin service actually
>> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
>> implements HttpServletRequest.)
>>
>> You can easily check by svn updating to r1819133 and removing the wrapper
>> in ContextFilter.java.
>>
>> You are right that it ties OFBiz to Tomcat. We took this decision
>> sometimes ago and we no longer support other webapp servers OOTB, nor tools
>> like Jetty.
>>
>> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows. So
>> I think it's safe to use a RequestFacade there.
>>
>> If an user feels the need to use another webapp servers or Jetty, I think
>> changing that would not be the most of the worries (the rest being much
>> more frightening, I know I did it once with Geronimo).
>>
>> Since the check is done at the service level, it's hard to do otherwise
>> But I must say I did not dig too much and it's maybe possible, we can
>> discuss that...
>>
>> Jacques
>>
>>
>>
>>
>> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>>
>>> Mmhhh, Jacques,
>>>
>>> I think this is problematic because it ties a special implementation for
>>> Tomcat to the service. I didn't see this anywhere else.
>>>
>>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
>>> unclear and I don't get the purpose of this change.
>>>
>>> Can you please explain more clearly which problem this changes solves and
>>> why we'll need org.apache.catalina.connector.RequestFacade as the type?
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>>
>>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>>
>>>> Author: jleroux
>>>> Date: Wed Mar 21 20:53:41 2018
>>>> New Revision: 1827439
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>>> Log:
>>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>>> (OFBIZ-10304)
>>>>
>>>> It should have been org.apache.catalina.connector.RequestFacade"
>>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>>> details
>>>>
>>>> Modified:
>>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>>> ices.xml
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>>> &r2=1827439&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>> (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>> Wed Mar 21 20:53:41 2018
>>>> @@ -379,7 +379,7 @@ under the License.
>>>> <service name="userLogin" engine="java" location="
>>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>>> <description>Used to Automatically Authenticate a
>>>> username/password; create a UserLogin object</description>
>>>> <implements service="authenticationInterface"/>
>>>> - <attribute name="request" mode="IN"
>>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>>> + <attribute name="request" mode="IN"
>>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>>> </service>
>>>> <service name="createUserLogin" engine="java" auth="false"
>>>> location="org.apache.ofbiz.common.login.LoginServices"
>>>> invoke="createUserLogin">
>>>>
>>>>
>>>>
>>>
Re: svn commit: r1827439 - /ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Scott Gray <sc...@hotwaxsystems.com>.
Something else must be wrong Jacques, I can't understand what you're saying
in the ticket description or in your reply above but I do know the
following:
OFBiz is perfectly capable of knowing that
org.apache.catalina.connector.RequestFacade
implements the javax.servlet.http.HttpServletRequest and it should pass
type validation without errors.
Since we know that, this commit becomes unnecessary.
Regards
Scott
On 22 March 2018 at 16:06, Jacques Le Roux <ja...@les7arts.com>
wrote:
> Michael,
>
> I commited http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
> l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
>
> Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
> committed
> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
> mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
> pathrev=1819133
>
> If I had not committed the wrapper in ContextFilter.java beforehand, James
> would not have been allowed to commit
>
> <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest"
> optional="true"/>
>
> Because it's then rejected, because then the userLogin service actually
> receive an org.apache.catalina.connector.RequestFacade (RequestFacade
> implements HttpServletRequest.)
>
> You can easily check by svn updating to r1819133 and removing the wrapper
> in ContextFilter.java.
>
> You are right that it ties OFBiz to Tomcat. We took this decision
> sometimes ago and we no longer support other webapp servers OOTB, nor tools
> like Jetty.
>
> So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows. So
> I think it's safe to use a RequestFacade there.
>
> If an user feels the need to use another webapp servers or Jetty, I think
> changing that would not be the most of the worries (the rest being much
> more frightening, I know I did it once with Geronimo).
>
> Since the check is done at the service level, it's hard to do otherwise
> But I must say I did not dig too much and it's maybe possible, we can
> discuss that...
>
> Jacques
>
>
>
>
> Le 22/03/2018 à 16:22, Michael Brohl a écrit :
>
>> Mmhhh, Jacques,
>>
>> I think this is problematic because it ties a special implementation for
>> Tomcat to the service. I didn't see this anywhere else.
>>
>> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit
>> unclear and I don't get the purpose of this change.
>>
>> Can you please explain more clearly which problem this changes solves and
>> why we'll need org.apache.catalina.connector.RequestFacade as the type?
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>>
>>> Author: jleroux
>>> Date: Wed Mar 21 20:53:41 2018
>>> New Revision: 1827439
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>>> Log:
>>> Fixed: The "request" attribute type of the userLogin service is wrong
>>> (OFBIZ-10304)
>>>
>>> It should have been org.apache.catalina.connector.RequestFacade"
>>> instead of javax.servlet.http.HttpServletRequest see the Jira for
>>> details
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
>>> ices.xml
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/common/servicedef/services.xml?rev=1827439&r1=1827438
>>> &r2=1827439&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>> (original)
>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>> Wed Mar 21 20:53:41 2018
>>> @@ -379,7 +379,7 @@ under the License.
>>> <service name="userLogin" engine="java" location="
>>> org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>>> <description>Used to Automatically Authenticate a
>>> username/password; create a UserLogin object</description>
>>> <implements service="authenticationInterface"/>
>>> - <attribute name="request" mode="IN"
>>> type="javax.servlet.http.HttpServletRequest" optional="true"/>
>>> + <attribute name="request" mode="IN"
>>> type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>>> </service>
>>> <service name="createUserLogin" engine="java" auth="false"
>>> location="org.apache.ofbiz.common.login.LoginServices"
>>> invoke="createUserLogin">
>>>
>>>
>>>
>>
>>
>
Re: svn commit: r1827439 -
/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Michael,
I commited
http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James committed
http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml?r1=1819133&r2=1819132&pathrev=1819133
If I had not committed the wrapper in ContextFilter.java beforehand, James would not have been allowed to commit
<attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest" optional="true"/>
Because it's then rejected, because then the userLogin service actually receive an org.apache.catalina.connector.RequestFacade (RequestFacade
implements HttpServletRequest.)
You can easily check by svn updating to r1819133 and removing the wrapper in ContextFilter.java.
You are right that it ties OFBiz to Tomcat. We took this decision sometimes ago and we no longer support other webapp servers OOTB, nor tools like Jetty.
So OFBiz OOTB totally depends on Tomcat as the build.gradle file shows. So I think it's safe to use a RequestFacade there.
If an user feels the need to use another webapp servers or Jetty, I think changing that would not be the most of the worries (the rest being much more
frightening, I know I did it once with Geronimo).
Since the check is done at the service level, it's hard to do otherwise But I must say I did not dig too much and it's maybe possible, we can discuss
that...
Jacques
Le 22/03/2018 à 16:22, Michael Brohl a écrit :
> Mmhhh, Jacques,
>
> I think this is problematic because it ties a special implementation for Tomcat to the service. I didn't see this anywhere else.
>
> The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a bit unclear and I don't get the purpose of this change.
>
> Can you please explain more clearly which problem this changes solves and why we'll need org.apache.catalina.connector.RequestFacade as the type?
>
> Thanks,
>
> Michael
>
>
> Am 21.03.18 um 21:53 schrieb jleroux@apache.org:
>> Author: jleroux
>> Date: Wed Mar 21 20:53:41 2018
>> New Revision: 1827439
>>
>> URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
>> Log:
>> Fixed: The "request" attribute type of the userLogin service is wrong
>> (OFBIZ-10304)
>>
>> It should have been org.apache.catalina.connector.RequestFacade"
>> instead of javax.servlet.http.HttpServletRequest see the Jira for details
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>>
>> Modified: ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml?rev=1827439&r1=1827438&r2=1827439&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml (original)
>> +++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/services.xml Wed Mar 21 20:53:41 2018
>> @@ -379,7 +379,7 @@ under the License.
>> <service name="userLogin" engine="java" location="org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
>> <description>Used to Automatically Authenticate a username/password; create a UserLogin object</description>
>> <implements service="authenticationInterface"/>
>> - <attribute name="request" mode="IN" type="javax.servlet.http.HttpServletRequest" optional="true"/>
>> + <attribute name="request" mode="IN" type="org.apache.catalina.connector.RequestFacade" optional="true"/>
>> </service>
>> <service name="createUserLogin" engine="java" auth="false"
>> location="org.apache.ofbiz.common.login.LoginServices" invoke="createUserLogin">
>>
>>
>
>