POP QUIZ: What's wrong with this code - part 1

Here is another trivia question.  Comment with your answer and I will post the comments Friday along with the answer.

What is wrong with this code

Using Forms Authentication, here is Login.aspx.cs (updated the code slightly, assume the AuthenticateRequest function will check the user input to make sure it is valid entries):

 using System;
using System.Data;
using System.Configuration;
using System.Collections;
using System.Web;
using System.Web.Security;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Web.UI.WebControls.WebParts;
using System.Web.UI.HtmlControls;

public partial class Login : System.Web.UI.Page
{
    protected void Page_Load(object sender, EventArgs e)
    {

    }

    void Logon_Click(object sender, EventArgs e)
    {
        string username = UserNameTextBox.Text;
        string password = UserPassTextBox.Text;

        if (AuthenticateRequest(username, password))
        {

            FormsAuthenticationTicket ticket = new FormsAuthenticationTicket(1,
              username,
              DateTime.Now,
              DateTime.Now.AddMinutes(30),
              isPersistent,
              userData,
              FormsAuthentication.FormsCookiePath);

            // Encrypt the ticket.
            string encTicket = FormsAuthentication.Encrypt(ticket);

            // Create the cookie.
            Response.Cookies.Add(new 
              HttpCookie(FormsAuthentication.FormsCookieName, 
              encTicket));

            FormsAuthentication.RedirectFromLoginPage
               (username, false);
        }
        else
        {
            throw new System.Exception(
                 "Error authenticating the user");
        }
    }

    bool AuthenticateRequest(string username, string password)
    {
        // Do authentication here
    }
}

So what is wrong here?

Comments

• Anonymous
  March 05, 2008
  You've been kicked (a good thing) - Trackback from DotNetKicks.com

• Anonymous
  March 05, 2008
  do you mean aside from not sanitizing the user input??

• Anonymous
  March 05, 2008
  isPersistent & userData are not declared? And why is this call: FormsAuthentication.RedirectFromLoginPage               (UserEmail.Text, Persist.Checked); using UserEmail.Text as username, when UserNameTextBox.Text was considered the username on the call to RedirectFromLoginPage()?

• Anonymous
  March 05, 2008
  Throwing a new Exception() is silly if the authentication fails. A misuse of Exceptions. That's the very first thing I notice. Second is FormsAuthentication.RedirectFromLoginPage               (UserEmail.Text, Persist.Checked). In this case, UserEmail is not validated. Third thing would be DateTime.Now.AddMinutes(30). That 30 should be a value retrieved from a web.config. At the very least, it should be a const.

• Anonymous
  March 05, 2008
  My guess is that the call to FormsAuthentication.RedirectFromLoginPage overwrites the encrypted cookie with a non-encrypted cookie.

• Anonymous
  March 05, 2008
  Well, upon authentication failure throwing out an exception is a bad idea - especially if it's unhandled. It will crash and the worker process will be restarted.

• Anonymous
  March 05, 2008
  I'll play. I'm assuming you're looking for code than coding guidelines (lack of method comments, private in front of the methods, etc) assuming that there's a page with a button and the event is attached and assuming that the end user is accepting cookies, and assuming that you're not looking for cross-site problems (the page is using SSL or some other means to validate that the person who pressed the 'login' button is receiving the cookie) and that there's easier ways of creating the ticket I'm going to go with two issues. 1) The code is setting a cookie and then redirecting away. The browser might not be nice and set the cookie on the 302 2) RedirectFromLoginPage sets its own cookie if CookiesSupported=true.

• Anonymous
  March 05, 2008
  Issue the authentication cookie two times...

• Anonymous
  March 05, 2008
  Issue the authentication cookie two times...

• Anonymous
  March 05, 2008
  The password is in clear text?

• Anonymous
  March 07, 2008
  Hey, the code was changed! As you can see from my previous comment, the code in one spot used to be: FormsAuthentication.RedirectFromLoginPage(UserEmail.Text, Persist.Checked); Now it says: FormsAuthentication.RedirectFromLoginPage(username, false); So what gives?  You change the code from a "QUIZ" without mentioning it was changed? Weak.

• Anonymous
  March 07, 2008
  Sorry, posted too quickly. Oh, now I see the update notice.  Very subtle.  I'd suggest bolding or italicizing when you change blog post content so it stands out.

• Anonymous
  March 07, 2008
  yeah, sorry about that.  I will be more careful before I post it next time.

• Anonymous
  March 10, 2008
  Apart from the other things, I would like to add that this Class name might create problems in production. CS0030: Cannot convert type 'ASP.login_aspx' to 'System.Web.UI.WebControls.Login' http://blogs.msdn.com/rahulso/archive/2006/07/03/cs0030-cannot-convert-type-asp-login-aspx-to-system-web-ui-webcontrols-login.aspx Cheers Rahul

• Anonymous
  March 11, 2008
  Very good point Rahul, depending on what version of .NET you are using, this could cause problems.