diff --git a/.scrutinizer.yml b/.scrutinizer.yml new file mode 100644 index 000000000..2bbc9279c --- /dev/null +++ b/.scrutinizer.yml @@ -0,0 +1,5 @@ +# This file just tells the wonderful code quality analyzer Scrutinizer (https://scrutinizer-ci.com/g/panique/huge/) +# that we are using external services (Travis) to generate code coverage stats +# TODO is this correct ? +tools: + external_code_coverage: true \ No newline at end of file diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 000000000..45ab71539 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,30 @@ +language: php + +php: + - 5.5 + - 5.6 + - hhvm + +before_install: +- sudo apt-get update > /dev/null + +before_script: + - sudo apt-get install apache2 + - sudo a2enmod rewrite + # configure apache virtual hosts, create vhost via travis-ci-apache file template + - sudo cp -f travis-ci-apache /etc/apache2/sites-available/default + - sudo sed -e "s?%TRAVIS_BUILD_DIR%?$(pwd)?g" --in-place /etc/apache2/sites-available/default + - sudo service apache2 restart + # composer + - composer self-update + - composer install --prefer-source --no-interaction --dev + # go to tests folder + - cd tests + +# run unit tests, create result file +script: phpunit --configuration phpunit.xml --coverage-text --coverage-clover=coverage.clover + +# gets tools from Scrutinizer, uploads unit tests results to Scrutinizer (?) +after_script: + - wget https://scrutinizer-ci.com/ocular.phar + - php ocular.phar code-coverage:upload --format=php-clover coverage.clover \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f7eb6b36..1451f4eed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,25 @@ # CHANGE LOG +## 3.1 (currently in develop branch) + +Code Quality at Scrutinizer 9.6 / 10, at Code Climate 3.9 / 10 + +**February 2015** + +- [panique] lots of code refactorings and simplifications all over the project +- [PR](https://github.com/panique/huge/pull/615) [Dominic28] Avatar can now be deleted by the user +- [panique] First Unit tests :) +- [panique] several code quality improvements all over the project +- [panique] avatarModel code improvements +- [panique] renamed AccountType stuff to UserRole, minor changes + ## 3.0 +Code Quality at Scrutinizer 9.3 / 10, at Code Climate 3.9 / 10 + **February 2015** -- [panique] AccountTypeModel reduced to one method (removed duplicate code) +- [panique] removed duplicate code in AccountTypeModel - [PR](https://github.com/panique/huge/pull/587) [upperwood] Facebook stuff completely removed from SQL - [panique] tiny text changes diff --git a/README.md b/README.md index f36e63824..97830e69e 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,11 @@ # HUGE +[![Scrutinizer Code Quality](https://scrutinizer-ci.com/g/panique/huge/badges/quality-score.png?b=develop)](https://scrutinizer-ci.com/g/panique/huge/?branch=develop) +[![Code Climate](https://codeclimate.com/github/panique/huge/badges/gpa.svg)](https://codeclimate.com/github/panique/huge) +[![Travis CI](https://travis-ci.org/panique/huge.svg?branch=develop)](https://travis-ci.org/panique/huge) +[![Dependency Status](https://www.versioneye.com/user/projects/54ca11fbde7924f81a000010/badge.svg?style=flat)](https://www.versioneye.com/user/projects/54ca11fbde7924f81a000010) + Just a simple user authentication solution inside a super-simple framework skeleton that works out-of-the-box (and comes with an auto-installer), using the future-proof official bcrypt password hashing/salting implementation of PHP 5.5+, plus some nice features that will speed up the time from idea to first usable prototype application @@ -16,11 +21,7 @@ applications that - surprisingly and intentionally - go back to the basics of pr static classes, extremely simple constructs, not-totally-DRY code etc. while keeping the code extremely readable ([StackOverflow](http://www.dev-metal.com/architecture-stackoverflow/), Wikipedia, SoundCloud). -Buzzwords: [KISS](http://en.wikipedia.org/wiki/KISS_principle), [YASNI](http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it). - -[![Scrutinizer Code Quality](https://scrutinizer-ci.com/g/panique/huge/badges/quality-score.png?b=master)](https://scrutinizer-ci.com/g/panique/huge/?branch=master) -[![Code Climate](https://codeclimate.com/github/panique/huge/badges/gpa.svg)](https://codeclimate.com/github/panique/huge) -[![Dependency Status](https://www.versioneye.com/user/projects/54ca11fbde7924f81a000010/badge.svg?style=flat)](https://www.versioneye.com/user/projects/54ca11fbde7924f81a000010) +Buzzwords: [KISS](http://en.wikipedia.org/wiki/KISS_principle), [YASNI](http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it). #### Quick-Index @@ -256,6 +257,23 @@ Then check your server's IP / domain. Everything should work fine. By default HUGE has a demo-user: username is `demo`, password is `12345678`. The user is already activated. +### What the hell are .travis.yml, .scrutinizer.yml etc. ? + +There are several files in the root folder of the project that might be irritating: + + - *.htaccess* (optionally) routes all traffic to /public/index.php! If you installed this project correctly, then this + file is not necessary, but as lots of people have problems setting up the vhost correctly, .htaccess it still there + to increase security, even on partly-broken-installations. + - *.scrutinizer.yml* (can be deleted): Configs for the external code quality analyzer Scrutinizer, just used here on + GitHub, you don't need this for your project. + - *.travis.yml* (can be deleted): Same like above. Travis is an external service that creates installations of this + repo after each code change to make sure everything runs fine. Also runs the unit tests. You don't need this inside + your project. + - *composer.json* (important): You should know what this does. ;) This file says what external dependencies are used. + - *travis-ci-apache* (can be deleted): Config file for Travis, see above, so Travis knows how to setup the Apache. + +*README* and *CHANGELOG* are self-explaining. + #### Documentation A real documentation is in the making. Until then, please have a look at the code and use your IDE's code completion diff --git a/application/config/texts.php b/application/config/texts.php index cf7471e95..6fd1c7c56 100644 --- a/application/config/texts.php +++ b/application/config/texts.php @@ -23,7 +23,7 @@ "FEEDBACK_USER_EMAIL_ALREADY_TAKEN" => "Sorry, that email is already in use. Please choose another one.", "FEEDBACK_USERNAME_CHANGE_SUCCESSFUL" => "Your username has been changed successfully.", "FEEDBACK_USERNAME_AND_PASSWORD_FIELD_EMPTY" => "Username and password fields were empty.", - "FEEDBACK_USERNAME_DOES_NOT_FIT_PATTERN" => "Username does not fit the name scheme: only a-Z and numbers are allowed, 2 to 64 characters.", + "FEEDBACK_USERNAME_DOES_NOT_FIT_PATTERN" => "Username does not fit the name pattern: only a-Z and numbers are allowed, 2 to 64 characters.", "FEEDBACK_EMAIL_DOES_NOT_FIT_PATTERN" => "Sorry, your chosen email does not fit into the email naming pattern.", "FEEDBACK_EMAIL_SAME_AS_OLD_ONE" => "Sorry, that email address is the same as your current one. Please choose another one.", "FEEDBACK_EMAIL_CHANGE_SUCCESSFUL" => "Your email address has been changed successfully.", @@ -31,7 +31,6 @@ "FEEDBACK_PASSWORD_REPEAT_WRONG" => "Password and password repeat are not the same.", "FEEDBACK_PASSWORD_TOO_SHORT" => "Password has a minimum length of 6 characters.", "FEEDBACK_USERNAME_TOO_SHORT_OR_TOO_LONG" => "Username cannot be shorter than 2 or longer than 64 characters.", - "FEEDBACK_EMAIL_TOO_LONG" => "Email cannot be longer than 64 characters.", "FEEDBACK_ACCOUNT_SUCCESSFULLY_CREATED" => "Your account has been created successfully and we have sent you an email. Please click the VERIFICATION LINK within that mail.", "FEEDBACK_VERIFICATION_MAIL_SENDING_FAILED" => "Sorry, we could not send you an verification mail. Your account has NOT been created.", "FEEDBACK_ACCOUNT_CREATION_FAILED" => "Sorry, your registration failed. Please go back and try again.", @@ -45,6 +44,9 @@ "FEEDBACK_AVATAR_UPLOAD_TOO_BIG" => "Avatar source file is too big. 5 Megabyte is the maximum.", "FEEDBACK_AVATAR_FOLDER_DOES_NOT_EXIST_OR_NOT_WRITABLE" => "Avatar folder does not exist or is not writable. Please change this via chmod 775 or 777.", "FEEDBACK_AVATAR_IMAGE_UPLOAD_FAILED" => "Something went wrong with the image upload.", + "FEEDBACK_AVATAR_IMAGE_DELETE_SUCCESSFUL" => "You successfully deleted your avatar.", + "FEEDBACK_AVATAR_IMAGE_DELETE_NO_FILE" => "You don't have a custom avatar.", + "FEEDBACK_AVATAR_IMAGE_DELETE_FAILED" => "Something went wrong while deleting your avatar.", "FEEDBACK_PASSWORD_RESET_TOKEN_FAIL" => "Could not write token to database.", "FEEDBACK_PASSWORD_RESET_TOKEN_MISSING" => "No password reset token.", "FEEDBACK_PASSWORD_RESET_MAIL_SENDING_ERROR" => "Password reset mail could not be sent due to: ", @@ -54,10 +56,7 @@ "FEEDBACK_PASSWORD_RESET_LINK_VALID" => "Password reset validation link is valid. Please change the password now.", "FEEDBACK_PASSWORD_CHANGE_SUCCESSFUL" => "Password successfully changed.", "FEEDBACK_PASSWORD_CHANGE_FAILED" => "Sorry, your password changing failed.", - "FEEDBACK_ACCOUNT_UPGRADE_SUCCESSFUL" => "Account upgrade was successful.", - "FEEDBACK_ACCOUNT_UPGRADE_FAILED" => "Account upgrade failed.", - "FEEDBACK_ACCOUNT_DOWNGRADE_SUCCESSFUL" => "Account downgrade was successful.", - "FEEDBACK_ACCOUNT_DOWNGRADE_FAILED" => "Account downgrade failed.", + "FEEDBACK_ACCOUNT_TYPE_CHANGE_SUCCESSFUL" => "Account type change successful", "FEEDBACK_ACCOUNT_TYPE_CHANGE_FAILED" => "Account type change failed", "FEEDBACK_NOTE_CREATION_FAILED" => "Note creation failed.", "FEEDBACK_NOTE_EDITING_FAILED" => "Note editing failed.", diff --git a/application/controller/LoginController.php b/application/controller/LoginController.php index bd798716c..65aa7aff7 100644 --- a/application/controller/LoginController.php +++ b/application/controller/LoginController.php @@ -135,13 +135,13 @@ public function editUserEmail_action() } /** - * Upload avatar + * Edit avatar * Auth::checkAuthentication() makes sure that only logged in users can use this action and see this page */ - public function uploadAvatar() + public function editAvatar() { Auth::checkAuthentication(); - $this->View->render('login/uploadAvatar', array( + $this->View->render('login/editAvatar', array( 'avatar_file_path' => AvatarModel::getPublicUserAvatarFilePathByUserId(Session::get('user_id')) )); } @@ -155,17 +155,28 @@ public function uploadAvatar_action() { Auth::checkAuthentication(); AvatarModel::createAvatar(); - Redirect::to('login/uploadAvatar'); + Redirect::to('login/editAvatar'); + } + + /** + * Delete the current user's avatar + * Auth::checkAuthentication() makes sure that only logged in users can use this action and see this page + */ + public function deleteAvatar_action() + { + Auth::checkAuthentication(); + AvatarModel::deleteAvatar(Session::get("user_id")); + Redirect::to('login/editAvatar'); } /** * Show the change-account-type page * Auth::checkAuthentication() makes sure that only logged in users can use this action and see this page */ - public function changeAccountType() + public function changeUserRole() { Auth::checkAuthentication(); - $this->View->render('login/changeAccountType'); + $this->View->render('login/changeUserRole'); } /** @@ -173,20 +184,21 @@ public function changeAccountType() * Auth::checkAuthentication() makes sure that only logged in users can use this action * POST-request */ - public function changeAccountType_action() + public function changeUserRole_action() { Auth::checkAuthentication(); if (Request::post('user_account_upgrade')) { // "2" is quick & dirty account type 2, something like "premium user" maybe. you got the idea :) - AccountTypeModel::changeAccountType(2); + UserRoleModel::changeUserRole(2); } + if (Request::post('user_account_downgrade')) { // "1" is quick & dirty account type 1, something like "basic user" maybe. - AccountTypeModel::changeAccountType(1); + UserRoleModel::changeUserRole(1); } - Redirect::to('login/changeAccountType'); + Redirect::to('login/changeUserRole'); } /** diff --git a/application/core/Application.php b/application/core/Application.php index 468270dc6..7f87e15ee 100644 --- a/application/core/Application.php +++ b/application/core/Application.php @@ -26,18 +26,8 @@ public function __construct() // create array with URL parts in $url $this->splitUrl(); - // check for controller: no controller given ? then make controller = default controller (from config) - if (!$this->controller_name) { - $this->controller_name = Config::get('DEFAULT_CONTROLLER'); - } - - // check for action: no action given ? then make action = default action (from config) - if (!$this->action_name OR (strlen($this->action_name) == 0)) { - $this->action_name = Config::get('DEFAULT_ACTION'); - } - - // rename controller name to real controller class/file name ("index" to "IndexController") - $this->controller_name = ucwords($this->controller_name) . 'Controller'; + // creates controller and action names (from URL input) + $this->createControllerAndActionNames(); // does such a controller exist ? if (file_exists(Config::get('PATH_CONTROLLER') . $this->controller_name . '.php')) { @@ -87,4 +77,24 @@ private function splitUrl() $this->parameters = array_values($url); } } + + /** + * Checks if controller and action names are given. If not, default values are put into the properties. + * Also renames controller to usable name. + */ + private function createControllerAndActionNames() + { + // check for controller: no controller given ? then make controller = default controller (from config) + if (!$this->controller_name) { + $this->controller_name = Config::get('DEFAULT_CONTROLLER'); + } + + // check for action: no action given ? then make action = default action (from config) + if (!$this->action_name OR (strlen($this->action_name) == 0)) { + $this->action_name = Config::get('DEFAULT_ACTION'); + } + + // rename controller name to real controller class/file name ("index" to "IndexController") + $this->controller_name = ucwords($this->controller_name) . 'Controller'; + } } diff --git a/application/core/Config.php b/application/core/Config.php index a3ce036b7..96ace4bed 100644 --- a/application/core/Config.php +++ b/application/core/Config.php @@ -2,15 +2,22 @@ class Config { - private static $config; + // this is public to allow better Unit Testing + public static $config; public static function get($key) { if (!self::$config) { - self::$config = require('../application/config/config.' . Environment::get() . '.php'); + + $config_file = '../application/config/config.' . Environment::get() . '.php'; + + if (!file_exists($config_file)) { + return false; + } + + self::$config = require $config_file; } return self::$config[$key]; } - } diff --git a/application/core/Mail.php b/application/core/Mail.php index 3b5360d58..2362b0d9c 100644 --- a/application/core/Mail.php +++ b/application/core/Mail.php @@ -67,10 +67,7 @@ public function sendMailWithPHPMailer($user_email, $from_email, $from_name, $sub $mail->Username = Config::get('EMAIL_SMTP_USERNAME'); $mail->Password = Config::get('EMAIL_SMTP_PASSWORD'); $mail->Port = Config::get('EMAIL_SMTP_PORT'); - } - - // if you want to send mail via PHPMailer using native mail() - if (!Config::get('EMAIL_USE_SMTP')) { + } else { $mail->IsMail(); } @@ -81,15 +78,16 @@ public function sendMailWithPHPMailer($user_email, $from_email, $from_name, $sub $mail->Subject = $subject; $mail->Body = $body; - // send mail + // try to send mail $mail->Send(); + if ($mail) { return true; + } else { + // if not successful, copy errors into Mail's error property + $this->error = $mail->ErrorInfo; + return false; } - - // if not successful, copy errors into Mail's error property - $this->error = $mail->ErrorInfo; - return false; } public function sendMail($user_email, $from_email, $from_name, $subject, $body) diff --git a/application/core/Text.php b/application/core/Text.php index cbbc5daab..80ac3e5ea 100644 --- a/application/core/Text.php +++ b/application/core/Text.php @@ -6,10 +6,21 @@ class Text public static function get($key) { + // if not $key + if (!$key) { + return null; + } + + // load config file (this is only done once per application lifecycle) if (!self::$texts) { self::$texts = require('../application/config/texts.php'); } + // check if array key exists + if (!array_key_exists($key, self::$texts)) { + return null; + } + return self::$texts[$key]; } diff --git a/application/model/AvatarModel.php b/application/model/AvatarModel.php index 7c55dc63f..09a90910c 100644 --- a/application/model/AvatarModel.php +++ b/application/model/AvatarModel.php @@ -61,23 +61,36 @@ public static function getPublicUserAvatarFilePathByUserId($user_id) /** * Create an avatar picture (and checks all necessary things too) - * TODO decoupling - * TODO total rebuild, this is too quick & dirty + * TODO decouple + * TODO total rebuild + */ + public static function createAvatar() + { + // check avatar folder writing rights, check if upload fits all rules + if (AvatarModel::isAvatarFolderWritable() AND AvatarModel::validateImageFile()) { + + // create a jpg file in the avatar folder, write marker to database + $target_file_path = Config::get('PATH_AVATARS') . Session::get('user_id'); + AvatarModel::resizeAvatarImage($_FILES['avatar_file']['tmp_name'], $target_file_path, Config::get('AVATAR_SIZE'), Config::get('AVATAR_SIZE'), Config::get('AVATAR_JPEG_QUALITY')); + AvatarModel::writeAvatarToDatabase(Session::get('user_id')); + Session::set('user_avatar_file', AvatarModel::getPublicUserAvatarFilePathByUserId(Session::get('user_id'))); + Session::add('feedback_positive', Text::get('FEEDBACK_AVATAR_UPLOAD_SUCCESSFUL')); + } + } + + /** + * Checks if the avatar folder exists and is writable * * @return bool success status */ - public static function createAvatar() + public static function isAvatarFolderWritable() { - // check if upload fits all rules - AvatarModel::validateImageFile(); - - // create a jpg file in the avatar folder, write marker to database - $target_file_path = Config::get('PATH_AVATARS') . Session::get('user_id'); - AvatarModel::resizeAvatarImage($_FILES['avatar_file']['tmp_name'], $target_file_path, Config::get('AVATAR_SIZE'), Config::get('AVATAR_SIZE'), Config::get('AVATAR_JPEG_QUALITY')); - AvatarModel::writeAvatarToDatabase(Session::get('user_id')); - Session::set('user_avatar_file', AvatarModel::getPublicUserAvatarFilePathByUserId(Session::get('user_id'))); - Session::add('feedback_positive', Text::get('FEEDBACK_AVATAR_UPLOAD_SUCCESSFUL')); - return true; + if (is_dir(Config::get('PATH_AVATARS')) AND is_writable(Config::get('PATH_AVATARS'))) { + return true; + } + + Session::add('feedback_negative', Text::get('FEEDBACK_AVATAR_FOLDER_DOES_NOT_EXIST_OR_NOT_WRITABLE')); + return false; } /** @@ -88,12 +101,7 @@ public static function createAvatar() */ public static function validateImageFile() { - if (!is_dir(Config::get('PATH_AVATARS')) OR !is_writable(Config::get('PATH_AVATARS'))) { - Session::add('feedback_negative', Text::get('FEEDBACK_AVATAR_FOLDER_DOES_NOT_EXIST_OR_NOT_WRITABLE')); - return false; - } - - if (!isset($_FILES['avatar_file']) OR empty ($_FILES['avatar_file']['tmp_name'])) { + if (!isset($_FILES['avatar_file'])) { Session::add('feedback_negative', Text::get('FEEDBACK_AVATAR_IMAGE_UPLOAD_FAILED')); return false; } @@ -113,10 +121,12 @@ public static function validateImageFile() return false; } - if (!($image_proportions['mime'] == 'image/jpeg' || $image_proportions['mime'] == 'image/png')) { + if (!($image_proportions['mime'] == 'image/jpeg')) { Session::add('feedback_negative', Text::get('FEEDBACK_AVATAR_UPLOAD_WRONG_TYPE')); return false; } + + return true; } /** @@ -143,7 +153,7 @@ public static function writeAvatarToDatabase($user_id) * @param int $final_height The desired height of the new image. * @param int $quality The quality of the JPG to produce 1 - 100 * - * TODO currently we just allow .jpg / .png + * TODO currently we just allow .jpg * * @return bool success state */ @@ -186,4 +196,49 @@ public static function resizeAvatarImage($source_image, $destination, $final_wid // default return return false; } + + /** + * Delete a user's avatar + * + * @param int $userId + * + * @return bool success + */ + public static function deleteAvatar($userId) + { + if (!ctype_digit($userId)) { + Session::add("feedback_negative", Text::get("FEEDBACK_AVATAR_IMAGE_DELETE_FAILED")); + return false; + } + + // Check if file exists + if (!file_exists(Config::get('PATH_AVATARS').$userId.".jpg")) { + Session::add("feedback_negative", Text::get("FEEDBACK_AVATAR_IMAGE_DELETE_NO_FILE")); + return false; + } + + // Delete avatar file + if (!unlink(Config::get('PATH_AVATARS').$userId.".jpg")) { + Session::add("feedback_negative", Text::get("FEEDBACK_AVATAR_IMAGE_DELETE_FAILED")); + return false; + } + + $database = DatabaseFactory::getFactory()->getConnection(); + + // Set user_has_avatar back to 0 + $sth = $database->prepare("UPDATE users SET user_has_avatar = 0 WHERE user_id = :user_id LIMIT 1"); + $sth->bindValue(":user_id", (int)$userId, PDO::PARAM_INT); + $sth->execute(); + + if ($sth->rowCount() == 1) { + // Reset avatar file path session + Session::set('user_avatar_file', self::getPublicUserAvatarFilePathByUserId($userId)); + + Session::add("feedback_positive", Text::get("FEEDBACK_AVATAR_IMAGE_DELETE_SUCCESSFUL")); + return true; + } else { + Session::add("feedback_negative", Text::get("FEEDBACK_AVATAR_IMAGE_DELETE_FAILED")); + return false; + } + } } diff --git a/application/model/LoginModel.php b/application/model/LoginModel.php index a1b3a0c77..814a362b7 100644 --- a/application/model/LoginModel.php +++ b/application/model/LoginModel.php @@ -24,52 +24,28 @@ public static function login($user_name, $user_password, $set_remember_me_cookie return false; } - // get all data of that user (to later check if password and password_hash fit) - $result = UserModel::getUserDataByUsername($user_name); + // checks if user exists, if login is not blocked (due to failed logins) and if password fits the hash + $result = self::validateAndGetUser($user_name, $user_password); - // Check if that user exists. We don't give back a cause in the feedback to avoid giving an attacker details. - if (!$result) { - Session::add('feedback_negative', Text::get('FEEDBACK_LOGIN_FAILED')); - return false; - } - - // block login attempt if somebody has already failed 3 times and the last login attempt is less than 30sec ago - if (($result->user_failed_logins >= 3) AND ($result->user_last_failed_login > (time() - 30))) { - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_WRONG_3_TIMES')); - return false; - } - - // if hash of provided password does NOT match the hash in the database: +1 failed-login counter - if (!password_verify($user_password, $result->user_password_hash)) { - LoginModel::incrementFailedLoginCounterOfUser($user_name); - // we say "password wrong" here, but less details like "login failed" would be better (= less information) - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_WRONG')); - return false; - } - - // from here we assume that the password hash fits the database password hash, as password_verify() was true - - // if user is not active (= has not verified account by verification mail) - if ($result->user_active != 1) { - Session::add('feedback_negative', Text::get('FEEDBACK_ACCOUNT_NOT_ACTIVATED_YET')); - return false; - } + if (!$result) { + return false; + } // reset the failed login counter for that user (if necessary) if ($result->user_last_failed_login > 0) { - LoginModel::resetFailedLoginCounterOfUser($user_name); + self::resetFailedLoginCounterOfUser($result->user_name); } // save timestamp of this login in the database line of that user - LoginModel::saveTimestampOfLoginOfUser($user_name); + self::saveTimestampOfLoginOfUser($result->user_name); // if user has checked the "remember me" checkbox, then write token into database and into cookie if ($set_remember_me_cookie) { - LoginModel::setRememberMeInDatabaseAndCookie($result->user_id); + self::setRememberMeInDatabaseAndCookie($result->user_id); } // successfully logged in, so we write all necessary data into the session and set "user_logged_in" to true - LoginModel::setSuccessfulLoginIntoSession( + self::setSuccessfulLoginIntoSession( $result->user_id, $result->user_name, $result->user_email, $result->user_account_type ); @@ -78,6 +54,49 @@ public static function login($user_name, $user_password, $set_remember_me_cookie return true; } + /** + * Validates the inputs of the users, checks if password is correct etc. + * If successful, user is returned + * + * @param $user_name + * @param $user_password + * + * @return bool|mixed + */ + private static function validateAndGetUser($user_name, $user_password) + { + // get all data of that user (to later check if password and password_hash fit) + $result = UserModel::getUserDataByUsername($user_name); + + // Check if that user exists. We don't give back a cause in the feedback to avoid giving an attacker details. + if (!$result) { + Session::add('feedback_negative', Text::get('FEEDBACK_LOGIN_FAILED')); + return false; + } + + // block login attempt if somebody has already failed 3 times and the last login attempt is less than 30sec ago + if (($result->user_failed_logins >= 3) AND ($result->user_last_failed_login > (time() - 30))) { + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_WRONG_3_TIMES')); + return false; + } + + // if hash of provided password does NOT match the hash in the database: +1 failed-login counter + if (!password_verify($user_password, $result->user_password_hash)) { + self::incrementFailedLoginCounterOfUser($result->user_name); + // we say "password wrong" here, but less details like "login failed" would be better (= less information) + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_WRONG')); + return false; + } + + // if user is not active (= has not verified account by verification mail) + if ($result->user_active != 1) { + Session::add('feedback_negative', Text::get('FEEDBACK_ACCOUNT_NOT_ACTIVATED_YET')); + return false; + } + + return $result; + } + /** * performs the login via cookie (for DEFAULT user account, FACEBOOK-accounts are handled differently) * TODO add throttling here ? @@ -88,40 +107,25 @@ public static function login($user_name, $user_password, $set_remember_me_cookie */ public static function loginWithCookie($cookie) { - // do we have a cookie ? if (!$cookie) { Session::add('feedback_negative', Text::get('FEEDBACK_COOKIE_INVALID')); return false; } - // check cookie's contents, check if cookie contents belong together + // check cookie's contents, check if cookie contents belong together or token is empty list ($user_id, $token, $hash) = explode(':', $cookie); - if ($hash !== hash('sha256', $user_id . ':' . $token)) { - Session::add('feedback_negative', Text::get('FEEDBACK_COOKIE_INVALID')); - return false; - } - - // do not log in when token is empty - if (empty($token)) { + if ($hash !== hash('sha256', $user_id . ':' . $token) OR empty($token)) { Session::add('feedback_negative', Text::get('FEEDBACK_COOKIE_INVALID')); return false; } // get data of user that has this id and this token $result = UserModel::getUserDataByUserIdAndToken($user_id, $token); - - // if user with that id and exactly that cookie token exists in database if ($result) { // successfully logged in, so we write all necessary data into the session and set "user_logged_in" to true - LoginModel::setSuccessfulLoginIntoSession( - $result->user_id, $result->user_name, $result->user_email, $result->user_account_type - ); + self::setSuccessfulLoginIntoSession($result->user_id, $result->user_name, $result->user_email, $result->user_account_type); // save timestamp of this login in the database line of that user - LoginModel::saveTimestampOfLoginOfUser($result->user_name); - - // NOTE: we don't set another remember_me-cookie here as the current cookie should always - // be invalid after a certain amount of time, so the user has to login with username/password - // again from time to time. This is good and safe ! ;) + self::saveTimestampOfLoginOfUser($result->user_name); Session::add('feedback_positive', Text::get('FEEDBACK_COOKIE_LOGIN_SUCCESSFUL')); return true; @@ -136,7 +140,7 @@ public static function loginWithCookie($cookie) */ public static function logout() { - LoginModel::deleteCookie(); + self::deleteCookie(); Session::destroy(); } diff --git a/application/model/PasswordResetModel.php b/application/model/PasswordResetModel.php index 6f65afe97..634073f5b 100644 --- a/application/model/PasswordResetModel.php +++ b/application/model/PasswordResetModel.php @@ -177,25 +177,18 @@ public static function saveNewUserPassword($user_name, $user_password_hash, $use { $database = DatabaseFactory::getFactory()->getConnection(); - $sql = "UPDATE users - SET user_password_hash = :user_password_hash, - user_password_reset_hash = NULL, + $sql = "UPDATE users SET user_password_hash = :user_password_hash, user_password_reset_hash = NULL, user_password_reset_timestamp = NULL - WHERE user_name = :user_name - AND user_password_reset_hash = :user_password_reset_hash - AND user_provider_type = :user_provider_type - LIMIT 1"; + WHERE user_name = :user_name AND user_password_reset_hash = :user_password_reset_hash + AND user_provider_type = :user_provider_type LIMIT 1"; $query = $database->prepare($sql); $query->execute(array( ':user_password_hash' => $user_password_hash, ':user_name' => $user_name, ':user_password_reset_hash' => $user_password_reset_hash, ':user_provider_type' => 'DEFAULT' )); - // if successful - if ($query->rowCount() == 1) { - return true; - } - return false; + // if one result exists, return true, else false. Could be written even shorter btw. + return ($query->rowCount() == 1 ? true : false); } /** @@ -212,6 +205,36 @@ public static function saveNewUserPassword($user_name, $user_password_hash, $use * @return bool success state of the password reset */ public static function setNewPassword($user_name, $user_password_reset_hash, $user_password_new, $user_password_repeat) + { + // validate the password + if (!self::validateNewPassword($user_name, $user_password_reset_hash, $user_password_new, $user_password_repeat)) { + return false; + } + + // crypt the password (with the PHP 5.5+'s password_hash() function, result is a 60 character hash string) + $user_password_hash = password_hash($user_password_new, PASSWORD_DEFAULT); + + // write the password to database (as hashed and salted string), reset user_password_reset_hash + if (PasswordResetModel::saveNewUserPassword($user_name, $user_password_hash, $user_password_reset_hash)) { + Session::add('feedback_positive', Text::get('FEEDBACK_PASSWORD_CHANGE_SUCCESSFUL')); + return true; + } else { + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_CHANGE_FAILED')); + return false; + } + } + + /** + * Validate the password submission + * + * @param $user_name + * @param $user_password_reset_hash + * @param $user_password_new + * @param $user_password_repeat + * + * @return bool + */ + public static function validateNewPassword($user_name, $user_password_reset_hash, $user_password_new, $user_password_repeat) { if (empty($user_name)) { Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_FIELD_EMPTY')); @@ -230,17 +253,6 @@ public static function setNewPassword($user_name, $user_password_reset_hash, $us return false; } - // crypt the user's password with the PHP 5.5+'s password_hash() function, result is a 60 character hash string - $user_password_hash = password_hash($user_password_new, PASSWORD_DEFAULT); - - // write user's new password hash into database, reset user_password_reset_hash - if (PasswordResetModel::saveNewUserPassword($user_name, $user_password_hash, $user_password_reset_hash)) { - Session::add('feedback_positive', Text::get('FEEDBACK_PASSWORD_CHANGE_SUCCESSFUL')); - return true; - } - - // default return - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_CHANGE_FAILED')); - return false; + return true; } } diff --git a/application/model/RegistrationModel.php b/application/model/RegistrationModel.php index 59d1cc71e..b03a7ca9d 100644 --- a/application/model/RegistrationModel.php +++ b/application/model/RegistrationModel.php @@ -89,35 +89,91 @@ public static function registrationInputValidation($captcha, $user_name, $user_p // perform all necessary checks if (!CaptchaModel::checkCaptcha($captcha)) { Session::add('feedback_negative', Text::get('FEEDBACK_CAPTCHA_WRONG')); - } else if (empty($user_name)) { - Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_FIELD_EMPTY')); - } else if (empty($user_password_new) OR empty($user_password_repeat)) { - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_FIELD_EMPTY')); - } else if ($user_password_new !== $user_password_repeat) { - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_REPEAT_WRONG')); - } else if (strlen($user_password_new) < 6) { - Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_TOO_SHORT')); - } else if (strlen($user_name) > 64 OR strlen($user_name) < 2) { - Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_TOO_SHORT_OR_TOO_LONG')); - } else if (!preg_match('/^[a-zA-Z0-9]{2,64}$/', $user_name)) { - Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_DOES_NOT_FIT_PATTERN')); - } else if (empty($user_email)) { - Session::add('feedback_negative', Text::get('FEEDBACK_EMAIL_FIELD_EMPTY')); - } else if (strlen($user_email) > 254) { - // @see http://stackoverflow.com/questions/386294/what-is-the-maximum-length-of-a-valid-email-address - Session::add('feedback_negative', Text::get('FEEDBACK_EMAIL_TOO_LONG')); - } else if (!filter_var($user_email, FILTER_VALIDATE_EMAIL)) { - Session::add('feedback_negative', Text::get('FEEDBACK_EMAIL_DOES_NOT_FIT_PATTERN')); - } else { - // if no validation failed, return true - // hmmm... maybe this could be written in a better way - return true; + return false; } + // if username, email and password are all correctly validated + if (self::validateUserName($user_name) AND self::validateUserEmail($user_email) AND self::validateUserPassword($user_password_new, $user_password_repeat)) { + return true; + } + // otherwise, return false return false; } + /** + * Validates the username + * + * @param $user_name + * @return bool + */ + public static function validateUserName($user_name) + { + if (empty($user_name)) { + Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_FIELD_EMPTY')); + return false; + } + + // if username is too short (2), too long (64) or does not fit the pattern (aZ09) + if (!preg_match('/^[a-zA-Z0-9]{2,64}$/', $user_name)) { + Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_DOES_NOT_FIT_PATTERN')); + return false; + } + + return true; + } + + /** + * Validates the email + * + * @param $user_email + * @return bool + */ + public static function validateUserEmail($user_email) + { + if (empty($user_email)) { + Session::add('feedback_negative', Text::get('FEEDBACK_EMAIL_FIELD_EMPTY')); + return false; + } + + // validate the email with PHP's internal filter + // side-fact: Max length seems to be 254 chars + // @see http://stackoverflow.com/questions/386294/what-is-the-maximum-length-of-a-valid-email-address + if (!filter_var($user_email, FILTER_VALIDATE_EMAIL)) { + Session::add('feedback_negative', Text::get('FEEDBACK_EMAIL_DOES_NOT_FIT_PATTERN')); + return false; + } + + return true; + } + + /** + * Validates the password + * + * @param $user_password_new + * @param $user_password_repeat + * @return bool + */ + public static function validateUserPassword($user_password_new, $user_password_repeat) + { + if (empty($user_password_new) OR empty($user_password_repeat)) { + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_FIELD_EMPTY')); + return false; + } + + if ($user_password_new !== $user_password_repeat) { + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_REPEAT_WRONG')); + return false; + } + + if (strlen($user_password_new) < 6) { + Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_TOO_SHORT')); + return false; + } + + return true; + } + /** * Writes the new user's data to the database * @@ -166,7 +222,8 @@ public static function rollbackRegistrationByUserId($user_id) } /** - * Sends the verification email (to confirm the account) + * Sends the verification email (to confirm the account). + * The construction of the mail $body looks weird at first, but it's really just a simple string. * * @param int $user_id user's id * @param string $user_email user's email @@ -176,27 +233,21 @@ public static function rollbackRegistrationByUserId($user_id) */ public static function sendVerificationEmail($user_id, $user_email, $user_activation_hash) { - // create email body $body = Config::get('EMAIL_VERIFICATION_CONTENT') . Config::get('URL') . Config::get('EMAIL_VERIFICATION_URL') . '/' . urlencode($user_id) . '/' . urlencode($user_activation_hash); - // create instance of Mail class, try sending and check $mail = new Mail; - $mail_sent = $mail->sendMail( - $user_email, - Config::get('EMAIL_VERIFICATION_FROM_EMAIL'), - Config::get('EMAIL_VERIFICATION_FROM_NAME'), - Config::get('EMAIL_VERIFICATION_SUBJECT'), - $body + $mail_sent = $mail->sendMail($user_email, Config::get('EMAIL_VERIFICATION_FROM_EMAIL'), + Config::get('EMAIL_VERIFICATION_FROM_NAME'), Config::get('EMAIL_VERIFICATION_SUBJECT'), $body ); if ($mail_sent) { Session::add('feedback_positive', Text::get('FEEDBACK_VERIFICATION_MAIL_SENDING_SUCCESSFUL')); return true; + } else { + Session::add('feedback_negative', Text::get('FEEDBACK_VERIFICATION_MAIL_SENDING_ERROR') . $mail->getError() ); + return false; } - - Session::add('feedback_negative', Text::get('FEEDBACK_VERIFICATION_MAIL_SENDING_ERROR') . $mail->getError() ); - return false; } /** diff --git a/application/model/UserModel.php b/application/model/UserModel.php index e1be1c272..0179da4dd 100644 --- a/application/model/UserModel.php +++ b/application/model/UserModel.php @@ -23,19 +23,15 @@ public static function getPublicProfilesOfAllUsers() $all_users_profiles = array(); foreach ($query->fetchAll() as $user) { - // a new object for every user. This is eventually not really optimal when it comes - // to performance, but it fits the view style better $all_users_profiles[$user->user_id] = new stdClass(); $all_users_profiles[$user->user_id]->user_id = $user->user_id; $all_users_profiles[$user->user_id]->user_name = $user->user_name; $all_users_profiles[$user->user_id]->user_email = $user->user_email; if (Config::get('USE_GRAVATAR')) { - $all_users_profiles[$user->user_id]->user_avatar_link = - AvatarModel::getGravatarLinkByEmail($user->user_email); + $all_users_profiles[$user->user_id]->user_avatar_link = AvatarModel::getGravatarLinkByEmail($user->user_email); } else { - $all_users_profiles[$user->user_id]->user_avatar_link = - AvatarModel::getPublicAvatarFilePathOfUser($user->user_has_avatar, $user->user_id); + $all_users_profiles[$user->user_id]->user_avatar_link = AvatarModel::getPublicAvatarFilePathOfUser($user->user_has_avatar, $user->user_id); } $all_users_profiles[$user->user_id]->user_active = $user->user_active; @@ -178,12 +174,6 @@ public static function saveNewEmailAddress($user_id, $new_user_email) */ public static function editUserName($new_user_name) { - // new username provided ? - if (empty($new_user_name)) { - Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_FIELD_EMPTY')); - return false; - } - // new username same as old one ? if ($new_user_name == Session::get('user_name')) { Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_SAME_AS_OLD_ONE')); @@ -210,11 +200,10 @@ public static function editUserName($new_user_name) Session::set('user_name', $new_user_name); Session::add('feedback_positive', Text::get('FEEDBACK_USERNAME_CHANGE_SUCCESSFUL')); return true; + } else { + Session::add('feedback_negative', Text::get('FEEDBACK_UNKNOWN_ERROR')); + return false; } - - // default fallback - Session::add('feedback_negative', Text::get('FEEDBACK_UNKNOWN_ERROR')); - return false; } /** diff --git a/application/model/AccountTypeModel.php b/application/model/UserRoleModel.php similarity index 59% rename from application/model/AccountTypeModel.php rename to application/model/UserRoleModel.php index b777e51b6..f45f088c7 100644 --- a/application/model/AccountTypeModel.php +++ b/application/model/UserRoleModel.php @@ -1,11 +1,11 @@ getConnection(); $query = $database->prepare("UPDATE users SET user_account_type = :new_type WHERE user_id = :user_id LIMIT 1"); diff --git a/application/view/_templates/header.php b/application/view/_templates/header.php index e3850e6ff..9859a71ea 100644 --- a/application/view/_templates/header.php +++ b/application/view/_templates/header.php @@ -39,25 +39,6 @@ - -