HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Login page code enhancement

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codeenhancementloginpage

Problem

I am starting to learn ASP.NET, and I am designing a page for an admin to login and do some stuff. I have the username and password for the admin stored in Web.config (Is that a good practice?).

Second thing, I am checking if the user filled the username and password fields, then I am checking if the values for the admin and password exist in the web.config, then I am checking if these values equal each other, so I am having 3 nested if. Is that a good practice as well?

Here's my code:

if (!string.IsNullOrEmpty(txtLoginId.Text) && !string.IsNullOrEmpty(txtPassword.Text))
            {
                string configLoginID = WebConfigurationManager.AppSettings["AdminLoginID"];
                string configPassword = WebConfigurationManager.AppSettings["AdminPassword"];
                if (!string.IsNullOrEmpty(configLoginID) && !string.IsNullOrEmpty(configPassword))
                {
                    if (txtLoginId.Text == configLoginID && txtPassword.Text == configPassword)
                    {
                        Session["ShoppingAdmin"] = "ShoppingAdmin";
                        Response.Redirect("~/Admin/AddNewProducts.aspx");
                    }
                    else
                    {
                        lblAlert.Text = "Incorrect Username/Password";
                    }

                }
            }
            else
            {
                lblAlert.Text = "Please enter a usernamd and password";
            }


Last thing, checking empty fields using !string.IsNullOrEmpty for each field, is that the right thing to do?

I would like to write a clean code, thanks for helping me with these 3 cases.

Solution

I have the username and password for the admin stored in Web.config (Is that a good practice?).

No, it's not good practice to store the Admin login (or any login, for that matter) in the Web.config.

Depending on what ASP.NET model you're using, you have a few options. MVC has a built-in Roles provider which makes it simple to make certain pages/areas only accessible by a specific role.

Web Forms is also pretty simple, though you have to roll your own access provisions. You can use the built-in Simple Membership if you like, which usually suits quite well once extended to fit your needs. (You can add properties to users pretty easily and such.)

You also aren't encrypting or hashing the password, which is a huge no-no. At the very least you should MD5 hash it, though for security I would recommend SHA1-salt hashing at the very weakest. Though, if you use the built-in Simple Membership for Web Forms or MVC, you'll get encryption and salting by default.

I didn't review the actual code as Heslacher did that just fine, but I do think you need to completely revisit the login system. ASP.NET has built-in providers for this, and they're super easy and, quite honestly, pretty fun to set up. (I don't know why no one has pointed this out in the several months this question has been here, or how it got on my radar, but hopefully you still get this advice.)

Context

StackExchange Code Review Q#127444, answer score: 2

Revisions (0)

No revisions yet.