1
\$\begingroup\$

I have limited experience with both making REST APIs and implementing security features, so I figured it'd be worth running this code through some other people's eyes before making it live.

My main questions are around the use of REST - e.g. are there better ways to implement my methods that return bools?

(Assume the "drivers" are keyed in the DB by Name)

public class Driver
{
    public string Name { get; set; }
    public string Pass { get; set; }
}

[HttpPost]
[RequireHttps]
[ActionName( "GetPasswordExists" )]
public HttpResponseMessage GetPasswordExists([FromBody] Driver driver)
{
    HttpResponseMessage response;
    try
    {
        bool passExists = false;
        // db operations
        response = Request.CreateResponse(HttpStatusCode.OK, passExists);
    } 
    catch (Exception)
    {
        response = Request.CreateResponse(HttpStatusCode.BadRequest);
    }
    return response;
}

[HttpPost]
[RequireHttps]
[ActionName("SetPassword")]
public HttpResponseMessage SetPassword([FromBody] Driver driver)
{
    HttpResponseMessage response;
    try
    {
        string hash = System.Web.Helpers.Crypto.HashPassword(driver.Pass);
            // db operations
            response = Request.CreateResponse(HttpStatusCode.OK);
    }
    catch (Exception)
    {
        response = Request.CreateResponse(HttpStatusCode.BadRequest);
    }
    return response;
}

[HttpPost]
[RequireHttps]
[ActionName("VerifyPassword")]
public HttpResponseMessage VerifyPassword([FromBody] Driver driver)
{
    HttpResponseMessage response;
    try
    {
        bool valid = false;
        string hash = null;
        // db operations
        valid = System.Web.Helpers.Crypto.VerifyHashedPassword(hash, driver.Pass);
        response = Request.CreateResponse(HttpStatusCode.OK, valid);
    }
    catch (Exception)
    {
        response = Request.CreateResponse(HttpStatusCode.BadRequest);
    }
    return response;
}
\$\endgroup\$
  • \$\begingroup\$ Not quite an answer, but just your prose paragraph makes me nervous. Are you a team of security researchers, each with decades of experience in the field? No? Don't roll your own crypto algorithms (stuff like hashing and encryption). It looks like you're not, but I'm not familiar enough with C#, and I just wanted to make sure you're not committing the cardinal sin of infosec. \$\endgroup\$ – Fund Monica's Lawsuit Dec 11 '18 at 1:40
  • \$\begingroup\$ I'm definitely not in a team like that, so I'm doing my very best to avoid any in-house nonsense. The algorithm used (in System.Web.Helpers.Crypto) appears to be the recommended one by others on SO who also use C# to encrypt passwords. If anyone else who is more familiar with C# wants to point out that this is not the case, or suggest an alternative, I'm all ears! \$\endgroup\$ – pinkie Dec 11 '18 at 1:55
  • \$\begingroup\$ Fair enough. Like I said, I'm not familiar enough with C# to tell here. I just wanted to reiterate that if you find yourself writing a custom cryptographic hash or encryption, you're probably doing something wrong :) \$\endgroup\$ – Fund Monica's Lawsuit Dec 11 '18 at 8:53
  • 1
    \$\begingroup\$ (@BCdotWEB: Valid observations. Then, even a single valid point presented well makes a good answer.) \$\endgroup\$ – greybeard Dec 11 '18 at 12:47
2
\$\begingroup\$

Some quick remarks:

  • Keep your controllers lean and mean:

    • Move all of the processing (that you seem to have removed by using the comment // db stuff) to dedicated classes,
    • bind it all together using MediatR,
    • validate incoming data with FluentValidation.

    There are plenty of examples that show you how to combine these two, e.g. this Stack Overflow question. Jimmy Bogard also has plenty of examples on his own blog, e.g. this post.

    Microsoft also promotes this kind of structure:"Implement the Command and Command Handler patterns". (In this case in the context of CQRS, which is something that you also could look into.)

  • Don't needlessly abbreviate (why "pass" instead of "password"?).

  • Don't use exceptions to control your logic. An invalid password should IMHO not throw an exception, it's an expected result. Even worse it that you seem to be "eating" your exceptions and don't even log them.

  • WRT your app's logic, you might want to look at the logic in similar applications, e.g. "Simple API for Authentication, Registration and User Management". Also think about things like "how many times can a user try to log in and fail before his account gets locked?" in order to avoid hackers who'll try to access the system by going through large lists of often-used passwords.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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