From 9ddaabc22d2cb0d93214db526e15aa458c571ae1 Mon Sep 17 00:00:00 2001 From: very-ape Date: Wed, 19 May 2021 12:57:58 -0700 Subject: [PATCH] Move the 'authenticate' hook deeper into the authentication flow so that password authentication addons are consulted whenever a user's password needs confirming. --- doc/Addons.md | 31 ++++++++++++++++++++++++++----- src/Model/User.php | 22 ++++++++++++++++++++-- src/Security/Authentication.php | 32 +++++--------------------------- 3 files changed, 51 insertions(+), 34 deletions(-) diff --git a/doc/Addons.md b/doc/Addons.md index be9dd42189..a058753605 100644 --- a/doc/Addons.md +++ b/doc/Addons.md @@ -31,7 +31,7 @@ Here's the structure: * Status: {Unsupported|Arbitrary status} */ ``` - + You can also provide a longer documentation in a `README` or `README.md` file. The latter will be converted from Markdown to HTML in the addon detail page. @@ -447,7 +447,7 @@ Form field array structure is: - **field**: Standard field data structure to be used by `field_checkbox.tpl` and `field_select.tpl`. For `checkbox`, **field** is: - - [0] (String): Form field name; Mandatory. + - [0] (String): Form field name; Mandatory. - [1]: (String): Form field label; Optional, default is none. - [2]: (Boolean): Whether the checkbox should be checked by default; Optional, default is false. - [3]: (String): Additional help text; Optional, default is none. @@ -458,7 +458,7 @@ For `select`, **field** is: - [1] (String): Form field label; Optional, default is none. - [2] (Boolean): Default value to be selected by default; Optional, default is none. - [3] (String): Additional help text; Optional, default is none. - - [4] (Array): Associative array of options. Item key is option value, item value is option label; Mandatory. + - [4] (Array): Associative array of options. Item key is option value, item value is option label; Mandatory. ### route_collection Called just before dispatching the router. @@ -470,7 +470,7 @@ Hook data is a `\FastRoute\RouterCollector` object that should be used to add ad Called before trying to detect the target network of a URL. If any registered hook function sets the `result` key of the hook data array, it will be returned immediately. -Hook functions should also return immediately if the hook data contains an existing result. +Hook functions should also return immediately if the hook data contains an existing result. Hook data: @@ -666,8 +666,13 @@ Here is a complete list of all hook callbacks with file locations (as of 24-Sep- Hook::callAll('event_updated', $event['id']); Hook::callAll("event_created", $event['id']); +### src/Model/Register.php + + Hook::callAll('authenticate', $addon_auth); + ### src/Model/User.php + Hook::callAll('authenticate', $addon_auth); Hook::callAll('register_account', $uid); Hook::callAll('remove_user', $user); @@ -675,6 +680,22 @@ Here is a complete list of all hook callbacks with file locations (as of 24-Sep- Hook::callAll('lockview_content', $item); +### src/Module/Settings/Delegation.php + + Hook::callAll('authenticate', $addon_auth); + +### src/Module/Settings/TwoFactor/Index.php + + Hook::callAll('authenticate', $addon_auth); + +### src/Security/Authenticate.php + + Hook::callAll('authenticate', $addon_auth); + +### src/Security/ExAuth.php + + Hook::callAll('authenticate', $addon_auth); + ### src/Content/ContactBlock.php Hook::callAll('contact_block_end', $arr); @@ -713,7 +734,7 @@ Here is a complete list of all hook callbacks with file locations (as of 24-Sep- ### src/Core/Authentication.php Hook::callAll('logged_in', $a->user); - + ### src/Core/StorageManager Hook::callAll('storage_instance', $data); diff --git a/src/Model/User.php b/src/Model/User.php index 4959c8c216..3b11ee6ce8 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -544,6 +544,24 @@ class User } return $user['uid']; + } else { + $addon_auth = [ + 'username' => $user['nickname'], + 'password' => $password, + 'authenticated' => 0, + 'user_record' => null + ]; + + /* + * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record + * Addons should never set 'authenticated' except to indicate success - as hooks may be chained + * and later addons should not interfere with an earlier one that succeeded. + */ + Hook::callAll('authenticate', $addon_auth); + + if ($addon_auth['authenticated'] && $addon_auth['user_record']) { + return $user['uid']; + } } throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed')); @@ -584,7 +602,7 @@ class User if (is_int($user_info)) { $user = DBA::selectFirst( 'user', - ['uid', 'password', 'legacy_password'], + ['uid', 'nickname', 'password', 'legacy_password'], [ 'uid' => $user_info, 'blocked' => 0, @@ -594,7 +612,7 @@ class User ] ); } else { - $fields = ['uid', 'password', 'legacy_password']; + $fields = ['uid', 'nickname', 'password', 'legacy_password']; $condition = [ "(`email` = ? OR `username` = ? OR `nickname` = ?) AND NOT `blocked` AND NOT `account_expired` AND NOT `account_removed` AND `verified`", diff --git a/src/Security/Authentication.php b/src/Security/Authentication.php index eab75ba5d0..61553907fa 100644 --- a/src/Security/Authentication.php +++ b/src/Security/Authentication.php @@ -239,34 +239,12 @@ class Authentication { $record = null; - $addon_auth = [ - 'username' => $username, - 'password' => $password, - 'authenticated' => 0, - 'user_record' => null - ]; - - /* - * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record - * Addons should never set 'authenticated' except to indicate success - as hooks may be chained - * and later addons should not interfere with an earlier one that succeeded. - */ - Hook::callAll('authenticate', $addon_auth); - try { - if ($addon_auth['authenticated']) { - $record = $addon_auth['user_record']; - - if (empty($record)) { - throw new Exception($this->l10n->t('Login failed.')); - } - } else { - $record = $this->dba->selectFirst( - 'user', - [], - ['uid' => User::getIdFromPasswordAuthentication($username, $password)] - ); - } + $record = $this->dba->selectFirst( + 'user', + [], + ['uid' => User::getIdFromPasswordAuthentication($username, $password)] + ); } catch (Exception $e) { $this->logger->warning('authenticate: failed login attempt', ['action' => 'login', 'username' => Strings::escapeTags($username), 'ip' => $_SERVER['REMOTE_ADDR']]); notice($this->l10n->t('Login failed. Please check your credentials.'));