Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./ framework/common/groovyScripts/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/

[Deepak Dixit-3]

Hi Jacques,

> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/
> java/org/apache/ofbiz/webapp/control/ContextFilter.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> framework/webapp/src/main/java/org/apache/ofbiz/webapp/
> control/ContextFilter.java?rev=1813679&r1=1813678&r2=1813679&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/
> java/org/apache/ofbiz/webapp/control/ContextFilter.java (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/
> java/org/apache/ofbiz/webapp/control/ContextFilter.java Sun Oct 29
> 11:02:00 2017
> @@ -18,11 +18,8 @@
>   ************************************************************
> *******************/
>  package org.apache.ofbiz.webapp.control;
>
> -import static org.apache.ofbiz.base.util.UtilGenerics.checkMap;
> -
>  import java.io.IOException;
>  import java.util.Enumeration;
> -import java.util.Map;
>
>  import javax.servlet.Filter;
>  import javax.servlet.FilterChain;
> @@ -31,13 +28,12 @@ import javax.servlet.ServletException;
>  import javax.servlet.ServletRequest;
>  import javax.servlet.ServletResponse;
>  import javax.servlet.http.HttpServletRequest;
> +import javax.servlet.http.HttpServletRequestWrapper;
>  import javax.servlet.http.HttpServletResponse;
>
>  import org.apache.ofbiz.base.util.Debug;
> -import org.apache.ofbiz.base.util.StringUtil;
>  import org.apache.ofbiz.base.util.UtilGenerics;
>  import org.apache.ofbiz.base.util.UtilHttp;
> -import org.apache.ofbiz.base.util.UtilObject;
>  import org.apache.ofbiz.base.util.UtilValidate;
>  import org.apache.ofbiz.entity.Delegator;
>  import org.apache.ofbiz.entity.DelegatorFactory;
> @@ -192,8 +188,29 @@ public class ContextFilter implements Fi
>              }
>          }
>
> +        HttpServletRequestWrapper wrapper = new HttpServletRequestWrapper(httpRequest)
> {
> +            @Override
> +            public String getHeader(String name) {
> +                String externalServerUserLoginId = request.getParameter(
> ExternalLoginKeysManager.EXTERNAL_SERVER_LOGIN_KEY);
> +                String value = null;
> +                if (externalServerUserLoginId != null) {
> +                    // ExternalLoginKeysManager .createJwt() arguments in
> order:
> +                    // id an Id, I suggest userLoginId
> +                    // issuer is who/what issued the token. I suggest the
> server DNS
> +                    // subject is the subject of the token. I suggest the
> destination webapp
> +                    // timeToLive is the token maximum duration
> +                    String webAppName = UtilHttp.getApplicationName(
> httpRequest);
> +                    String dnsName = ExternalLoginKeysManager.
> getExternalServerName(httpRequest);
> +                    long timeToLive = ExternalLoginKeysManager.
> getJwtTokenTimeToLive(httpRequest);
> +                    // We would need a Bearer token (in Authorisation
> request header) if we were using Oauth2, here we don't, so no Bearer
> +                    value = ExternalLoginKeysManager.createJwt(externalServerUserLoginId,
> dnsName, webAppName , timeToLive);
> +                }
> +                if (value != null) return value;
> +                return super.getHeader("Authorisation");
>

I think this should be

return super.getHeader(name);
>

> +            }
> +        };
>          // we're done checking; continue on
> -        chain.doFilter(request, httpResponse);
> +        chain.doFilter(wrapper, httpResponse);
>      }
>
>      /**
>
>
>

Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co

[Jacques Le Roux]

Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>> +                return super.getHeader("Authorisation");
>>
> I think this should be
>
> return super.getHeader(name);
Thanks Deepak,

Actually let me explain the context here (maybe not for you but at large)
In the case of ExternalLoginKeysManager, it's always "Authorisation". It's explained there
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization
https://en.wikipedia.org/wiki/Basic_access_authentication
And it's the only usage of the wrapper so far.

Note that in the case of JWT token used in OAuth 2 you normally need to use a bearer token
https://www.google.fr/search?q=http+authorization+header+bearer&ie=UTF-8
But in the case I committed it was not necessary (it's not OAuth 2, just a JWT token) and I must say I got issue trying to encode things with it

Anyway you are right, why not using name there, it will not change things, and the wrapper idea could be then reused/refactored when adding other
related features, will do :)

Jacques

[Deepak Dixit-3]

Thanks Jacques for detail,
but I think name is not always Authorisation in code we are having lots of
request.getHeader usage and its breaks their usage.
I'll confirm and reply here (just for reference.)

Need to backport this to 17.12 as well.

Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co

On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
[hidden email]> wrote:

> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>
>> +                return super.getHeader("Authorisation");
>>>
>>> I think this should be
>>
>> return super.getHeader(name);
>>
> Thanks Deepak,
>
> Actually let me explain the context here (maybe not for you but at large)
> In the case of ExternalLoginKeysManager, it's always "Authorisation". It's
> explained there
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization
> https://en.wikipedia.org/wiki/Basic_access_authentication
> And it's the only usage of the wrapper so far.
>
> Note that in the case of JWT token used in OAuth 2 you normally need to
> use a bearer token
> https://www.google.fr/search?q=http+authorization+header+bearer&ie=UTF-8
> But in the case I committed it was not necessary (it's not OAuth 2, just a
> JWT token) and I must say I got issue trying to encode things with it
>
> Anyway you are right, why not using name there, it will not change things,
> and the wrapper idea could be then reused/refactored when adding other
> related features, will do :)
>
> Jacques
>
>

[Jacques Le Roux]

Thanks Deepak,

Ha indeed, I see your point now.

Sorry for the confusion!

Jacques


Le 16/01/2018 à 11:30, Deepak Dixit a écrit :

> Thanks Jacques for detail,
> but I think name is not always Authorisation in code we are having lots of
> request.getHeader usage and its breaks their usage.
> I'll confirm and reply here (just for reference.)
>
> Need to backport this to 17.12 as well.
>
> Thanks & Regards
> --
> Deepak Dixit
> www.hotwaxsystems.com
> www.hotwax.co
>
> On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>>
>>> +                return super.getHeader("Authorisation");
>>>> I think this should be
>>> return super.getHeader(name);
>>>
>> Thanks Deepak,
>>
>> Actually let me explain the context here (maybe not for you but at large)
>> In the case of ExternalLoginKeysManager, it's always "Authorisation". It's
>> explained there
>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization
>> https://en.wikipedia.org/wiki/Basic_access_authentication
>> And it's the only usage of the wrapper so far.
>>
>> Note that in the case of JWT token used in OAuth 2 you normally need to
>> use a bearer token
>> https://www.google.fr/search?q=http+authorization+header+bearer&ie=UTF-8
>> But in the case I committed it was not necessary (it's not OAuth 2, just a
>> JWT token) and I must say I got issue trying to encode things with it
>>
>> Anyway you are right, why not using name there, it will not change things,
>> and the wrapper idea could be then reused/refactored when adding other
>> related features, will do :)
>>
>> Jacques
>>
>>

[Shi Jinghai-3]

Should it be "Authorization"? z, not s. If s is right for some specific environments, we can add an additional step to check "Authorisation" if getting "Authorization" header failed.

-----邮件原件-----
发件人: Jacques Le Roux [mailto:[hidden email]]
发送时间: 2018年1月16日 18:52
收件人: [hidden email]
主题: Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./ framework/common/groovyScripts/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/

Thanks Deepak,

Ha indeed, I see your point now.

Sorry for the confusion!

Jacques


Le 16/01/2018 à 11:30, Deepak Dixit a écrit :

> Thanks Jacques for detail,
> but I think name is not always Authorisation in code we are having
> lots of request.getHeader usage and its breaks their usage.
> I'll confirm and reply here (just for reference.)
>
> Need to backport this to 17.12 as well.
>
> Thanks & Regards
> --
> Deepak Dixit
> www.hotwaxsystems.com
> www.hotwax.co
>
> On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>>
>>> +                return super.getHeader("Authorisation");
>>>> I think this should be
>>> return super.getHeader(name);
>>>
>> Thanks Deepak,
>>
>> Actually let me explain the context here (maybe not for you but at
>> large) In the case of ExternalLoginKeysManager, it's always
>> "Authorisation". It's explained there
>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorizati
>> on https://en.wikipedia.org/wiki/Basic_access_authentication
>> And it's the only usage of the wrapper so far.
>>
>> Note that in the case of JWT token used in OAuth 2 you normally need
>> to use a bearer token
>> https://www.google.fr/search?q=http+authorization+header+bearer&ie=UT
>> F-8 But in the case I committed it was not necessary (it's not OAuth
>> 2, just a JWT token) and I must say I got issue trying to encode
>> things with it
>>
>> Anyway you are right, why not using name there, it will not change
>> things, and the wrapper idea could be then reused/refactored when
>> adding other related features, will do :)
>>
>> Jacques
>>
>>

[Jacques Le Roux]

You are right Jinghai,

This got unnoticed because it's only used internally. But the right word is indeed "Authorization"

I'll change that. It will have no side effects because this is only used temporarily during the translation to the other server.

It would be misleading to keep it as is. Because in the general case it's only "Authorization" and not "Authorisation"

I used "Authorisation" because of my intuitive preference for British English[1] I must say due of my French background[2],  a bit pedantic... phew ;)

Thanks!

Jacques

[1] https://www.etymonline.com/word/authorisation

[2] https://fr.wiktionary.org/wiki/autoriser (so the US are right finally: https://en.wiktionary.org/wiki/auctorizare)

Le 18/01/2018 à 06:49, Shi Jinghai a écrit :

> Should it be "Authorization"? z, not s. If s is right for some specific environments, we can add an additional step to check "Authorisation" if getting "Authorization" header failed.
>
> -----邮件原件-----
> 发件人: Jacques Le Roux [mailto:[hidden email]]
> 发送时间: 2018年1月16日 18:52
> 收件人: [hidden email]
> 主题: Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./ framework/common/groovyScripts/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/
>
> Thanks Deepak,
>
> Ha indeed, I see your point now.
>
> Sorry for the confusion!
>
> Jacques
>
>
> Le 16/01/2018 à 11:30, Deepak Dixit a écrit :
>> Thanks Jacques for detail,
>> but I think name is not always Authorisation in code we are having
>> lots of request.getHeader usage and its breaks their usage.
>> I'll confirm and reply here (just for reference.)
>>
>> Need to backport this to 17.12 as well.
>>
>> Thanks & Regards
>> --
>> Deepak Dixit
>> www.hotwaxsystems.com
>> www.hotwax.co
>>
>> On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>>>
>>>> +                return super.getHeader("Authorisation");
>>>>> I think this should be
>>>> return super.getHeader(name);
>>>>
>>> Thanks Deepak,
>>>
>>> Actually let me explain the context here (maybe not for you but at
>>> large) In the case of ExternalLoginKeysManager, it's always
>>> "Authorisation". It's explained there
>>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorizati
>>> on https://en.wikipedia.org/wiki/Basic_access_authentication
>>> And it's the only usage of the wrapper so far.
>>>
>>> Note that in the case of JWT token used in OAuth 2 you normally need
>>> to use a bearer token
>>> https://www.google.fr/search?q=http+authorization+header+bearer&ie=UT
>>> F-8 But in the case I committed it was not necessary (it's not OAuth
>>> 2, just a JWT token) and I must say I got issue trying to encode
>>> things with it
>>>
>>> Anyway you are right, why not using name there, it will not change
>>> things, and the wrapper idea could be then reused/refactored when
>>> adding other related features, will do :)
>>>
>>> Jacques
>>>
>>>

[Shi Jinghai-3]

Love you. :)

-----邮件原件-----
发件人: Jacques Le Roux [mailto:[hidden email]]
发送时间: 2018年1月18日 21:02
收件人: [hidden email]
主题: Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./ framework/common/groovyScripts/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/

You are right Jinghai,

This got unnoticed because it's only used internally. But the right word is indeed "Authorization"

I'll change that. It will have no side effects because this is only used temporarily during the translation to the other server.

It would be misleading to keep it as is. Because in the general case it's only "Authorization" and not "Authorisation"

I used "Authorisation" because of my intuitive preference for British English[1] I must say due of my French background[2],  a bit pedantic... phew ;)

Thanks!

Jacques

[1] https://www.etymonline.com/word/authorisation

[2] https://fr.wiktionary.org/wiki/autoriser (so the US are right finally: https://en.wiktionary.org/wiki/auctorizare)

Le 18/01/2018 à 06:49, Shi Jinghai a écrit :

> Should it be "Authorization"? z, not s. If s is right for some specific environments, we can add an additional step to check "Authorisation" if getting "Authorization" header failed.
>
> -----邮件原件-----
> 发件人: Jacques Le Roux [mailto:[hidden email]]
> 发送时间: 2018年1月16日 18:52
> 收件人: [hidden email]
> 主题: Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./
> framework/common/groovyScripts/
> framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/
>
> Thanks Deepak,
>
> Ha indeed, I see your point now.
>
> Sorry for the confusion!
>
> Jacques
>
>
> Le 16/01/2018 à 11:30, Deepak Dixit a écrit :
>> Thanks Jacques for detail,
>> but I think name is not always Authorisation in code we are having
>> lots of request.getHeader usage and its breaks their usage.
>> I'll confirm and reply here (just for reference.)
>>
>> Need to backport this to 17.12 as well.
>>
>> Thanks & Regards
>> --
>> Deepak Dixit
>> www.hotwaxsystems.com
>> www.hotwax.co
>>
>> On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>>>
>>>> +                return super.getHeader("Authorisation");
>>>>> I think this should be
>>>> return super.getHeader(name);
>>>>
>>> Thanks Deepak,
>>>
>>> Actually let me explain the context here (maybe not for you but at
>>> large) In the case of ExternalLoginKeysManager, it's always
>>> "Authorisation". It's explained there
>>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorizat
>>> i on https://en.wikipedia.org/wiki/Basic_access_authentication
>>> And it's the only usage of the wrapper so far.
>>>
>>> Note that in the case of JWT token used in OAuth 2 you normally need
>>> to use a bearer token
>>> https://www.google.fr/search?q=http+authorization+header+bearer&ie=U
>>> T
>>> F-8 But in the case I committed it was not necessary (it's not OAuth
>>> 2, just a JWT token) and I must say I got issue trying to encode
>>> things with it
>>>
>>> Anyway you are right, why not using name there, it will not change
>>> things, and the wrapper idea could be then reused/refactored when
>>> adding other related features, will do :)
>>>
>>> Jacques
>>>
>>>

[Jacques Le Roux]

Hi Jinghai,

Actually both are accepted. I did not find a place where this is wrote, but you can try both work.

Anyway I changed at revision: 1821600

Jacques


Le 18/01/2018 à 06:49, Shi Jinghai a écrit :

> Should it be "Authorization"? z, not s. If s is right for some specific environments, we can add an additional step to check "Authorisation" if getting "Authorization" header failed.
>
> -----邮件原件-----
> 发件人: Jacques Le Roux [mailto:[hidden email]]
> 发送时间: 2018年1月16日 18:52
> 收件人: [hidden email]
> 主题: Re: svn commit: r1813679 - in /ofbiz/ofbiz-framework/trunk: ./ framework/common/groovyScripts/ framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/
>
> Thanks Deepak,
>
> Ha indeed, I see your point now.
>
> Sorry for the confusion!
>
> Jacques
>
>
> Le 16/01/2018 à 11:30, Deepak Dixit a écrit :
>> Thanks Jacques for detail,
>> but I think name is not always Authorisation in code we are having
>> lots of request.getHeader usage and its breaks their usage.
>> I'll confirm and reply here (just for reference.)
>>
>> Need to backport this to 17.12 as well.
>>
>> Thanks & Regards
>> --
>> Deepak Dixit
>> www.hotwaxsystems.com
>> www.hotwax.co
>>
>> On Tue, Jan 16, 2018 at 3:26 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> Le 16/01/2018 à 09:53, Deepak Dixit a écrit :
>>>
>>>> +                return super.getHeader("Authorisation");
>>>>> I think this should be
>>>> return super.getHeader(name);
>>>>
>>> Thanks Deepak,
>>>
>>> Actually let me explain the context here (maybe not for you but at
>>> large) In the case of ExternalLoginKeysManager, it's always
>>> "Authorisation". It's explained there
>>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorizati
>>> on https://en.wikipedia.org/wiki/Basic_access_authentication
>>> And it's the only usage of the wrapper so far.
>>>
>>> Note that in the case of JWT token used in OAuth 2 you normally need
>>> to use a bearer token
>>> https://www.google.fr/search?q=http+authorization+header+bearer&ie=UT
>>> F-8 But in the case I committed it was not necessary (it's not OAuth
>>> 2, just a JWT token) and I must say I got issue trying to encode
>>> things with it
>>>
>>> Anyway you are right, why not using name there, it will not change
>>> things, and the wrapper idea could be then reused/refactored when
>>> adding other related features, will do :)
>>>
>>> Jacques
>>>
>>>