Authorization - Create and unlock a User

Question

I just wrote this, but I am not realy happy with that. I think there is an easier way to do this. First I am checking if a user has an ID (from a portal we are using, it is in the header). After that I have to check if the user exists in my application, if not a user will be created, if the user exists, the datetime lastlogon will be updated. (here is my first problem, CheckIfUserExists always returns true, except on an Exception, is this ok?). After that I am checking if userExists is true. If a user is created it has to be unblocked by an admin. If this happened the user can access the page. If the user is in Role Admin, Viewbag is true otherwise false. If userExists is false, he is getting the default error.

I think my chain of thought is realy complicated.

public ActionResult Index()
{
 bool userExists = false;
 if (HelperClass.UserId != "")
 userExists = CheckIfUserExists(HelperClass.UserId);
 else
 return RedirectToAction("AccessDenied", "Error");
 if (userExists)
 {
 bool unblocked = CheckIfUnblocked(HelperClass.UserId);
 if (unblocked)
 {
 if (User.IsInRole("Admin"))
 ViewBag.Admin = true;
 else
 ViewBag.Admin = false;
 return View("Index");
 }
 return RedirectToAction("AccessDenied", "Error");
 }
 else
 {
 return RedirectToAction("Index", "Error");
 }
}

EDIT: Adding Code

public bool CheckIfUserExists(string u_gvid)
{
 try
 {
 using (NpgsqlConnection con = new NpgsqlConnection(_Connection))
 {
 con.Open();
 using (NpgsqlCommand cmd = new NpgsqlCommand())
 {
 cmd.Connection = con;
 //qry check if user exists
 var result = cmd.ExecuteScalar().ToString().ToLower();
 if (result == "false")
 {
 return CreateUser(HelperClass.UserId, HelperClass.Name);
 }
 else
 {
 //Update lastlogonn time
 //qry
 cmd.ExecuteScalar();
 return true;
 }
 }
 }
 }
 catch (NpgsqlException ex)
 {
 //log
 RedirectToAction("DatabaseError", "Error");
 }
 catch (Exception ex)
 {
 //log
 RedirectToAction("Index", "Error");
 }
 return false;
}
public bool CheckIfUnblocked(string u_gvid)
{
 try
 {
 using (NpgsqlConnection con = new NpgsqlConnection(_Connection))
 {
 con.Open();
 using (NpgsqlCommand cmd = new NpgsqlCommand())
 {
 //qry "SELECT unblocked FROM tbl.users WHERE uid = @uid";
 var result = (bool)cmd.ExecuteScalar();
 if (result)
 return true;
 else
 return false;
 }
 }
 }
 catch (NpgsqlException ex)
 {
 //log
 RedirectToAction("DatabaseError", "Error");
 }
 catch (Exception ex)
 {
 //log
 RedirectToAction("Index", "Error");
 }
 return false;
}

• Spedo De La Rossa: @Heslacher I edited it.

Answer 1

CheckIfUserExists()

Well this method doesn't do what its name implies. You should either rename it or you only should check if the user exists. Because this method is public it should validate the method parameters.

---

If we assume you have changed the way CheckIfUserExists() works your code could look like so

public ActionResult Index()
{
 if (string.IsNullOrWhiteSpace(HelperClass.UserId)) { return RedirectToAction("AccessDenied", "Error"); }
 if (!CheckIfUserExists(HelperClass.UserId) && !CreateUser(HelperClass.UserId, HelperClass.Name))
 {
 return RedirectToAction("Index", "Error");
 }
 if (!CheckIfUnblocked(HelperClass.UserId)) { return RedirectToAction("AccessDenied", "Error"); }
 ViewBag.Admin = User.IsInRole("Admin");
 return View("Index");
}

I refactored the code by

• using guard conditions to return early which saves some indentation
• assigning the return value of a bool method directly to a bool variable

• Spedo De La Rossa: Ty, but in CheckIfUserExists() I am checking with a qry statement if an user exists, if not CreateUser() will be called?
• Heslacher: Thats why I wrote "If we assume you have changed the way CheckIfUserExists() works" which is what I suggest you should do.
• Spedo De La Rossa: Okay, so it should only check if a user exists and nothing else, ty.

Answer 2

Some quick remarks:

• Don't name things "class", e.g. HelperClass.

• Don't use Hungarian notation and give your variables meaningful names: u_gvid is a complete mystery to me.

• I don't get why CheckIfUserExists and CheckIfUnblocked are public, and even less why they'd use RedirectToAction. I expect these methods to return a boolean, not to execute redirects etc.

• NpgsqlConnection and NpgsqlCommand don't follow the Microsoft naming guidelines: they are compound words, so the "s" in "sql" should be capitalized.

• Don't write ADO.NET code. You're using 15+ lines to write something that could be expressed much simpler by using Dapper. The var result = cmd.ExecuteScalar().ToString().ToLower(); and if (result == "false") bit is even worse. You don't provide us with the relevant SQL, so perhaps there is a good reason to return a boolean-like string, but I wouldn't be surprised if the query logic could be much simplified.

• CheckIfUserExists and CheckIfUnblocked are IMHO bad method names. To me they should be something like DoesUserExist and IsUserUnblocked, which are the kind of methods I'd expect to return a boolean.

• CheckIfUserExists should not execute CreateUser, unless you rename the method. And I wouldn't expect a method called CreateUser to return a boolean. Avoid this temptation by moving these methods to a service etc. instead of them being part of the controller. Keep your controllers as light as possible, things like data processing etc. should be handled by dedicated classes.

• In CheckIfUnblocked you have a boolean: var result = (bool)cmd.ExecuteScalar();. Why then not simply return this instead of doing the whole if (result) return true; / else return false; dance? You spend six lines on something that could be expressed by a single one.

• Spedo De La Rossa: Ty for all those hints and tipps! :) Hungarian notation: So instead of userExists i should use userexists? And CheckIfUnblocked would be checkifunblocked? For me the hungarian notation (didn't know about that) is easier to read?
• BCdotWEB: @SpedoDeLaRossa This is Hungarian notation: en.wikipedia.org/wiki/Hungarian_notation Which you should not use. Please read docs.microsoft.com/en-us/dotnet/standard/design-guidelines/… and other pages in docs.microsoft.com/en-us/dotnet/standard/design-guidelines/… WRT naming standard etc.
• Spedo De La Rossa: When you don't expect those methods to redirect to anywhere (example: in the exceptions) then I would have a try catch in the Index method? So in this case, i should only redirect in the Index action?
• BCdotWEB: @SpedoDeLaRossa IMHO: yes. Otherwise you're starting to mix presentation and business logic. Also, avoid using exceptions to implement logic. There's really no reason to expect any exception in for instance the user check method. Yes, ExecuteScalar can cause an exception in your code when it returns null, but that's not an exception, but something your code should handle (and not by using a try ... catch block).