HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Customized Template Login

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
customizedtemplatelogin

Problem

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.

  • 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.



  • 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.



  • 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:

  • Is this the correct approach to add these options?



  • Are there any glaring errors with this code, aside from the fact I can probably refactor it?



  • Any glaring security issues I missed?



```
_
_
_
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

Solution

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).

Code Snippets

Dim username = GetUserNamebyEmail(model.UserName)
Dim localID = GetUserIDbyEmail(model.UserName)
Dim localID = GetUserIDbyUserName(model.UserName)

Context

StackExchange Code Review Q#45308, answer score: 3

Revisions (0)

No revisions yet.