Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java webapps/docs/changelog.xml

[Rémy Maucherat]

On Mon, Dec 4, 2017 at 9:22 PM, Mark Thomas  wrote:

> On 04/12/17 19:50, Mark Thomas wrote:
> > On 04/12/17 18:03, Rémy Maucherat wrote:
>
> 
>
> >> Another "feature" that looks almost impossible to implement I guess.
> >
> > Hmm. I only read the first part of the Javadoc. I'm not really sure what
> > the second part is getting at with "... a container generated token...".
> > I'll have a look back at the archive to see if there was any EG
> > discussion on this point.
>
> That second part was part of the original proposal and there was never
> any discussion about what it actually meant.
>
> Thinking about it, I think we could do the following and be spec compliant:
>
> - Set a header e.g. "Authorization: x-push"
> - Copy the authenticated Principal from the base request to the
>   pushTarget
>
> That meets the requirements:
> - "an Authorization header will be set with a container generated token"
> - "result in equivalent Authorization for the pushed request"
>
> The spec does imply that it is the token that results in authorization
> but it doesn't actually mandate it. I think there is enough flexibility
> in the wording that the above would be OK.
>
> Thoguhts?
>
> Indeed, it doesn't say that it has to be an autorization header that would
normally work, only a token.

Rémy

Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java webapps/docs/changelog.xml

[Mark Thomas]

On 04/12/17 19:50, Mark Thomas wrote:
> On 04/12/17 18:03, Rémy Maucherat wrote:



>> Another "feature" that looks almost impossible to implement I guess.
> 
> Hmm. I only read the first part of the Javadoc. I'm not really sure what
> the second part is getting at with "... a container generated token...".
> I'll have a look back at the archive to see if there was any EG
> discussion on this point.

That second part was part of the original proposal and there was never
any discussion about what it actually meant.

Thinking about it, I think we could do the following and be spec compliant:

- Set a header e.g. "Authorization: x-push"
- Copy the authenticated Principal from the base request to the
  pushTarget

That meets the requirements:
- "an Authorization header will be set with a container generated token"
- "result in equivalent Authorization for the pushed request"

The spec does imply that it is the token that results in authorization
but it doesn't actually mandate it. I think there is enough flexibility
in the wording that the above would be OK.

Thoguhts?

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java webapps/docs/changelog.xml

[Mark Thomas]

On 04/12/17 18:03, Rémy Maucherat wrote:
> On Mon, Dec 4, 2017 at 6:05 PM, Mark Thomas  wrote:
> 
>> On 04/12/17 16:59, r...@apache.org wrote:
>>> Author: remm
>>> Date: Mon Dec  4 16:59:12 2017
>>> New Revision: 1817105
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1817105=rev
>>> Log:
>>> Minor push builder fixes: don't remove the auth header,
>>
>> -1.
>>
>> The Javadoc for PushBuilder explicitly lists Authorization headers as
>> one of the types that are not transferred to the pushed request.
>>
> 
> And then:
> If the request was authenticated, an Authorization header will be set with
> a container generated token that will result in equivalent Authorization
> for the pushed request.
> 
> So it worked just fine for basic.
> 
> Another "feature" that looks almost impossible to implement I guess.

Hmm. I only read the first part of the Javadoc. I'm not really sure what
the second part is getting at with "... a container generated token...".
I'll have a look back at the archive to see if there was any EG
discussion on this point.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java webapps/docs/changelog.xml

[Rémy Maucherat]

On Mon, Dec 4, 2017 at 6:05 PM, Mark Thomas  wrote:

> On 04/12/17 16:59, r...@apache.org wrote:
> > Author: remm
> > Date: Mon Dec  4 16:59:12 2017
> > New Revision: 1817105
> >
> > URL: http://svn.apache.org/viewvc?rev=1817105=rev
> > Log:
> > Minor push builder fixes: don't remove the auth header,
>
> -1.
>
> The Javadoc for PushBuilder explicitly lists Authorization headers as
> one of the types that are not transferred to the pushed request.
>

And then:
If the request was authenticated, an Authorization header will be set with
a container generated token that will result in equivalent Authorization
for the pushed request.

So it worked just fine for basic.

Another "feature" that looks almost impossible to implement I guess.

Rémy


> > and exception on an empty method.
>
> Good catch.
>
> Mark
>
> [1]
> https://github.com/javaee/servlet-spec/blob/master/src/
> main/java/javax/servlet/http/PushBuilder.java
>
>
>
> >
> > Modified:
> > tomcat/trunk/java/org/apache/catalina/core/
> ApplicationPushBuilder.java
> > tomcat/trunk/webapps/docs/changelog.xml
> >
> > Modified: tomcat/trunk/java/org/apache/catalina/core/
> ApplicationPushBuilder.java
> > URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/
> catalina/core/ApplicationPushBuilder.java?rev=1817105=1817104=
> 1817105=diff
> > 
> ==
> > --- tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> (original)
> > +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> Mon Dec  4 16:59:12 2017
> > @@ -98,7 +98,6 @@ public class ApplicationPushBuilder impl
> >  headers.remove("if-range");
> >  headers.remove("range");
> >  headers.remove("expect");
> > -headers.remove("authorization");
> >  headers.remove("referer");
> >  // Also remove the cookie header since it will be regenerated
> >  headers.remove("cookie");
> > @@ -108,7 +107,6 @@ public class ApplicationPushBuilder impl
> >  if (request.getQueryString() != null) {
> >  referer.append('?');
> >  referer.append(request.getQueryString());
> > -
> >  }
> >  addHeader("referer", referer.toString());
> >
> > @@ -184,7 +182,7 @@ public class ApplicationPushBuilder impl
> >  @Override
> >  public PushBuilder method(String method) {
> >  String upperMethod = method.trim().toUpperCase();
> > -if (DISALLOWED_METHODS.contains(upperMethod)) {
> > +if (DISALLOWED_METHODS.contains(upperMethod) ||
> upperMethod.length() == 0) {
> >  throw new IllegalArgumentException(
> >  sm.getString("applicationPushBuilder.methodInvalid",
> upperMethod));
> >  }
> >
> > Modified: tomcat/trunk/webapps/docs/changelog.xml
> > URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/
> changelog.xml?rev=1817105=1817104=1817105=diff
> > 
> ==
> > --- tomcat/trunk/webapps/docs/changelog.xml (original)
> > +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec  4 16:59:12 2017
> > @@ -53,6 +53,9 @@
> >
> >  Update the Java EE 8 XML schema to the released versions.
> (markt)
> >
> > +  
> > +Minor HTTP/2 push fixes. (remm)
> > +  
> >  
> >
> >
> >
> >
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> > For additional commands, e-mail: dev-h...@tomcat.apache.org
> >
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>

Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java webapps/docs/changelog.xml

[Mark Thomas]

On 04/12/17 16:59, r...@apache.org wrote:
> Author: remm
> Date: Mon Dec  4 16:59:12 2017
> New Revision: 1817105
> 
> URL: http://svn.apache.org/viewvc?rev=1817105=rev
> Log:
> Minor push builder fixes: don't remove the auth header,

-1.

The Javadoc for PushBuilder explicitly lists Authorization headers as
one of the types that are not transferred to the pushed request.

> and exception on an empty method.

Good catch.

Mark

[1]
https://github.com/javaee/servlet-spec/blob/master/src/main/java/javax/servlet/http/PushBuilder.java



> 
> Modified:
> tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> tomcat/trunk/webapps/docs/changelog.xml
> 
> Modified: 
> tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> URL: 
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java?rev=1817105=1817104=1817105=diff
> ==
> --- tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java 
> (original)
> +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java 
> Mon Dec  4 16:59:12 2017
> @@ -98,7 +98,6 @@ public class ApplicationPushBuilder impl
>  headers.remove("if-range");
>  headers.remove("range");
>  headers.remove("expect");
> -headers.remove("authorization");
>  headers.remove("referer");
>  // Also remove the cookie header since it will be regenerated
>  headers.remove("cookie");
> @@ -108,7 +107,6 @@ public class ApplicationPushBuilder impl
>  if (request.getQueryString() != null) {
>  referer.append('?');
>  referer.append(request.getQueryString());
> -
>  }
>  addHeader("referer", referer.toString());
>  
> @@ -184,7 +182,7 @@ public class ApplicationPushBuilder impl
>  @Override
>  public PushBuilder method(String method) {
>  String upperMethod = method.trim().toUpperCase();
> -if (DISALLOWED_METHODS.contains(upperMethod)) {
> +if (DISALLOWED_METHODS.contains(upperMethod) || upperMethod.length() 
> == 0) {
>  throw new IllegalArgumentException(
>  sm.getString("applicationPushBuilder.methodInvalid", 
> upperMethod));
>  }
> 
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: 
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1817105=1817104=1817105=diff
> ==
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec  4 16:59:12 2017
> @@ -53,6 +53,9 @@
>
>  Update the Java EE 8 XML schema to the released versions. (markt)
>
> +  
> +Minor HTTP/2 push fixes. (remm)
> +  
>  
>
>
> 
> 
> 
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
> 


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org