You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shiro.apache.org by "Phil Steitz (JIRA)" <ji...@apache.org> on 2011/06/23 00:36:47 UTC

[jira] [Created] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

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.1.0, 1.0.0
            Reporter: Phil Steitz


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

        

[jira] [Commented] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054169#comment-13054169 ] 

Les Hazlewood commented on SHIRO-307:
-------------------------------------

Hi Phil,

I applied the patch and made some modifications - namely:

- Allow DomainPermission to have null values (one or two constructors allowed null values, so I made this assumption throughout the class).
- Minor modifications to String-related split/toString support to be more congruent with other similar methods in StringUtils (return a value instead of accept an argument, etc).

Do you see any issues with this based on your testing?  Any objections?

> 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

        

[jira] [Updated] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phil Steitz updated SHIRO-307:
------------------------------

    Attachment: DomainPermission.patch

Replaced attachment.  Added missing constructor and serialVersionUID to DomainPermission

> 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

        

[jira] [Issue Comment Edited] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
    [ 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:23 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 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 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

        

[jira] [Commented] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054227#comment-13054227 ] 

Phil Steitz commented on SHIRO-307:
-----------------------------------

Thanks, Wes.  Works4me other than missing the constructor that I added in the second version of the patch:
{code}
public DomainPermission(String domain, Set<String> actions, Set<String> targets) {
        this.domain = domain;
        this.actions = actions;
        this.targets = targets;
        setParts(domain, actions, targets);
    }
{code}

I don't actually use or depend on the properties of this class and I wonder if in fact we need them, since (at least in the use I am making of it) it is essentially just a facade for WildcardPermission.  I don't know what the general Shiro philosophy is on mutability and in particular mutable permissions, but it might be simpler to not introduce it in the facade.  I guess that's why you made the setters protected?  But then how exactly are they expected to be used?   So maybe a) the setters should be dropped... or b) the fields themselves should be dropped...  or even c) drop the class entirely and just add a constructor as above to WildcardPermission

Sorry if my newbie-ness is leading to confusion here.

> 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

        

[jira] [Issue Comment Edited] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054227#comment-13054227 ] 

Phil Steitz edited comment on SHIRO-307 at 6/24/11 5:08 AM:
------------------------------------------------------------

Thanks, Wes.  Works4me other than missing the constructor that I added in the second version of the patch:

{code}
public DomainPermission(String domain, Set<String> actions, Set<String> targets) {
        this.domain = domain;
        this.actions = actions;
        this.targets = targets;
        setParts(domain, actions, targets);
    }
{code}

I don't actually use or depend on the properties of this class and I wonder if in fact we need them, since (at least in the use I am making of it) it is essentially just a facade for WildcardPermission.  I don't know what the general Shiro philosophy is on mutability and in particular mutable permissions, but it might be simpler to not introduce it in the facade.  I guess that's why you made the setters protected?  But then how exactly are they expected to be used?   So maybe a) the setters should be dropped... or b) the fields themselves should be dropped...  or even c) drop the class entirely and just add a constructor as above to WildcardPermission

Sorry if my newbie-ness is leading to confusion here.

      was (Author: psteitz):
    Thanks, Wes.  Works4me other than missing the constructor that I added in the second version of the patch:
{code}
public DomainPermission(String domain, Set<String> actions, Set<String> targets) {
        this.domain = domain;
        this.actions = actions;
        this.targets = targets;
        setParts(domain, actions, targets);
    }
{code}

I don't actually use or depend on the properties of this class and I wonder if in fact we need them, since (at least in the use I am making of it) it is essentially just a facade for WildcardPermission.  I don't know what the general Shiro philosophy is on mutability and in particular mutable permissions, but it might be simpler to not introduce it in the facade.  I guess that's why you made the setters protected?  But then how exactly are they expected to be used?   So maybe a) the setters should be dropped... or b) the fields themselves should be dropped...  or even c) drop the class entirely and just add a constructor as above to WildcardPermission

Sorry if my newbie-ness is leading to confusion here.
  
> 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

        

[jira] [Updated] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phil Steitz updated SHIRO-307:
------------------------------

    Attachment:     (was: DomainPermission.patch)

> 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
>
> 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

        

[jira] [Commented] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054106#comment-13054106 ] 

Les Hazlewood commented on SHIRO-307:
-------------------------------------

Cool - thanks Phil.

> 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

        

[jira] [Assigned] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Les Hazlewood reassigned SHIRO-307:
-----------------------------------

    Assignee: Les Hazlewood

> 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
>
> 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

        

[jira] [Updated] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phil Steitz updated SHIRO-307:
------------------------------

    Attachment: DomainPermission.patch

Here is a patch showing one way (possibly wrong) to think about how to use this class.  It includes a lot of incompatible changes - most significantly changing the actions and targets properties to be Sets.   After playing a bit with trying to define the right abstract methods and then a useable subclass, I came to the conclusion that it might be more convenient to just make this a concrete class with a constructor and accessors (which it sort of already had).  I could be missing the point here.  I added a couple of utility methods needed for splitting / joining strings to/from sets mostly stolen from Commons Lang.  I did not see Lang in the pom.  If I missed it, it would be better to just use Lang.  I have not tested or gotten this to work with my Realm yet.  If it is on the right track, I will improve and add tests.

> 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

        

[jira] [Commented] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054819#comment-13054819 ] 

Phil Steitz commented on SHIRO-307:
-----------------------------------

OK, now I get the original intention of the class.  The way that I am using it s really as a bridge between my permissions persistence store and WildcardPermission.  I store <domain, actions, targets> as <String, Set<String>, Set<String>> and use the setParts code in DomainPermission to construct WildcardPermissions from these.  I guess I could just do the string encoding myself and work directly with WildcardPermissions, but it is handy to have the setParts stuff provided.

I now see what you are saying about <String, Set<String>, Set<String>> being app-specific, but you could argue that it is a natural extension of the default <String, String, String>.  It certainly seemed natural to me :)

Another logical option, I guess, is to use generics in the constructor and make setParts abstract.  But that leaves a class of questionable value.

So I am fine with deprecating.  Adding the <String, Set<String>, Set<String> constructor to WildcardPermission or otherwise Keeping somewhere the setParts code that acts as a WildcardPermission factory would be useful to me at least.





> 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

        

[jira] [Commented] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/SHIRO-307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13054792#comment-13054792 ] 

Les Hazlewood commented on SHIRO-307:
-------------------------------------

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

        

[jira] [Issue Comment Edited] (SHIRO-307) DomainPermission does not fully support domain, actions and targets properties

Posted by "Les Hazlewood (JIRA)" <ji...@apache.org>.
    [ 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