Skip to content

Commit

Permalink
Various improvements to number verification workflow in MobileMessagi…
Browse files Browse the repository at this point in the history 
…ng (#22668)

* Adjust Development SMS provider to log messages instead of a notification

Notifications don't work here correctly anymore, as the messages are sent through the API, where access to the session may not be possible"

* Extend verification code to 6 alhpanumeric characters

* Limit number of unverified phone numbers to 3 per user

* Add confirmation when removing a phone number

* Store verification details and ensure that verification code expires after 3 failed tries or 10 minutes

* Sort numbers in UI so unverified are listed first

* improve usability of phone number management

* Add possibility to resend a verification code

* fix a couple of tests

* Adds some more tests

* move UI tests to plugin

* adds some more UI tests

* Only validate numbers when adding them

* apply some review feedback
  • Loading branch information
@sgiehl
sgiehl authored Oct 14, 2024
1 parent 6ba0934 commit 840029e
Show file tree
Hide file tree
Showing 19 changed files with 730 additions and 262 deletions.
149 changes: 99 additions & 50 deletions plugins/MobileMessaging/API.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace Piwik\Plugins\MobileMessaging;

use Piwik\Common;
use Piwik\Date;
use Piwik\Piwik;
use Piwik\Plugins\MobileMessaging\SMSProvider;

Expand Down Expand Up @@ -39,7 +40,7 @@ public function __construct(Model $model)
*
* @return bool true if SMS API credential are available for the current user
*/
public function areSMSAPICredentialProvided()
public function areSMSAPICredentialProvided(): bool
{
Piwik::checkUserHasSomeViewAccess();

Expand All @@ -64,10 +65,9 @@ public function getSMSProvider()
*
* @param string $provider SMS API provider
* @param array $credentials array with data like API Key or username
*
* @return bool true if SMS API credential were validated and saved, false otherwise
* @return void
*/
public function setSMSAPICredential($provider, $credentials = array())
public function setSMSAPICredential(string $provider, array $credentials = []): void
{
$this->checkCredentialManagementRights();

Expand All @@ -80,57 +80,106 @@ public function setSMSAPICredential($provider, $credentials = array())
$settings[MobileMessaging::API_KEY_OPTION] = $credentials;

$this->model->setCredentialManagerSettings($settings);

return true;
}

/**
* add phone number
* Adds a phone number for the current user
*
* @param string $phoneNumber
* @return void
*/
public function addPhoneNumber(string $phoneNumber): void
{
Piwik::checkUserIsNotAnonymous();

$phoneNumber = $this->sanitizePhoneNumber($phoneNumber);

// Check format matches the international public telecommunication numbering plan (E.164)
// See https://en.wikipedia.org/wiki/E.164
if (!preg_match('/^\+[0-9]{5,30}$/', $phoneNumber)) {
throw new \Exception("The phone number $phoneNumber does not match the expected number format.");
}

$phoneNumbers = $this->model->getPhoneNumbers(Piwik::getCurrentUserLogin(), false);

$unverifiedPhoneNumbers = array_filter(
$phoneNumbers,
function ($phoneNumber) {
return !$phoneNumber['verified'];
}
);

if (count($unverifiedPhoneNumbers) >= 3) {
throw new \Exception(Piwik::translate('MobileMessaging_TooManyUnverifiedNumbersError'));
}

$this->sendVerificationCodeAndAddPhoneNumber($phoneNumber);
}

/**
* Requests a new verification code for the given phone number
*
* @return bool true
* @param string $phoneNumber
* @return void
*/
public function addPhoneNumber($phoneNumber)
public function resendVerificationCode(string $phoneNumber): void
{
Piwik::checkUserIsNotAnonymous();

$phoneNumber = $this->sanitizePhoneNumber($phoneNumber);

$verificationCode = "";
for ($i = 0; $i < self::VERIFICATION_CODE_LENGTH; $i++) {
$verificationCode .= Common::getRandomInt(0, 9);
$phoneNumbers = $this->model->getPhoneNumbers(Piwik::getCurrentUserLogin(), false);

if (empty($phoneNumbers[$phoneNumber])) {
throw new \Exception("The phone number $phoneNumber has not yet been added.");
}

if (true === $phoneNumbers[$phoneNumber]['verified']) {
throw new \Exception("The phone number $phoneNumber has already been verified.");
}

if ($phoneNumbers[$phoneNumber]['requestTime'] > Date::getNowTimestamp() - 60) {
throw new \Exception(Piwik::translate('MobileMessaging_VerificationCodeRecentlySentError', $phoneNumber));
}

$this->sendVerificationCodeAndAddPhoneNumber($phoneNumber);
}

private function sendVerificationCodeAndAddPhoneNumber(string $phoneNumber): void
{
$verificationCode = Common::getRandomString(6, 'abcdefghijklmnoprstuvwxyz0123456789');

$smsText = Piwik::translate(
'MobileMessaging_VerificationText',
array(
$verificationCode,
Piwik::translate('General_Settings'),
Piwik::translate('MobileMessaging_SettingsMenu')
$verificationCode,
Piwik::translate('General_Settings'),
Piwik::translate('MobileMessaging_SettingsMenu')
)
);

$this->model->sendSMS($smsText, $phoneNumber, self::SMS_FROM);

$phoneNumbers = $this->model->retrievePhoneNumbers(Piwik::getCurrentUserLogin());
$phoneNumbers[$phoneNumber] = $verificationCode;
$this->model->savePhoneNumbers(Piwik::getCurrentUserLogin(), $phoneNumbers);

$this->model->increaseCount(Piwik::getCurrentUserLogin(), MobileMessaging::PHONE_NUMBER_VALIDATION_REQUEST_COUNT_OPTION, $phoneNumber);

return true;
$this->model->addPhoneNumber(Piwik::getCurrentUserLogin(), $phoneNumber, $verificationCode);
}

/**
* sanitize phone number
* Sanitize phone number
*
* @param string $phoneNumber
* @return string sanitized phone number
*/
private function sanitizePhoneNumber($phoneNumber)
{
return str_replace(' ', '', $phoneNumber);
// remove common formatting characters: - _ ( )
$phoneNumber = str_replace(['-', '_', ' ', '(', ')'], '', $phoneNumber);

// Avoid that any method tries to handle phone numbers that are obviously too long
if (strlen($phoneNumber) > 100) {
throw new \Exception("The phone number $phoneNumber does not match the expected number format.");
}

return $phoneNumber;
}

/**
Expand All @@ -149,20 +198,31 @@ public function getCreditLeft()
);
}

/**
* @return array
* @throws \Piwik\NoAccessException
*/
public function getPhoneNumbers()
{
Piwik::checkUserIsNotAnonymous();

return $this->model->getPhoneNumbers(Piwik::getCurrentUserLogin(), false);
}

/**
* remove phone number
*
* @param string $phoneNumber
*
* @return bool true
* @return void
*/
public function removePhoneNumber($phoneNumber)
public function removePhoneNumber(string $phoneNumber): void
{
Piwik::checkUserIsNotAnonymous();

$phoneNumbers = $this->model->retrievePhoneNumbers(Piwik::getCurrentUserLogin());
unset($phoneNumbers[$phoneNumber]);
$this->model->savePhoneNumbers(Piwik::getCurrentUserLogin(), $phoneNumbers);
$phoneNumber = $this->sanitizePhoneNumber($phoneNumber);

$phoneNumbers = $this->model->removePhoneNumber(Piwik::getCurrentUserLogin(), $phoneNumber);

/**
* Triggered after a phone number has been deleted. This event should be used to clean up any data that is
Expand All @@ -179,41 +239,31 @@ public function removePhoneNumber($phoneNumber)
* @param string $phoneNumber The phone number that was just deleted.
*/
Piwik::postEvent('MobileMessaging.deletePhoneNumber', array($phoneNumber));

return true;
}

/**
* validate phone number
* Verify a phone number
*
* @param string $phoneNumber
* @param string $verificationCode
*
* @return bool true if validation code is correct, false otherwise
* @return bool true if verification was successful, false otherwise
*/
public function validatePhoneNumber($phoneNumber, $verificationCode)
public function validatePhoneNumber(string $phoneNumber, string $verificationCode)
{
Piwik::checkUserIsNotAnonymous();

$phoneNumbers = $this->model->retrievePhoneNumbers(Piwik::getCurrentUserLogin());

if (isset($phoneNumbers[$phoneNumber])) {
if ($verificationCode == $phoneNumbers[$phoneNumber]) {
$phoneNumbers[$phoneNumber] = null;
$this->model->savePhoneNumbers(Piwik::getCurrentUserLogin(), $phoneNumbers);
return true;
}
}
$phoneNumber = $this->sanitizePhoneNumber($phoneNumber);

return false;
return $this->model->verifyPhoneNumber(Piwik::getCurrentUserLogin(), $phoneNumber, $verificationCode);
}

/**
* delete the SMS API credential
*
* @return bool true
* @return void
*/
public function deleteSMSAPICredential()
public function deleteSMSAPICredential(): void
{
$this->checkCredentialManagementRights();

Expand All @@ -222,16 +272,15 @@ public function deleteSMSAPICredential()
$settings[MobileMessaging::API_KEY_OPTION] = null;

$this->model->setCredentialManagerSettings($settings);

return true;
}

/**
* Specify if normal users can manage their own SMS API credential
*
* @param bool $delegatedManagement false if SMS API credential only manageable by super admin, true otherwise
* @return void
*/
public function setDelegatedManagement($delegatedManagement)
public function setDelegatedManagement(bool $delegatedManagement): void
{
Piwik::checkUserHasSuperUserAccess();
$this->model->setDelegatedManagement($delegatedManagement);
Expand All @@ -242,7 +291,7 @@ public function setDelegatedManagement($delegatedManagement)
*
* @return bool false if SMS API credential only manageable by super admin, true otherwise
*/
public function getDelegatedManagement()
public function getDelegatedManagement(): bool
{
Piwik::checkUserHasSomeViewAccess();
return $this->model->getDelegatedManagement();
Expand Down
2 changes: 0 additions & 2 deletions plugins/MobileMessaging/Controller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,6 @@ private function setManageVariables(View $view)
}
$view->countries = $countries;

$view->phoneNumbers = $model->getPhoneNumbers(Piwik::getCurrentUserLogin());

$this->setBasicVariablesView($view);
}

Expand Down
6 changes: 5 additions & 1 deletion plugins/MobileMessaging/MobileMessaging.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class MobileMessaging extends \Piwik\Plugin
public const PROVIDER_OPTION = 'Provider';
public const API_KEY_OPTION = 'APIKey';
public const PHONE_NUMBERS_OPTION = 'PhoneNumbers';
public const PHONE_NUMBER_VALIDATION_REQUEST_COUNT_OPTION = 'PhoneNumberValidationRequestCount';
public const SMS_SENT_COUNT_OPTION = 'SMSSentCount';
public const DELEGATED_MANAGEMENT_OPTION_DEFAULT = 'false';
public const USER_SETTINGS_POSTFIX_OPTION = '_MobileMessagingSettings';
Expand Down Expand Up @@ -127,6 +126,11 @@ public function getClientSideTranslationKeys(&$translationKeys)
$translationKeys[] = 'MobileMessaging_Settings_DeleteAccountConfirm';
$translationKeys[] = 'MobileMessaging_Settings_SuspiciousPhoneNumber';
$translationKeys[] = 'MobileMessaging_SettingsMenu';
$translationKeys[] = 'MobileMessaging_ConfirmRemovePhoneNumber';
$translationKeys[] = 'MobileMessaging_Settings_ResendVerification';
$translationKeys[] = 'MobileMessaging_Settings_NewVerificationCodeSent';
$translationKeys[] = 'General_Yes';
$translationKeys[] = 'General_No';
}

public function validateReportParameters(&$parameters, $reportType)
Expand Down
Loading

0 comments on commit 840029e

Please sign in to comment.