Patrick,

I think you're right, but the devil might be in the implementation details here -at least as far as the services code is concerned. Until I've looked at the code again, I'm hesitant to assert complete concurrence -i.e., agree :-). A 20 minute review of the code will give me a bit more confidence.

-Richard

On 8/22/2011 11:58 AM, Patrick Schmitz wrote:

So, should the permissions for the base resources properly have a '/' prepended? Seems like it should to me.

As to why the app/UI shear off the leading slash when they are building role/perm associations, that is another story. The result is that the role-perm association built by the app declares a non-existent permission in the resource, but since the id reference is correct, the enforcement works. AIUI, the app could declare the permission_resource value to be "/MonkeyHeadAle/OnlyOnThursdays" and so long as the id matched a real permission, the AuthZ enforcement would function.

I can see why we would denormalize the permission resource into the permissions_roles table, but I cannot see why we would trust a caller to set that value - the services should properly take a role id and a perm id, and build the permissions_roles entry, IMHO.

We should consider changing PermissionRoleValidatorHandler.isPermissionInvalid() to also assert that the resource name for the found permission (when we validate the id) should match the resourceName in the passed PermissionValue structure. Similarly, I would assert a match on the role names as well. Since we already validate the id reference, the change should not slow down the logic in place.

This is another case in which I think we would have precluded bugs and confusion, if the services team had put in place stricter validation of our payloads. This takes more time in development, and can constrain the callers somewhat, but IMO it would prevent more headaches than it might cause.

Comments?

Patrick

________________________________

From: Richard Millet [mailto:richard.millet@berkeley.edu]
Sent: Monday, August 22, 2011 11:43 AM
To: Patrick Schmitz
Cc: 'Chris Martin'; 'Richard Millet'; 'Kasper Galschiot Markus'; tech@lists.collectionspace.org; 'Yura Zenevich'
Subject: Re: [Tech] Possible blocker for 1.9: UI for pre-built roles broken onnightly

I'm not sure about the "/" prefixes and suffixes. We'll need to look at the code to understand what is actually required and why. But I do know why workflow is different. Here is my recollection:

The service's protoype "tenant-bindings-proto.xml" might shed some light on this. Resources (and subresources) that can be bound to permissions (i.e., can be protected) are declared using elements. The base resources are implicitly declared using the service name. A while back, we made the decision that all (almost all) sub-resources should inherit the base resource's permissions. "Workflow" sub-resources however, are a special case, and don't (can't and shouldn't) inherit permissions from the base resources.

Before we made the decision to have sub-resources inherit base resource permissions, the proper way to declare a sub-resource was the following format:

>/${baseResource}/*/${subResource}/

The "workflow" work we did just followed this format when declaring sub-resources.

On 8/22/2011 11:19 AM, Patrick Schmitz wrote:

I looked at this a bit more, and what I saw was that the pre-built roles use the resourceName defined in the permission, and the ones built through the application UI shear off the leading slash.

This is only an issue for the workflow permissions. The permissions for our base service resources all lack a leading slash (e.g., acquisitions), whereas all the permissions for the associated workflow resources have a leading (and trailing) slash (e.g., /intakes/*/workflow/). Richard, any idea why we have this discrepancy?

Is it possible the the app layer is getting the permission resource not from the resourceName element, but from the objectIdentityResource element? In the latter, the leading the slash is omitted in the payload (i.e., in the results of a get to /cspace-services/authorization/permissions). Again, I am unclear why there is this discrepancy.

Patrick

List roles on nightly:
http://nightly.collectionspace.org:8180/cspace-services/authorization/roles
List perms on nightly:
http://nightly.collectionspace.org:8180/cspace-services/authorization/pe...
List perms for tenant admin role on nightly (at least today - CSID will change):
http://nightly.collectionspace.org:8180/cspace-services/authorization/ro...

________________________________

From: Chris Martin [mailto:csm22@caret.cam.ac.uk]
Sent: Monday, August 22, 2011 9:28 AM
To: Patrick Schmitz
Cc: 'Kasper Galschiot Markus'; tech@lists.collectionspace.org; 'Yura Zenevich'
Subject: Re: [Tech] Possible blocker for 1.9: UI for pre-built roles broken onnightly

It should be all fixed now.. it was a tiny tiny little typo that only occurred on the TENANT roles as they were the only ones that prefixed their resourcename with a slash.. in /role/***/permrole not sure why some do and some don't but for good measure I remove the slash now

Chris

On 22/08/2011 17:07, Patrick Schmitz wrote:

Thanks, Kasper.

Curious thing is that I thought Yura fixed something related to this in 1.8, so am curious what happened to it.

Yura, when you get a chance, can you take a look at this. Am wondering if a checkin got lost in our various branches or something. I thought you fixed a bug where the prebuilt roles were missing their associated permissions, and I thought it had something to do with the role names or something.

Thanks - Patrick

________________________________

From: tech-bounces@lists.collectionspace.org [mailto:tech-bounces@lists.collectionspace.org] On Behalf Of Kasper Galschiot Markus
Sent: Monday, August 22, 2011 7:48 AM
To: tech@lists.collectionspace.org
Subject: Re: [Tech] Possible blocker for 1.9: UI for pre-built roles broken onnightly

Created CSPACE-4338 for this - as critical with fix-for 1.11

~Kasper

On 08/20/2011 02:12 AM, Patrick Schmitz wrote:

I was checking some functionality around roles, and saw that on nightly, in the Admin section, on the Roles+Perms tab, if you select one of the pre-built roles TENANT_ADMINISTRATOR or TENANT_READER, the right-side display no longer shows the display name or description. I verified that the values are still in the DB, and that the services calls (e.g., http://localhost:8180/cspace-services/authorization/roles) return the correct values.

I am not sure why this would work this way, as a display name is a display name, and roles created by users seem to work fine.

However, this seems like a bug to me. It actually precludes users from touching the prebuilt roles, which is a desired result. However, it is a fairly odd way of achieving this end...

_______________________________________________
Tech mailing list
Tech@lists.collectionspace.org
http://lists.collectionspace.org/mailman/listinfo/tech_lists.collections...