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

Alternative mechanism of obtaining of CommonProfile and ProfileManager #32

Closed
yegorius opened this issue Nov 26, 2017 · 12 comments
Closed
Assignees
Milestone

Comments

@yegorius
Copy link

Per jax-rs spec there is SecurityContext, which can be injected into a resource with @Context annotation and pac4j-jaxrs already has it's own implementation of SecurityContext, which is JaxRsProfileManager.Pac4JSecurityContext. The SecurityContext class is a native way of providing security features to a resource, so I was wondering whether it is possible to add a method to Pac4JSecurityContext like .getProfileManager() (and also .getProfile() to PrincipalImpl) which would return ProfileManager (and CommonProfile). The only thing a resource would be required to do is to cast SecurityContext to Pac4JSecurityContext. What do you think?

@victornoel victornoel self-assigned this Nov 26, 2017
@victornoel
Copy link
Member

You are right on point :)

I was exactly in the process of moving everything pac4j-related that could be accessed during a request to the jax-rs SecurityContext while migrating to pac4j 3.0.0.

It is maybe a better idea to do this now and not wait for pac4j 3 actually (while staying backward compatible with present functionalities until the move to pac4j 3).

@victornoel victornoel added this to the 2.2.0 milestone Nov 26, 2017
@victornoel
Copy link
Member

Hm, it's not so easy because of the way pac4j currently work: basically, the big problem is that I'm not able to know if I should look for the profiles into the session or not so I cannot construct a Principal...

This is fixed in pac4j 3 and that's why it was natural to do it at that point. Do you need this feature or was it just an desirable enhancement? Can it wait for pac4j 3 (few months)?

@victornoel
Copy link
Member

Actually I found an interesting alternative.

Do you think we should get access to the CommonProfile (so that we don't have to bother to know if we should read from session or not) and the WebContext (from which we can build what we want) or just the CommonProfile actually?

@yegorius
Copy link
Author

Oh great to know you are working on this already! I can get my job done with current annotations and my proposal is just, yes, a desirable enhancement :)

Moreover, I have already tried to implement what I was talking about for RestEasy. I was almost done with implementation but at the end it turned out that ReatEasy injects SecurityConext wrapped in a Proxy, so when you try to cast the reference to SecurityContext in a resource method to Pac4JSecurityContext you get ClassCastException.

See this line: ContextParameterInjector.java:38

@yegorius
Copy link
Author

Re: CommonProfile + WebContext. I am very new to Pac4J and I haven't yet used every feature of it, so I am probably not the best source of opinion :)

@victornoel
Copy link
Member

Moreover, I have already tried to implement what I was talking about for RestEasy. I was almost done with implementation but at the end it turned out that ReatEasy injects SecurityConext wrapped in a Proxy, so when you try to cast the reference to SecurityContext in a resource method to Pac4JSecurityContext you get ClassCastException.

This means you can't cast the SecurityContext at all in resteasy? That's really bothering :/
Anyway, that was in my plans so it's cool to introduce this, we will see if an easy solution to exploit it with resteasy emerges, I will be careful not to remove anything that will be needed in the case Pac4JSecurityContext is not usable with resteasy.

@yegorius
Copy link
Author

Yes, you can't cast SecurityContext if it's injected. I am thinking about reaching out to RestEasy developers for an advice.

@victornoel
Copy link
Member

Yep, good idea, there are maybe other ways to handle that situation...

@victornoel
Copy link
Member

Actually, Jersey neither inject directly the SecurityContext.
An alternative is thus to:

  • For jersey, get the ContainerRequestContext injected in your resource (@Inject ContainerRequestContext requestContext to then retrieve the real SecurityContext).
  • For RestEasy, use ResteasyProviderFactory.getContextData(SecurityContext.class) directly.

victornoel added a commit to victornoel/jax-rs-pac4j that referenced this issue Dec 2, 2017
victornoel added a commit to victornoel/jax-rs-pac4j that referenced this issue Dec 2, 2017
 - deprecate the need to specify if the profile should be retrived from session
 - use the SecurityContext as the source of information: JaxRsContext
   and profiles can be accessed from it now
 - simplify code and refactor a bit
 - improve reasteasy injection and add author information
 - bump to 2.2.0

Closes pac4j#32
@victornoel
Copy link
Member

@yegorius could you check #33 (in particular the tests and the updated README) to see if this kind of things are answering what you had in mind? :)

btw, I added your name as author of the reasteasy profile injection implementation, hope that's ok :)

@yegorius
Copy link
Author

yegorius commented Dec 2, 2017

I have checked #33 and everything looks good to me.

@victornoel
Copy link
Member

thanks :)

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

No branches or pull requests

2 participants