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_r1575690967
##
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:
@ffang https://issues.apache.org/jira/browse/CXF-9005
--
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_r1575258821
##
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:
@ffang Did some more tests. 4.1.0-SNAPSHOT only produces 3 JAX-RS TCK
Failures (3.1.5) - quite promising :)
--
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_r1574802910
##
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:
Awesome! Thanks for the update @rzo1 !
--
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_r1573294891
##
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 @ffang ,
TomEE is happy with your fix. CI is green again ;-)
Thx for it !
Gruß
Richard
--
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
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] 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]
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]
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] 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]
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] 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]
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
