Verify if user has at least one of required roles to be able to update a record owned by a certain user

Question

I have a Microsoft CRM 2016 plugin that checks if the user that triggers an update message has at least one of the required roles, when the updated record is owned by a specific user.

Here's the code:

public class CheckIfOwnedByPool : BasePlugin
{
    public CheckIfOwnedByPool(string unsecure, string secure)
        : base(unsecure, secure)
    {

    }


    public override void ExecutePluginLogic(XrmObjects xrmObjects)
    {

        if (xrmObjects.PluginContext.MessageName.ToLower() == "update" && xrmObjects.PluginContext.PreEntityImages.Contains("entity"))
        {

            Guid PoolId = Guid.Empty;
            List<Guid> allowedRoles = new List<Guid>();
            var entity = xrmObjects.PluginContext.PreEntityImages["entity"];

            //retrieve id of "POOL" team and allowed roles GUIDs from plugin step XML config. GUID varies from environment to environment...
            try
            {
                PoolId = new Guid(GetAllNodes("ownerconfig")[0].Attributes["value"].InnerText);
                GetAllNodes("roles")[0].Attributes["value"].InnerText.Split(';').ToList().ForEach(x => allowedRoles.Add(new Guid(x)));
            }
            catch (Exception ex)
            {
                throw new InvalidPluginExecutionException(string.Format("Error deserializing plugin step XML config: {0}", ex.Message));
            }

            if (PoolId == Guid.Empty || allowedRoles.Count == 0)
                return;

            //if entity is owned by team called "POOL"
            if (((EntityReference)entity["ownerid"]).Id == PoolId)
            {
                using (XrmServiceContext ctx = new XrmServiceContext(xrmObjects.Service))
                {
                    //retrieve current user roles
                    var userRoles = ctx.SystemUserRolesSet.Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId).ToList();

                    //check if current user has one of required roles
                    if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))
                        throw new InvalidPluginExecutionException("You do not have access to modify this record. Please see with your supervisor");

                }

            }

        }
    }
}

This seems to work fine and is registered in pre-operation of Update of any entity. As observed, if one plugin in pre-operation throws an exception, all the plugin stack is stopped, meaning all other pre/post operation plugins won't execute, which is what I want.

Do you see anything that can go wrong? The only issue is that every update done on an entity owned by "POOL", there will be a retrieve to get the user's role. This could hurt performance, but by how much ?

BasePlugin is an abstract class implementing IPlugin and xrmObjects holds useful members such as the tracing service, the plugin exec. context, etc. GetAllNodes is a member of BasePlugin which allows for easy access of the XML plugin step config.

Answer 1

try
{
    PoolId = new Guid(GetAllNodes("ownerconfig")[0].Attributes["value"].InnerText);
    GetAllNodes("roles")[0].Attributes["value"].InnerText.Split(';').ToList().ForEach(x => allowedRoles.Add(new Guid(x)));
}
catch (Exception ex)
{
    throw new InvalidPluginExecutionException(string.Format("Error deserializing plugin step XML config: {0}", ex.Message));
}

This is really bad. You are hiding the original exception. If you don't want to pass it as an inner exception at least do ex.ToString() to be able to see the stack-trace later if necessary.

As a matter of fact this try/catch could be a separate method that gets the PoolId from the xml you could then just return the PoolId without the helper variable outside this try/catch block.

---

if (xrmObjects.PluginContext.MessageName.ToLower() == "update" && xrmObjects.PluginContext.PreEntityImages.Contains("entity")) {

Make the method return if the condition is false to reduce nesting.

---

if (PoolId == Guid.Empty || allowedRoles.Count == 0)
   return;

and

if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))
   throw new InvalidPluginExecutionException(...)

I suggest you always use curly braces {}. It can prevent a lot of errors.

---

if (((EntityReference)entity["ownerid"]).Id == PoolId)

Same here, return if false.

---

using (XrmServiceContext ctx = new XrmServiceContext(xrmObjects.Service))

and

var userRoles = 

Be consistant. Use either var or types everywhere.

---

var userRoles = ctx.SystemUserRolesSet.Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId).ToList();

//check if current user has one of required roles
if (!userRoles.Any(x => allowedRoles.Any(y => y == x.RoleId.Value)))

You can merge it into:

var userHasRequiredRole = 
    ctx.SystemUserRolesSet
    .Where(x => x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId)
    .Any(x => allowedRoles.Any(y => y == x.RoleId.Value));

so you can remove the comment and simply write:

if (!userHasRequiredRole) { throw... }

or even shorter:

var userHasRequiredRole = 
    ctx.SystemUserRolesSet
    .Any(x => 
        x.SystemUserId == xrmObjects.PluginContext.InitiatingUserId &&
        allowedRoles.Any(y => y == x.RoleId.Value));

although personally I'd use the Where+Any, it looks cleaner