Validating user access to a function

Question

I was reviewing the following code block and noticed there was validation check at the top. The 3rd line in the function checks if the invoice belongs to the logged in customer.

I am wondering what the correct way of doing this might be. Some options that come to me head are:

1. GetInvoice should take an overload and also take in the loggedInCustomerId. The GetInvoice method would then throw an exception if it was an invalid call. (Would the business logic be the best place for this)
2. Filter attribute should be created (i think this is overkill)

What would the best approach to something like this be?

    public ActionResult ViewInvoice(Guid invnum)
    {
        int loggerInCustomerId = GetTheLoggedInCustomerId();
        Invoice invoice = _invoiceLogic.GetInvoice(invnum);

        if (invoice.CustomerId != loggerInCustomerId)
        {
            //Invalid Action
            return RedirectToAction("Index", "MyInvoices");
        }
        //do other stuff as normal
    }

Answer 1

public ActionResult ViewInvoice(Guid invnum)  

Regardless that one could figure out that invnum means "invoice number" this name isn't good. First it isn't a number but a globally unique identifier and second using abbreviations for variable naming shouldn't be done because it reduces readability.

-> how about renaiming this to invoiceId ?

int loggerInCustomerId = GetTheLoggedInCustomerId();  

The variable name is misspelled you should change it to loggedInCustomerId

---

The "validation" part wether a invoice belongs to the current loggedin customer should be done by querying the database (if one is used) to get the invoice which matches the invoiceId and the loggedInCustomerId. An overloaded GetInvoice(GUID, int) could be used in the business layer (will be _invoiceLogic) which shouldn't throw an exception but return either null or some Invoice.Empty constant. Throwing an exception doesn't buy you much but will slow down the application because you will also need to handle it.

Maybe a method like the Dictioary<T>.TryGetValue() would be a good fit so one could write something along this lines

public ActionResult ViewInvoice(Guid invoiceId)
{
    int loggedInCustomerId = GetTheLoggedInCustomerId();
    Invoice invoice;

    if (!_invoiceLogic.TryGetInvoice(invoiceId, loggedInCustomerId, out invoice)
    {
        return RedirectToAction("Index", "MyInvoices");
    }
    //do other stuff as normal
}  

A version without accessing the data layer could then look like

public bool TryGetInvoice(Guid invoiceId, int customerId, out Invoice invoice)
{
    invoice = GetInvoice(invoiceId);
    if (invoice.CustomerId == customerId) 
    {
        return true;
    }

    invoice = null;
    return false;
}  

A version with a fictive data layer (because I don't know if you use one) could look like this

public bool TryGetInvoice(Guid invoiceId, int customerId, out Invoice invoice)
{
    invoice = _dataLayer.GetInvoice(invoiceId, customerId);

    return invoice != null;
}  

If you use a TryGetXXX method you need to make sure that such a method never throws an exception because it is expected that it doesn't.