Re: [security-dev] Some thoughts on PL Subsystem

There's a not insignificant cost in creating an IdentityManager instance. If we can't have a new instance injected each time with @Resource, then it's better to not support the feature at all rather than resorting to a hack to make it work. On 12/04/13 08:52, Pedro Igor Silva wrote: > We're already making the IMF available. The idea is support both the IMF and IMs. > > The IdentityManager idea came up after some discussions earlier. I have proposed that but I at that time I was not sure if we should do it or not. As Pete agreed with this, I did it. > > If you think this is not ideal, we can review that and let only the IMF exposed via JNDI. As it was before. > > Regards. > Pedro Igor > > ----- Original Message ----- > From: "Shane Bryzak" <sbryzak(a)redhat.com&gt; > To: "Pedro Igor Silva" <psilva(a)redhat.com&gt; > Cc: security-dev(a)lists.jboss.org > Sent: Thursday, April 11, 2013 7:29:37 PM > Subject: Re: [security-dev] Some thoughts on PL Subsystem > > This is not ideal at all. A better option would just be to make the > IdentityManagerFactory available instead, and the application can then > create its own IdentityManager instances. > > On 12/04/13 08:05, Pedro Igor Silva wrote: >> To overcome this I'm thinking to use a wrapper class that creates a fresh IdentityManager instance for each method invocation. >> >> public class IdentityManagerProxy implements IdentityManager { >> >> ... >> >> public void add(IdentityType identityType) throws IdentityManagementException { >> createIdentityManager().add(identityType); >> } >> >> private IdentityManager createIdentityManager() { >> return this.identityManagerFactory.createIdentityManager(new Realm(this.realm)); >> } >> >> ... >> } >> >> I'm still testing, but it seems we can use that. >> >> Thanks. >> Pedro Igor >> >> ----- Original Message ----- >> From: "Shane Bryzak" <sbryzak(a)redhat.com&gt; >> To: security-dev(a)lists.jboss.org >> Sent: Thursday, April 11, 2013 6:26:55 PM >> Subject: Re: [security-dev] Some thoughts on PL Subsystem >> >> On 12/04/13 01:30, Pedro Igor Silva wrote: >>> So, the updated list would be: >>> >>> 1) Rename the attribute jndi-url to jndi-name; >>> 2) Publish in JNDI an IdentityManager for each realm. >> Keep in mind that each IdentityManager instance has its own >> SecurityContext, which is designed to be request-scoped. If we don't >> have the capacity to support request-scoped instances in JNDI, then they >> should be stateless (i.e. a new instance created every time). >> >>> 3) Support custom entities using a attribute to specify a module from >>> where the @IDMEntity classes are + persistence.xml; >>> >>> ----- Original Message ----- >>> From: "Pete Muir" <pmuir(a)redhat.com&gt; >>> To: "Stian Thorgersen" <stian(a)redhat.com&gt; >>> Cc: "Pedro Igor Silva" <psilva(a)redhat.com&gt;, security-dev(a)lists.jboss.org >>> Sent: Thursday, April 11, 2013 12:22:54 PM >>> Subject: Re: [security-dev] Some thoughts on PL Subsystem >>> >>> If you come up with one, let me know - this is something no one has solved in any situation ;-) >>> >>> On 11 Apr 2013, at 16:11, Stian Thorgersen <stian(a)redhat.com&gt; wrote: >>> >>>> I think for now we should drop the default attribute + @Realm, and only support @Resource (i.e. user has to create @Produce @Resource to be able to inject IdentityManager for sub-system). If we can think of a nice way to inject @IdentityManager allowing user to specify correct identity-management and realm that would be great, but I don't think we have an approach to this at the moment. >>>> >>>> ----- Original Message ----- >>>>> From: "Pedro Igor Silva" <psilva(a)redhat.com&gt; >>>>> To: "Pete Muir" <pmuir(a)redhat.com&gt; >>>>> Cc: "Stian Thorgersen" <stian(a)redhat.com&gt;, security-dev(a)lists.jboss.org >>>>> Sent: Thursday, 11 April, 2013 3:49:35 PM >>>>> Subject: Re: [security-dev] Some thoughts on PL Subsystem >>>>> >>>>> So, if you guys agree we can start working on the following improvements: >>>>> >>>>> 1) Rename the attribute jndi-url to jndi-name; >>>>> 2) Publish in JNDI an IdentityManager for each realm. That would look >>>>> like this: >>>>> >>>>> picketlink/MyIdentityManagerFactory >>>>> picketlink/MyIdentityManagerFactory/default (for the default realm) >>>>> picketlink/MyIdentityManagerFactory/SomeRealm >>>>> picketlink/MyIdentityManagerFactory/AnotherRealm >>>>> >>>>> 3) Add the default attribute for the identity-management element and >>>>> handle it properly >>>>> >>>>> 4) Supports a @Realm annotation in order to allow the injection of >>>>> IdentityManager that maps to a specific realm >>>>> >>>>> 5) Support custom entities using a attribute to specify a module from >>>>> where the @IDMEntity classes are + persistence.xml; >>>>> >>>>> What do you think ? >>>>> >>>>> ----- Original Message ----- >>>>> From: "Pete Muir" <pmuir(a)redhat.com&gt; >>>>> To: "Stian Thorgersen" <stian(a)redhat.com&gt; >>>>> Cc: "Pedro Igor Silva" <psilva(a)redhat.com&gt;, security-dev(a)lists.jboss.org >>>>> Sent: Thursday, April 11, 2013 11:16:14 AM >>>>> Subject: Re: [security-dev] Some thoughts on PL Subsystem >>>>> >>>>> >>>>> On 11 Apr 2013, at 14:35, Stian Thorgersen <stian(a)redhat.com&gt; wrote: >>>>> >>>>>> For custom entity classes I have two use cases in mind that we need should >>>>>> test/support: >>>>>> >>>>>> * Layered product that needs to use custom entity classes for sub-systems - >>>>>> in this case there's no JavaEE deployments and the entity classes needs to >>>>>> be within a module. It's also fairly cumbersome to create an >>>>>> EntityManagerFactory from a subsystem so I don't think that should be >>>>>> required >>>>>> >>>>>> * Two applications sharing the same custom entity classes - for example >>>>>> there's a main web app that contains the custom entity classes and the >>>>>> persistence.xml, then there's a utility war that contains one single >>>>>> @Startup @Singleton that is used to create some initial users - the >>>>>> utility war would load a lot quicker than the main web app, so the EMF may >>>>>> not be registered in JNDI in time >>>>>> >>>>>> >>>>>> ----- Original Message ----- >>>>>>> From: "Pedro Igor Silva" <psilva(a)redhat.com&gt; >>>>>>> To: "Stian Thorgersen" <stian(a)redhat.com&gt; >>>>>>> Cc: security-dev(a)lists.jboss.org >>>>>>> Sent: Thursday, 11 April, 2013 2:04:17 PM >>>>>>> Subject: Re: [security-dev] Some thoughts on PL Subsystem >>>>>>> >>>>>>> Hi Stian, >>>>>>> >>>>>>> Your thoughts make a lot of sense to me. Comments inline. >>>>>>> >>>>>>> ----- Original Message ----- >>>>>>>> From: "Stian Thorgersen" <stian(a)redhat.com&gt; >>>>>>>> To: security-dev(a)lists.jboss.org >>>>>>>> Sent: Thursday, April 11, 2013 9:37:59 AM >>>>>>>> Subject: [security-dev] Some thoughts on PL Subsystem >>>>>>>> >>>>>>>> I've had a look at https://community.jboss.org/wiki/PicketLink3Subsystem >>>>>>>> and >>>>>>>> also had a bit of a play with it. It's starting to look really good. I've >>>>>>>> just got a few suggestions: >>>>>>>> >>>>>>>> >>>>>>>> Suppress logging >>>>>>>> ---------------- >>>>>>>> At the moment there's a lot of logging at info level produced by the >>>>>>>> subsystem, this is mostly Hibernate. It would be great if we could >>>>>>>> somehow >>>>>>>> manage to suppress this logging output, might be problematic though as >>>>>>>> Hibernate logs this stuff at INFO level when it really should be DEBUG. >>>>>>>> There's also a few WARN's we might want to look into fixing. >>>>>>> Review the logging and messages is one of the things in our TODO list. >>>>>>> >>>>>>>> JNDI names in standalone.xml >>>>>>>> ---------------------------- >>>>>>>> I think it makes sense to use the same format for JNDI names as the >>>>>>>> datasource element, since folks will already be used to that. So I >>>>>>>> suggest >>>>>>>> we change it slightly to look like this: >>>>>>>> >>>>>>>> <jpa-store data-source=”java:jboss/datasources/ExampleDS" ...> >>>>>>>> <identity-management jndi-name="java:picketlink/ExampleIDM" ...> >>>>>>>> >>>>>>>> * Full jndi name (including java:) and use jndi-name instead of jndi-url >>>>>>> +1 for that. Not sure from where I got the jndi-url if the jndi-name is >>>>>>> like >>>>>>> a pattern used by other subsystems :) >>>>>>> >>>>>>>> Manifest.mf >>>>>>>> ----------- >>>>>>>> We need to make sure it works when including org.picketlink, >>>>>>>> org.picketlink.idm, etc in manifest.mf as well as >>>>>>>> jboss-deployment-structure.xml. The documentation should also reflect >>>>>>>> this. >>>>>>>> One thing I also thought of is that for the future it may be nice to have >>>>>>>> something that detects PicketLink usage in a deployment and automatically >>>>>>>> adds dependencies as required. For example if deployment uses >>>>>>>> @IdentityManager, @Identity, etc. annotations. >>>>>>>> >>>>>>> +1. I like the idea, ans also mark them as IDM or Core deployments and >>>>>>> handle >>>>>>> them properly. >>>>>>> >>>>>>>> JNDI >>>>>>>> ---- >>>>>>>> @Resource doesn't require CDI, so it should be possible to do the >>>>>>>> following >>>>>>>> without CDI (and without org.picketlink.core): >>>>>>>> >>>>>>>> @Resource(lookup = "java:/picketlink/DevIdentityManager") >>>>>>>> private IdentityManagerFactory identityManagerFactory; >>>>>>>> >>>>>>>> I was wondering if we wanted to have the IdentityManager available in >>>>>>>> JNDI >>>>>>>> as >>>>>>>> well? >>>>>>> The problem in publishing the IdentityManager in JNDI is related with >>>>>>> realms. >>>>>>> If the IDM config has multiple realms which one should we put ? The >>>>>>> default >>>>>>> ? >>>>>>> >>>>>>> Give to users the IdentityManagerFactory instead, allow them to use their >>>>>>> configurations as they want. >>>>>>> >>>>>>> One thing that I thought about that is if is a good idea to publish all >>>>>>> IdentityManager instances for each configured realm. So, if the IDM config >>>>>>> defines multiple realms, we publish a IdentityManager instance for each of >>>>>>> them. But as we discussed this may become messy. >>>>> I think this is the right approach. >>>>> >>>>>>> What do you think ? >>>>>>> >>>>>>>> CDI >>>>>>>> --- >>>>>>>> I was thinking about a nice way to do the CDI support of injecting a >>>>>>>> 'default' IdentityManager. I propose adding the attribute 'default' to >>>>>>>> the >>>>>>>> 'identity-management' element (<identity-management default="true" ...>). >>>>>>>> We >>>>>>>> should throw a warning if a user has specified multiple, then we just >>>>>>>> pick >>>>>>>> one (first one?). >>>>>>> I think we had some discussion about that. I'm +1 for the default >>>>>>> attribute. >>>>>>> >>>>>>> Ideally, we should throw an exception if multiple configurations are >>>>>>> provided >>>>>>> with the default attribute, IMO. >>>>> Agreed, this should be an error. >>>>> >>>>>>>> This does mean that if a 'identity-management' has the 'default' >>>>>>>> attribute >>>>>>>> set on it all deployments will by default have that IdentityManager >>>>>>>> injected >>>>>>>> into it. We also need a way for users to override this on a >>>>>>>> per-deployment >>>>>>>> basis. Can we easily detect if a deployment contains configuration for a >>>>>>>> IdentityManager itself? >>>>>>> The IMF can be obtained today in the following ways: >>>>>>> >>>>>>> 1) From JNDI (@Resource, InitialContext, etc) >>>>>>> 2) Providing a @Producer that produces a IdentityConfiguration. In this >>>>>>> case the deployment provides its own configuration, instead of using >>>>>>> the >>>>>>> subsystem config. >>>>>>> 3) When using the Core services, the deployment must specify a >>>>>>> web.xml#resource-ref. Otherwise the deployment must provides its own >>>>>>> configuration (normal usage of PicketLink Core) >>>>>>> >>>>>>> Considering 2), if no IdentityConfiguration is produced, we can >>>>>>> automatically >>>>>>> choose the default. >>>>>>> >>>>>>> Considering 3), if no web.xml@resource-ref is defined, we can >>>>>>> automatically >>>>>>> choose the default. >>>>>>> >>>>>>>> Further we need to have a way for a user to specify which IdentityManager >>>>>>>> to >>>>>>>> inject. I think this should be done based on the 'alias' attribute and >>>>>>>> not >>>>>>>> the 'jndi-name', as we should leave jndi completely out of the picture >>>>>>>> for >>>>>>>> CDI (resource-ref in web.xml/ejb.xml should be used for JNDI lookup, >>>>>>>> InitialContext#lookup and @Resource, I find it confusing to use this for >>>>>>>> CDI). I propose that we use the ServiceRegistry to retrieve the >>>>>>>> IdentityManagerFactory service based on the alias specified by a @Alias >>>>>>>> qualifer: >>>>>>> If you look at the Infinispan subsystem, this is the way it works. Using >>>>>>> the >>>>>>> @Resource annotation to inject cachecontainers, etc. >>>>>>> >>>>>>> I like that because it is very simple, and requires very little from our >>>>>>> and >>>>>>> users side. >>>>> This is also the approach the spec defines to access server resources. >>>>> >>>>>>> We have a test case that shows how to use CDI qualifiers. It is quite >>>>>>> simple. >>>>>>> >>>>>>> But at the same time, I agree that use the name is more beautiful than the >>>>>>> jndi-name :). >>>>>>> >>>>>>> We can try that, if you want. >>>>> We shouldn't do this, it encourages the CDI anti-pattern of using string >>>>> based qualifiers. >>>>> >>>>>>>> @Inject >>>>>>>> @Alias(“development”) >>>>>>>> private IdentityManager identityManager; >>>>>>>> >>>>>>>> Obviously users should also be able to add their own qualifiers, I think >>>>>>>> this >>>>>>>> should work: >>>>>>>> >>>>>>>> @Inject @Alias(“development”) >>>>>>>> @Produces @Development >>>>>>>> private IdentityManager identityManager; >>>>> This won't work, CDI will give you a definition error. You need to use >>>>> @Resource to access server resources, or what Pedro suggests below. >>>>> >>>>>>>> One alternative to the above is to change 'alias' to 'name' then we could >>>>>>>> use >>>>>>>> the standard @Named annotation instead of @Alias. >>>>>>> We are not injecting the IdentityManager anymore, but the >>>>>>> IdentityManagerFactory. The @Alias makes sense to get a IdentityManager >>>>>>> instance for a configured realm. Maybe we should consider @Realm, instead. >>>>>>> >>>>>>>> Custom Entity Classes >>>>>>>> --------------------- >>>>>>>> Personally I don't like the idea of custom entity classes (and >>>>>>>> persistence.xml) being deployed as JavaEE deployments (i.e. >>>>>>>> standalone/deployments). This is also problematic for sub-systems that >>>>>>>> wants >>>>>>>> to use the IDM if they need to use custom entity classes (there's a good >>>>>>>> chance we'll need this for EventJuggler). I also think this will be >>>>>>>> problematic if multiple deployments uses the same IdentityManager. >>>>>>>> >>>>>>>> One idea I had was that we could create a module that contains the custom >>>>>>>> Entity classes, then specify that on the 'jpa-store' element: >>>>>>>> >>>>>>>> <jpa-store data-source=”java:jboss/datasources/ExampleDS" >>>>>>>> custom-entity-module='org.company.acme.pl' /> >>>>> This should work IMO. >>>>> >>>>>>>> The module 'org.company.acme.pl' would contain a single jar with the >>>>>>>> Entity >>>>>>>> classes. When 'custom-entity-module' is used we include that module >>>>>>>> instead >>>>>>>> of 'org.picketlink.idm.schema' module when creating the EMF + we should >>>>>>>> be >>>>>>>> able to detect the correct classes using the @IDMEntity. >>>>>>> The JPA store lets you use the EMF in two ways: >>>>>>> >>>>>>> 1) Using a embedded persistence unit. In this case you need only yo >>>>>>> provide the datasource. The built-in schema (pl-idm-schema) will be >>>>>>> used. >>>>>>> 2) Using your own persistence unit. In this case you need to expose your >>>>>>> EMF via JNDI. >>>>>>> >>>>>>> Regarding 2), you are not forced to deploy your persistence.xml as a >>>>>>> separated deployment. You can also use the persistence unit deployed with >>>>>>> your application. >>>>>>> >>>>>>> I'm going to create some tests so check a possible classloader issue when >>>>>>> using custom entity classes. >>>>>>>