Don’t write code like this…

October 21, 2003 at 7:22 pm

I’ve just finished doing a security review of my MSDN columns, and as part of that,
I needed to add some security to an add-in architecture, so the add-in architecture
won’t run antisocial code. The approach taken by .NET security is reminiscent of ACLs,
which means (at least to me) that it’s confusing and I need to read the code 5 or
6 times before I understand it. Along the way, I got a chunk of code from somebody
on the .NET security team, which I’ll reproduce here:

private NamedPermissionSet FindNamedPermissionSet(string name)
{
	IEnumerator policyEnumerator = SecurityManager.PolicyHierarchy();

	while (policyEnumerator.MoveNext())
	{
		PolicyLevel currentLevel = (PolicyLevel)policyEnumerator.Current;

		if (currentLevel.Label == "Machine")
		{
			IList namedPermissions = currentLevel.NamedPermissionSets;
			IEnumerator namedPermission = namedPermissions.GetEnumerator();

			while (namedPermission.MoveNext())
			{
				if (((NamedPermissionSet)namedPermission.Current).Name == name)
				{
					return ((NamedPermissionSet)namedPermission.Current);
				}
			}
		}
	}
	return null;
}
    

Ugly, isn’t it?

The root of the problem is that PolicyHierarchy returns an IEnumerator instead of
an object that supports IENumerable.  This means that I can’t use foreach on
it, and have to write the traversal myself. This decision also means that, without
a collection returned, there’s no place to hang a useful method like “LookupLabel()”
on, so all users are stuck writing the search code themselves (or encapsulating it
off into a separate method).

PolicyLevel.NamedPermissionSets uses a different approach, that of returning an IList,
which is better than an enumerator, as I could use foreach on the IList, but the person
who wrote this sample didn’t because they didn’t know about foreach. If they had,
they would have done the right thing.  

We need to not do this.