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

Pac4JProfileInjectorFactory disable CDI Injection Resteasy #34

Closed
jarnaiz opened this issue Jan 19, 2018 · 12 comments
Closed

Pac4JProfileInjectorFactory disable CDI Injection Resteasy #34

jarnaiz opened this issue Jan 19, 2018 · 12 comments
Labels

Comments

@jarnaiz
Copy link

jarnaiz commented Jan 19, 2018

Im using jax-rs-pac4j with resteasy and wildfly 10.1.0.Final Im using this config:

@Provider
public class Pac4JFeature implements Feature {

    @Inject
    @Props("vodafone")
    protected Properties properties;

    public Config getConfig() {

        OidcConfiguration config = new OidcConfiguration();

        config.setClientId(properties.getProperty("oidc.client-id"));
        config.setSecret(properties.getProperty("oidc.client-secret"));
        config.setDiscoveryURI(properties.getProperty("oidc.discoveryURL"));
        //
        OidcClient oidcClient = new OidcClient(config);
        oidcClient.setName("appDirect");
        Config result = new Config(oidcClient);
        result.getClients().setDefaultClient(oidcClient);
        result.getClients().setUrlResolver(new JaxRsUrlResolver());
        result.getClients().setCallbackUrl("/openid/login/check");
        return result;
    }

    @Override
    public boolean configure(FeatureContext context) {

        context
            .register(new JaxRsConfigProvider(getConfig()))
            .register(new Pac4JSecurityFeature())
            /*
             Pac4JProfileInjectorFactory breaks CDI injection
             .register(new Pac4JProfileInjectorFactory()) // only with Resteasy
             */
            .register(new ServletJaxRsContextFactoryProvider());

        return true;
    }
}

And this controller:

@Path("/")
@Stateless
public class OpenIdController {

    private static final Logger log = LoggerFactory.getLogger(OpenIdController.class);

    @Inject
    private TokenLogin tokenLogin;

    @Path("/openid/login")
    @Pac4JSecurity(clients = "appDirect")
    @GET
    public Response login(@Context SecurityContext sec, @Pac4jProfile CommonProfile profile) {
        log.info("SecurityContext: {}", sec);
        log.info("Logged, redirecting to dashboard the user {}", profile);

        return buildLoginToken(profile.getEmail());
    }

    @GET
    @Path("/openid/login/check")
    @Pac4JCallback(skipResponse = true)
    public Response loginCB(@Context SecurityContext sec, @Pac4jProfile CommonProfile profile) {
        if (profile != null) {
            log.info("SecurityContext: {}", sec);
            log.info("Logged, redirecting to dashboard the user {}", profile);
            return buildLoginToken(profile.getEmail());
        } else {
            throw new WebApplicationException(401);
        }
    }

    private Response buildLoginToken(String email) {
        String token = tokenLogin.create(email);
        return Response.temporaryRedirect(URI.create("https://dummyyeah.com?token=" + token)).build();
    }
}

The class TokenLogin is a POJO with any annotations. If I add the Pac4JProfileInjectorFactory to FeatureContext context, the tokenLogin is not injected and is null and without the Pac4JProfileInjectorFactory the tokenLogin is injected as expected.

My workaround is this:

@Priority(Priorities.AUTHORIZATION + 50)
@Provider
public class Pac4JProfileInterceptor implements ContainerRequestFilter {

    @Context
    private HttpServletRequest req;
    @Context
    private HttpServletResponse res;

    @Override
    public void filter(ContainerRequestContext requestContext) throws IOException {
        Optional<CommonProfile> profile = new ProfileManager<>(new J2EContext(req, res)).get(true);
        if (profile.isPresent()) {
            ResteasyProviderFactory.pushContext(CommonProfile.class, profile.get());
        }
    }
}

And replace all the @Pac4jProfile with @Context.
Another option is to cast the SecurityContext#getPrincipal

Im doing something wrong or is a bug?
Thanks!

@victornoel
Copy link
Member

@jarnaiz thanks for the report.

To be sure I understand, the situation is that:

  • With Pac4JProfileInjectorFactory, TokenLogin is not injected but the @Pac4jProfile CommonProfile profile is as expect.
  • Without Pac4JProfileInjectorFactory, TokenLogin is injected as expected but then of course @Pac4jProfile CommonProfile profile is not.

Your problem is that you want both (which makes sense :) ?

Could you clarify to me what is this TokenLogin thing? Is that something from resteasy? or is that a class you introduce but you rely on some documented behaviour of resteasy to be injected as desired? Which documented behaviour is it exactly?

The workaround is applied while you don't register Pac4JProfileInjectorFactory right?

@jarnaiz
Copy link
Author

jarnaiz commented Jan 22, 2018

Hello @victornoel

You are right in all the questions:

  • With Pac4JProfileInjectorFactory the parameters @Pac4jProfile CommonProfile profile is injected as expected but the TokenLogin is not.
  • Without Pac4JProfileInjectorFactory, TokenLogin is injected as expected but then of course @Pac4jProfile CommonProfile profile is not.

I want to inject both, yes :)

The TokenLogin is a POJO created by me and put in the same maven module.
Also I have the beans.xml in the META-INFwith the content:

<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://xmlns.jcp.org/xml/ns/javaee"
       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/beans_1_1.xsd"
       version="1.2" bean-discovery-mode="all">
</beans>

And with CDI 1.2 with discovery-mode=all any class can be injected with @Inject with any other configuration or annotation.

The Java CDI documentation is hard and extensive.. I found this document from the wildfly implementation: http://weld.cdi-spec.org/documentation/

@victornoel
Copy link
Member

Ok, so it is most certainly a bug, but I'm a bit out of my depth concerning CDI and how the implementation for profile injection is breaking it...

Maybe @yegorius, since you first wrote Pac4JProfileInjectorFactory, you could tell us what you make of this and see what could be the problem? :)

For the record, the only advantage of using the shipper injection mechanism (Pac4JProfileInjectorFactory) over your workaround is that there are extra checks made in Pac4JProfileInjectorFactory.

@victornoel victornoel added the bug label Jan 27, 2018
@yegorius
Copy link

Please read this Reasteasy doc very carefully. It says:

Without the integration code, annotating a class suitable for being a CDI bean with JAX-RS annotations leads into a faulty result (JAX-RS component not managed by CDI).

Pac4JProfileInjectorFactory correctly implements InjectorFactoryImpl interface provided by Resteasy. It obeys API contract by calling super.createParameterExtractor(...). So far I don't see any faulty behaviour in Pac4JProfileInjectorFactory and I think the problem stems from the aforementioned conflict between JAX-RS and CDI.

@victornoel
Copy link
Member

victornoel commented Jan 28, 2018

Thanks for your feedback!

I also read this:

During a web service invocation, resteasy-cdi asks the CDI container for the managed instance of a JAX-RS component. Then, this instance is passed to RESTEasy. If a managed instance is not available for some reason (the class is placed in a jar which is not a bean deployment archive), RESTEasy falls back to instantiating the class itself.

From what I understand, maybe something like this is happening:

  • CDI tries to instantiates the @Path annotated component
  • it fails because of our injector or something like that
  • resteasy takes over to instantiate but then does not instantiate the CDI related things

@jarnaiz do you have maybe some logs that would show us more details about this to confirm or infirm our analysis? Do you use the resteasy-cdi dependency?

@victornoel
Copy link
Member

You could also try to remove the @Stateless annotation, since this is not stateless because of our injector that relies on the current request!

@jarnaiz
Copy link
Author

jarnaiz commented Jan 31, 2018

ok, thanks @victornoel I will try what you have told me (remove @stateless)

Regarding your question: Do you use the resteasy-cdi dependency? Yes I have it.

@ganeshs
Copy link

ganeshs commented Apr 25, 2018

@jarnaiz @victornoel I have made some some changes in #42 that could possibly fix this issue as well. I have added tests for resteasy with CDI as part of that.

@jarnaiz you could possibly take those changes and see if it helps your case. https://github.com/pac4j/jax-rs-pac4j/pull/42/files#diff-e6b8d4359ecd20a7589465155d7f2ebe

@victornoel
Copy link
Member

@jarnaiz yes please do so :)
thanks a lot @ganeshs

@victornoel
Copy link
Member

@jarnaiz can you confirm that latest 3.0.0-SNAPSHOT (or 2.3.0-SNAPSHOT if you are still using pac4j v2) fixes this problem?
The documentation and the tests now contain some explanation as to how to configure cdi with resteasy.

@victornoel
Copy link
Member

@jarnaiz did you get the opportunity to see if things were working as expected? For the record, we are going to release 2.3.0 and 3.0.0 soon (see #44)

@victornoel
Copy link
Member

I'm closing this, please open a new issue if this is still a problem :)

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

4 participants