Login script check

Question

I have written a script that sits on the admin portion on my website.

Here I assume the user is valid as I have code that checks that already.

The below code is checks if the user is Admin. If they are Admin they will be flagged with a "Y" on the database (this will be a "1" for optimization later but for sanity's sake with testing Y was easier).

App Code:

Public Function IsUserAdmin(ByVal iUserID As Long) As Boolean
 Dim sConnString As String = System.Web.Configuration.WebConfigurationManager.ConnectionStrings("mySQL").ConnectionString
 Dim dsNames As SqlDataSource
 Dim bReturn As Boolean = False
 dsNames = New SqlDataSource
 dsNames.ConnectionString = sConnString
 Dim sSQL As String
 sSQL = "SELECT IsAdmin FROM [SystemUsers] WHERE ID=@UserID"
 dsNames.SelectCommand = sSQL
 dsNames.SelectParameters.Clear()
 dsNames.SelectParameters.Add("UserID", iUserID)
 For Each datarow As Data.DataRowView In dsNames.Select(DataSourceSelectArguments.Empty) ‘ do I need a loop?
 If datarow("IsAdmin").ToString().ToUpper = "Y" Then
 bReturn = True
 End If
 Next
 Return bReturn
dsNames.dispose
 End Function

.Net Code

‘Assuming basic login was okay we have a UserObject/UserID
Dim vAdmin as string 
vAdmin = IsUserAdmin(Session("UserObject"))
 If vAdmin = True Then
 'Valid User
 Else
 Response.Redirect("../Default.aspx")
 End If

• ChrisW: If you want to edit the code, put your edited code after (not replacing) the existing code; otherwise that invalidates existing answers: see Can I edit my own question to include suggested changes from answers? for further details.
• ChrisW: Also it's not sSQL that should be disposed (strings don't have a Dispose method): it's dsNames.
• indofraiser: @ChrisW That's me having a blonde moment! Thanks

Answer 1

I see you're not using the role manager built into .NET (together with a built-in membership provider). If you were, then this could be codeless and configured in the Web.config.

For example, the Web.config of my Logs directory (which contains log files) look like this:

<?xml version="1.0"?>
<configuration>
 <system.web>
 <authorization>
 <allow roles="Supervisor"/>
 <deny users="*"/>
 </authorization>
 </system.web>
 <system.webServer>
 <directoryBrowse enabled="true"/>
 </system.webServer>
</configuration>

Second, ideally you should call the Dispose method of your SqlDataSource when you finish using it.

• indofraiser: I'll have to have a good read up! Will this also work for hiding sections on a webpage. There are places we want to only show certain data to a user.
• ChrisW: For hiding data within a web page I would use the System.Web.Security.Roles.Role class in the code-behind, for example the Role.IsUserInRole(string roleName) method, to show/hide sections of the page.
• indofraiser: That link and guidance looks really good. I'll have to pen in a day and really get into the meat of it.
• ChrisW: A (perhaps small) benefit of the Web.config is that it applies to all files, even for example text files which cannot include code to check the user's permissions and to redirect if the user is not permitted.
• indofraiser: I suspect I will use Web.Config to control "Front End Users" against "Admin" and then once into those sections use the Security.Roles from different pages where I need to limit further. i.e. "Admin" login will be lowest level login to that part of the site.

Answer 2

Naming

Per Microsoft, Hungarian notation is to be avoided. It was developed for untyped and weakly typed languages. vb.net is a strongly typed language. The IDE will tell you what type a variable is. Don't use Hungarian notation.

Bugs

 For Each datarow As Data.DataRowView In dsNames.Select(DataSourceSelectArguments.Empty) ‘ do I need a loop?
 If datarow("IsAdmin").ToString().ToUpper = "Y" Then
 bReturn = True
 End If
 Next
 Return bReturn

What happens if (God forbid) there are two records for a single user id and one of them is an Admin and the other isn't? So long as the last record IsAdmin you're fine, but if it's not, you will say that this user is not an Admin. Confusion ensues.

This can be fixed by returning as soon as you find a record that IsAdmin.

 For Each datarow As Data.DataRowView In names.Select(DataSourceSelectArguments.Empty) ‘ do I need a loop?
 If datarow("IsAdmin").ToString().ToUpper = "Y" Then
 Return True
 End If
 Next
 Return False

Note that the bReturn variable is no longer needed.

Also note that this always stops execution prior to disposing of names. You should use a Try...Finally block and dispose of names in the Finally part. (This is true of both the code I just presented and your original.)

Inconsistencies

I like that you declare and set the connection string all at once.

Dim sConnString As String = System.Web.Configuration.WebConfigurationManager.ConnectionStrings("mySQL").ConnectionString

But you don't do the same for sSQL.

Dim sSQL As String
sSQL = "SELECT IsAdmin FROM [SystemUsers] WHERE ID=@UserID"

While we're at it, we should take note that sSQL does not (and should not) be changed at runtime. Thus, it should be declared as a constant.

Const sql As String = "SELECT IsAdmin FROM [SystemUsers] WHERE ID=@UserID"