4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – ChrisW Feb 18 '14 at 11:25
  • 1
    \$\begingroup\$ Also it's not sSQL that should be disposed (strings don't have a Dispose method): it's dsNames. \$\endgroup\$ – ChrisW Feb 18 '14 at 11:26
  • \$\begingroup\$ @ChrisW That's me having a blonde moment! Thanks \$\endgroup\$ – indofraiser Feb 18 '14 at 11:29
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – indofraiser Feb 18 '14 at 11:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – ChrisW Feb 18 '14 at 11:44
  • \$\begingroup\$ That link and guidance looks really good. I'll have to pen in a day and really get into the meat of it. \$\endgroup\$ – indofraiser Feb 18 '14 at 11:50
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – ChrisW Feb 18 '14 at 12:00
  • \$\begingroup\$ 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. \$\endgroup\$ – indofraiser Feb 18 '14 at 12:02
2
\$\begingroup\$

Naming

Per Microsoft, Hungarian notation is to be avoided. It was developed for untyped and weakly typed languages. 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"
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.