Friday, April 16, 2010

Be Careful What You Fix

I wake up with odd memories. I think my brain is just getting exercise, staving off senility maybe.

Yesterday I woke with a memory of a bug fix from long ago, probably 15 years or more. I was given a project which led me into the permissions-checking code of a billing system. When I got there, I saw the code was written in such a way as:

permission* perm = GetPermission(thisPerm);
if ( perm != null && perm.Allow == false) {
return false;
return true;

I know that's silly, but I am recreating from an old memory, and I remember the code having some silly uses of null & the like.

I remember noticing right away that if a permission did not exist, it meant that the user can perform the activity. GetPermission() read from a database table, I think, so that all a user had to do to get unlimited access was to drop records from a table. Furthermore, if an installation tech didn't deny access to unpaid features, the customer got them for free.

I don't want to reflect on the validity of the business model here, whether restricting features and paying for use of them is a good idea or whatever. It was a simple code error and a simple code fix to me and I only needed to know if the behavior was intended or accidental.

I quickly consulted some people who told me that it was not the right behavior. They said that some users had paid extra for features and would be angry if other customers were getting them for free. A little research, and we find that most customers had most features for free. As requested I fixed the feature so that having no permission meant having no feature. They figured that they were safe since they didn't ship documentation for features that weren't covered in the contract (security by obscurity). I had approval for the fix, and completed the ticket quickly, and you'd think that was the end of the story.

To the sales and marketing, however, it was a fiasco. After the next release there were angry people asking why they couldn't do their normal reporting and administrative tasks, why they couldn't do various normal feats that they could do only the week before with no issue. We had taken back from our users the features that we'd granted.

People did not rely on the manuals to tell them what the software did. That would be silly. Interfaces are rightly made explorable to people can learn it for themselves. Omitting docs did not lock down features.

Not testing the permission systems wasn't all that smart either, because nobody realized that it was "in promiscuous mode" until one of us programmers stumbled onto it.

Maybe the most damning error, though, was taking back what we had given. Whether they were entitled to the extra features or not, they'd had them. The genie was out of the bottle, the toothpaste sans tube. A technical correction was a marketplace error.

It was a decent lesson to learn, though I soaked the blame for screwing up the customers by fixing the code. I had permission, had talked it over, and had an approved ticket to fix it, but ultimately it was my fingers typing the code and my hands testing the correction, so I had to wear it.

I got in more trouble for fixing things than I ever did for breaking them. There's a lesson in that, too.