Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
implement verification for additional mails
- mails added by (sub)admins are automatically verified
- provisioning_api controller as verification endpoint
- IAccountProperty gets a locallyVerified property
- IPropertyCollection gets a method to fetch an IAccountProperty by value
  - an remove equivalent was already present
- AccountManager always initiates mail verification on update if necessary
- add core success template for arbitrary title and message

Signed-off-by: Arthur Schiwon <[email protected]>
  • Loading branch information
blizzz committed Sep 9, 2021
commit aacaad2a3f56893c6be463ec7a21c868322654ee
4 changes: 4 additions & 0 deletions apps/provisioning_api/appinfo/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,8 @@
['name' => 'AppConfig#setValue', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'POST'],
['name' => 'AppConfig#deleteKey', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'DELETE'],
],
'routes' => [
// Verification
['name' => 'Verification#verifyMail', 'url' => '/mailVerification/{key}/{token}/{userId}', 'verb' => 'GET'],
]
];
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
'OCA\\Provisioning_API\\Controller\\AppsController' => $baseDir . '/../lib/Controller/AppsController.php',
'OCA\\Provisioning_API\\Controller\\GroupsController' => $baseDir . '/../lib/Controller/GroupsController.php',
'OCA\\Provisioning_API\\Controller\\UsersController' => $baseDir . '/../lib/Controller/UsersController.php',
'OCA\\Provisioning_API\\Controller\\VerificationController' => $baseDir . '/../lib/Controller/VerificationController.php',
'OCA\\Provisioning_API\\FederatedShareProviderFactory' => $baseDir . '/../lib/FederatedShareProviderFactory.php',
'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => $baseDir . '/../lib/Listener/UserDeletedListener.php',
'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => $baseDir . '/../lib/Middleware/Exceptions/NotSubAdminException.php',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class ComposerStaticInitProvisioning_API
'OCA\\Provisioning_API\\Controller\\AppsController' => __DIR__ . '/..' . '/../lib/Controller/AppsController.php',
'OCA\\Provisioning_API\\Controller\\GroupsController' => __DIR__ . '/..' . '/../lib/Controller/GroupsController.php',
'OCA\\Provisioning_API\\Controller\\UsersController' => __DIR__ . '/..' . '/../lib/Controller/UsersController.php',
'OCA\\Provisioning_API\\Controller\\VerificationController' => __DIR__ . '/..' . '/../lib/Controller/VerificationController.php',
'OCA\\Provisioning_API\\FederatedShareProviderFactory' => __DIR__ . '/..' . '/../lib/FederatedShareProviderFactory.php',
'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/UserDeletedListener.php',
'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => __DIR__ . '/..' . '/../lib/Middleware/Exceptions/NotSubAdminException.php',
Expand Down
4 changes: 2 additions & 2 deletions apps/provisioning_api/composer/composer/installed.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
'type' => 'library',
'install_path' => __DIR__ . '/../',
'aliases' => array(),
'reference' => 'fa56c13484afa1baf908b93ed5b6990c6a0e9ad6',
'reference' => '2e49000abb5acb09de041369a2239db23fa63ec7',
'name' => '__root__',
'dev' => false,
),
Expand All @@ -16,7 +16,7 @@
'type' => 'library',
'install_path' => __DIR__ . '/../',
'aliases' => array(),
'reference' => 'fa56c13484afa1baf908b93ed5b6990c6a0e9ad6',
'reference' => '2e49000abb5acb09de041369a2239db23fa63ec7',
'dev_requirement' => false,
),
),
Expand Down
14 changes: 10 additions & 4 deletions apps/provisioning_api/lib/Controller/UsersController.php
Original file line number Diff line number Diff line change
Expand Up @@ -621,18 +621,19 @@ public function editUserMultiValue(
throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
}

$subAdminManager = $this->groupManager->getSubAdmin();
$isAdminOrSubadmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID())
|| $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser);

$permittedFields = [];
if ($targetUser->getUID() === $currentLoggedInUser->getUID()) {
// Editing self (display, email)
$permittedFields[] = IAccountManager::COLLECTION_EMAIL;
$permittedFields[] = IAccountManager::COLLECTION_EMAIL . self::SCOPE_SUFFIX;
} else {
// Check if admin / subadmin
$subAdminManager = $this->groupManager->getSubAdmin();
if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())
|| $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
if ($isAdminOrSubadmin) {
// They have permissions over the user

$permittedFields[] = IAccountManager::COLLECTION_EMAIL;
} else {
// No rights
Expand All @@ -652,6 +653,11 @@ public function editUserMultiValue(
$mailCollection->removePropertyByValue($key);
if ($value !== '') {
$mailCollection->addPropertyWithDefaults($value);
$property = $mailCollection->getPropertyByValue($key);
if ($isAdminOrSubadmin && $property) {
// admin set mails are auto-verified
$property->setLocallyVerified(IAccountManager::VERIFIED);
}
}
$this->accountManager->updateAccount($userAccount);
break;
Expand Down
121 changes: 121 additions & 0 deletions apps/provisioning_api/lib/Controller/VerificationController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2021 Arthur Schiwon <[email protected]>
*
* @author Arthur Schiwon <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*
*/

namespace OCA\Provisioning_API\Controller;

use InvalidArgumentException;
use OC\Security\Crypto;
use OCP\Accounts\IAccountManager;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Security\VerificationToken\InvalidTokenException;
use OCP\Security\VerificationToken\IVerificationToken;

class VerificationController extends Controller {

/** @var IVerificationToken */
private $verificationToken;
/** @var IUserManager */
private $userManager;
/** @var IL10N */
private $l10n;
/** @var IUserSession */
private $userSession;
/** @var IAccountManager */
private $accountManager;
/** @var Crypto */
private $crypto;

public function __construct(
string $appName,
IRequest $request,
IVerificationToken $verificationToken,
IUserManager $userManager,
IL10N $l10n,
IUserSession $userSession,
IAccountManager $accountManager,
Crypto $crypto
) {
parent::__construct($appName, $request);
$this->verificationToken = $verificationToken;
$this->userManager = $userManager;
$this->l10n = $l10n;
$this->userSession = $userSession;
$this->accountManager = $accountManager;
$this->crypto = $crypto;
}

/**
* @NoCSRFRequired
*/
public function verifyMail(string $token, string $userId, string $key) {
try {
if ($this->userSession->getUser()->getUID() !== $userId) {
throw new InvalidArgumentException('Logged in user is not mail address owner');
}
$email = $this->crypto->decrypt($key);
$ref = \substr(hash('sha256', $email), 0, 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason we limit it to the first 8 chars here? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider it sufficient to avoid collisions – this is just part of the configkey stored in the db and used to avoid collisions only. If you manage to craft a collision – mind it applies per user – you manage to overwrite a previously stored token. It could have a security implication, if you manage to create a token for a different user that fits the collision, and yet the payload still needs to be valid and pass the checks.


$user = $this->userManager->get($userId);
$this->verificationToken->check($token, $user, 'verifyMail' . $ref, $email);

$userAccount = $this->accountManager->getAccount($user);
$emailProperty = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL)
->getPropertyByValue($email);

if ($emailProperty === null) {
throw new InvalidArgumentException($this->l10n->t('Email was already removed from account and cannot be confirmed anymore.'));
}
$emailProperty->setLocallyVerified(IAccountManager::VERIFIED);
$this->accountManager->updateAccount($userAccount);
} catch (InvalidTokenException $e) {
$error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED
? $this->l10n->t('Could not verify mail because the token is expired.')
: $this->l10n->t('Could not verify mail because the token is invalid.');
} catch (InvalidArgumentException $e) {
$error = $e->getMessage();
} catch (\Exception $e) {
$error = $this->l10n->t('An unexpected error occurred. Please consult your sysadmin.');
}

if (isset($error)) {
return new TemplateResponse(
'core', 'error', [
'errors' => [['error' => $error]]
], 'guest');
}

return new TemplateResponse(
'core', 'success', [
'title' => $this->l10n->t('Email confirmation successful'),
'message' => $this->l10n->t('Email confirmation successful'),
], 'guest');
}
}
13 changes: 13 additions & 0 deletions core/templates/success.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php
/** @var array $_ */
/** @var \OCP\IL10N $l */
/** @var \OCP\Defaults $theme */
?>

<div class="update">
<h2><?php p($_['title']) ?></h2>
<p><?php p($_['message']) ?></p>
<p><a class="button primary" href="<?php p(\OC::$server->get(\OCP\IURLGenerator::class)->linkTo('', 'index.php')) ?>">
<?php p($l->t('Go to %s', [$theme->getName()])); ?>
</a></p>
</div>
Loading