portals-jetspeed-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ate Douma <...@douma.nu>
Subject Re: Odd critera and API types in Security
Date Wed, 04 Feb 2009 20:54:32 GMT
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


Mime
View raw message