snapframework / snap

Top-level package for the official Snap Framework libraries, includes the snaplets API as well as infrastructure for sessions, auth, and templates.
http://snapframework.com/
BSD 3-Clause "New" or "Revised" License
455 stars 68 forks source link

Snap.Snaplet.Auth loginUser should not check for hardcoded "1" for the "remember me checkbox" value #96

Closed nurpax closed 10 years ago

nurpax commented 10 years ago

In reference to Snap.Snaplet.Auth.loginUser. It can be improved to have better defaults for handling the "remember me" input.

It should be fine in HTML to define a "remember me" checkbox let's say as follows:

      <div class="form-group row">
        <div class="col-md-12">
          <label>
            <input type="checkbox" id="remember" name="remember">Remember me
          </label>
        </div>
      </div>

The form data that gets POST'd for this type of a login form will be of form:

login=nurpax&password=foo&remember=on

But if you consider the implementation of loginUser (below), the "remember=on" field won't be interpreted as "checked":

loginUser' unf pwdf remf = do
    mbUsername <- lift $ getParam unf
    mbPassword <- lift $ getParam pwdf
    remember   <- lift $ liftM (fromMaybe False)
                    (runMaybeT $
                    do field <- MaybeT $ return remf
                       value <- MaybeT $ getParam field
                       return $ value == "1")

Snap only accepts a value "1" for checked. This means the HTML template must explicitly set a value="1" field into the input, like so:

      <div class="form-group row">
        <div class="col-md-12">
          <label>
            <input type="checkbox" id="remember" name="remember" value="1">Remember me
          </label>
        </div>
      </div>

Perhaps it'd be better to just check for the existence of the "remember" input instead of requiring its value to be '1'? Some SO posts seem to suggest this is the way to go, e.g. http://stackoverflow.com/questions/4554758/how-to-read-if-a-checkbox-is-checked-in-php.

As the browser sends "on" for checked checkbox inputs, I'd argue that checking for "on" would be better than "1" but I don't have good references for backing this up.

gregorycollins commented 10 years ago

We should look for "1" or "on" here.