2
\$\begingroup\$

Here's a bit of one of my web forms which is reached by clicking on a link in a calendar. Based on what you click, you end up at this page with a different QueryString. Based on that QueryString, we hide/show and enable/disable controls related to actions associated with the QueryString, as well as the role of user.

We have three different types of events:

  • VacationDays (days where people took paid vacation)
  • VacationDaysUnpaid (days where people took unpaid vacation)
  • CalendarEvents (everything else such as meetings and client visits)

Here's a slimmed down version of how the page is prepared:

protected void Page_Load(object sender, EventArgs e)
{
    if (!IsPostBack)
    {
        InitializePage();
    }
}

private void InitializePage()
{
    CheckRoles();

    if (!String.IsNullOrEmpty(Request.QueryString["CalendarEventID"]))
    {
        if (!HttpContext.Current.User.IsInRole("Guest"))
        {
            //if this is just a regular calendar event, allow non-guests to edit
            btnedit.Enabled = true;
            btndelete.Visible = false;
        }
    }
    if (!String.IsNullOrEmpty(Request.QueryString["VacationDayID"]))
    {
        if (!HttpContext.Current.User.IsInRole("Guest"))
        {
            //for vacation days, edits are not allowed, but the ability to delete is
            btnedit.Visible = false;
            btndelete.Enabled = true;
            btndelete.Visible = true;
        }
    }
    if (!String.IsNullOrEmpty(Request.QueryString["VacationDayUnpaidID"]))
    {
        //unpaid vacation days can only be deleted by administrators
        if (HttpContext.Current.User.Identity.Name.ToString().ToLower() == "administrator")
        {
            btndelete.Visible = true;
        }
        else
        {
            btndelete.Visible = false;
        }
    }
}

private void CheckRoles()
{
    if (HttpContext.Current.User.IsInRole("Guest"))
    {
        //disable all write controls if guest
        btnedit.Visible = false;
        btndelete.Visible = false;
        btnsave.Visible = false;
    }
}

It's jumbled, it's confusing, and it's difficult to maintain. Is there any kind of common design or plan of attack for doing what I'm trying to do here? I mean this works, I can work with it, I guess, but I really feel like there must just be some technique that I don't know about which will help improve this into something much easier and manageable.

One idea that I'm considering, and that I know will work, is to separate the code related to the different types of events into separate files as "partial"

That way in my InitializePage() method, for each type of QueryString, I'll have code that says like EnableCalendarEventMode(), which would be located in the partial class and would contain all the code related to calendar events. This idea will make it easier to manage for me personally, but it still doesn't change the code, it just moves it around.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

For one thing, everything is just setting booleans. Condense down all those nested if blocks down into boolean statements and use good names, and it'll even make sense in "plain English".

protected void Page_Load(object sender, EventArgs e) {
    if (IsPostBack) return;
    Func<string, bool> QueryStrHas = key => !String.IsNullOrEmpty(Request.QueryString[key]);
    var isCalEvent = QueryStrHas("CalendarEventID");
    var isVacDay  = QueryStrHas("VacationDayID");
    var isUnpaidVac = QueryStrHas("VacationDayUnpaidID");

    var user = HttpContext.Current.User;
    var isGuest = user.IsInRole("Guest");
    var isAdmin = user.Identity.Name.ToString().ToLower() == "administrator";

    // guests cannot edit
    // non-guests can edit regular calendar events, but not vacation days
    // non-guests can delete vacation days
    // unpaid vacation days can only be deleted by administrators
    btnedit.Enabled = !isGuest && isCalEvent && !isVacDay;
    btndelete.Enabled = !isGuest && isVacDay;
    btndelete.Visible = !isGuest &&
                        (isVacDay||
                        (isAdmin && isUnpaidVac));
}

Make sure you double-check the boolean arithmetic by testing it though. There're some missing cases (kind of), and you should make sure it's all well-defined.

Oh, by the way, just cosmetically "hiding" the btnedit when a user isn't an Admin does not count for true security. Anybody can actually modify all these values when it reaches their browser client-side.

\$\endgroup\$
2
  • \$\begingroup\$ Ooo, great answer. Thanks for that. Can you briefly explain how I would be able to make a page actually secure, not just hiding the button? The design request is that the user can still access the page and see stuff if Guest, so just shutting out the whole page is no good. I'm guessing I need to add something in the button click event that checks once again to make sure you actually have the right role, and if not, do not proceed with the execution/saving. Something along those lines? \$\endgroup\$ Oct 12, 2012 at 0:03
  • \$\begingroup\$ Just don't rely on the client-side buttons for security. Someone who's not authorized could write their own html/javascript and tell the server that he pressed the button. The server needs to be able to determine whether or not the user clicking is allowed to execute the command. If you do that check in the ASP.net button.click event handler, you should be fine. \$\endgroup\$
    – Kache
    Oct 12, 2012 at 0:57
0
\$\begingroup\$

Here's an alternative to the approved answer that provides a different perspective. I try to write self-documenting code so the next developer can understand it without have to think about it. I believe this code achieves that goal. It does end up being more code but when the requirements change it will be easy to make changes to this code.

protected void Page_Load(object sender, EventArgs e)
{
    if (!IsPostBack)
        InitializePage();
}

private void InitializePage()
{
    CheckRoles();
    CheckCalendarEvent();
    CheckVacationDay();
    CheckVacationDayUnpaid();
}

private void CheckRoles()
{
    if (IsGuest())
    {
        //disable all write controls if guest
        btnedit.Visible = false;
        btndelete.Visible = false;
        btnsave.Visible = false;
    }
}

private void CheckCalendarEvent()
{
    if (ParameterExists("CalendarEventID") && !IsGuest())
    {
        //if this is just a regular calendar event, allow non-guests to edit
        btnedit.Enabled = true;
        btndelete.Visible = false;
    }
}

private void CheckVacationDay()
{
    if (ParameterExists("VacationDayID") && !IsGuest())
    {
        //for vacation days, edits are not allowed, but the ability to delete is
        btnedit.Visible = false;
        btndelete.Enabled = true;
        btndelete.Visible = true;
    }
}

private void CheckVacationDayUnpaid()
{
    //unpaid vacation days can only be deleted by administrators
    btndelete.Visible = ParameterExists("VacationDayUnpaidID") && IsAdministrator();
}

private bool IsGuest()
{
    return HttpContext.Current.User.IsInRole("Guest");
}

private bool IsAdminstrator()
{
    return HttpContext.Current.User.Identity.Name.ToString().ToLower() == "administrator";
}

private bool ParameterExists(string p)
{
    return !String.IsNullOrEmpty(Request.QueryString[p]);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.