2

I am new to asp.net web API. i have made a functions that should validate the user the front end sends data and then i search for data in database. But when the account is not found i always got a exception how should i handle that exception to send to the front end information also what should i return when the first if statement is not true as null dose not work.

public UserData ByPassword(string emailAddress, string password)
        {
            if (emailAddress != null && password != null)
            {
                    Account account = db.Accounts.Where(acc => acc.AccMail == emailAddress && acc.AccPassword == password.ToLower()).Single();
                    string token = OurAuthorizationAttribute.CreateTicket(account.AccID, false);
                    UserData data = new UserData();
                    data.Id = account.AccID;
                    data.Token = token;
                    return data;

            }

her also i have add try and catch block but still the same issue.

public UserData ByPassword(string emailAddress, string password)
        {
            if (emailAddress != null && password != null)
            {
                try
                {
                    Account account = db.Accounts.Where(acc => acc.AccMail == emailAddress && acc.AccPassword == password.ToLower()).Single();
                    string token = OurAuthorizationAttribute.CreateTicket(account.AccID, false);
                    UserData data = new UserData();
                    data.Id = account.AccID;
                    data.Token = token;
                    return data;
                }
                catch
                {
                    throw new OurException(OurExceptionType.InvalidCredentials);
                }
            }
             throw new OurException(OurExceptionType.InvalidCredentials);
        }
MohamedAbbas
  • 1,149
  • 4
  • 15
  • 31
  • it fires when emailAddress or password are not found in database – MohamedAbbas Jul 02 '14 at 07:22
  • No, it doesn't. You should use `SingleOrDefault()` instead of `Single()`, then you'll get `null` if the data isn't in the database. And in any case, oh god, I hope you're not doing authentication like this in a production environment! What's wrong with using ASP.NET Identity (2.0)? It handles all the tricky parts for you and is working using EntityFramework too, so it's easy to use in your case. – Luaan Jul 02 '14 at 07:28
  • Thanks Luann i will consider authentication as you mentioned. any links as starting point – MohamedAbbas Jul 02 '14 at 07:39

1 Answers1

2

System.InvalidOperationException indicates a programming error. You handle it by fixing your code.

In this particular case the error is on this line:

Account account = db.Accounts.Where(acc => acc.AccMail == emailAddress && acc.AccPassword == password.ToLower()).Single();

Your code makes an assumption that Accounts must contain a record for any {emailAddress, password} pair, which is not true. Replacing Single with SingleOrDefault will make the exception go away. Of course you would need to null-check the result to see if the record was there or not.

Here is how you can change your code:

public UserData ByPassword(string emailAddress, string password) {
    // null-check your arguments to detect programming errors in the "upstream" code
    if (emailAddress == null) throw new ArgumentNullException("emailAddress");
    if (password == null) throw new ArgumentNullException("password");
    // Now that your arguments are "sanitized", fix the Single() call
    Account account = db.Accounts.Where(acc => acc.AccMail == emailAddress && acc.AccPassword == password.ToLower()).SingleOrDefault();
    // Missing credentials is not a programming error - throw your specific exception here:
    if (account == null) {
        throw new OurException(OurExceptionType.InvalidCredentials);
    }
    string token = OurAuthorizationAttribute.CreateTicket(account.AccID, false);
    UserData data = new UserData();
    data.Id = account.AccID;
    data.Token = token;
    return data;
}

NOTE : Although the above change would fix the coding error, it would not address a major design flaw of storing passwords in plain text. See this question for an in-depth discussion on storing passwords in databases.

Community
  • 1
  • 1
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Won't exception thrown by `Single` be catched in second case? – Sergey Berezovskiy Jul 02 '14 at 07:33
  • but when the result is null what should i do as a response to the front end and also stop the rest of the code from executing – MohamedAbbas Jul 02 '14 at 07:36
  • @SergeyBerezovskiy Yes, the catch all clause would swallow that exception, along with any other exceptions that code might produce. My point is that the approach is fundamentally incorrect: it is a coding error to call `Single()` on a query with an *optional* result, with or without the catch-all clause. – Sergey Kalinichenko Jul 02 '14 at 07:36
  • Thanks for password hint. – MohamedAbbas Jul 02 '14 at 07:40
  • one more thing in OurException i have defined enum which contains InvalidCredentials = 201, but when i see the response InvalidCredentials is returned not 201 – MohamedAbbas Jul 02 '14 at 07:46
  • 1
    @user2918388 That is strange - perhaps something else is going on before or after the call to `ByPassword` that "masks" `OurException` thrown in the method above. This is a danger of blind `try {} catch {}` blocks that do not care what exception they catch, similar to what your second fragment shows. – Sergey Kalinichenko Jul 02 '14 at 07:51
  • Her is the response. "Message": "An error has occurred.", "ExceptionMessage": "Our own exception of type: InvalidCredentials", "ExceptionType": "AccPlusREST.OurException", "StackTrace": " at AccPlusREST.Controllers.AuthenticationController.ByPassword(String emailAddress, String password) – MohamedAbbas Jul 02 '14 at 07:54
  • 1
    @user2918388 That response looks right to me - you pass an `enum` with a nice-looking name, and you see an `enum` with a nice-looking name. The fact that `InvalidCredentials` is not used in the code that produces the response string. If you want the string to read `"ExceptionMessage": "Our own exception of type: InvalidCredentials"`, change the code that produces the `Message` in `OurException` to cast the type to an `int` for printing. – Sergey Kalinichenko Jul 02 '14 at 08:01