Add support for resource-location attributes #4
Conversation
Related changes: - removed CategoryContainer(Action,Subject,Resource) string ids - Added support for PEP config defaults for resource, action, subject - renamed policy files to match test class name - removed orphoned config files
|
Anyone? |
|
The changes look good. I would like to merge these in. Bear with me till I figure that out please. |
|
As I mentioned, the changes look good overall. However, there is one item that popped up. The PepConfig properties - defaultSubjectId, defaultResourceId and defaultActionId were introduced with idea of catering to a use case where in the user may decide to go with non-standard attribute identifiers for subject-id, action-id and resource-id attributes. So let's say, instead of "urn:oasis:names:tc:xacml:1.0:subject:subject-id", one could use "my-namespace:my-subject-id". I am not entirely sure what the XACML standard has to say about this though. Regardless, of late I have been thinking that custom subject, action and resource mappers are probably the right solution for this. Anyway, it looks like you have interpreted these properties as representing default attribute values rather than identifiers. I guess the existing implementation may have caused the confusion. But, if that wasn't the case, I am interested to know if you have come across use cases where configuring the default values has been useful. |
|
Thanks @ajithreghu for explaining the reasoning behind the attribute identifiers defaults. Yeah, I believe I was mostly confused by the defaults and decided to clean things up. However, If there are use cases out there where someone wants to use let's say an attribute keys for subject which is not following the XACML standard I'd agree this change is not ideal. Let me reverse some of these changes to keep the existing functionality so users can overwrite the default attribute identifiers. |
This mainly addresses a concern raised where someone may want to set an attribute id (e.g. key of subject-id) which deviates from what XACML defines in its specification.
|
@ajithreghu did you have a chance to look at the latest changes? |
| Action a = (Action) o; | ||
| String id = a.getId(); | ||
| Action action = (Action) o; | ||
| String id = action.getId(); | ||
| if (id == null) { | ||
| id = getPepConfig().getDefaultActionId(); |
There was a problem hiding this comment.
The value returned by getDefaultActionId() is an identifier key but I believe it's treated as an identifier value, looking at the next few lines.
There was a problem hiding this comment.
yes, when I changed this code, given the lack of documentation, my impression was that getPepConfig().getDefaultActionId() returns a default action id value and not the action id key (which IMO would make more sense).
|
Just looked at the updates you made recently and added a comment. I guess it applies to all three mappers. I have some other thoughts though. I believe the design of customizable/configurable attribute identifiers was not done intuitively to start with (my bad) and so all the confusion :( I guess API users or contributors will have a hard time with it and most likely use it incorrectly, just like you were mislead. So, what if we take this feature out and support only standard identifier names by default. However, in case the user wants custom attribute identifiers, they could still do it by providing custom Subject, Action and Resource mappers. This way the design is clean, simple and intuitive. What do you think ? |
|
@ajithreghu: I think I'd agree with your suggestion. I kind of regret to put all the changes along with introducing |
This commit removes the rather 'confusing' feature of being able to override the default key for subject, resource and action ids defined by XACML spec. Users who relied on feature can easily switch to use #addAttribute to achieve the same.
|
Any further feedback/concerns with the most recent changes made? |
|
Thank you. I've merged your latest changes on to the wip branch. Also, I added a new test to demonstrate custom mapper registration to handle non-standard subject,action and resource identifiers. |
Related changes: