[ 
https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054792#comment-13054792
 ] 

Les Hazlewood edited comment on SHIRO-307 at 6/25/11 1:22 AM:
--------------------------------------------------------------

Ah yes - good questions.

I removed that constructor and had the others protected because typically the 
DomainPermission (at least in my experience) would always subclassed to 
represent the 'domain' (or more accurately, the resource type) being protected.

For example, 

DocumentPermission extends DomainPermission

Based on this use case at least, there is no need for a public constructor 
taking in the 'domain' argument since it is always derived from the class name.

So, in essence, DomainPermission was intended as a way to represent resources 
in a compile-time type-safe manner.  It doesn't really provide any benefit 
other than that (and maybe a way to codify explicit 'targets' and 'actions' 
whereas those concepts are not mandated in the WildcardPermission superclass).

On another note, I forgot about the mutability issue - Permissions are intended 
to be immutable, so we won't need any of the mutators (also helps w/ 
concurrency).  I suppose someone could make a mutable Permission 
implementation, but I haven't seen the need for one and no-one seems to have 
problems with them being immutable (like you've pointed out).

I guess my vote would be to deprecate DomainPermission entirely since whether 
the actions/targets representation could be stored as a String or a Set of 
Strings depending on how you wish to store that data - which is application 
specific.  Thoughts?

      was (Author: lhazlewood):
    Ah yes - good questions.

I removed that constructor and had the others protected because typically the 
DomainPermission (at least in my experience) would always subclassed to 
represent the 'domain' (or more accurately, the resource type) being protected.

For example, 

DocumentPermission extends DomainPermission

Based on this use case at least, there is no need for a public constructor 
taking in the 'domain' argument since it is always derived from the class name.

So, in essence, DomainPermission was intended as a way to represent resources 
in a compile-time type-safe manner.  It doesn't really provide any benefit 
other than that (and maybe a way to codify explicit 'targets' and 'actions' 
whereas those concepts are not mandated in the WildcardPermission superclass).

On another note, I forgot about the mutability issue - Permissions are intended 
to be immutable, so I we won't need any of the mutators (also helps w/ 
concurrency).  I suppose someone could make a mutable Permission 
implementation, but I haven't seen the need for one and no-one seems to have 
problems with them being immutable (like you've pointed out).

I guess my vote would be to deprecate DomainPermission entirely since whether 
the actions/targets representation could be stored as a String or a Set of 
Strings depending on how you wish to store that data - which is application 
specific.  Thoughts?
  
> DomainPermission does not fully support domain, actions and targets properties
> ------------------------------------------------------------------------------
>
>                 Key: SHIRO-307
>                 URL: https://issues.apache.org/jira/browse/SHIRO-307
>             Project: Shiro
>          Issue Type: Bug
>          Components: Authorization (access control) 
>    Affects Versions: 1.0.0, 1.1.0
>            Reporter: Phil Steitz
>            Assignee: Les Hazlewood
>         Attachments: DomainPermission.patch
>
>
> Per the class javadoc, DomainPermission is designed to be a base class for 
> Permission implementations that persist permission parts as separate 
> properties.  It defines private fields for domain, actions and targets and 
> exposes getters/setters for these, but the setParts and constructor methods 
> that set Permission state do not call the property setters and the property 
> setters don't call setParts.   Property synchronization needs to be added to 
> this class.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to