Determining complex security and permissions

Question

I've been put on a project that has a messy class that writes buttons to a page. The app is a document manager and has a popup list of buttons, such as download, email, and print. Depending on the user's roles, and the state of the document, different buttons are displayed.

Among other WTFs is something like this:

bool showEditButton = document.docTypeId == documentEnum.docType.text && 
( document.statusId == documentEnum.docStatus.Editable || (user.UserStatus == userEnum.Status.SuperUser) || ( user.UserID == document.CreatedByUserId ) )

And so on and so forth until I can't figure out what's going on.

I don't know if this is just a side effect of a deeper architectural flaw, or if there's a good method to deal with checking a mixture of permissions and status values. Should I just put all of these crazy conditions in a method and just forget about it? That doesn't benefit the next programmer to inherit the project though.

I chuckled, and wondered if you inherited some code that I also inherited once. Every day that I'd open up the source I'd find something to laugh about and shake my head. (that is to say that I've seen the line you posted-- or one very similar-- before... my solution was eventually running for the door.) — Jeremy, Jul 29 '11 at 21:32

score 2 · Accepted Answer · answered Jul 29 '11 at 17:57

It's just a bunch of boolean logic in your example, but the readability could be cleaned up with a Compose Method refactoring. If you had a class that accepted a document and the current user principal, then you could have something like:

public class DocumentPermissions
{
    private Document document;
    private User user;

    public DocumentPermissions(Document doc, User currentUser)
    {
        document = doc;
        user = currentUser;
    }

    public bool ShouldShowEditButton()
    {
        if(!IsTextDocument())
        {
            return false;
        }
        return IsSuperUser() || IsDocumentOwner() ||  DocumentIsEditable();
    }

    private bool IsTextDocument()
    {
        return document.docTypeId == documentEnum.docType.text;
    }

    private bool IsSuperUser()
    {
        return user.UserStatus == userEnum.Status.SuperUser;
    }

    private bool IsDocumentOwner()
    {
        return user.UserID == document.CreatedByUserId ;
    }

    private bool DocumentIsEditable()
    {
        return document.statusId == documentEnum.docStatus.Editable ;
    }
}

Obviously this is a lot of code so I hope you can make reuse of many of the private methods.

Thanks, this is a good solution. It allows me to get it out of the way but also adds some readability to it, even though it's a little verbose. I think one of the answers is that if any one condition is false, the entire thing is false, right? — MrGrigg, Jul 29 '11 at 18:15

score 1 · Answer 2 · answered Jul 29 '11 at 18:14

Alternatively you could use:

bool showEditButton = (document.statusId == documentEnum.docStatus.Editable); //show if Editable..
showEditButton |= (user.UserStatus == userEnum.Status.SuperUser); //or a superuser or
showEditButton |= (user.UserID == document.CreatedByUserId); //the Creator
showEditButton &= (document.docTypeId == documentEnum.docType.text); //and a text Doc

Although, I prefer Ryan's answer, I will throw this out for another way that is at least marginally more readable and give a better spot for some comments.

Thanks for this. I wasn't familiar with the assignment operators, so if anything you've expanded my knowledge a little bit. I also think this is a good intermediate option between where I am now and what Ryan suggested. — MrGrigg, Jul 29 '11 at 18:23

score 0 · Answer 3 · answered Jul 29 '11 at 17:57

0

You own it for the time being; if you've been assigned to refactor it, refactor it. If you have other more pressing issues, deal with them, but you should take your spare time to refactor it if you can (Don't do TOO good a job, they might make you permanent owner). Regarding your other questions, security etc., not enough info.

http://en.wikipedia.org/wiki/Refactoring

answered Jul 29 '11 at 17:57

gangelo

3,034
4
29
43

Yeah, we don't want to go too crazy :) I suppose for the time being, it works. It's just confusing and difficult to try and add or remove things if you have to. – MrGrigg Jul 29 '11 at 18:14

score 0 · Answer 4 · answered Jul 29 '11 at 17:58

0

To be honest, the example code doesn't look too bad. I've certainly seen a lot worse.

It's quite readable and has no "magic strings" or "magic numbers". I'm sure you can find more pressing opportunities for a clean-up.

answered Jul 29 '11 at 17:58

Steve Morgan

12,978
2
40
49

Well, the example was supposed to be easy to understand. The one I was looking at today takes into account about 16 separate conditions, and once you're down to the third or fourth logical operator, you start to lose where you are. – MrGrigg Jul 29 '11 at 18:13

Determining complex security and permissions

4 Answers4