Don’t write code like this…


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.

 

Comments (8)

  1. Chris Kinsman says:

    Been there, wrote that code, cursed the guy who create the PolicyHierarchy method.

  2. RichardH says:

    Hi,

    Its probably not the right place to post this, but while we’re on Enumerators, I was writing a rot13 implementation in C# the other day and realised that CharEnumerator returns a read-only char, e.g.

    foreach (char alpha in "A String") {
    alpha = ‘a’ ; // this isn’t allowed as alpha is read-only ???

    What I don’t understand is that char is value type and what I have in alpha is a copy and not a reference so, how it becomes read-only ? I am not writing into the string, just assigning to a char variable.

    Regards,
    Richard.

  3. Drew Marsh says:

    Ick! Yeah, that is pretty ugly. I also find it quite interesting to hear that person who coded this "didn’t know about foreach". Scary.

  4. Drew Marsh says:

    Ick! Yeah, that is pretty ugly. I also find it quite interesting to hear that person who coded this "didn’t know about foreach". Scary.

  5. Val Savvateev says:

    Yeah, looks ugly, but IMO using Foreach instead is not going to add alot of improvement – you’ll still have to loop through, have your own lookup routine – something that should’ve been provided by an interface returned from the methods in first place.

  6. Grant says:

    Disagree with the comment above. It’s not a question of what but where.

    Agree with Eric and others on IEnumerators vs. IEnumerable. The urge to refactor into something cleaner when you hit this is palpable.

  7. Mark Hurd says:

    I was going to say that you can do this in VB.NET because I remembered the spec mentions something about working with a class if it has the methods that are specified by IEnumerable even it doesn’t explicitly declare implementing that interface.

    But this only happens if the object is obtained by a call to GetEnumerator, like the inner loop, and C# can do it too.

    So I see this as an arbitrary limitation of both languages.

    My VB workaround that highlights this:

    Private Class MyPolicyHierarchy
    Public Function GetEnumerator() As IEnumerator
    Return SecurityManager.PolicyHierarchy
    End Function
    End Class
    Private Function FindNamedPermissionSet(ByVal name As String)
    _ As NamedPermissionSet
    For Each currentLevel As PolicyLevel In New MyPolicyHierarchy()
    If currentLevel.Label = "Machine" Then
    For Each namedPermission As NamedPermissionSet _
    In currentLevel.NamedPermissionSets
    If namedPermission.Name = name Then
    Return namedPermission
    End If
    Next
    End If
    Next
    Return Nothing
    End Function

    And a DotLisp sample that doesn’t have this problem (though it needs to jump through other hoops to stop searching when the PermissionSet is found):

    (for-each current-level (SecurityManager:PolicyHierarchy)
    (when (== current-level.Label "Machine")
    (for-each named-permission current-level.NamedPermissionSets
    (when (== named-permission.Name "LocalIntranet")
    (set intranet-ps named-permission)))))

  8. MBA says:

    Helpful For MBA Fans.