From f0ab7713cc29e128dc27ec93025f4c26a74bb7fc Mon Sep 17 00:00:00 2001 From: ngoomie Date: Mon, 8 May 2023 04:26:11 -0600 Subject: [PATCH] Finish improving Auth.pm control structure; fix other minor mistakes both `register_do` and `login_do` in `CharmBoard::Controller::Auth` now use two try/catch blocks instead of heavily nested if/elsif/else blocks. I also reverted the change where I removed all of the `use experimental 'feature-name'` declarations because, while Mojolicious still whines about experimental features being experimental, if you don't have `use experimental 'try'` set somewhere, then it just does not work, unlike smartmatch which will work either way but cause perl to get mad at you. plus, it feels weird not having those there anyways! --- database.sql | 24 ++--- lib/CharmBoard.pm | 1 + lib/CharmBoard/Controller/Auth.pm | 120 ++++++++++++++---------- lib/CharmBoard/Schema/Result/Session.pm | 10 +- script/CharmBoard | 2 +- 5 files changed, 90 insertions(+), 67 deletions(-) diff --git a/database.sql b/database.sql index 545ffe4..a1e3720 100644 --- a/database.sql +++ b/database.sql @@ -1,5 +1,5 @@ -- --- File generated with SQLiteStudio v3.4.4 on Sun. May 7 22:15:23 2023 +-- File generated with SQLiteStudio v3.4.4 on Mon. May 8 03:12:22 2023 -- -- Text encoding used: UTF-8 -- @@ -9,7 +9,7 @@ BEGIN TRANSACTION; -- Table: categories DROP TABLE IF EXISTS categories; -CREATE TABLE IF NOT EXISTS categories ( +CREATE TABLE categories ( cat_id INTEGER NOT NULL ON CONFLICT ROLLBACK UNIQUE ON CONFLICT ROLLBACK, cat_name TEXT, @@ -22,7 +22,7 @@ CREATE TABLE IF NOT EXISTS categories ( -- Table: posts DROP TABLE IF EXISTS posts; -CREATE TABLE IF NOT EXISTS posts ( +CREATE TABLE posts ( post_id INTEGER NOT NULL ON CONFLICT ROLLBACK UNIQUE ON CONFLICT ROLLBACK, user_id INTEGER NOT NULL ON CONFLICT ROLLBACK, @@ -45,12 +45,12 @@ CREATE TABLE IF NOT EXISTS posts ( -- Table: sessions DROP TABLE IF EXISTS sessions; -CREATE TABLE IF NOT EXISTS sessions ( - user_id INTEGER PRIMARY KEY - REFERENCES users (user_id) - NOT NULL, +CREATE TABLE sessions ( session_key TEXT NOT NULL - UNIQUE, + UNIQUE + PRIMARY KEY, + user_id INTEGER REFERENCES users (user_id) + NOT NULL, session_expiry NUMERIC NOT NULL, is_ip_bound INTEGER (1, 1) NOT NULL DEFAULT (0), @@ -61,7 +61,7 @@ CREATE TABLE IF NOT EXISTS sessions ( -- Table: subforums DROP TABLE IF EXISTS subforums; -CREATE TABLE IF NOT EXISTS subforums ( +CREATE TABLE subforums ( subf_id INTEGER PRIMARY KEY UNIQUE ON CONFLICT ROLLBACK NOT NULL ON CONFLICT ROLLBACK, @@ -76,7 +76,7 @@ CREATE TABLE IF NOT EXISTS subforums ( -- Table: threads DROP TABLE IF EXISTS threads; -CREATE TABLE IF NOT EXISTS threads ( +CREATE TABLE threads ( thread_id INTEGER NOT NULL ON CONFLICT ROLLBACK, thread_title TEXT NOT NULL ON CONFLICT ROLLBACK, thread_subf INTEGER REFERENCES categories (cat_id), @@ -89,7 +89,7 @@ CREATE TABLE IF NOT EXISTS threads ( -- Table: users DROP TABLE IF EXISTS users; -CREATE TABLE IF NOT EXISTS users ( +CREATE TABLE users ( user_id INTEGER NOT NULL ON CONFLICT ROLLBACK UNIQUE ON CONFLICT ROLLBACK, username TEXT NOT NULL ON CONFLICT ROLLBACK @@ -98,7 +98,7 @@ CREATE TABLE IF NOT EXISTS users ( NOT NULL ON CONFLICT ROLLBACK, password TEXT NOT NULL ON CONFLICT ROLLBACK, salt TEXT NOT NULL ON CONFLICT ROLLBACK, - signup_date REAL NOT NULL, + signup_date INTEGER NOT NULL, PRIMARY KEY ( user_id AUTOINCREMENT ) diff --git a/lib/CharmBoard.pm b/lib/CharmBoard.pm index 47698fc..f6c438b 100644 --- a/lib/CharmBoard.pm +++ b/lib/CharmBoard.pm @@ -1,4 +1,5 @@ package CharmBoard; +use experimental 'try', 'smartmatch'; use Mojo::Base 'Mojolicious', -signatures; use CharmBoard::Schema; diff --git a/lib/CharmBoard/Controller/Auth.pm b/lib/CharmBoard/Controller/Auth.pm index dd8a244..ee2eefa 100644 --- a/lib/CharmBoard/Controller/Auth.pm +++ b/lib/CharmBoard/Controller/Auth.pm @@ -1,4 +1,5 @@ package CharmBoard::Controller::Auth; +use experimental 'try', 'smartmatch'; use Mojo::Base 'Mojolicious::Controller', -signatures; use CharmBoard::Crypt::Password; use CharmBoard::Crypt::Seasoning; @@ -8,52 +9,53 @@ sub register ($app) { $app->render( template => 'register', error => $app->flash('error'), - message => $app->flash('message') - )}; + message => $app->flash('message'))}; # process submitted registration form -# TODO: implement email validation here at some point sub register_do ($app) { my $username = $app->param('username'); my $email = $app->param('email'); my $password = $app->param('password'); my $confirmPassword = $app->param('confirm-password'); - # check to make sure all required fields are filled - if (! $username || ! $password || ! $confirmPassword) { - $app->flash(error => 'All fields required.'); - $app->redirect_to('register')}; + my $catchError; - # check to make sure both passwords match - # TODO: add check on frontend for this for people with JS enabled - if ($confirmPassword ne $password) { - $app->flash(error => 'Passwords do not match.'); - $app->redirect_to('register')}; + # declare vars used through multiple try/catch blocks with + # 'our' so they work throughout the entire subroutine + our ($userCheck, $emailCheck, $salt, $hash); - # check to make sure username and/or email isn't already in use; - # if not, continue with registration - ## search for input username and email in database - my $userCheck = $app->schema->resultset('Users')->search({username => $username})->single; - my $emailCheck = $app->schema->resultset('Users')->search({email => $email})->single; + try { # make sure registration info is valid + # TODO: implement email validation here at some point - if ($userCheck || $emailCheck) { - if ($userCheck && $emailCheck) { # notify user that username and email are both already being used - $app->flash(error => 'Username and email already in use.'); - $app->redirect_to('register')} + # check to make sure all required fields are filled + ($username, $email, $password, $confirmPassword) ne undef + or die "Please fill out all required fields."; - elsif ($userCheck) { # notify user that only username is already in use - $app->flash(error => 'Username is already in use.'); - $app->redirect_to('register')} + # check to make sure both passwords match + # TODO: add check on frontend for this for people with JS enabled + $password eq $confirmPassword + or die "Passwords do not match"; - elsif ($emailCheck) { # notify user that only email is already in use - $app->flash(error => 'email is already in use.'); - $app->redirect_to('register')}} + # check to make sure username and/or email isn't already in use; + # if not, continue with registration + ## search for input username and email in database + $userCheck = $app->schema->resultset('Users')->search({username => $username})->single; + $emailCheck = $app->schema->resultset('Users')->search({email => $email})->single; - else { # TODO: add more error handling here, in case SQL transact fails - # append pepper to pass before hashing + ($userCheck && $emailCheck) eq undef + or die "Username already in use.\nemail already in use."; + ($userCheck) eq undef + or die "Username already in use."; + ($emailCheck) eq undef + or die "email already in use."} + catch ($catchError) { + $app->flash(error => $catchError); + $app->redirect_to('register');} + + try { $password = $app->pepper . ':' . $password; # return hashed result + salt - my ($salt, $hash) = passgen($password); + ($salt, $hash) = passgen($password) or die; # add user info and pw/salt to DB $app->schema->resultset('Users')->create({ @@ -61,9 +63,15 @@ sub register_do ($app) { email => $email, password => $hash, salt => $salt, - signup_date => time }); + signup_date => time }) or die; $app->flash(message => 'User registered successfully!'); + $app->redirect_to('register')} + catch ($catchError) { + print $catchError; + $app->flash(error => 'Your registration info was correct, + but a server error prevented you from registering. + This has been logged so your administrator can fix it.'); $app->redirect_to('register')}}; sub login ($app) { @@ -76,42 +84,56 @@ sub login_do ($app) { my $username = $app->param('username'); my $password = $app->pepper . ':' . $app->param('password'); - try { + my $catchError; + + # declare vars used through multiple try/catch blocks with + # 'our' so they work throughout the entire subroutine + our ($userInfo, $passCheck, $userID, $sessionKey); + + try { # check user credentials first # check to see if user by entered username exists - my $userInfo = $app->schema->resultset('Users')->search({username => $username}); + $userInfo = $app->schema->resultset('Users')->search({username => $username}); $userInfo or die; # now check password validity - my $passCheck = passchk($userInfo->get_column('salt')->first, + $passCheck = passchk($userInfo->get_column('salt')->first, $userInfo->get_column('password')->first, $password); - $passCheck or die; + $passCheck or die;} + catch ($catchError) { # redirect to login page on fail + print $catchError; + $app->flash(error => 'Username or password incorrect.'); + $app->redirect_to('login');} + + try { # now attempt to create session # get user ID for session creation - my $userID = $userInfo->get_column('user_id')->first; + $userID = $userInfo->get_column('user_id')->first; - # gen session key and set expiry time - my $sessionKey = seasoning(16); - my $sessionExpiry = time + 604800; + # gen session key + $sessionKey = seasoning(16); # add session to database $app->schema->resultset('Session')->create({ - user_id => $userID, session_key => $sessionKey, - session_expiry => $sessionExpiry, + user_id => $userID, + session_expiry => time + 604800, is_ip_bound => 0, - bound_ip => undef }); + bound_ip => undef }) or die; # now create session cookie for user - $app->session(is_auth => 1); - $app->session(user_id => $userID); + $app->session(is_auth => 1); + $app->session(user_id => $userID); $app->session(session_key => $sessionKey); - $app->session(expires => $sessionExpiry); + $app->session(expiration => 604800); # redirect to index upon success $app->redirect_to('/')} - catch ($error) { # redir to login page on fail - print $error; - $app->flash(error => 'Username or password incorrect.'); - $app->redirect_to('login')}}; + + catch ($catchError) { # redirect to login page on fail + print $catchError; + $app->flash(error => 'Your username and password were correct, + but a server error prevented you from logging in. + This has been logged so your administrator can fix it.'); + $app->redirect_to('login')}} 1; \ No newline at end of file diff --git a/lib/CharmBoard/Schema/Result/Session.pm b/lib/CharmBoard/Schema/Result/Session.pm index 6c08a17..ef743ca 100644 --- a/lib/CharmBoard/Schema/Result/Session.pm +++ b/lib/CharmBoard/Schema/Result/Session.pm @@ -3,14 +3,14 @@ use base qw(DBIx::Class::Core); __PACKAGE__->table('sessions'); __PACKAGE__->add_columns( - user_id => { - data_type => 'integer', - is_auto_increment => 0, - is_nullable => 0, }, session_key => { data_type => 'text', is_auto_increment => 0, is_nullable => 0, }, + user_id => { + data_type => 'integer', + is_auto_increment => 0, + is_nullable => 0, }, session_expiry => { data_type => 'numeric', is_auto_increment => 0, @@ -24,7 +24,7 @@ __PACKAGE__->add_columns( is_auto_increment => 0, is_nullable => 1, }); -__PACKAGE__->set_primary_key('user_id'); +__PACKAGE__->set_primary_key('session_key'); __PACKAGE__->belongs_to( user_id => diff --git a/script/CharmBoard b/script/CharmBoard index b786670..1d01019 100755 --- a/script/CharmBoard +++ b/script/CharmBoard @@ -1,5 +1,5 @@ #!/usr/bin/env perl - +use experimental 'try', 'smartmatch'; use strict; use warnings; use utf8;