0

Ok so this is driving me nuts. I am probably tired and the answer is looking at me.

 public ActionResult _Login(LoginViewModel loginViewModel)
    {
        if (User.Identity.IsAuthenticated)
        {
            return JavaScript("window.location=" + "'" + loginViewModel.ReturntUrl + "'");
        }
        if (ModelState.IsValid)
        {
            if (Session["loginCount"] == null) //setup the session var with 0 count
            {
                Session.Add("loginCount", 0);
            }
            _loginStatus =  _authenticationService.Authenticate(loginViewModel.SiteLoginViewModel.EmailAddress,
                                                loginViewModel.SiteLoginViewModel.Password);
            if(!_loginStatus.UserExists)
            {
                ModelState.AddModelError("SiteLoginViewModel.EmailAddress", _loginStatus.ErrorMessage);
                return PartialView();
            }
            // This will only be true if the user types in the correct password
            if(!_loginStatus.IsAuthenticated)
            {
                Session["loginCount"] = (int)Session["loginCount"] + 1;
                Response.Write(Session["loginCount"]); // Counter is incremented twice!!!!


                //_userService.SetInvalidLoginAttempts(loginViewModel.SiteLoginViewModel.EmailAddress, 1);


                ModelState.AddModelError("SiteLoginViewModel.EmailAddress", _loginStatus.ErrorMessage);
                return PartialView();
            }

            // DELETE ANY OPENID Cookies
            var openidCookie = new HttpCookie("openid_provider");

            if (openidCookie.Value != null)
            {
                openidCookie.Expires = DateTime.Now.AddDays(-1d);
                Response.Cookies.Add(openidCookie);
            }
            _userService.SetInvalidLoginAttempts(loginViewModel.SiteLoginViewModel.EmailAddress, 0);
            SetAuthTicket(loginViewModel.SiteLoginViewModel.EmailAddress, _userService.GetUserId(loginViewModel.SiteLoginViewModel.EmailAddress),
                                  loginViewModel.SiteLoginViewModel.RemeberLogin);


            if (!string.IsNullOrEmpty(loginViewModel.ReturntUrl))
            {
                return JavaScript("window.location=" + "'" + loginViewModel.ReturntUrl + "'");
            }
            return JavaScript("location.reload(true)");

        }
        return PartialView();
    }

This almost seems that the request is being processed twice however when i step through with the debugger I only see it once. Please ignore the non important parts of the ActionMethod

CrazyCoderz
  • 1,351
  • 1
  • 15
  • 30
  • I am trying to save a trip to the databse. if the counter gets incremented to say 5 times, I lock the user account out. One trip to the DB rather than one for every invalid attempt. – CrazyCoderz Aug 28 '11 at 16:50
  • Ok I think I may have found the issue. I found http://stackoverflow.com/questions/4062568/form-submitted-twice-after-updating-from-asp-mvc-3-preview-to-beta Sure enough the damn script was included in the layout as well as the login page.... – CrazyCoderz Aug 28 '11 at 16:59

2 Answers2

0

This looks like you are tying to code for stuff that you automatically get with .Net's Membership provider.

Your first line "User.Identity.IsAuthenticated" looks like you are using part of membership provider but it would seem the rest is trying to code around it.

Also, why are you returning javascript to direct the user's browser to a new URL? Regarless of what .net platform you are on there are plenty of ways to redirect the user's browser without having to return raw javascript, which in my book is REALLY BAD.

SLoret
  • 1,531
  • 3
  • 18
  • 31
  • I have written my own auth system for several reasons. First of all the builtin provider models do not play well with Dependency Injection. Secondly I don't want Microsoft dictating to me how my database should be designed. As far as the redirect that is the correct way to do so considering the fact that a response.redirect and such don't work on AJAX calls. – CrazyCoderz Aug 28 '11 at 17:02
  • Your answer has nothing to do with the original post -1 – CrazyCoderz Aug 28 '11 at 17:05
  • I just had to go through the "Membership Provider" delemma. Check out this question and eventual answer I came too. http://stackoverflow.com/questions/7197337/using-asp-net-membership-provider-with-an-existing-user-database In the end, using a "Custom Membership provider" was the right answer for me so that I could aviod writting login rules like what you have above. – SLoret Aug 28 '11 at 17:07
  • The .Net Membership Provider is terrible on many levels. As I previously stated It does not play well with IOC/DI. I also wanted control of the encryption and the ability to properly salt my hashses. I don't like rainbow attacks. – CrazyCoderz Aug 28 '11 at 17:10
  • "User.Identity.IsAuthenticated" is a FormsAuth thing by the way. – CrazyCoderz Aug 28 '11 at 17:15
  • I haven't researched the IOC/DI part but a custom membership provider gives you complete control over encryption and properly salting. It's "custom" you can write whatever you want in the ValidateUser or ChangePassword routines. If you have them, I'd love to see any links that also feel the custom provider is "terrible" as that is the path I have chosen. – SLoret Aug 28 '11 at 17:16
  • You don't have to look to hard: http://mattbriggs.net/posts/asp-net-membership-sucks – CrazyCoderz Aug 28 '11 at 17:31
  • If you would like my authentication system I will give it to you when it is finished. – CrazyCoderz Aug 28 '11 at 17:32
  • Thanks for the article. Appreciate the offer and will take you up on it. FYI - I posted the following question on programmers.stackexchange since it is subjective. It will be interesting to see the responses. http://programmers.stackexchange.com/questions/104268/what-are-the-disadvantages-of-using-writting-a-net-custom-membership-provider – SLoret Aug 28 '11 at 17:46
  • Cool. I am finishing up the Login Status enums and hope to get unit test done in the next few days. – CrazyCoderz Aug 28 '11 at 19:54
  • @CrazyCoderz - not wanting to use a database design because you don't want to be told what to do does not justify reinventing the wheel. – JeffO Aug 29 '11 at 01:11
  • Sure it is. I would love to see the "lazy coders" using the built in .Net membership provider pass a real security audit. WOW.. Read the article at the link I posted. The .Net membership provider is a total piece of trash. – CrazyCoderz Aug 29 '11 at 13:56
0

@@ This fixed the problem and will be removed rather than commented out. Including this twice is very bad obviously :)

CrazyCoderz
  • 1,351
  • 1
  • 15
  • 30