Validating users with Roles

Question

I am using the following to validate my user which works but i wish to incoperate roles so I used the reg iis to add the standard tables aspnet_UsersInRoles.

What I basically want is the ability to have variables canView canDelete canEdit and that I can just access them when my dal is called.

protected void btnLogin_Click1(object sender, EventArgs e)
{
 Users _loginUser = _dal.VerifyPassword(txtUserName.Text, txtPassword.Text);
 if (failedLogin == false)
 lblerror.Text = "Invalid Login";
 else
 {
 UserData userData = new UserData
 {
 FirstName = _loginUser
 };
 FormsAuthenticationTicket ticket = new FormsAuthenticationTicket(
 1, // ticket version
 _loginUser.UserName, // authenticated username
 DateTime.Now, // issueDate
 DateTime.Now.AddMinutes(30), // expiryDate
 isPersistent, // true to persist across browser sessions
 userData, // can be used to store additional user data
 FormsAuthentication.FormsCookiePath); // the path for the cookie
 // Encrypt the ticket using the machine key
 string encryptedTicket = FormsAuthentication.Encrypt(ticket);
 // Add the cookie to the request to save it
 HttpCookie cookie = new HttpCookie(FormsAuthentication.FormsCookieName, encryptedTicket);
 cookie.HttpOnly = true;
 Response.Cookies.Add(cookie);
 lblerror.Text = "Success!";
 Response.Redirect("default.aspx");
 }
}

My Question is how would i adapt the following routine to inlcude the roles aspnet_UsersInRoles table to check if has administrative privellages Of a guid type of ED85788D-72DA-4D0A-8D5E-B5378FC00592.

public Users VerifyPassword(string userName, string password)
{
 //The ".FirstOrDefault()" method will return either the first matched
 //result or null
 Users myUser = SoccerEntities.Users
 .FirstOrDefault(u => u.UserName == userName
 && u.password == password);
 if (myUser == null) //User was not found
 {
 //Proceed with your login process...
 return myUser;
 }
 else //User was found
 {
 if (isPlayerdb(userName))
 isPlayer = true;
 if (isAdmindb(userName))
 isAdmin = true;
 return myUser; 
 //Do something to let them know that their credentials were not valid
 }
}

Cause at min im doing this setting the isAdmin variable on a other function isAdmindb so thats three hits to db which is not very effiecent

protected bool isAdmindb(string userName)
{
 try{
 Users adminUsers = (from users in SoccerEntities.Users
 where users.roleId == new Guid("ED85788D-72DA-4D0A-8D5E-B5378FC00592") && users.UserName == userName
 select users).FirstOrDefault();
 if (adminUsers != null)
 return true;
 else
 return false;
 }
 catch (Exception ex)
 {
 throw new EntityContextException("isAdmin failed.", ex);
 }
}

Edit a had a typo in my return !!!

• 200_success: VerifyPassword() always returns null? What's the point of that?
• dotnetdevcsharp: i had a typo sorry updated with correct code
• TheCoffeeCup: Hi! Welcome to Code Review. Good job on your first post!

Answer 1

public Users VerifyPassword(string userName, string password)
{
 //The ".FirstOrDefault()" method will return either the first matched
 //result or null
 Users myUser = SoccerEntities.Users
 .FirstOrDefault(u => u.UserName == userName
 && u.password == password);
 if (myUser == null) //User was not found
 {
 //Proceed with your login process...
 return myUser;
 }
 else //User was found
 {
 if (isPlayerdb(userName))
 isPlayer = true;
 if (isAdmindb(userName))
 isAdmin = true;
 return myUser; 
 //Do something to let them know that their credentials were not valid
 }
} 

The comments in this method don't serve any purpose an don't add any value to the code, so get rid of them. Comments should describe why something is done but shouldn't state the obvious.

A class named Users implies to hold a collections of User but in your case it seems to be singular so you should rename this class to User.

Not using braces {} will lead to error prone code. You should use them always although they might be optional.

The else part is redundant because if myUser == null it won't be reached, you should get rid of the else.

If for instance isAdmin had been true before this call it will stay that way which is bad. Just assign the returned value of isAdmindb to the variable. The same goes for isPlayer.

Applying this points except for Users vs User

public Users VerifyPassword(string userName, string password)
{
 Users user = SoccerEntities.Users
 .FirstOrDefault(u => u.UserName == userName
 && u.password == password);
 if (user == null) { return null; }
 isPlayer = isPlayerdb(userName);
 isAdmin = isAdmindb(userName);
 return user; 
} 

looks much cleaner, doesn't it ? But hold on, we can do better.

If we take a look at isAdmindb() we notice at first, that the method name doesn't match the NET naming guidelines regarding the used case of the name. Method names should be named using PascalCase casing.

The next thing is that a user is considered an admin if its roleId matches a specific Guid. So let us take this Guid and use it in the VerifyPassword() method. But wait, this doesn't seem right because the responsibility of that method should be only to verify that the used username and password are correct. So there shouldn't be anything related like in the former method with the isAdmin stuff.

So let us change the isAdmindb() method to take a Users opject as a method argument and compare the user's roleId with the said Guid which we extract to a class member constant.

private const Guid adminRoleId = new Guid("ED85788D-72DA-4D0A-8D5E-B5378FC00592");
protected bool IsAdmin(Users user)
{
 return user.roleId == adminRoleId ;
} 

now thats short and readable and doesn't need to hit the db.

Now we should take the VerifyPassword() and change it to GetUser() and remove all this stuff about the isAdmin and isPlayer leaving only this

public Users GetUser(string userName, string password)
{
 return SoccerEntities.Users
 .FirstOrDefault(u => u.UserName == userName
 && u.password == password);
} 

Now let us add a FormsAuthenticationTicket GetAuthenticationTicket(Users) method which will do some of the work which had formerly been done by the eventhandler of that button.

private FormsAuthenticationTicket GetAuthenticationTicket(Users user)
{
 return new FormsAuthenticationTicket(
 version: 1, 
 name: user.UserName, 
 issueDate: DateTime.Now, 
 expiration: DateTime.Now.AddMinutes(30), 
 isPersistent: isPersistent, 
 userData: user.ToString(), 
 cookiePath: FormsAuthentication.FormsCookiePath);
} 

as you see by using named arguments there is no need for any comment anymore.

Now the former eventhandler code will look like

protected void btnLogin_Click1(object sender, EventArgs e)
{
 Users user = GetUser(txtUserName.Text, txtPassword.Text);
 if (user == null)
 {
 lblerror.Text = "Invalid Login";
 return;
 }
 FormsAuthenticationTicket ticket = GetAuthenticationTicket(user);
 isAdmin = IsAdmin(user);
 isPlayer = IsPlayer(user); // should be implemented by you in a similiar way
 string encryptedTicket = FormsAuthentication.Encrypt(ticket);
 HttpCookie cookie = new HttpCookie(FormsAuthentication.FormsCookieName, encryptedTicket);
 cookie.HttpOnly = true;
 Response.Cookies.Add(cookie);
 lblerror.Text = "Success!";
 Response.Redirect("default.aspx");
}

Answer 2

Naming

Users adminUsers = (from users in SoccerEntities.Users
 where users.roleId == new Guid("ED85788D-72DA-4D0A-8D5E-B5378FC00592") && users.UserName == userName
 select users).FirstOrDefault();

It's not clear why the type is called Users or why the variable is called adminUsers when both the type and the variable appear to refer to only a single instance. Consider User and adminUser instead.

Var

Use var when the type of the local variable is obvious by its assignment.

UserData userData = new UserData
 {
 FirstName = _loginUser
 };

Should be

 var userData = new UserData
 {
 FirstName = _loginUser
 };

Design

You have a lot of logic in what looks to be view code behind. Consider a framework such as MVVM to separate your view and domain logic.

I am concerned that your DAL has an isPlayer field on it. What does this do? It sounds like it would be better to place this in your Users object.

isAdminDB seems like a useless function to me. Why go back to the database to get a field that should be on your object?

Instead, what's wrong with this?

public Users VerifyPassword(string userName, string password)
{
 //The ".FirstOrDefault()" method will return either the first matched
 //result or null
 Users myUser = SoccerEntities.Users
 .FirstOrDefault(u => u.UserName == userName
 && u.password == password);
 if (myUser == null) //User was not found
 {
 //Proceed with your login process...
 return myUser;
 }
 else //User was found
 {
 if (isPlayerdb(userName))
 isPlayer = true;
 if(myUser.roleId == "ED85788D-72DA-4D0A-8D5E-B5378FC00592")
 {
 isAdmin = true;
 }
 return myUser; 
 //Do something to let them know that their credentials were not valid
 }
}

Presumably you can do the same with your isPlayerdb function.

Also, you have a massive magic string with your GUID. Make it a const, give it a name. No maintenance developer is going to know what "ED85788D-72DA-4D0A-8D5E-B5378FC00592" means off the top of their head.

Lastly, your use of GUIDs is concerning to me. I'm not convinced this is the best way to represent the role for a user. Instead, could you give the user a reference to the object from the role table? If you brought this over at the same time you wouldn't even need to check. Just go:

if(myUser.Role.Type == Admin)

or something like that.

• dotnetdevcsharp: how do i had constants in c# though is that not just a vb thing
• dotnetdevcsharp: well then how would you suggest i add the type into the role table then
• Nick Udell: You very much can use constants in C#. Check out msdn.microsoft.com/en-us/library/…
• Nick Udell: As for how to add the "type" into the role table, I recommend adding a field to your role table called "type" with either "admin" or "player" entered as values.