Dancer2::Manual: Problems with discussion of authentication of user against a database

[jkeenan]

Continuing to work my way through Dancer2::Manual, I came to the discussion of
authentication against a database found here:

https://metacpan.org/source/XSAWYERX/Dancer2-0.200002/lib/Dancer2/Manual.pod#L950

    post '/login' => sub {
        my $user_value = body_parameters->get('user');
        my $pass_value = body_parameters->get('pass');

        my $user = database->quick_select('users',
            { username => $user_value }
        );
        if (!$user) {
            warning "Failed login for unrecognised user $user_value";
            redirect '/login?failed=1';
        } else {
            if (Crypt::SaltedHash->validate($user->{password}, $pass_value))
            {
                debug "Password correct";
                # Logged in successfully
                session user => $user;
                redirect body_parameters->get('path') || '/';
            } else {
                debug("Login failed - password incorrect for " . $user_value);
                redirect '/login?failed=1';
            }
        }
    };

I encountered several problems trying to get this code working on my system,
where I wanted to authenticate against a Postgresql database. Certain
problems were of my own making, and I was assisted in resolving them by nudges
from nfg on #dancer. However, other problems lie in the documentation.

First, this is the only instance in which the quick_select method is
discussed or illustrated in the entire Dancer2 distribution. I was eventually
able to find that discussion here:

https://github.com/bigpresh/Dancer-Plugin-Database/blob/master/Shared/lib/Dancer/Plugin/Database/Core/Handle.pm#L102

It took a while to locate that non-standard Shared part of the
Dancer-Plugin-Database distribution. I recommend putting an explicit link to
it at this point in Dancer2::Manual.

Second, it appears that the usage of Crypt::SaltedHash is incorrect. Note
that in the above the use of this library is found in only one line:

            if (Crypt::SaltedHash->validate($user->{password}, $pass_value))

However, this library's documentation (found at
https://metacpan.org/source/GSHANK/Crypt-SaltedHash-0.09/lib/Crypt/SaltedHash.pm#L22)
suggests that a bit more code is needed.

        my $csh = Crypt::SaltedHash->new(algorithm => 'SHA-1');
        $csh->add('secret');

        my $salted = $csh->generate;
        my $valid = Crypt::SaltedHash->validate($salted, 'secret');

Note the explicit constructor, new(). I assume that 'secret' is a
password. This password is used twice, first as the argument to the add()
method, then as the second argument to the validate() method. The first
argument to validate() is explicitly created by calling the
generate() method.

When I rewrote my code to follow this documentation, I was able to establish a
connection between my Dancer2 application and the Postgresql database.

post '/login' => sub {
    my $user_value = body_parameters->get('user');
    my $user = database->quick_select('users', { username => $user_value });

    if (! $user) {
        warning "Failed login for unrecognised user $user_value";
        redirect '/login?failed=1';
    }
    else {
        my $csh = Crypt::SaltedHash->new(algorithm => 'SHA-1');
        $csh->add($user->{password});
        my $salted = $csh->generate;
        if (Crypt::SaltedHash->validate($salted, $user->{password})) {
            debug "Password correct";
            session user => $user;
            redirect body_parameters->get('path') || '/';
        }
        else {
            debug "Login failed; password incorrect for: " . $user_value;
            redirect '/login?failed=1';
        }
    }
};

My recommendation is that others try to confirm my finding about the need for
three additional Crypt::SaltedHash statements in order to have successful
authentication. Once confirmed, we should modify the code sample in
Dancer2::Manual to reflect those three additional statements.

Thank you very much.
Jim Keenan

[racke]

I would prefer to remove this example. I suggest to rather recommend Dancer2::Plugin::Auth::Extensible than let the user invent the wheel again.
This is definitely not a good idea IMHO.

[hvoers]

@jkeenan Your code does not test the given password.

[jkeenan]

@hvoers Thank you for spotting that. Does the following look correct?

post '/login' => sub {
    my $user_value = body_parameters->get('user');
    my $pass_value = body_parameters->get('pass');
    my $user = database->quick_select('users', { username => $user_value });

    if (! $user) {
        warning "Failed login for unrecognised user $user_value";
        redirect '/login?failed=1';
    }
    else {
        my $csh = Crypt::SaltedHash->new(algorithm => 'SHA-1');
        $csh->add($user->{password});
        my $salted = $csh->generate;
        if (Crypt::SaltedHash->validate($salted, $pass_value)) {
            debug "Password correct";
            session user => $user;

            redirect body_parameters->get('path') || '/';
        }
        else {
            debug "Login failed; password incorrect for: " . $user_value;
            redirect '/login?failed=1';
        }
    }
};