IAuthorizationServiceEventHandler and RolesBasedAuthorizationService

Topics: Core, Customizing Orchard, Troubleshooting, Writing modules
Sep 26, 2013 at 2:09 AM
I've implemented an IAuthorizationServiceEventHandler in a custom module that applies additional security checks in the .Complete() method to deny access to specific content types in somes cases.

Specifically, if someone attempts to edit, via the built-in contents controller, a content type that is defined by this module, then I check for more specific security permissions defined my the custom module.

(I know I could possibly use the content type permissions module, but I have some specific requirements that it doesn't quite meet)

In my implementation of the .Complete() method, I check if the content type is my specific content type, and I check if the permission being checked is Edit Content (context.Permission == Core.Contents.Permissions.EditContent).

The problem that I've hit is that in some cases, when the .Complete() method is called, the value in context.Permission has been changed from EditContent to EditOwnContent.

This occurs in the RolesBasedAuthorizationService.TryCheckAccess() method when it calls .Adjust in the for loop. Basically what has happened is the default EditContent check fails, it then loops calls .Adjust() and another IAuthorizationServiceEventHandler (specifically Core/Contents/AuthorizationEventHandler.cs) adjusts the permission.

I understand why this happens (if EditContent fails, it will try EditOwnContent, or possibly even some more specific other content type permission).

However, I think that after the adjustment loop has completed (it attempts adjusting up to three times), I think the code in RolesBasedAuthorizationService.TryCheckAccess() should revert the context.Permission value back to the originally requested value before calling IAuthorizationServiceEventHandler.Complete(). This way any implementations of this method can rely on this value not having changed from the originally requested permission.

Alternatively, the CheckAccessContext class should store the originally requested permission in an "OriginalPermission" member, or maybe if an adjustment occurs, it should instead be stored to an "AdjustedPermission" member leaving the original value untouched in the existing "Permission" member.

Apart from my custom module, I wonder if this is already causing issues in core code? For example, I can see that if a content type has the Common part attached to it, then the .Adjust() method in the /Core/Contents/AuthorizationEventHandler class can in some cases change the context.Permission from EditContent to EditOwnContent. If someone also has Content Item Permissions module enabled and associated with the same content type, then its check for specific content item permissions could fail because in its implementation of the .Complete() method, it too only checks for EditContent (not EditOwnContent).

Thoughts?
Developer
Sep 26, 2013 at 3:45 AM
Edited Sep 26, 2013 at 3:46 AM
It's not a problem because there is a link between EditContent and EditOwnContent permissions through the ImpliedBy property.
So you should not only check for the exact match but also look for the match within the ImpliedBy permission collection (drill down).
Sep 26, 2013 at 6:51 AM
Edited Feb 13, 2014 at 11:31 AM
Thanks @pszmyd,

I understand what you are saying, so in my implementation of Complete() in my event handler, I now not only look for the EditContent permission directly on the context.Permission value that is passed in, but also check up the ImpliedBy chain of the context.Permission value that is passed in. This works for me since EditOwnContent is implied by EditContent, therefore EditContent will be contained in the ImpliedBy collection of the EditOwnContent permission.

Given this change, I still wonder if there's not a bug in the ContentPermissions module in its implementation of IAuthorizationServiceEventHandler.Complete()?

Looking at the section of code in the Complete() method...
if (context.Permission == Core.Contents.Permissions.ViewContent) {
    authorizedRoles = (hasOwnership ? part.ViewOwnContent : part.ViewContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (context.Permission == Core.Contents.Permissions.PublishContent) {
    authorizedRoles = (hasOwnership ? part.PublishOwnContent : part.PublishContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (context.Permission == Core.Contents.Permissions.EditContent) {
    authorizedRoles = (hasOwnership ? part.EditOwnContent : part.EditContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (context.Permission == Core.Contents.Permissions.DeleteContent) {
    authorizedRoles = (hasOwnership ? part.DeleteOwnContent : part.DeleteContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else {
    return;
}
I am thinking this would not work if the Adjust() method on the core content type handler changes context.Permission (eg. from EditContent to EditOwnContent). Is it reasonable to expect the above code really should be doing something as follows?
// Determine which set of permissions would satisfy the access check 
// (this gets all permissions that are implied by context.Permission)
var contextPermissions = 
    PermissionNames(context.Permission, Enumerable.Empty<Permission>())
        .Distinct().ToList();

if (contextPermissions.Contains(Core.Contents.Permissions.ViewContent.Name)) {
    authorizedRoles = (hasOwnership ? part.ViewOwnContent : part.ViewContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (contextPermissions.Contains(Core.Contents.Permissions.PublishContent.Name)) {
    authorizedRoles = (hasOwnership ? part.PublishOwnContent : part.PublishContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (contextPermissions.Contains(Core.Contents.Permissions.EditContent.Name)) {
    authorizedRoles = (hasOwnership ? part.EditOwnContent : part.EditContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else if (contextPermissions.Contains(Core.Contents.Permissions.DeleteContent.Name)) {
    authorizedRoles = (hasOwnership ? part.DeleteOwnContent : part.DeleteContent)
        .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
}
else {
    return;
}
Feb 13, 2014 at 11:10 AM
Edited Feb 13, 2014 at 11:12 AM
@mjy78: un-indenting that code, i.e. removing the whitespace from the beginning of every line, will dramatically improve legibility of your code.
You could even format it in a way that all code lines will be at most the 88 chars that are shown per line ;-)
Feb 13, 2014 at 11:33 AM
@_oliver_: Fair call. I had just copied and pasted the code form the original source file including indentation. Above code now updated for readability :-)