[SHIRO-337] basic cdi support

[rmannibucau]

[jeansossmeier]

Will this be available in 1.4.0? I wrote my own Shiro CDI support, but having this built in is just great!

[bdemers]

@feverdog not sure, but please try out the code and see if works for your use cases!

[jeansossmeier]

Well, reading the code it seems awesome to me but I haven't really tested it because, unfortunately, to save time, I've decided to use what I already had and mix with this commit's @Nonbinding wrapper to the existing Shiro annotations, a great alternative that works like a charm.

One thing that I would take note is that on ShiroExtension.java, I have removed the following code:

        for (final Class<?> type : asList(
                ShiroInterceptorBridge.RequiresRolesInterceptor.class,
                 ShiroInterceptorBridge.RequirePermissionsInterceptor.class,
                 ShiroInterceptorBridge.RequiresAuthenticationInterceptor.class,
                 ShiroInterceptorBridge.RequiresUserInterceptor.class,
                 ShiroInterceptorBridge.RequiresGuestInterceptor.class)) {
             beforeBeanDiscovery.addAnnotatedType(bm.createAnnotatedType(type));
         }

This snippet caused double intecepting annotated methods for me - I'm using OpenWebBeans and DeltaSpike.

[rmannibucau]

Tested with openwebbeans too (deltaspike doesnt do anything there I think). This code was to ensure the interceptors are used even if not in a scanned module (useful for some containers and deployment mode). If scanned both metadata should be merged since there is no id provided. Not sure how you can get a double interception, double interceptor binding in your mix?

[bdemers]

I had a chance to take a closer look at this yesterday, I'll leave comments inline.

[bdemers]

Bunch of comments above, nothing that seems major though.

[bdemers]

@rmannibucau thanks again for working on this! let me know if you think it would be easier to track this as an official feature branch, we create a JIRA component as well if that helps track anything else that is needed.

[rmannibucau]

think you have more cards than me for that but happy with both options

[bdemers]

Actually, yeah, lets use the branch cdi-feature, so it can be setup in CI and this thread will be easier to follow. I also just added a CDI JIRA component, we can be pretty relaxed with it initially though.

@rmannibucau if you can re-target this pull request to that branch that would be great.

[rmannibucau]

oki, will do as soon as it hits github asf copy

edit: @bdemers did you create this branch or did you mean cdi-idea?

[xfh]

Is there a way I could test this in a Java EE7 application? Could you give me some help regarding the configuration? I am currently running Shiro with an INI Configuration and custom EnvironmentLoaderListener, to manually configure my realms.

I am facing a problem with EJB pooled threads and stale SecurityManager/Subject data. Due to Shiro's ThreadContext binding these pooled threads are only properly initialised for the first subject that uses them. Every other subject reusing a pooled thread will unintentionally impersonate the first subject. That is a complete show stopper for me. However, I really like Shiro's flexible permission architecture, so I was wondering if CDI injection would as a side effect solve my problem.

[rmannibucau]

@xfh depends how you use your EJBS:

• @Remote (but not through http of your app)/@Scheduled/@Asynchronous: shiro doesnt have yet an integration for that, you need to write your own interceptor setting the context based on your business requirement
• through a http request: this PR should work but shiro-web too

[xfh]

I am using @Scheduled & @Asynchronous. Could you point me towards the right direction, so I could implement such an interceptor?

[rmannibucau]

maybe have a look to https://github.com/apache/shiro/pull/58/files#r101155240

[xfh]

I'll give it a try, but I am quite inexperienced with proxys and interceptors. Do you have a demo setup to play with the CDI integration?

[rmannibucau]

dont have one there cause no idea how to get the context but on EE 7 you just do:

@Interceptor
@MyShiroBinding
@Priority(/*put a number matching your need*/)
public class MyShiroInterceptor {
    @AroundInvoke public Object around(InvocationContext ctx) throws Exception {
        Object old = setShiroSubject();
        try {
            return ctx.proceed();
        } finally {
            resetShiroSubject(old);
        }
    }
}

Tip: don't forget you can kind of stack contexts, that's why old is used to reset to previous state.

[xfh]

Thanks, that looks promising!

[fpapon]

Hi @rmannibucau thanks for this PR! I will make some test case with the current master (1.5.0).

[asfgit]

Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/Shiro-pr/109/

Build result: FAILURE

[...truncated 1.16 MB...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/jaxrs/target/shiro-jaxrs-1.4.0-SNAPSHOT-javadoc.jar to org.apache.shiro/shiro-jaxrs/1.4.0-SNAPSHOT/shiro-jaxrs-1.4.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/jaxrs/target/shiro-jaxrs-1.4.0-SNAPSHOT-sources.jar to org.apache.shiro/shiro-jaxrs/1.4.0-SNAPSHOT/shiro-jaxrs-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/samples/guice/pom.xml to org.apache.shiro.samples/samples-guice/1.4.0-SNAPSHOT/samples-guice-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/crypto/cipher/pom.xml to org.apache.shiro/shiro-crypto-cipher/1.4.0-SNAPSHOT/shiro-crypto-cipher-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/crypto/cipher/target/shiro-crypto-cipher-1.4.0-SNAPSHOT.jar to org.apache.shiro/shiro-crypto-cipher/1.4.0-SNAPSHOT/shiro-crypto-cipher-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/crypto/cipher/target/shiro-crypto-cipher-1.4.0-SNAPSHOT-javadoc.jar to org.apache.shiro/shiro-crypto-cipher/1.4.0-SNAPSHOT/shiro-crypto-cipher-1.4.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/crypto/cipher/target/shiro-crypto-cipher-1.4.0-SNAPSHOT-sources.jar to org.apache.shiro/shiro-crypto-cipher/1.4.0-SNAPSHOT/shiro-crypto-cipher-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/samples/web/pom.xml to org.apache.shiro.samples/samples-web/1.4.0-SNAPSHOT/samples-web-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/servlet-plugin/pom.xml to org.apache.shiro/shiro-servlet-plugin/1.4.0-SNAPSHOT/shiro-servlet-plugin-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/servlet-plugin/target/shiro-servlet-plugin-1.4.0-SNAPSHOT.jar to org.apache.shiro/shiro-servlet-plugin/1.4.0-SNAPSHOT/shiro-servlet-plugin-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/servlet-plugin/target/shiro-servlet-plugin-1.4.0-SNAPSHOT-sources.jar to org.apache.shiro/shiro-servlet-plugin/1.4.0-SNAPSHOT/shiro-servlet-plugin-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/spring/pom.xml to org.apache.shiro/shiro-spring/1.4.0-SNAPSHOT/shiro-spring-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/spring/target/shiro-spring-1.4.0-SNAPSHOT.jar to org.apache.shiro/shiro-spring/1.4.0-SNAPSHOT/shiro-spring-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/spring/target/shiro-spring-1.4.0-SNAPSHOT-javadoc.jar to org.apache.shiro/shiro-spring/1.4.0-SNAPSHOT/shiro-spring-1.4.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/spring/target/shiro-spring-1.4.0-SNAPSHOT-sources.jar to org.apache.shiro/shiro-spring/1.4.0-SNAPSHOT/shiro-spring-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/guice/pom.xml to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/guice/target/shiro-guice-1.4.0-SNAPSHOT.jar to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/guice/target/shiro-guice-1.4.0-SNAPSHOT-tests.jar to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/guice/target/shiro-guice-1.4.0-SNAPSHOT-javadoc.jar to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr/support/guice/target/shiro-guice-1.4.0-SNAPSHOT-sources.jar to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT-sources.jarchannel stoppedSetting status of 71694f6ea8b8327fc3e27f6e91a576fcba2d4a88 to FAILURE with url https://builds.apache.org/job/Shiro-pr/109/ and message: 'FAILURE 'Using context: Jenkins: mvn clean install

[fpapon]

@bdemers I see 2 PR related to this one: https://github.com/apache/shiro/pull/24 https://github.com/apache/shiro/pull/56 How can we deal with it? I can start review/test. I think it will be great to add CDI support for the 1.6.0 or 2.0.0 release.

[rmannibucau]

Checked out quickly other PRs, seems they miss tests on user beans with a normal scope and producers should likely be active only if not overriden by the user (otherwise they should be vetoed). Also I think that cdi2 can simplify the extension and it can be acceptable to skip cdi1 now. Happy to take some time to do a proposal if it makes sense.

Side note: being asf we should test with owb first I guess ;)

[fpapon]

@rmannibucau thanks for take a look! I agree to skip cdi1 and also to test with owb as it's an asf project ;)

[coheigea]

@bdemers What is the current status of this PR from your POV?

[asf-ci]

Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/Shiro-pr-jdk11/15/

Build result: FAILURE

[...truncated 81.72 KB...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/quickstart-guice/pom.xml to org.apache.shiro.samples/samples-quickstart-guice/1.4.0-SNAPSHOT/samples-quickstart-guice-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/all/pom.xml to org.apache.shiro/shiro-all/1.4.0-SNAPSHOT/shiro-all-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/spring-mvc/pom.xml to org.apache.shiro.samples/samples-spring-mvc/1.4.0-SNAPSHOT/samples-spring-mvc-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/spring-boot/spring-boot-web-starter/pom.xml to org.apache.shiro/shiro-spring-boot-web-starter/1.4.0-SNAPSHOT/shiro-spring-boot-web-starter-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/config/core/pom.xml to org.apache.shiro/shiro-config-core/1.4.0-SNAPSHOT/shiro-config-core-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/test-coverage/pom.xml to org.apache.shiro/shiro-test-coverage/1.4.0-SNAPSHOT/shiro-test-coverage-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/crypto/pom.xml to org.apache.shiro/shiro-crypto/1.4.0-SNAPSHOT/shiro-crypto-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/core/pom.xml to org.apache.shiro/shiro-core/1.4.0-SNAPSHOT/shiro-core-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/aspectj/pom.xml to org.apache.shiro.samples/samples-aspectj/1.4.0-SNAPSHOT/samples-aspectj-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/spring-boot-web/pom.xml to org.apache.shiro.samples/samples-spring-boot-web/1.4.0-SNAPSHOT/samples-spring-boot-web-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/spring-client/pom.xml to org.apache.shiro.samples/samples-spring-client/1.4.0-SNAPSHOT/samples-spring-client-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/tools/hasher/pom.xml to org.apache.shiro.tools/shiro-tools-hasher/1.4.0-SNAPSHOT/shiro-tools-hasher-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/spring-boot/spring-boot-starter/pom.xml to org.apache.shiro/shiro-spring-boot-starter/1.4.0-SNAPSHOT/shiro-spring-boot-starter-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/jaxrs/pom.xml to org.apache.shiro/shiro-jaxrs/1.4.0-SNAPSHOT/shiro-jaxrs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/guice/pom.xml to org.apache.shiro.samples/samples-guice/1.4.0-SNAPSHOT/samples-guice-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/crypto/cipher/pom.xml to org.apache.shiro/shiro-crypto-cipher/1.4.0-SNAPSHOT/shiro-crypto-cipher-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/samples/web/pom.xml to org.apache.shiro.samples/samples-web/1.4.0-SNAPSHOT/samples-web-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/servlet-plugin/pom.xml to org.apache.shiro/shiro-servlet-plugin/1.4.0-SNAPSHOT/shiro-servlet-plugin-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/spring/pom.xml to org.apache.shiro/shiro-spring/1.4.0-SNAPSHOT/shiro-spring-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/Shiro-pr-jdk11/support/guice/pom.xml to org.apache.shiro/shiro-guice/1.4.0-SNAPSHOT/shiro-guice-1.4.0-SNAPSHOT.pomchannel stoppedSetting status of 71694f6ea8b8327fc3e27f6e91a576fcba2d4a88 to FAILURE with url https://builds.apache.org/job/Shiro-pr-jdk11/15/ and message: 'FAILURE 'Using context: Jenkins: mvn clean install

[bmarwell]

Hi,

I'm not a ASF committer, but from looking at this code, I have some questions.

SubjectProducer is @ApplicationScoped and there is commented @RequestScoped. Why not just use @Dependent?

Line 40 of this class is Subject.class.cast(Proxy.newProxyInstance( and could be changed to (Subject) Proxy.newProxyInstance(…

This part needs some additional comments why it uses a new proxy instance. The comment should mention Shiro's ThreadContext.

Why does it need a Classloader utility Loader? ShiroExension::addSecurityManagerIfNeeded could be updated to use a AtomicReference::updateAndGet

The geronimo specs are still in there, please update to the jakarta dependencies. Geronimo's dependencies do not even have javadoc available.

beans.xml could be updated to cdi2. I'd also rename the module to CDI2, just in case CDI 3 is not compatible.

Also, you should not leave out beans.xml although it is allowed. It is still good habit to ship an empty one, just because some containers allow a setting to scan only jar files for beans which do have such a file (which speeds up deployment times massively).

Also, some comments might be helpful which action in the extension is done for which purpose.

I haven't used extensions a lot, but I think this code should still be elegible for inclusion. If merged, I'd also like to contribute another integration test using the maven invoker plugin, starting an OpenLiberty instance.

[bdemers]

thanks for looking at this @bmhm!

[rmannibucau]

Hmm,

Scope was there for perf reasons - dependent being generally slower for nothing in apps (recreating again and again the proxy if enclosing instance is not app scope),

+-0 for line 40 change, #codestyle ;)

Proxy is there to ensure context is the right one whatever was the creation context, guess it could fit shiro-core too. One common use case is servlet asynccontext where there can be 2 request scope threadlocal for the same real request,

Spec jar are not that important since they must be provided so not impacting end users but geronimo are better in osgi in general and the ones we want in shiro tests (used by our - asf - servers like tomee, karaf, ...) so rather -0 to move to jakarta there.

Isnt the module cdi1 and 1.1 friendly so beans.xml should stay cdi1?

Module has an extension so scanning is optional and annotated is not used so no issue to not have a beans.xml IMHO even if cdi2 trim mode would be a good fit if cdi 1 compat is not intended - no real strong position, deltaspike feedback is some ee6 servers are still out there but i doubt they use this new module.

Hope it gives some other pointers

[bmarwell]

Great, I think 99% of your github comment should directly go into javadocs.

Well, performance claims should have a test. I have no reason to not believe you, but I just wouldn't keep that dangling comment and just put the explanation into the javadocs.

About this statement however:

+-0 for line 40 change, #codestyle

Uhm my first commits were for the maven checkstyle plugin. It got rejected because I used a guard statement instead of an else. That's also just style. But it is not consistent with the rest of the code.

I learned that consistency beats style, and I haven't seen any casting done this way in other source files. Also, without any variable, it creates a lot of nesting. That's the reason. Not because I don't like it or wouldn't understand it. It's just about consistency which helps in keeping a project maintainable. 😉

[bmarwell]

I also wonder which PR superseeds which (see https://github.com/apache/shiro/pull/58#issuecomment-545116989). I think I saw multiple jira issues as well.

[lprimak]

@bmarwell there are already plenty of tests that @bdemers did in another PR. Also, I am going to pick this PR apart to reconcile w/Jakarta EE PR that was already merged, and make it easier to document, plus reconcile all other work that @bdemers already did on tests.