7
\$\begingroup\$

While the MVC4 template provided by Microsoft is useful, I feel there are a few scenarios that should be covered to help out users trying to log in.

  1. Allow the user to log in with their email address instead of their user name (they can still choose to use their user name). The former is generally easier to remember.
  2. If they don't have a local account and try to use one, check if they have previously used an external provider (such as google) to log in and let them know to use it instead.
  3. If they have registered an account locally but have not yet confirmed their email, let them know. The current template just warns that the username or password is wrong.

Maybe I'm over-thinking it, but I want to provide the user every opportunity to successfully log in and use the site. My questions:

  1. Is this the correct approach to add these options?
  2. Are there any glaring errors with this code, aside from the fact I can probably refactor it?
  3. Any glaring security issues I missed?
<HttpPost()> _
<AllowAnonymous()> _
<ValidateAntiForgeryToken()> _
Public Function Login(ByVal model As LoginModel, ByVal returnUrl As String) As ActionResult
    If ModelState.IsValid Then
        If IsEmail(model.UserName) Then
            'the username is an email address
            Dim username = GetUserNamebyEmail(model.UserName)
            If username IsNot Nothing Then
                If WebSecurity.Login(username, model.Password, persistCookie:=model.RememberMe) Then
                    Return RedirectToLocal(returnUrl)
                End If
                'check if there is a local account
                Dim localID = GetUserIDbyEmail(model.UserName)
                If localID Is Nothing Then
                    'no local account means the username is wrong
                    ModelState.AddModelError("", "The user name or password provided is incorrect.")
                Else
                    If Not OAuthWebSecurity.HasLocalAccount(localID) Then
                        'registered via external provider
                        ModelState.AddModelError("", "Please login with the External Provider you have previously used.")
                    Else
                        If Not WebSecurity.IsConfirmed(model.UserName) Then
                            'has a local account, but hasn't confirmed email
                            ModelState.AddModelError("", "You have not yet confirmed your email.")
                        Else
                            'password is wrong
                            ModelState.AddModelError("", "The user name or password provided is incorrect.")
                        End If
                    End If
                End If
            Else
                ModelState.AddModelError("", "The email you entered is incorrect.")
            End If
        Else
            'must be the regular user name, so log in as normal
            If WebSecurity.Login(model.UserName, model.Password, persistCookie:=model.RememberMe) Then
                Return RedirectToLocal(returnUrl)
            End If
            'check if there is a local account
            Dim localID = GetUserIDbyUserName(model.UserName)
            If localID Is Nothing Then
                'no local account means the username is wrong
                ModelState.AddModelError("", "The user name or password provided is incorrect.")
            Else
                If Not OAuthWebSecurity.HasLocalAccount(localID) Then
                    'registered via external provider
                    ModelState.AddModelError("", "Please login with the External Provider you have previously used.")
                Else
                    If Not WebSecurity.IsConfirmed(model.UserName) Then
                        'has a local account, but hasn't confirmed email
                        ModelState.AddModelError("", "You have not yet confirmed your email.")
                    Else
                        'password is wrong
                        ModelState.AddModelError("", "The user name or password provided is incorrect.")
                    End If
                End If
            End If
        End If
    End If
    Return View(model)
End Function

'check if input is an email address
Public Function IsEmail(ByVal input As String) As Boolean
    Return Regex.IsMatch(input, "\A(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)\Z")
End Function

Public Function GetUserNamebyEmail(ByVal email As String) As String
    Dim username As String = Nothing
    Dim conn As SqlConnection = New SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings("DefaultConnection").ConnectionString)
    Dim cmd As SqlCommand = New SqlCommand("select username from user_info where Email = @Email", conn)
    cmd.Parameters.Add(New SqlParameter("@Email", System.Data.SqlDbType.NVarChar))
    cmd.Parameters("@Email").Value = email
    conn.Open()
    Dim reader As SqlDataReader = cmd.ExecuteReader()
    Try
        While reader.Read
            username = reader(0)
        End While
    Finally
        reader.Close()
    End Try
    conn.Close()
    Return username
End Function

Public Function GetUserIDbyEmail(ByVal email As String) As Integer?
    Dim userID As Integer?
    Dim conn As SqlConnection = New SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings("DefaultConnection").ConnectionString)
    Dim cmd As SqlCommand = New SqlCommand("select UserID from user_info where Email = @Email", conn)
    cmd.Parameters.Add(New SqlParameter("@Email", System.Data.SqlDbType.NVarChar))
    cmd.Parameters("@Email").Value = email
    conn.Open()
    Dim reader As SqlDataReader = cmd.ExecuteReader()
    Try
        While reader.Read
            userID = reader(0)
        End While
    Finally
        reader.Close()
    End Try
    conn.Close()
    Return userID
End Function

Public Function GetUserIDbyUserName(ByVal username As String) As Integer?
    Dim userID As Integer?
    Dim conn As SqlConnection = New SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings("DefaultConnection").ConnectionString)
    Dim cmd As SqlCommand = New SqlCommand("select UserID from user_info where UserName = @username", conn)
    cmd.Parameters.Add(New SqlParameter("@username", System.Data.SqlDbType.NVarChar))
    cmd.Parameters("@username").Value = username
    conn.Open()
    Dim reader As SqlDataReader = cmd.ExecuteReader()
    Try
        While reader.Read
            userID = reader(0)
        End While
    Finally
        reader.Close()
    End Try
    conn.Close()
    Return userID
End Function
\$\endgroup\$
3
\$\begingroup\$

Just a quick comment (my background is mostly C# & VB6 - I never really played with VB.NET): you're hitting the database too often. When you hit it here:

Dim username = GetUserNamebyEmail(model.UserName)

(isn't that declaration missing a type? Is it Object or String?)

...Instead of returning a String, I'd be returning a User, so this call would be moot:

Dim localID = GetUserIDbyEmail(model.UserName)

(isn't that declaration missing a type? Is it Object or String?)

And then in the Else block you're making another one:

Dim localID = GetUserIDbyUserName(model.UserName)

(again)

I think I'd try to restructure the code so as to be able to return a User object regardless of whether model.UserName is an email address or not (i.e. move the IsEmail check into the logic that fetches the User).

\$\endgroup\$
  • 1
    \$\begingroup\$ You can use implicit declaration in VB.Net. If you look at the function 'GetUserNamebyEmail' it returns a string of the username, which is then used in 'Websecurity.Login'. As for hitting the DB - at most it gets hit twice (user uses email to login, but then login fails), at best it never gets hit when user uses their correct username and password to login. I will consider grabbing both username and email for the first scenario (IsEmail = true), or moving that call into the logic that grabs it. Thanks. And thanks for the EDIT. I didn't realize C# tag screws up the formatting. \$\endgroup\$ – merlot Mar 25 '14 at 14:37
  • \$\begingroup\$ @merlot interesting.. so it's like C#'s var then. I'll get back to this post when I have a chance, I want to take a deeper look at your code ;) \$\endgroup\$ – Mathieu Guindon Mar 25 '14 at 14:40

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.