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

"Just" dropping in jax-rs-pac4j-2.0.0-RC1.jar breaks webapp #18

Closed
rwalkerands opened this issue Mar 8, 2017 · 19 comments
Closed

"Just" dropping in jax-rs-pac4j-2.0.0-RC1.jar breaks webapp #18

rwalkerands opened this issue Mar 8, 2017 · 19 comments
Labels
Milestone

Comments

@rwalkerands
Copy link

I'm just (today) getting started with pac4j, and I'm adding it to an existing webapp built on Jersey. (I use Tomcat as my Servlet container.)

I have happily been using Jersey's (JAX-RS's) auto-scanning to find resources and providers, i.e., I have no Application or ResourceConfig subclass.

My setup is as described at https://jersey.java.net/documentation/latest/deployment.html#deployment.servlet.3.pluggability.noapp (4.7.2.3.1. JAX-RS application without an Application subclass).

I started by dropping in the pac4j-core-2.0.0-RC1.jar and a few others (for http, jwt, ldap). No problem with the webapp starting up after that.

But then if I add jax-rs-pac4j-2.0.0-RC1.jar, I now get an error on startup:

Mar 08, 2017 4:49:01 PM org.apache.catalina.core.ApplicationContext log
SEVERE: StandardWrapper.Throwable
MultiException stack 1 of 1
java.lang.NoSuchMethodException: Could not find a suitable constructor in org.pac4j.jax.rs.features.Pac4JSecurityFeature class.
	at org.glassfish.jersey.internal.inject.JerseyClassAnalyzer.getConstructor(JerseyClassAnalyzer.java:192)
	at org.jvnet.hk2.internal.Utilities.getConstructor(Utilities.java:179)
	at org.jvnet.hk2.internal.Utilities.justCreate(Utilities.java:993)
	at org.jvnet.hk2.internal.ServiceLocatorImpl.create(ServiceLocatorImpl.java:963)
	at org.jvnet.hk2.internal.ServiceLocatorImpl.createAndInitialize(ServiceLocatorImpl.java:1055)
	at org.jvnet.hk2.internal.ServiceLocatorImpl.createAndInitialize(ServiceLocatorImpl.java:1047)
	at org.glassfish.jersey.model.internal.CommonConfig.configureFeatures(CommonConfig.java:696)
	at org.glassfish.jersey.model.internal.CommonConfig.configureMetaProviders(CommonConfig.java:644)
	at org.glassfish.jersey.server.ResourceConfig.configureMetaProviders(ResourceConfig.java:829)
	at org.glassfish.jersey.server.ApplicationHandler.initialize(ApplicationHandler.java:453)
	at org.glassfish.jersey.server.ApplicationHandler.access$500(ApplicationHandler.java:184)
	at org.glassfish.jersey.server.ApplicationHandler$3.call(ApplicationHandler.java:350)
	at org.glassfish.jersey.server.ApplicationHandler$3.call(ApplicationHandler.java:347)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:315)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:297)
	at org.glassfish.jersey.internal.Errors.processWithException(Errors.java:255)
	at org.glassfish.jersey.server.ApplicationHandler.<init>(ApplicationHandler.java:347)
	at org.glassfish.jersey.servlet.WebComponent.<init>(WebComponent.java:390)
	at org.glassfish.jersey.servlet.ServletContainer.init(ServletContainer.java:172)
	at org.glassfish.jersey.servlet.ServletContainer.init(ServletContainer.java:364)
	at javax.servlet.GenericServlet.init(GenericServlet.java:158)
	at org.apache.catalina.core.StandardWrapper.initServlet(StandardWrapper.java:1284)
	at org.apache.catalina.core.StandardWrapper.load(StandardWrapper.java:1090)
	at org.apache.catalina.core.StandardContext.loadOnStartup(StandardContext.java:5266)
	at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5554)
	at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:150)
	at org.apache.catalina.core.StandardContext.reload(StandardContext.java:4033)
	at org.apache.catalina.loader.WebappLoader.backgroundProcess(WebappLoader.java:425)
	at org.apache.catalina.core.ContainerBase.backgroundProcess(ContainerBase.java:1345)
	at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1546)
	at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1556)
	at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1556)
	at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.run(ContainerBase.java:1524)
	at java.lang.Thread.run(Thread.java:745)

And then every request generates an exception (javax.servlet.ServletException: Servlet.init() for servlet javax.ws.rs.core.Application threw exception ...).

So is it the case that I can't keep using Jersey's auto-scanning if I want to use jax-rs-pac4j?
Do I have to create an Application/ResourceConfig subclass?

The README.md seems to assume I have already done that.

I think it would be "nice" for users (not just me) if you could support configuring the providers and features when using the other "modes" of defining resources, i.e., so that you don't have to define an Application/ResourceConfig subclass "just" to configure jax-rs-pac4j.

I should say: I have been able to create a ResourceConfig subclass that "works": webapp startup no longer fails, and resources seem to respond correctly. But now I "worry" if I'm missing something by bypassing Jersey's auto-scanning.

@victornoel
Copy link
Member

You are right, nothing was ever done to make it work correctly with autoscanning and we should fix that :)

The problem is that all the features in jax-rs-pac4j need a pac4j Config to work, so it is not possible to simply mark them as @Provider (which I did, that's why Jersey tries to load it via autoscanning and fails, because it does not make sense without a pac4j Config).

I'm not so familiar with autoscanning, maybe there is ways for you to declare the pac4j Config in a way that can be exploited by autoloaded jax-rs features.

The first step I will take is remove the problematic @Provider on Pac4JSecurityFeature.

For the second step, I will need some time (or help) to find an elegant way to make it work with autoscanning.
How were you expecting to specify the pac4j Config originally?

victornoel added a commit that referenced this issue Mar 8, 2017
victornoel added a commit that referenced this issue Mar 8, 2017
@victornoel
Copy link
Member

I committed the first step.
For now, you could try to see if it is possible to disable autoscanning for a single package… or maybe rely on the snapshot version of jax-rs-pac4j.
It is fixed both in 1.x and 2.x :)

@victornoel victornoel added the bug label Mar 8, 2017
@rwalkerands
Copy link
Author

How were you expecting to specify the pac4j Config originally?

I had no expectations at all.

However, is it possible to do it in a way that is similar to what you are already doing (and the way that JAX-RS does it in general): i.e., by allowing the user to define a @Provider that implements an interface that is specifically for initialization? E.g., something like JAX-RS's ContextResolver interface.

@victornoel
Copy link
Member

However, is it possible to do it in a way that is similar to what you are already doing (and the way that JAX-RS does it in general): i.e., by allowing the user to define a @Provider that implements an interface that is specifically for initialization? E.g., something like JAX-RS's ContextResolver interface.

That would make sense yes! As I said, I'm not familiar with these aspects of JAX-RS so your input is helpful :)

What bother me (only as a perfectionist :) with ContextResolver is that it is meant to be used at runtime in the context of a given request (Providers takes a MediaType for example to resolve a given ContextResolver). Actually I use it at runtime under the hood to provide pac4j's WebContext to the filters but it's not very elegant: https://github.com/pac4j/jax-rs-pac4j/blob/master/src/main/java/org/pac4j/jax/rs/filters/AbstractFilter.java#L39

Still, we can use it anyway, but I wonder if there is not a better way of injecting something into a feature like we want to… And actually, I'm not even sure that during Feature initialisation, it is possible to already access a ContextResolver, I will have to check.

@rwalkerands do you know of other means to do this kind of things? I did a quick search but couldn't find anything applicable… thanks for your hep :)

@rwalkerands
Copy link
Author

I am no expert on JAX-RS, just a "happy customer" of some of the most basic features.

In particular, I was not suggesting using ContextResolver, but "something like" ContextResolver. But perhaps even that suggestion was wrong. :-(

@victornoel
Copy link
Member

@rwalkerands it was a good suggestion :) I'm just looking for the idiomatic way to do what we are trying to do and was wondering if you knew. I will continue looking a bit and then implement it during the week-end I think.

@victornoel
Copy link
Member

Yep, no other way than using ContextResolver, I will manage with that :)

@victornoel
Copy link
Member

@rwalkerands It's not exactly what you wanted, but as a starter, could you try to see if the following work with you:

  • Use jax-rs-pac4j 2.0.0-RC2-SNAPSHOT
  • Create the following two classes in your classpath to set it up:
@Provider
public class Pac4JFeature implements Feature {

    @Override
    public boolean configure(FeatureContext context) {
        context
            //.register(new JaxRsConfigProvider(config))
            .register(new Pac4JSecurityFeature())
            .register(new Pac4JValueFactoryProvider.Binder())
            .register(new ServletJaxRsContextFactoryProvider());

        return true;
    }
}
@Provider
public class Pac4JConfig implements ContextResolver<Config> {

    @Override
    public Config getContext(Class<?> type) {
        JaxRsConfig config = new JaxRsConfig();
        
        // setup the pc4j configuration
        
        return config;
    }
}

Note that you can either setup the Config from Pac4JConfig or from Pac4JFeature and uncomment the commented register for JaxRsConfigProvider.

My current problem is that for now, jax-rs-pac4j contains multiple implementations of mutually exclusive Provider (depending on the container), so if we annotate all of them, I think it will break stuffs, and if we annotate none, you will need the above Feature.
The obvious solution would be to decompose jax-rs-pac4j in multiple maven artefacts but it's an heavy process, so I'm still looking for simpler solutions...
Until then, it would be cool if you could tell me if the above solution works and what you think of it :)

Thanks for your help!

@rwalkerands
Copy link
Author

I was able to get it to work, though it wasn't as straightforward as you suggest ... I needed to use the snapshot of pac4j-core as well, because you are relying on it (e.g., the change from JaxRsCallbackUrlResolver to JaxRsUrlResolver). That meant also changing my AuthorizationGenerator (it must now also take a Context parameter and return the Profile).

Also, it is important to implement the Pac4JConfig.getContext() method as a singleton method. (E.g., define a private static field which is initialized only once, and which is returned on all subsequent method calls.) Otherwise, that initialization code is repeated for every request.

(I am using AnonymousClient. Before making the getContext() method into a singleton, I was seeing the warning message AnonymousClient is an advanced feature: be careful when using it to avoid any security issue! three times for every request.)

@victornoel
Copy link
Member

Ha yes, good points!

You are right, it's not correct to build the Config everytime the ContextResolver is used, I forgot about that…
Still, are you sure you need the field to be static in the ContextResolver? I would expect Jersey to instantiate it only once (so you initialise the Config in the constructor for example), but call the method multiple times, but I admit I haven't tried :)
And if not, you could try adding a @Singleton annotation to the Pac4JConfig class, in order to avoid using a static at least (statics are ugly :).

Anyway, except for the bother of porting your application to latest pac4j snapshot and the need to ensure the Config is created only once, do you see other shortcoming of doing things like this or other problems I haven't noticed?

@rwalkerands
Copy link
Author

rwalkerands commented Mar 14, 2017

are you sure you need the field to be static in the ContextResolver?

You are right. The field does not have to be static.

I don't (yet) see any other problems.

👍

@victornoel
Copy link
Member

Thanks for the feedback!

So for now, you can either:

  1. use pac4j 1.2.2 (well, when it will be released, but if you tell us you are going to use it, we will release it very soon since there is no other bugs waiting to be fixed on it) and setup things by hand (I only removed the @Provider in the 1.x branch, not introduced the use of a ContextResolver for the Config, but it shouldn't be so hard to setup still).
  2. wait for the 2.x release to be finished. I'm not sure what is the expected timeline of pac4j, but I don't think it will be finished before 1 month at least. I will see if it is practical to make things work without the Pac4JFeature I talked about before.

I would recommend going with the 1.2.2 since it is more stable anyway :)

@leleuj
Copy link
Member

leleuj commented Mar 15, 2017

  1. pac4j v2.0.0-RC2 will be released next week along the implementations depending on it and then, only bugs will be fixed for a final release at the end of April or beginning of May.

@victornoel
Copy link
Member

ok, great, thanks for the extra information :)

By the way, I wanted to ask you @leleuj, our problem here is that JAX-RS has a mechanism to autoload some modules (those annotated with @Provider) without explicitly configuring them (except putting their jar in the classpath).
It is great but the problem is that jax-rs-pac4j is containing multiple modules for different jax-rs containers (jersey vs resteasy, undertow vs grizzly, servlet vs non-servlet, etc), so for this autoload mechanism to work nicely, we would need to have multiple jars (i.e. multiple maven project), one for each of the modules (and people would depends on multiple jars then).
On the other hand, it is possible to circumvent this situation by not annotating the modules and to let people define a module like explained in #18 (comment).

What do you think of this situation and which solution seems to be better? I would go for the second one (with good documentation) because it is less hassle :)

@rwalkerands
Copy link
Author

rwalkerands commented Mar 15, 2017

You wrote:

  1. use pac4j 1.2.2

Actually, I am a bit confused. Did you mean to write "jax-rs-pac4j 1.2.2", or "pac4j 1.9.7", or ...?

As I mentioned, I started using pac4j only last week, with v2.0.0-RC1. Whether this was a good idea ... anyway, what I have, is working. So I don't think I need/want to go "back" to pac4j 1.9.x.

@leleuj
Copy link
Member

leleuj commented Mar 16, 2017

pac4j 1.2.2 must have been abandoned for years, it was about using jax-rs-pac4j 1.2.2. In all cases, we focus our efforts on the incoming version 2.0.0; so it's better to start using it.

@victornoel : having multiple modules seems a huge burden to me, I would choose to not annotate the modules and document the @Provider solution. We can revisit this choice if the question arises again and again.

@victornoel
Copy link
Member

Ok, I will document that, cleanup things and then close this issue which should integrate 2.0.0-RC2 :)

@victornoel victornoel added this to the 2.0.0-RC2 milestone Mar 16, 2017
@victornoel
Copy link
Member

@rwalkerands FYI, jax-rs-pac4j 2.0.0-RC2 was released today, I will soon document what I explained here in the README, but you can already start using it (and report any problem that you have with it ;). Thanks again (I will close this issue when the documenting is done!)

victornoel added a commit that referenced this issue Apr 16, 2017
Also add some badges
@victornoel
Copy link
Member

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

No branches or pull requests

3 participants