CodeSOD: A Secure Item

Kirill writes:

I’ve worked in this small company for a year, and on a daily basis I’ve come across things that make my eyes sink back into their sockets in fear, but mostly I’ve been too busy fixing them to post anything. It being my last day however, here’s a classic

We’ll take this one in parts. First, every element of the UI the user can navigate to is marked with an enum, defined thus:

enum UiItem
{ SectionA, SectionB, SectionC,...SectionG }

These names are not anonymized, so already I hate it. But it’s the next enum that starts my skin crawling:

enum SecurityUiItem
{
  SectionA = UiItem.SectionA,
  SectionB = UiItem.SectionB,
  ...
  SectionG = UiItem.SectionG
}

A SecurityUiItem is a different type, but the values are identical to UiItem.

These enums are used when trying to evaluate role-based permissions for access, and that code looks like this:

if ((currentAccess.ContainsKey(SecurityUiItem.SectionA) && currentAccess[SecurityUiItem.SectionA] != AccessLevel.NoAccess))
        return UiItem.SectionA;
else if (!currentAccess.ContainsKey(SecurityUiItem.SectionB)
        || (currentAccess.ContainsKey(SecurityUiItem.SectionB) && currentAccess[SecurityUiItem.SectionB] != AccessLevel.NoAccess))
        return UiItem.SectionB;
else if (!currentAccess.ContainsKey(SecurityUiItem.SectionC)
        || (currentAccess.ContainsKey(SecurityUiItem.SectionC) && currentAccess[SecurityUiItem.SectionC] != AccessLevel.NoAccess))
        return UiItem.SectionC;
.....
else if (!currentAccess.ContainsKey(SecurityUiItem.SectionG)
        || (currentAccess.ContainsKey(SecurityUiItem.SectionG) && currentAccess[SecurityUiItem.SectionG] != AccessLevel.NoAccess))
        return UiItem.SectionG;
else
        return UiItem.Unknown;

Honestly, I don’t hate the idea of having one data type representing the actual UI objects and a separate data type which represents permissions, and having a function which can map between these two things. But this is a perfect example of a good idea executed poorly.

I also have to wonder about the fall-through pattern. If I have access to SectionA, I only seem to get SectionA out of this function. Are these permissions hierarchical? I have no idea, but I suspect there’s a WTF underpinning this whole thing.

Congratulations on Kirill’s last day.

Source: Read More