Add getRequest method to RequestFacade, to get the wrapped Request

[arjantijms]

For consistency with Request, and practically for external code that needs to integrate with Tomcat and e.g. needs access to the notes.

Signed-off-by: arjantijms [email protected]

[rmannibucau]

@arjantijms why not using a valve as most integrators?

[arjantijms]

@arjantijms why not using a valve as most integrators?

You can't add a valve from your war, so that puts additional constraints on the user. For a simple integration with Jakarta Security, not much is needed really, I only need to obtain the Subject which the Tomcat code sets via a note:

https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/authenticator/AuthenticatorBase.java#L973

It's much easier to just include (say) Soteria in your war as a dependency, than it is to instruct the user to modify an existing Tomcat installation every time.

That said, for tighter integration, I need to go the Valve way and specifically install a custom Realm.

For the general utility of this PR, I think it's always a good idea to be able to access the object you're wrapping. Many other wrapper types have this in Tomcat, and all Servlet wrappers have this too.

[rmannibucau]

@arjantijms I see, my point is that the facade is about preventing the user to access the internals (request here) or any API not from the spec so I don't think it should be broken. It is done for all servlet code (look for all *Facade instances) and it generally prevents to access any internal from a spec code (you see it the other way for your case but it is designed the other way). I see a few options to help - there are probably more:

1. Add an "app valve" in tomcat code, it would reuse tomcat reflection factory to instantiate lazily - when app loader exists - a valve. it is close to https://github.com/apache/tomee/blob/master/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/valve/LazyValve.java but only using tomcat internal. It enables you to write a valve in your war without tomcat tuning. Similar solution would enable a valve to fail to be created from the context.xml but it would be retried when the app loader exists (I prefer previous option which is clearer on when it must work). I know it is not tomcat habit to enable internals from the app but I agree it is not uncommon so having some wrappers enabling it can help - tomee also has the real wrapper since you mentionned it.
2. Use reflection as several other integrators
3. Drop jaspic from tomcat and use plain servlet filters (big +1 from me since it would also make jaspic optional for tomcat embed)
4. Use GenericPrincipal (or TomcatPrincipal/ a new JaspicWrappingPrincipal) to host the subject instead of a note
5. add in base authenticator an attribute to set it in a request attribute too (likely the easiest)

[markt-asf]

Valves can be installed via a WAR. You can specify an application specific Valve in a META-INF/context.xml file included in the WAR.
I share the concern that adding a getRequest() method may undermine the use of a facade.

[arjantijms]

I'll investigate the application specific Valve. Can META-INF/context.xml exist on the class path as well, or does it have to be inside [war root]/META-INF?

Use reflection as several other integrators

I'm now using reflection indeed:

    private static Subject getSubject(HttpServletRequest httpServletRequest) {
        return (Subject) getRequest(httpServletRequest).getNote(REQ_JASPIC_SUBJECT_NOTE);
    }

    private static Request getRequest(HttpServletRequest servletRequest) {
        return getRequest((RequestFacade) servletRequest);
    }

    private static Request getRequest(RequestFacade facade) {
        try {
            Field requestField = RequestFacade.class.getDeclaredField("request");
            requestField.setAccessible(true);

            return (Request) requestField.get(facade);
        } catch (NoSuchFieldException | SecurityException | IllegalArgumentException | IllegalAccessException e) {
            throw new IllegalStateException(e);
        }
    }

@markt-asf I'm working on a standalone implementation of Jakarta Authorization (JACC), and so far it works really well on Tomcat (and Piranha). It's actually interesting to see how the JACC way to match URL patterns and the Tomcat native way are really not that different.

[arjantijms]

I share the concern that adding a getRequest() method may undermine the use of a facade.

Isn't the facade intended as a service to the user to not accidentally access specifics? It's not a protection mechanism, is it? As with reflection one can get to the wrapped instance anyway.

With a getRequest() method, the user has to do 2 very specific actions: cast to RequestFacade and then call that method. Compare to e.g. JPA's EntityManager, which is a facade for (mainly) Hibernate, but still has a method to get the underlying Hibernate specific manager.

[rmannibucau]

Guess a sane EE way to solve that is to propose to servlet spec to add an unwrap(Class type) method in most of its objects (or Unwrappable instance).
It would solve a lot of issues with such pattern - being said the getRequest() does not really work in all cases in your example - and enables to traverse proxies/wrappers.
Wdyt?

[arjantijms]

being said the getRequest() does not really work in all cases in your example

In what cases doesn't it work?

Guess a sane EE way to solve that is to propose to servlet spec to add an unwrap(Class type) method in most of its objects

Might not be a bad idea, thanks!

[rmannibucau]

@arjantijms when you can't cast the request because a filter/valve wrapped it (even if a bit nasty for valves it can happen) which is not uncommon for session distribution, security (potentially ran before jaspic) or framework (cdi for ex) setup. I know we are all tempated to say security first but actually it is common to put distribution and framework before just to ensure security context exists properly.

[arjantijms]

@rmannibucau oh yeah, of course, then it won't work. It'll throw an exception, so at least you know it won't work ;)

requestInitialized() is really early though, so in most cases should be early enough. Just to be sure I'll take your suggestion and add a default unwrapFully:

    @SuppressWarnings("unchecked")
    private <T extends ServletRequest> T unwrapFully(ServletRequest request) {
        ServletRequest currentRequest = request;
        while (currentRequest instanceof ServletRequestWrapper) {
            ServletRequestWrapper wrapper = (ServletRequestWrapper) currentRequest;
            currentRequest = wrapper.getRequest();
        }
        return (T) currentRequest;
    }

Thanks!

[rmannibucau]

@arjantijms maybe test the type and throw an exception with the request type when it is not a wrapper, some people do it 👼 ;)

[rmannibucau]

@markt-asf can we add a org.apache.catalina.api.Unwrappable and make the facade objects impl it? Would it be an option?

[rmaucher]

The plan was to keep the Servlet container implementation classes away from the webapps, so this is a request to do the opposite. It's hard to see how this concept can ever be a good idea.

[arjantijms]

It's hard to see how this concept can ever be a good idea.

I personally think it's a brilliant idea. Then again, I'm probably biased ;)

[rmannibucau]

@rmaucher it is not about letting a webapp use internal and by default I agree it should stick to the spec API, but it is common to need to go further, in particular when combining some servlet with valves or realms, and in this case being able to link both is important and can need the request access (even potentially the coyote one!). Enabling it by allowing to cast to "unwrappable" is good enough IMHO. In any case it is not worse than today in terms of access since today you can do it using reflection, it is just about making it more fluent for end users IMO. In TomEE we do it in several places and even unwrap the request until the context, having an unwrappable would make it smoother for ex, without breaking the facade concept at all. Only constraint is to not add to facade API any method out of the spec but another interface implemented by the same object does not violate this rule.

[markt-asf]

I would be against exposing container internals via the Servlet API as it undermines portability.

What information, specifically, do you need to access and how (read, write)? I think I'd rather a solution that provided the specific access required. An added complication is that Tomcat has always aimed to support the use case of running "untrusted" apps. As such providing all apps with write access to anything considered security sensitive is going to be tricky and would probably require an configuration option to explicitly enable it.

[rmannibucau]

@markt-asf requests notes. How adding RequestFacade implements HttpServletRequest, Unwrappable is less secure than current impl? if it helps I guess you can use the security manager if set un unwrap(), would be 1-1 in terms of security with current state where using reflection falls in the same bucket, no?

[rmaucher]

@markt-asf If you really want to encourage this, there is the privileged flag on the Context that allows using container servlets.
public Request getRequest() {
if (request.getContext().getPrivileged()) { return request; } else throw new SecurityException(); // :)
}

[arjantijms]

I would be against exposing container internals via the Servlet API as it undermines portability.

Well, actually it could be used by integration libraries to improve portability, the exact opposite. Integration libraries enumerate all known implementations, adapting the internals of each to some common functionality.

As an example, look at how Soteria is able to implement an SPI for Weld:

https://github.com/eclipse-ee4j/soteria/blob/master/spi/bean-decorator/weld/src/main/java/org/glassfish/soteria/spi/bean/decorator/weld/WeldBeanDecorator.java

It allows to get the Weld specific BeanManager from the spec compliant BeanManager:

BeanManagerProxy.unwrap(beanManager));

Another example, using enumeration of known implementations:

https://github.com/omnifaces/exousia/blob/master/src/main/java/org/omnifaces/exousia/spi/impl/DefaultRoleMapper.java#L374

It uses reflection, but you may get the idea.

Without using reflecting and specifically for Tomcat:

https://github.com/omnifaces/exousia/blob/master/src/main/java/org/omnifaces/exousia/spi/integration/IntegrationInitializer.java#L42

It now only has Tomcat, but I'm planning to add Jetty there later.

The internal APIs are kinda used for essentially a polyfill.

[arjantijms]

Yet another example, Weld is using similar tricks to integrate with Tomcat:

https://github.com/weld/core/blob/master/environments/servlet/core/src/main/java/org/jboss/weld/environment/tomcat/WeldForwardingInstanceManager.java#L97

  // Hack into Tomcat to replace the InstanceManager using
  // reflection to access private fields
  ApplicationContext appContext = (ApplicationContext) getContextFieldValue((ApplicationContextFacade) context, ApplicationContextFacade.class);

It's not the average application that needs this, but libs like Weld (CDI) or Soteria (Jakarta Security) definitely have a need for this.