Simple login user authentication

Question

I've created a simple User Login page. Here User is authenticated [username, password] and if it is a valid user, values like username, name and role of the user is stored in the Session.

And according to the role of the user, the user is redirected to different pages. This is simple code, but please help me to make my login page more secure and add more functionality.

using System;
using System.Data;
namespace PROJECT.view.User
{
 public partial class LoginUser : System.Web.UI.Page
 {
 protected void Page_Load(object sender, EventArgs e)
 {
 try
 {
 Session.Clear();
 Session.Abandon();
 }
 catch (Exception)
 {
 throw;
 }
 }
 protected void btnSignIn_Click(object sender, EventArgs e)
 {
 try
 {
 DataTable dt = new DataTable();
 dt = new C_User().Get_LoginUser(inputUserName.Value, inputPwd.Value);
 if(dt != null && dt.Rows.Count > 0)
 { 
 DataRow dr = dt.Rows[0];
 Session["username"] = dr["USERNAME"].ToString();
 Session["name"] = dr["NAME"].ToString();
 Session["role"] = dr["ROLE"].ToString();
 #region affiliated user pages
 Int16 userRole = 0;
 userRole = Convert.ToInt16(Session["role"]);
 switch (userRole)
 {
 case 1:
 Response.Redirect("Registration.aspx", false);
 break;
 case 2:
 Response.Redirect("Medicine_1.aspx", false);
 break;
 case 3:
 Response.Redirect("PortalRegister.aspx", false);
 break;
 case 4:
 Response.Redirect("Billing.aspx", false);
 break;
 }
 #endregion
 }
 else
 {
 Response.Redirect("LoginUser.aspx", false);
 return;
 }
 }
 catch (Exception)
 {
 throw;
 }
 }
 }
}

• Guillaume Beauvois: inputPwd.Value seems to mean you store the password in plain text. Don't do that. dt.Rows.Count > 0 is a wrong test. It makes me feel like you can have 2 or more users with the same userName and password. Test should be dt.Rows.Count == 1

Answer 1

Let's review from top to bottom.

Page_Load()

• No need to have a try..catch here if the code only throw inside the catch. Omitting the try..catch will lead to the same result and removes one level of indentation.

btnSignIn_Click()

• Like in Page_Load() there is no need to have that try..catch either.

• Classes should be named using PascalCase casing (see: .NET Naming Guidelines), hint: C_User. The same applies to naming of methods. hint: Get_LoginUser(()

• By reversing the condition of if(dt != null && dt.Rows.Count > 0) you can return early hence there won't be the need of the else part which saves you another level of indentation like so

  if(dt == null || dt.Rows.Count == 0)
  { 
   Response.Redirect("LoginUser.aspx", false);
   return;
  }
  DataRow dr = dt.Rows[0];
  Session["username"] = dr["USERNAME"].ToString();
  ....
  

• As regions may be acceptable inside a class they are phroned upon inside a method. Having a region inside a method is usually a clear sign that this method is doing to much.

• Why wasting vertical space by first declaring an int and setting it to 0 and on the next line of code another value is assigned to it. Just do

  Int16 userRole = Convert.ToInt16(Session["role"]);
  

• The switch could be improved by replacing the magic numbers with meaningful constants.

Answer 2

Making your website/application truly secure is a complex matter! If you're using username, password and roles I'd suggest to use the Out of the Box ASP.Net feature. You can always read it here for ASP.Net Web Forms.

I'd follow the tips from Heslacher and other than that I'd recommend using a Dictionary<int, string> to make it a bit more readable instead of the Switch:

using System;
using System.Data;
namespace PROJECT.view.User
{
 public partial class LoginUser : System.Web.UI.Page
 {
 private readonly Dictionary<int, string> _redirects = new Dictionary<int, string>
 {
 {1, "Registration.aspx"},
 {2, "Medicine_1.aspx"},
 {3, "PortalRegister.aspx"},
 {4, "Billing.aspx"}
 };
 // All the other code
 // Then instead of your switch in btnSignIn_Click(..) you could do:
 #region affiliated user pages
 Response.Redirect(_redirects[Convert.ToInt16(Session["role"])], false);
 #endregion
 // All the other code
 }
}

• user4221591: Is this a method or just declaring Dictionary? Please clarify the code. Thank You!
• 321X: It's just a declaration in the class. I'll expand the example