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 bool
s?
(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;
}
1 Answer 1
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.)
- Move all of the processing (that you seem to have removed by using the comment
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.
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\$