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;
}

\$\begingroup\$ @Heslacher I edited it. \$\endgroup\$

Spedo De La Rossa
– Spedo De La Rossa

2018-11-20 09:52:18 +00:00
Commented Nov 20, 2018 at 9:52 — Spedo De La Rossa
– Spedo De La Rossa, Commented Nov 20, 2018 at 9:52

Heslacher · Accepted Answer · 2018-11-20 10:11:35Z

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

answered Nov 20, 2018 at 10:11

Heslacher

51k5 gold badges83 silver badges177 bronze badges

\$\begingroup\$ Ty, but in CheckIfUserExists() I am checking with a qry statement if an user exists, if not CreateUser() will be called? \$\endgroup\$

Spedo De La Rossa
– Spedo De La Rossa

2018-11-20 10:57:47 +00:00
Commented Nov 20, 2018 at 10:57
1

\$\begingroup\$ Thats why I wrote "If we assume you have changed the way CheckIfUserExists() works" which is what I suggest you should do. \$\endgroup\$

Heslacher
– Heslacher

2018-11-20 11:00:12 +00:00
Commented Nov 20, 2018 at 11:00
1

\$\begingroup\$ Okay, so it should only check if a user exists and nothing else, ty. \$\endgroup\$

Spedo De La Rossa
– Spedo De La Rossa

2018-11-20 11:07:08 +00:00
Commented Nov 20, 2018 at 11:07

Add a comment |

BCdotWEB · Accepted Answer · 2018-11-20 11:22:13Z

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.

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? — Spedo De La Rossa
– Spedo De La Rossa, Commented Nov 20, 2018 at 12:15
@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. — BCdotWEB
– BCdotWEB, Commented Nov 20, 2018 at 13:05
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? — Spedo De La Rossa
– Spedo De La Rossa, Commented Nov 20, 2018 at 14:55
@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). — BCdotWEB
– BCdotWEB, Commented Nov 20, 2018 at 15:45

Stack Exchange Network

Authorization - Create and unlock a User

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Authorization - Create and unlock a User

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions