From 4d26a9afa01aaf069e29a62f4e9547c34156ea01 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 18 Mar 2022 20:08:07 +0100 Subject: [PATCH] Allow to tweak default scopes for accounts Close #6582 Signed-off-by: Thomas Citharel --- config/config.sample.php | 14 +++ lib/private/Accounts/AccountManager.php | 62 ++++++++----- lib/public/Accounts/IAccountManager.php | 36 ++++++++ tests/lib/Accounts/AccountManagerTest.php | 106 +++++++++++++++------- 4 files changed, 159 insertions(+), 59 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index bf6643458fa..c3a0048625c 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2168,4 +2168,18 @@ $CONFIG = [ * the database storage. */ 'enable_file_metadata' => true, + +/** + * Allows to override the default scopes for Account data. + * The list of overridable properties and valid values for scopes are in + * OCP\Accounts\IAccountManager. Values added here are merged with + * default values, which are in OC\Accounts\AccountManager + * + * For instance, if the phone property should default to the private scope + * instead of the local one: + * [ + * \OCP\Accounts\IAccountManager::PROPERTY_PHONE => \OCP\Accounts\IAccountManager::SCOPE_PRIVATE + * ] + */ +'account_manager.default_property_scope' => [] ]; diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 7f79ab46c37..b80c7887591 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -14,6 +14,7 @@ * @author Lukas Reschke * @author Morris Jobke * @author Roeland Jago Douma + * @author Thomas Citharel * @author Vincent Petry * * @license AGPL-3.0 @@ -119,6 +120,23 @@ class AccountManager implements IAccountManager { private $l10nfactory; private CappedMemoryCache $internalCache; + /** + * The list of default scopes for each property. + */ + public const DEFAULT_SCOPES = [ + self::PROPERTY_DISPLAYNAME => self::SCOPE_FEDERATED, + self::PROPERTY_ADDRESS => self::SCOPE_LOCAL, + self::PROPERTY_WEBSITE => self::SCOPE_LOCAL, + self::PROPERTY_EMAIL => self::SCOPE_FEDERATED, + self::PROPERTY_AVATAR => self::SCOPE_FEDERATED, + self::PROPERTY_PHONE => self::SCOPE_LOCAL, + self::PROPERTY_TWITTER => self::SCOPE_LOCAL, + self::PROPERTY_ORGANISATION => self::SCOPE_LOCAL, + self::PROPERTY_ROLE => self::SCOPE_LOCAL, + self::PROPERTY_HEADLINE => self::SCOPE_LOCAL, + self::PROPERTY_BIOGRAPHY => self::SCOPE_LOCAL, + ]; + public function __construct( IDBConnection $connection, IConfig $config, @@ -649,81 +667,84 @@ class AccountManager implements IAccountManager { /** * build default user record in case not data set exists yet - * - * @param IUser $user - * @return array */ - protected function buildDefaultUserRecord(IUser $user) { + protected function buildDefaultUserRecord(IUser $user): array { + $scopes = array_merge(self::DEFAULT_SCOPES, array_filter($this->config->getSystemValue('account_manager.default_property_scope', []), static function (string $scope, string $property) { + return in_array($property, self::ALLOWED_PROPERTIES, true) && in_array($scope, self::ALLOWED_SCOPES, true); + }, ARRAY_FILTER_USE_BOTH)); + return [ [ 'name' => self::PROPERTY_DISPLAYNAME, 'value' => $user->getDisplayName(), - 'scope' => self::SCOPE_FEDERATED, + // Display name must be at least SCOPE_LOCAL + 'scope' => $scopes[self::PROPERTY_DISPLAYNAME] === self::SCOPE_PRIVATE ? self::SCOPE_LOCAL : $scopes[self::PROPERTY_DISPLAYNAME], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_ADDRESS, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_ADDRESS], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_WEBSITE, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_WEBSITE], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_EMAIL, 'value' => $user->getEMailAddress(), - 'scope' => self::SCOPE_FEDERATED, + // Email must be at least SCOPE_LOCAL + 'scope' => $scopes[self::PROPERTY_EMAIL] === self::SCOPE_PRIVATE ? self::SCOPE_LOCAL : $scopes[self::PROPERTY_EMAIL], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_AVATAR, - 'scope' => self::SCOPE_FEDERATED + 'scope' => $scopes[self::PROPERTY_AVATAR], ], [ 'name' => self::PROPERTY_PHONE, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_PHONE], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_TWITTER, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_TWITTER], 'verified' => self::NOT_VERIFIED, ], [ 'name' => self::PROPERTY_ORGANISATION, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_ORGANISATION], ], [ 'name' => self::PROPERTY_ROLE, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_ROLE], ], [ 'name' => self::PROPERTY_HEADLINE, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_HEADLINE], ], [ 'name' => self::PROPERTY_BIOGRAPHY, 'value' => '', - 'scope' => self::SCOPE_LOCAL, + 'scope' => $scopes[self::PROPERTY_BIOGRAPHY], ], [ @@ -790,17 +811,8 @@ class AccountManager implements IAccountManager { // valid case, nothing to do } - static $allowedScopes = [ - self::SCOPE_PRIVATE, - self::SCOPE_LOCAL, - self::SCOPE_FEDERATED, - self::SCOPE_PUBLISHED, - self::VISIBILITY_PRIVATE, - self::VISIBILITY_CONTACTS_ONLY, - self::VISIBILITY_PUBLIC, - ]; foreach ($account->getAllProperties() as $property) { - $this->testPropertyScope($property, $allowedScopes, true); + $this->testPropertyScope($property, self::ALLOWED_SCOPES, true); } $oldData = $this->getUser($account->getUser(), false); diff --git a/lib/public/Accounts/IAccountManager.php b/lib/public/Accounts/IAccountManager.php index ae5f6b1e542..e41327171b4 100644 --- a/lib/public/Accounts/IAccountManager.php +++ b/lib/public/Accounts/IAccountManager.php @@ -8,6 +8,7 @@ declare(strict_types=1); * @author Christoph Wurst * @author Joas Schilling * @author Julius Härtl + * @author Thomas Citharel * @author Vincent Petry * * @license GNU AGPL version 3 or any later version @@ -89,6 +90,21 @@ interface IAccountManager { */ public const VISIBILITY_PUBLIC = 'public'; + /** + * The list of allowed scopes + * + * @since 25.0.0 + */ + public const ALLOWED_SCOPES = [ + self::SCOPE_PRIVATE, + self::SCOPE_LOCAL, + self::SCOPE_FEDERATED, + self::SCOPE_PUBLISHED, + self::VISIBILITY_PRIVATE, + self::VISIBILITY_CONTACTS_ONLY, + self::VISIBILITY_PUBLIC, + ]; + public const PROPERTY_AVATAR = 'avatar'; public const PROPERTY_DISPLAYNAME = 'displayname'; public const PROPERTY_PHONE = 'phone'; @@ -122,6 +138,26 @@ interface IAccountManager { */ public const PROPERTY_PROFILE_ENABLED = 'profile_enabled'; + /** + * The list of allowed properties + * + * @since 25.0.0 + */ + public const ALLOWED_PROPERTIES = [ + self::PROPERTY_AVATAR, + self::PROPERTY_DISPLAYNAME, + self::PROPERTY_PHONE, + self::PROPERTY_EMAIL, + self::PROPERTY_WEBSITE, + self::PROPERTY_ADDRESS, + self::PROPERTY_TWITTER, + self::PROPERTY_ORGANISATION, + self::PROPERTY_ROLE, + self::PROPERTY_HEADLINE, + self::PROPERTY_BIOGRAPHY, + self::PROPERTY_PROFILE_ENABLED, + ]; + public const COLLECTION_EMAIL = 'additional_mail'; public const NOT_VERIFIED = '0'; diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 69deaf17d3c..2eaec755b50 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -1,9 +1,11 @@ - * * @copyright Copyright (c) 2016, ownCloud, Inc. + * + * @author Björn Schießle + * @author Thomas Citharel + * * @license AGPL-3.0 * * This code is free software: you can redistribute it and/or modify @@ -62,26 +64,25 @@ class AccountManagerTest extends TestCase { /** @var IFactory|MockObject */ protected $l10nFactory; - /** @var \OCP\IDBConnection */ + /** @var IDBConnection */ private $connection; - /** @var IConfig|MockObject */ + /** @var IConfig|MockObject */ private $config; /** @var EventDispatcherInterface|MockObject */ private $eventDispatcher; - /** @var IJobList|MockObject */ + /** @var IJobList|MockObject */ private $jobList; - /** @var string accounts table name */ - private $table = 'accounts'; + /** accounts table name */ + private string $table = 'accounts'; /** @var LoggerInterface|MockObject */ private $logger; - /** @var AccountManager */ - private $accountManager; + private AccountManager $accountManager; protected function setUp(): void { parent::setUp(); @@ -115,7 +116,7 @@ class AccountManagerTest extends TestCase { protected function tearDown(): void { parent::tearDown(); $query = $this->connection->getQueryBuilder(); - $query->delete($this->table)->execute(); + $query->delete($this->table)->executeStatement(); } protected function makeUser(string $uid, string $name, string $email = null): IUser { @@ -423,6 +424,7 @@ class AccountManagerTest extends TestCase { ], ], ]; + $this->config->expects($this->exactly(count($users)))->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]); foreach ($users as $userInfo) { $this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], null, false]); } @@ -431,10 +433,9 @@ class AccountManagerTest extends TestCase { /** * get a instance of the accountManager * - * @param array $mockedMethods list of methods which should be mocked * @return MockObject | AccountManager */ - public function getInstance($mockedMethods = null) { + public function getInstance(?array $mockedMethods = null) { return $this->getMockBuilder(AccountManager::class) ->setConstructorArgs([ $this->connection, @@ -449,19 +450,15 @@ class AccountManagerTest extends TestCase { $this->urlGenerator, $this->crypto ]) - ->setMethods($mockedMethods) + ->onlyMethods($mockedMethods) ->getMock(); } /** * @dataProvider dataTrueFalse * - * @param array $newData - * @param array $oldData - * @param bool $insertNew - * @param bool $updateExisting */ - public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) { + public function testUpdateUser(array $newData, array $oldData, bool $insertNew, bool $updateExisting) { $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser']); /** @var IUser $user */ $user = $this->createMock(IUser::class); @@ -487,7 +484,6 @@ class AccountManagerTest extends TestCase { function ($eventName, $event) use ($user, $newData) { $this->assertSame('OC\AccountManager::userUpdated', $eventName); $this->assertInstanceOf(GenericEvent::class, $event); - /** @var GenericEvent $event */ $this->assertSame($user, $event->getSubject()); $this->assertSame($newData, $event->getArguments()); } @@ -603,6 +599,7 @@ class AccountManagerTest extends TestCase { 'value' => '1', ], ]; + $this->config->expects($this->once())->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]); $defaultUserRecord = $this->invokePrivate($this->accountManager, 'buildDefaultUserRecord', [$user]); $result = $this->invokePrivate($this->accountManager, 'addMissingDefaultValues', [$input, $defaultUserRecord]); @@ -610,18 +607,6 @@ class AccountManagerTest extends TestCase { $this->assertSame($expected, $result); } - private function addDummyValuesToTable($uid, $data) { - $query = $this->connection->getQueryBuilder(); - $query->insert($this->table) - ->values( - [ - 'uid' => $query->createNamedParameter($uid), - 'data' => $query->createNamedParameter(json_encode($data)), - ] - ) - ->execute(); - } - public function testGetAccount() { $accountManager = $this->getInstance(['getUser']); /** @var IUser $user */ @@ -668,9 +653,6 @@ class AccountManagerTest extends TestCase { /** * @dataProvider dataParsePhoneNumber - * @param string $phoneInput - * @param string $defaultRegion - * @param string|null $phoneNumber */ public function testParsePhoneNumber(string $phoneInput, string $defaultRegion, ?string $phoneNumber): void { $this->config->method('getSystemValueString') @@ -790,6 +772,8 @@ class AccountManagerTest extends TestCase { * @dataProvider dataCheckEmailVerification */ public function testCheckEmailVerification(IUser $user, ?string $newEmail): void { + // Once because of getAccount, once because of getUser + $this->config->expects($this->exactly(2))->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]); $account = $this->accountManager->getAccount($user); $emailUpdated = false; @@ -812,4 +796,58 @@ class AccountManagerTest extends TestCase { $oldData = $this->invokePrivate($this->accountManager, 'getUser', [$user, false]); $this->invokePrivate($this->accountManager, 'checkEmailVerification', [$account, $oldData]); } + + public function dataSetDefaultPropertyScopes(): array { + return [ + [ + [], + [ + IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED, + IAccountManager::PROPERTY_ADDRESS => IAccountManager::SCOPE_LOCAL, + IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_FEDERATED, + IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_LOCAL, + ] + ], + [ + [ + IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED, + IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_LOCAL, + IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE, + ], [ + IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED, + IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_LOCAL, + IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE, + ] + ], + [ + [ + IAccountManager::PROPERTY_ADDRESS => 'invalid scope', + 'invalid property' => IAccountManager::SCOPE_LOCAL, + IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE, + ], + [ + IAccountManager::PROPERTY_ADDRESS => IAccountManager::SCOPE_LOCAL, + IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_FEDERATED, + IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE, + ] + ], + ]; + } + + /** + * @dataProvider dataSetDefaultPropertyScopes + */ + public function testSetDefaultPropertyScopes(array $propertyScopes, array $expectedResultScopes): void { + $user = $this->makeUser('steve', 'Steve Smith', 'steve@steve.steve'); + $this->config->expects($this->once())->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn($propertyScopes); + + $result = $this->invokePrivate($this->accountManager, 'buildDefaultUserRecord', [$user]); + $resultProperties = array_column($result, 'name'); + + $this->assertEmpty(array_diff($resultProperties, IAccountManager::ALLOWED_PROPERTIES), "Building default user record returned non-allowed properties"); + foreach ($expectedResultScopes as $expectedResultScopeKey => $expectedResultScopeValue) { + $resultScope = $result[array_search($expectedResultScopeKey, $resultProperties)]['scope']; + $this->assertEquals($expectedResultScopeValue, $resultScope, "The result scope doesn't follow the value set into the config or defaults correctly."); + } + } }