Odd critera and API types in Security

[Randy Watler <wa...@wispertel.net>]

Ate/Dennis,

As you know, I am trolling through Security porting it over to JPA. I 
noticed a few minor things in the process that you might be interested 
in looking at:

1. In JetspeedSecurityPersistenceManager revoke[All]Permission() 
methods, the 'domainId' criteria appear to be incorrect. Should these 
really be criteria on 'principal.domainId'?

2. In the security API JetspeedPermissionAccessManager, there are two 
methods specified with concrete class argument types instead of API 
interfaces: getPermissions(PersistentJetspeedPrincipal principal) and 
getPrincipals(PersistentJetspeedPermission permission, ...). I have had 
to change these to JetspeedPrincipal and JetspeedPermission, 
respectively, so that I can implement a JPA version of the access 
manager. Please let me know if we need to implement 'persistence' 
capable interfaces or if this was just an oversight.

Thanks,

Randy


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Odd critera and API types in Security

[Ate Douma <at...@douma.nu>]

Ate Douma wrote:
> Hi Randy,
> 
> 
> Finally, concerning the first issue about the 'domainId' criteria usage 
> in JetspeedSecurityPersistenceManager revoke[All]Permission() methods: 
> yes, AFAICS that is needed in the case of a passed in 
> TransientJetspeedPrincipal or JetspeedPrincipal without an id.
> Now that we have domain separation of Principals within the database 
> (currently only used so far for SSO principals, but in the future we 
> want to expand this to a full multiple domain security feature 
> throughout Jetspeed), if you don't have a JetspeedPrincipal id (uniquely 
> identifying a principal *across* domains) adding the (currently only 
> used default) domainId as filter is required to prevent accidentally 
> revoking [All] permissions from more than one principal which just 
> happen to use the same name (and type).
As Randy just pointed out to me, the domainId usage in above methods *is* incorrect (not the using it in the first place).
I just misunderstood the question :)

Thanks for pointing this one out Randy, I'll fix it right away.
Luckily JPA is better at validating queries upfront, in contrast to OJB (or our testcases for that matter...)

Regards,

Ate

> 
> Regards,
> 
> Ate
> 
> Randy Watler wrote:
>> Ate/Dennis,
>>
>> I got a little confused with the interface/class names with 
>> PersistentJetspeedPermission... it is obviously fine for APIs since it 
>> is an interface! The similarly named PersistentJetspeedPrincipal is 
>> not an interface. I suppose I am wondering if we should try to make 
>> the naming consistent by introducing a PersistentJetspeedPrincipalImpl 
>> class and define PersistentJetspeedPrincipal as an interface or just 
>> use JetspeedPrincipal as the interface in place of 
>> PersistentJetspeedPrincipal in the APIs as I have done so far. Let me 
>> know!
>>
>> Randy
>>
>> Randy Watler wrote:
>>> Ate/Dennis,
>>>
>>> WRT #2, yes, the JetspeedPermissionStoreManager API also has many 
>>> direct references to PersistentJetspeedPermission which I am changing 
>>> to JetspeedPermission as well.
>>>
>>> Randy
>>>
>>> Randy Watler wrote:
>>>> Ate/Dennis,
>>>>
>>>> As you know, I am trolling through Security porting it over to JPA. 
>>>> I noticed a few minor things in the process that you might be 
>>>> interested in looking at:
>>>>
>>>> 1. In JetspeedSecurityPersistenceManager revoke[All]Permission() 
>>>> methods, the 'domainId' criteria appear to be incorrect. Should 
>>>> these really be criteria on 'principal.domainId'?
>>>>
>>>> 2. In the security API JetspeedPermissionAccessManager, there are 
>>>> two methods specified with concrete class argument types instead of 
>>>> API interfaces: getPermissions(PersistentJetspeedPrincipal 
>>>> principal) and getPrincipals(PersistentJetspeedPermission 
>>>> permission, ...). I have had to change these to JetspeedPrincipal 
>>>> and JetspeedPermission, respectively, so that I can implement a JPA 
>>>> version of the access manager. Please let me know if we need to 
>>>> implement 'persistence' capable interfaces or if this was just an 
>>>> oversight.
>>>>
>>>> Thanks,
>>>>
>>>> Randy
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>>>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>>>
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>
>>
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
> 
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Odd critera and API types in Security

[Ate Douma <at...@douma.nu>]

Hi Randy,

First of all, sorry for the late response. Somehow my mail filter configuration has been broken the last few days and I only today noticed that.

With regards to your questions: yeah, I can understand the confusion :)

The direct class usage of PersistentJetspeedPrincipal instead of the JetspeedPrincipal interface by the JetspeedPermissionAccessManager 
probably is a mistake (by me). AFAICS changing that to JetspeedPrincipal should not be a problem or cause unexpected side-effects.

Why PersistentJetspeedPrincipal isn't called PersistentJetspeedPrincipalImpl and then implementing a interface called 
PersistentJetspeedPrincipal does have a reason though.
(Note: I had to dive real deep in my memory to "rediscover" the actual reason and to have it make sense to myself as well again)

First of all, part of the idea behind this is that PersistentJetspeedPrincipal is not *just* an implementation of *a* interface, but really 
*the* primary class which provides *all* needed and persistence capable (data) members.
If someone wants to add an (persistent) extension for the PersistentJetspeedPrincipal, like Group, they should extend this *class*, not 
merely provide their own implementation of JetspeedPrincipal.
Furthermore, the goal also was to be able to simply add new extensions without having to modify the OJB mapping. For that, I needed a single 
and pre-defined concrete class (as well as the awkward custom OJB rowreader to support "polymorphing" a loaded instance).

While renaming PersistentJetspeedPrincipal to PersistentJetspeedPrincipalImpl (and then probably likewise TransientJetspeedPrincipalImpl) 
technically is no problem, adding a PersistentJetspeedPrincipal and TransientJetspeedprincipal doesn't really add anything to the existing 
JetspeedPrincipal as these would end up being just marker interfaces, adding no additional methods.
Because of that, I saw (at the time I choose the current naming) no point of adding these interfaces. And because of that, naming giving 
these classes an -Impl extension didn't really seem appropriate either as (to me) that normally implicates there is a corresponding 
interface as well which isn't the case.

So, it really depends on the "rule" one would choose for coming up with the correct naming here:
- if an interface is *always* required (regardless of the above described reasons why not in this case), then yes: adding a -Impl extension 
would be the logical result
- but, if an -Impl extension also *implies* an interface implementation which isn't defined, not using a -Impl extension might be better

I'm not really "hung" up on the current naming however. If you think the first "rule" should be followed, I'm OK with adding the marker 
interfaces and renaming the classes. And if you have an technical requirement to do so because of JPA, then of course, no discussion, go ahead.

I hope the above make some sense :)

Finally, concerning the first issue about the 'domainId' criteria usage in JetspeedSecurityPersistenceManager revoke[All]Permission() 
methods: yes, AFAICS that is needed in the case of a passed in TransientJetspeedPrincipal or JetspeedPrincipal without an id.
Now that we have domain separation of Principals within the database (currently only used so far for SSO principals, but in the future we 
want to expand this to a full multiple domain security feature throughout Jetspeed), if you don't have a JetspeedPrincipal id (uniquely 
identifying a principal *across* domains) adding the (currently only used default) domainId as filter is required to prevent accidentally 
revoking [All] permissions from more than one principal which just happen to use the same name (and type).

Regards,

Ate

Randy Watler wrote:
> Ate/Dennis,
> 
> I got a little confused with the interface/class names with 
> PersistentJetspeedPermission... it is obviously fine for APIs since it 
> is an interface! The similarly named PersistentJetspeedPrincipal is not 
> an interface. I suppose I am wondering if we should try to make the 
> naming consistent by introducing a PersistentJetspeedPrincipalImpl class 
> and define PersistentJetspeedPrincipal as an interface or just use 
> JetspeedPrincipal as the interface in place of 
> PersistentJetspeedPrincipal in the APIs as I have done so far. Let me know!
> 
> Randy
> 
> Randy Watler wrote:
>> Ate/Dennis,
>>
>> WRT #2, yes, the JetspeedPermissionStoreManager API also has many 
>> direct references to PersistentJetspeedPermission which I am changing 
>> to JetspeedPermission as well.
>>
>> Randy
>>
>> Randy Watler wrote:
>>> Ate/Dennis,
>>>
>>> As you know, I am trolling through Security porting it over to JPA. I 
>>> noticed a few minor things in the process that you might be 
>>> interested in looking at:
>>>
>>> 1. In JetspeedSecurityPersistenceManager revoke[All]Permission() 
>>> methods, the 'domainId' criteria appear to be incorrect. Should these 
>>> really be criteria on 'principal.domainId'?
>>>
>>> 2. In the security API JetspeedPermissionAccessManager, there are two 
>>> methods specified with concrete class argument types instead of API 
>>> interfaces: getPermissions(PersistentJetspeedPrincipal principal) and 
>>> getPrincipals(PersistentJetspeedPermission permission, ...). I have 
>>> had to change these to JetspeedPrincipal and JetspeedPermission, 
>>> respectively, so that I can implement a JPA version of the access 
>>> manager. Please let me know if we need to implement 'persistence' 
>>> capable interfaces or if this was just an oversight.
>>>
>>> Thanks,
>>>
>>> Randy
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>
>>
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
> 
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Odd critera and API types in Security

[Randy Watler <wa...@wispertel.net>]

Ate/Dennis,

I got a little confused with the interface/class names with 
PersistentJetspeedPermission... it is obviously fine for APIs since it 
is an interface! The similarly named PersistentJetspeedPrincipal is not 
an interface. I suppose I am wondering if we should try to make the 
naming consistent by introducing a PersistentJetspeedPrincipalImpl class 
and define PersistentJetspeedPrincipal as an interface or just use 
JetspeedPrincipal as the interface in place of 
PersistentJetspeedPrincipal in the APIs as I have done so far. Let me know!

Randy

Randy Watler wrote:
> Ate/Dennis,
>
> WRT #2, yes, the JetspeedPermissionStoreManager API also has many 
> direct references to PersistentJetspeedPermission which I am changing 
> to JetspeedPermission as well.
>
> Randy
>
> Randy Watler wrote:
>> Ate/Dennis,
>>
>> As you know, I am trolling through Security porting it over to JPA. I 
>> noticed a few minor things in the process that you might be 
>> interested in looking at:
>>
>> 1. In JetspeedSecurityPersistenceManager revoke[All]Permission() 
>> methods, the 'domainId' criteria appear to be incorrect. Should these 
>> really be criteria on 'principal.domainId'?
>>
>> 2. In the security API JetspeedPermissionAccessManager, there are two 
>> methods specified with concrete class argument types instead of API 
>> interfaces: getPermissions(PersistentJetspeedPrincipal principal) and 
>> getPrincipals(PersistentJetspeedPermission permission, ...). I have 
>> had to change these to JetspeedPrincipal and JetspeedPermission, 
>> respectively, so that I can implement a JPA version of the access 
>> manager. Please let me know if we need to implement 'persistence' 
>> capable interfaces or if this was just an oversight.
>>
>> Thanks,
>>
>> Randy
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
>> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org

Re: Odd critera and API types in Security

[Randy Watler <wa...@wispertel.net>]

Ate/Dennis,

WRT #2, yes, the JetspeedPermissionStoreManager API also has many direct 
references to PersistentJetspeedPermission which I am changing to 
JetspeedPermission as well.

Randy

Randy Watler wrote:
> Ate/Dennis,
>
> As you know, I am trolling through Security porting it over to JPA. I 
> noticed a few minor things in the process that you might be interested 
> in looking at:
>
> 1. In JetspeedSecurityPersistenceManager revoke[All]Permission() 
> methods, the 'domainId' criteria appear to be incorrect. Should these 
> really be criteria on 'principal.domainId'?
>
> 2. In the security API JetspeedPermissionAccessManager, there are two 
> methods specified with concrete class argument types instead of API 
> interfaces: getPermissions(PersistentJetspeedPrincipal principal) and 
> getPrincipals(PersistentJetspeedPermission permission, ...). I have 
> had to change these to JetspeedPrincipal and JetspeedPermission, 
> respectively, so that I can implement a JPA version of the access 
> manager. Please let me know if we need to implement 'persistence' 
> capable interfaces or if this was just an oversight.
>
> Thanks,
>
> Randy
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org