[PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
rzo1 opened a new pull request, #1822: URL: https://github.com/apache/cxf/pull/1822 Hi all, I am currently checking the integration of CXF 4.1.0-SNAPSHOT into TomEE 10 and I noticed a change in `AbstractHTTPDestination`. An additional `try-catch` was added around `getUserPrincipal()` catching any kind of `Exception` and just returning `null`. Was this intentional (or required by the spec)? I think, that we shouldn't catch everything here. I noticed this change by debugging TomEE's Microprofile JWT integration and one specific TCK test was failing: ``` org.eclipse.microprofile.jwt.tck.container.jaxrs.EmptyTokenTest#invalidToken ``` The provided token is invalid, we cannot parse a principal out of it and since it is in an invalid format anyway, our impl will throw an exception. This exception is now caught and mapped to `null`, which is quite bad (as it will just return http 200 instead of 401). wdyt? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
rzo1 commented on PR #1822: URL: https://github.com/apache/cxf/pull/1822#issuecomment-2066319454 Ok from what I can read in the commit history, it seems, it was introduced because of jetty 12? The underlying exception (reason for failing GH actions) is a NPE in Jetty: ``` [INFO] Running org.apache.cxf.systest.ws.rm.CachedOutClientPersistenceTest java.lang.NullPointerException: Cannot invoke "org.eclipse.jetty.server.Request.getAttribute(String)" because "request" is null at org.eclipse.jetty.server.Request.getAuthenticationState(Request.java:949) at org.eclipse.jetty.security.AuthenticationState.getAuthenticationState(AuthenticationState.java:45) at org.eclipse.jetty.ee10.servlet.ServletApiRequest.getAuthentication(ServletApiRequest.java:254) at org.eclipse.jetty.ee10.servlet.ServletApiRequest.getUndeferredAuthentication(ServletApiRequest.java:259) at org.eclipse.jetty.ee10.servlet.ServletApiRequest.getUserPrincipal(ServletApiRequest.java:478) at org.apache.cxf.transport.http.AbstractHTTPDestination$2.getUserPrincipal(AbstractHTTPDestination.java:412) at org.apache.cxf.ext.logging.event.DefaultLogEventMapper.getPrincipal(DefaultLogEventMapper.java:117) at org.apache.cxf.ext.logging.event.DefaultLogEventMapper.map(DefaultLogEventMapper.java:104) at org.apache.cxf.ext.logging.LoggingInInterceptor.handleMessage(LoggingInInterceptor.java:93) at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:307) at org.apache.cxf.phase.PhaseInterceptorChain.resume(PhaseInterceptorChain.java:277) at org.apache.cxf.ws.addressing.impl.InternalContextUtils$1.run(InternalContextUtils.java:319) at org.apache.cxf.workqueue.AutomaticWorkQueueImpl$3.run(AutomaticWorkQueueImpl.java:413) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at org.apache.cxf.workqueue.AutomaticWorkQueueImpl$AWQThreadFactory$1.run(AutomaticWorkQueueImpl.java:346) at java.base/java.lang.Thread.run(Thread.java:840) ``` I've adjusted my PR to only check for the NPE, which was causing the failures. Bascially, the introduced catch of a generic exception was hiding this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CXF-8951: Concurrent WebClient usage causes massive thread overhead [cxf]
reta commented on PR #1777: URL: https://github.com/apache/cxf/pull/1777#issuecomment-2066394896 Thanks a lot @jimma ! > For the httpclient executor, should it set to some shared new fixed size thread pool like cpu processor count *2 by default > instead of using JDK's `Executors.newCachedThreadPool()` ? I think we could have a follow up improvement to allow executor to be configurable, wdyt? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Bump org.testng:testng from 7.10.0 to 7.10.1 [cxf]
reta merged PR #1820: URL: https://github.com/apache/cxf/pull/1820 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Bump com.googlecode.maven-download-plugin:download-maven-plugin from 1.8.1 to 1.9.0 [cxf]
reta merged PR #1821: URL: https://github.com/apache/cxf/pull/1821 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
ffang commented on PR #1822: URL: https://github.com/apache/cxf/pull/1822#issuecomment-2066538612 Hi @rzo1 , Yes, this change is caused by Jetty12 upgrade. And it's good that we just catch NPE from jetty other than more generic exceptions. Thanks for reporting and fixing this! Freeman -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
reta commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572368151
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
@rzo1 thanks for catching it, @ffang do you have an idea why `request` could
be `null` in `ServletApiRequest`? (may be we have problem somewhere with Jetty
12 integration)
```
at
org.eclipse.jetty.server.Request.getAuthenticationState(Request.java:949)
at
org.eclipse.jetty.security.AuthenticationState.getAuthenticationState(AuthenticationState.java:45)
at
org.eclipse.jetty.ee10.servlet.ServletApiRequest.getAuthentication(ServletApiRequest.java:254)
at
org.eclipse.jetty.ee10.servlet.ServletApiRequest.getUndeferredAuthentication(ServletApiRequest.java:259)
at
org.eclipse.jetty.ee10.servlet.ServletApiRequest.getUserPrincipal(ServletApiRequest.java:478)
at
org.apache.cxf.transport.http.AbstractHTTPDestination$2.getUserPrincipal(AbstractHTTPDestination.java:412)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CXF-8951: Concurrent WebClient usage causes massive thread overhead [cxf]
reta merged PR #1777: URL: https://github.com/apache/cxf/pull/1777 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
rzo1 commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572491679
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
In CXF 4.0.x where was'nt a a catch block at all. So this PR only cures a
symptom and it might be worth to dig deeper on why it's actually null. Don't
know enough about CXF internals though 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
ffang commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572581658
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
I'm on it guys.
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Bump jakarta.enterprise:jakarta.enterprise.cdi-api from 4.0.1 to 4.1.0 [cxf]
dependabot[bot] commented on PR #1816: URL: https://github.com/apache/cxf/pull/1816#issuecomment-2066966329 OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting `@dependabot ignore this major version` or `@dependabot ignore this minor version`. You can also ignore all major, minor, or patch releases for a dependency by adding an [`ignore` condition](https://docs.github.com/en/code-security/supply-chain-security/configuration-options-for-dependency-updates#ignore) with the desired `update_types` to your config file. If you change your mind, just re-open this PR and I'll resolve any conflicts on it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Bump jakarta.enterprise:jakarta.enterprise.cdi-api from 4.0.1 to 4.1.0 [cxf]
reta closed pull request #1816: Bump jakarta.enterprise:jakarta.enterprise.cdi-api from 4.0.1 to 4.1.0 URL: https://github.com/apache/cxf/pull/1816 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add check to prevent redundant values in headers when using GZIPOutInterceptor [cxf]
reta commented on code in PR #1819:
URL: https://github.com/apache/cxf/pull/1819#discussion_r1572837369
##
core/src/main/java/org/apache/cxf/transport/common/gzip/GZIPOutInterceptor.java:
##
@@ -337,7 +337,10 @@ private static void addHeader(Message message, String
name, String value) {
if (header.isEmpty()) {
header.add(value);
} else {
-header.set(0, header.get(0) + "," + value);
+String existingValue = header.get(0);
+if (!existingValue.contains(value)) {
Review Comment:
I am wondering if this is even correct, why not to add values?
I believe we may just use ` header.add`, the multiple values are
concatenated together using `,` (at least, should):
```
if (!header.contains(value)) {
header.add(value);
}
```
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
ffang commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572882731
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
Hi Guys,
I think I figured out the root cause of the NPE.
This will happen in case of oneway or ReplyTo is specified when
ws-addressing is used, which means we need to switch thread context on the
server side. Basically it's pause the current interceptor chain and resume it
in another thread, and before the resumed current interceptor chain is
completed, the underlying transport might discard any data on the original
stream, that's why we need to cache the InputStream in this case and use it
later if necessary. And this peace of code is in
org.apache.cxf.ws.addressing.impl.InternalContextUtils.rebaseResponse.
Specifically we have
```
if (ContextUtils.retrieveAsyncPostResponseDispatch(inMessage)
&& !robust) {
//need to suck in all the data from the input stream
as
//the transport might discard any data on the stream
when this
//thread unwinds or when the empty response is sent
back
DelegatingInputStream in =
inMessage.getContent(DelegatingInputStream.class);
if (in != null) {
in.cacheInput();
}
}
```
Which in turn will call into
org.apache.cxf.transport.http.AbstractHTTPDestination.setupMessage
```
DelegatingInputStream in = new
DelegatingInputStream(req.getInputStream()) {
public void cacheInput() {
if (!cached && (exchange.isOneWay() ||
isWSAddressingReplyToSpecified(exchange))) {
//For one-ways and WS-Addressing invocations with
ReplyTo address,
//we need to cache the values of the HttpServletRequest
//so they can be queried later for things like paths and
schemes
//and such like that.
//Please note, exchange used to always get the "current"
message
exchange.getInMessage().put(HTTP_REQUEST, new
HttpServletRequestSnapshot(req));
}
if (req.getContentLengthLong() != 0) {
super.cacheInput();
}
}
private boolean isWSAddressingReplyToSpecified(Exchange ex) {
AddressingProperties map =
ContextUtils.retrieveMAPs(ex.getInMessage(), false, false, false);
return map != null &&
!ContextUtils.isGenericAddress(map.getReplyTo());
}
};
```
So in this case the HttpServletRequestSnapshot which is a wrapper of
original HttpServletRequest is saved in InMessage, and we should always use
pre-saved HTTP_REQUEST from InMessage to populate SecurityContext, as pre-saved
HTTP_REQUEST from InMessage could be the original HttpServletRequest or the
Cached HttpServletRequest when it's necessary.
We don't see this change is necessary for Jetty 11 and before.
Just send a PR https://github.com/apache/cxf/pull/1823 to address this for
Jetty12.
Thanks and Best Regards
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
rzo1 commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572898811
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
Nice, I will close my PR and do a test in TomEE with a local build of your
PR tmrw 🙃
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
rzo1 closed pull request #1822: Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x URL: https://github.com/apache/cxf/pull/1822 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CXF-9004]Jetty12 : always use pre-saved HTTP_REQUEST from InMessage … [cxf]
ffang merged PR #1823: URL: https://github.com/apache/cxf/pull/1823 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CXF-9004]Jetty12 : always use pre-saved HTTP_REQUEST from InMessage … [cxf]
ffang commented on PR #1823: URL: https://github.com/apache/cxf/pull/1823#issuecomment-2067229753 Thanks @reta ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Exception for getUserPrincipal() in AbstractHTTPDestination is slurped since CXF 4.1.x [cxf]
ffang commented on code in PR #1822:
URL: https://github.com/apache/cxf/pull/1822#discussion_r1572923309
##
rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java:
##
@@ -410,8 +410,12 @@ private boolean isWSAddressingReplyToSpecified(Exchange
ex) {
public Principal getUserPrincipal() {
try {
return req.getUserPrincipal();
-} catch (Exception ex) {
-return null;
+} catch (Exception e) {
Review Comment:
Hi @rzo1 ,
PR merged to main branch, so probably the snapshot build will be available
from Apache Snapshot maven repo by tomorrow.
Cheers
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cxf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
