Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getRequest method to RequestFacade, to get the wrapped Request #399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arjantijms
Copy link

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
Copy link
Contributor

@arjantijms why not using a valve as most integrators?

@arjantijms
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

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

@rmannibucau
Copy link
Contributor

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

@rmaucher
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants