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

WIP: CDI feature import SHIRO-337 #24

Open
wants to merge 6 commits into
base: 1.3.x
from

Conversation

Projects
None yet
4 participants
@bdemers
Member

bdemers commented Jul 6, 2016

I spent about a day looking into this, the original code was pretty good, but I ran into a few issues when adding a sample project

Added CDI README.md
Added TODO's:

Comments above are just initial thoughts and are here to start discussion.

@hwellmann, I know it has been a while since you looked into this, but your thoughts would be appreciated

@bdemers

This comment has been minimized.

Show comment
Hide comment
@bdemers

bdemers Jul 6, 2016

Member

NOTE: history is in tacked at the moment, but will be squashed before merging

Member

bdemers commented Jul 6, 2016

NOTE: history is in tacked at the moment, but will be squashed before merging

@panchitoboy

This comment has been minimized.

Show comment
Hide comment
@panchitoboy

panchitoboy Sep 22, 2016

Hi,

Did you have any estimations for this merge?

I have a library (shiro-jwt) to use apache shiro with Json Web Tokens. I use pax-shiro-cdi-web but i want to remove the dependency.

Also if you are interested i can help to integrate my library in the project. I implement a JWTOrFormAuthenticationFilter that could be usefull as default filter in REST applications.

Regards,

panchitoboy commented Sep 22, 2016

Hi,

Did you have any estimations for this merge?

I have a library (shiro-jwt) to use apache shiro with Json Web Tokens. I use pax-shiro-cdi-web but i want to remove the dependency.

Also if you are interested i can help to integrate my library in the project. I implement a JWTOrFormAuthenticationFilter that could be usefull as default filter in REST applications.

Regards,

@bdemers

This comment has been minimized.

Show comment
Hide comment
@bdemers

bdemers Sep 22, 2016

Member

I was going to take another look at it in the next couple of weeks. I've been looking into updating spring and guice support as well. I'm hoping those will dovetail a bit with this, but priority wise, I want to focus on getting current modules updated first.

That said, if you want to take a shot at this, we would love the help. Same with adding JWT support. Send over a pull request and we can start that thread.

Member

bdemers commented Sep 22, 2016

I was going to take another look at it in the next couple of weeks. I've been looking into updating spring and guice support as well. I'm hoping those will dovetail a bit with this, but priority wise, I want to focus on getting current modules updated first.

That said, if you want to take a shot at this, we would love the help. Same with adding JWT support. Send over a pull request and we can start that thread.

@dmaidaniuk

This comment has been minimized.

Show comment
Hide comment
@dmaidaniuk

dmaidaniuk Sep 23, 2016

Hi guys,

We also used in our application Shiro with CDI a lot. Thus I would like to help with given task. We looked into pax-shiro-cdi-web and pax-shiro-cdi and have ideas how to make them even more CDI-oriented, without need for shiro-web dependency, e.g. own Environment implementation etc. I guess I will start with pulling of existing changes and see how to add our improvements there.

Regards,
Dmytro

dmaidaniuk commented Sep 23, 2016

Hi guys,

We also used in our application Shiro with CDI a lot. Thus I would like to help with given task. We looked into pax-shiro-cdi-web and pax-shiro-cdi and have ideas how to make them even more CDI-oriented, without need for shiro-web dependency, e.g. own Environment implementation etc. I guess I will start with pulling of existing changes and see how to add our improvements there.

Regards,
Dmytro

@gilbertoca

This comment has been minimized.

Show comment
Hide comment
@gilbertoca

gilbertoca Jan 17, 2017

@bdemers @dmaidaniuk Any hope to include this in the final 1.4.0 release?

gilbertoca commented Jan 17, 2017

@bdemers @dmaidaniuk Any hope to include this in the final 1.4.0 release?

@bdemers

This comment has been minimized.

Show comment
Hide comment
@bdemers

bdemers Jan 17, 2017

Member

@gilbertoca I poked around with this a few weeks ago, and I thought I had updated this thread
Take a look at this branch: https://github.com/apache/shiro/tree/cdi-idea (I just rebased it to master)

It was the result of a few late nights of hacking around. I was originally trying to see if there some base code we could reuse for Spring, Guice, and CDI. But there didn't seem to be (unless we want to annotate everything that is).

Anyway, I'd like to hear thoughts on this approach vs requiring a shiro.ini.

Member

bdemers commented Jan 17, 2017

@gilbertoca I poked around with this a few weeks ago, and I thought I had updated this thread
Take a look at this branch: https://github.com/apache/shiro/tree/cdi-idea (I just rebased it to master)

It was the result of a few late nights of hacking around. I was originally trying to see if there some base code we could reuse for Spring, Guice, and CDI. But there didn't seem to be (unless we want to annotate everything that is).

Anyway, I'd like to hear thoughts on this approach vs requiring a shiro.ini.

@gilbertoca

This comment has been minimized.

Show comment
Hide comment
@gilbertoca

gilbertoca Jan 18, 2017

@bdemers @panchitoboy @dmaidaniuk My background is of someone who maintains webapps using shiro (And possibly will not contribute much).
Initially we make the app work with a simple authe/autho setup. After we need to make some customization, mainly in the realm area. This customization is intensively done in the shiro,ini. Specifically in JEE containers, that customization needs to be CDI aware and inevitably we will start with a shiro.ini.
Looking at the web sample I could see you did use shiro.ini - so this initial CDI support needs/requires one.
I'm sorry I can't be of more help to you in this regard.

gilbertoca commented Jan 18, 2017

@bdemers @panchitoboy @dmaidaniuk My background is of someone who maintains webapps using shiro (And possibly will not contribute much).
Initially we make the app work with a simple authe/autho setup. After we need to make some customization, mainly in the realm area. This customization is intensively done in the shiro,ini. Specifically in JEE containers, that customization needs to be CDI aware and inevitably we will start with a shiro.ini.
Looking at the web sample I could see you did use shiro.ini - so this initial CDI support needs/requires one.
I'm sorry I can't be of more help to you in this regard.

@bdemers

This comment has been minimized.

Show comment
Hide comment
@bdemers

bdemers Jan 18, 2017

Member

@gilbertoca Thanks, that is good feedback.

After looking at it again, it looks like the cdi-web sample wasn't really updated to show off anything CDI, I'll fix that soon. But take a look at other example and see if that would fit your needs.

This example still uses an INI file, but is ONLY used for the IniRealm's users (I'll probably update that too, to avoid any confusion)

Member

bdemers commented Jan 18, 2017

@gilbertoca Thanks, that is good feedback.

After looking at it again, it looks like the cdi-web sample wasn't really updated to show off anything CDI, I'll fix that soon. But take a look at other example and see if that would fit your needs.

This example still uses an INI file, but is ONLY used for the IniRealm's users (I'll probably update that too, to avoid any confusion)

@dmaidaniuk

This comment has been minimized.

Show comment
Hide comment
@dmaidaniuk

dmaidaniuk Jan 23, 2017

Hi @bdemers and @gilbertoca,

Sorry for so big delay. I saw last changes in cdi-idea branch and they differ from original PAX implementation a bit. I will see more closely to them in nearest days. CDI applications can communicate with clients not only through HTTP requests, but by other channels, e.g. REST or EJB remote calls. Thus also required to test such scenarios also, when only cdi-core support library used.
Also would be good to reorganize common dependencies and move them to cdi-parent POM, e.g. cdi-api and javax.annotation-api.

Regards,
Dmytro

dmaidaniuk commented Jan 23, 2017

Hi @bdemers and @gilbertoca,

Sorry for so big delay. I saw last changes in cdi-idea branch and they differ from original PAX implementation a bit. I will see more closely to them in nearest days. CDI applications can communicate with clients not only through HTTP requests, but by other channels, e.g. REST or EJB remote calls. Thus also required to test such scenarios also, when only cdi-core support library used.
Also would be good to reorganize common dependencies and move them to cdi-parent POM, e.g. cdi-api and javax.annotation-api.

Regards,
Dmytro

@bdemers

This comment has been minimized.

Show comment
Hide comment
@bdemers

bdemers Jan 23, 2017

Member

Thanks for the feedback @dmaidaniuk !

Member

bdemers commented Jan 23, 2017

Thanks for the feedback @dmaidaniuk !

@bdemers bdemers referenced a pull request that will close this pull request Jan 24, 2017

Open

IDEA: CDI - no shiro.ini required #56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment