Skip to content

Commit 4d26a9a

Browse files
tcitworldtcitworld
committedMay 16, 2022
Allow to tweak default scopes for accounts
Close #6582 Signed-off-by: Thomas Citharel <tcit@tcit.fr>
·
v31.0.2v25.0.0beta1
1 parent ab0548e commit 4d26a9a

File tree

4 files changed

+158
-58
lines changed

4 files changed

+158
-58
lines changed
 

‎config/config.sample.php

+14
Original file line numberDiff line numberDiff line change
@@ -2168,4 +2168,18 @@
21682168
* the database storage.
21692169
*/
21702170
'enable_file_metadata' => true,
2171+
2172+
/**
2173+
* Allows to override the default scopes for Account data.
2174+
* The list of overridable properties and valid values for scopes are in
2175+
* OCP\Accounts\IAccountManager. Values added here are merged with
2176+
* default values, which are in OC\Accounts\AccountManager
2177+
*
2178+
* For instance, if the phone property should default to the private scope
2179+
* instead of the local one:
2180+
* [
2181+
* \OCP\Accounts\IAccountManager::PROPERTY_PHONE => \OCP\Accounts\IAccountManager::SCOPE_PRIVATE
2182+
* ]
2183+
*/
2184+
'account_manager.default_property_scope' => []
21712185
];

‎lib/private/Accounts/AccountManager.php

+37-25
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
* @author Lukas Reschke <lukas@statuscode.ch>
1515
* @author Morris Jobke <hey@morrisjobke.de>
1616
* @author Roeland Jago Douma <roeland@famdouma.nl>
17+
* @author Thomas Citharel <nextcloud@tcit.fr>
1718
* @author Vincent Petry <vincent@nextcloud.com>
1819
*
1920
* @license AGPL-3.0
@@ -119,6 +120,23 @@ class AccountManager implements IAccountManager {
119120
private $l10nfactory;
120121
private CappedMemoryCache $internalCache;
121122

123+
/**
124+
* The list of default scopes for each property.
125+
*/
126+
public const DEFAULT_SCOPES = [
127+
self::PROPERTY_DISPLAYNAME => self::SCOPE_FEDERATED,
128+
self::PROPERTY_ADDRESS => self::SCOPE_LOCAL,
129+
self::PROPERTY_WEBSITE => self::SCOPE_LOCAL,
130+
self::PROPERTY_EMAIL => self::SCOPE_FEDERATED,
131+
self::PROPERTY_AVATAR => self::SCOPE_FEDERATED,
132+
self::PROPERTY_PHONE => self::SCOPE_LOCAL,
133+
self::PROPERTY_TWITTER => self::SCOPE_LOCAL,
134+
self::PROPERTY_ORGANISATION => self::SCOPE_LOCAL,
135+
self::PROPERTY_ROLE => self::SCOPE_LOCAL,
136+
self::PROPERTY_HEADLINE => self::SCOPE_LOCAL,
137+
self::PROPERTY_BIOGRAPHY => self::SCOPE_LOCAL,
138+
];
139+
122140
public function __construct(
123141
IDBConnection $connection,
124142
IConfig $config,
@@ -649,81 +667,84 @@ protected function writeUserDataProperties(IQueryBuilder $query, array $data): v
649667

650668
/**
651669
* build default user record in case not data set exists yet
652-
*
653-
* @param IUser $user
654-
* @return array
655670
*/
656-
protected function buildDefaultUserRecord(IUser $user) {
671+
protected function buildDefaultUserRecord(IUser $user): array {
672+
$scopes = array_merge(self::DEFAULT_SCOPES, array_filter($this->config->getSystemValue('account_manager.default_property_scope', []), static function (string $scope, string $property) {
673+
return in_array($property, self::ALLOWED_PROPERTIES, true) && in_array($scope, self::ALLOWED_SCOPES, true);
674+
}, ARRAY_FILTER_USE_BOTH));
675+
657676
return [
658677
[
659678
'name' => self::PROPERTY_DISPLAYNAME,
660679
'value' => $user->getDisplayName(),
661-
'scope' => self::SCOPE_FEDERATED,
680+
// Display name must be at least SCOPE_LOCAL
681+
'scope' => $scopes[self::PROPERTY_DISPLAYNAME] === self::SCOPE_PRIVATE ? self::SCOPE_LOCAL : $scopes[self::PROPERTY_DISPLAYNAME],
662682
'verified' => self::NOT_VERIFIED,
663683
],
664684

665685
[
666686
'name' => self::PROPERTY_ADDRESS,
667687
'value' => '',
668-
'scope' => self::SCOPE_LOCAL,
688+
'scope' => $scopes[self::PROPERTY_ADDRESS],
669689
'verified' => self::NOT_VERIFIED,
670690
],
671691

672692
[
673693
'name' => self::PROPERTY_WEBSITE,
674694
'value' => '',
675-
'scope' => self::SCOPE_LOCAL,
695+
'scope' => $scopes[self::PROPERTY_WEBSITE],
676696
'verified' => self::NOT_VERIFIED,
677697
],
678698

679699
[
680700
'name' => self::PROPERTY_EMAIL,
681701
'value' => $user->getEMailAddress(),
682-
'scope' => self::SCOPE_FEDERATED,
702+
// Email must be at least SCOPE_LOCAL
703+
'scope' => $scopes[self::PROPERTY_EMAIL] === self::SCOPE_PRIVATE ? self::SCOPE_LOCAL : $scopes[self::PROPERTY_EMAIL],
683704
'verified' => self::NOT_VERIFIED,
684705
],
685706

686707
[
687708
'name' => self::PROPERTY_AVATAR,
688-
'scope' => self::SCOPE_FEDERATED
709+
'scope' => $scopes[self::PROPERTY_AVATAR],
689710
],
690711

691712
[
692713
'name' => self::PROPERTY_PHONE,
693714
'value' => '',
694-
'scope' => self::SCOPE_LOCAL,
715+
'scope' => $scopes[self::PROPERTY_PHONE],
695716
'verified' => self::NOT_VERIFIED,
696717
],
697718

698719
[
699720
'name' => self::PROPERTY_TWITTER,
700721
'value' => '',
701-
'scope' => self::SCOPE_LOCAL,
722+
'scope' => $scopes[self::PROPERTY_TWITTER],
702723
'verified' => self::NOT_VERIFIED,
703724
],
704725

705726
[
706727
'name' => self::PROPERTY_ORGANISATION,
707728
'value' => '',
708-
'scope' => self::SCOPE_LOCAL,
729+
'scope' => $scopes[self::PROPERTY_ORGANISATION],
709730
],
710731

711732
[
712733
'name' => self::PROPERTY_ROLE,
713734
'value' => '',
714-
'scope' => self::SCOPE_LOCAL,
735+
'scope' => $scopes[self::PROPERTY_ROLE],
715736
],
716737

717738
[
718739
'name' => self::PROPERTY_HEADLINE,
719740
'value' => '',
720-
'scope' => self::SCOPE_LOCAL,
741+
'scope' => $scopes[self::PROPERTY_HEADLINE],
721742
],
722743

723744
[
724745
'name' => self::PROPERTY_BIOGRAPHY,
725746
'value' => '',
726-
'scope' => self::SCOPE_LOCAL,
747+
'scope' => $scopes[self::PROPERTY_BIOGRAPHY],
727748
],
728749

729750
[
@@ -790,17 +811,8 @@ public function updateAccount(IAccount $account): void {
790811
// valid case, nothing to do
791812
}
792813

793-
static $allowedScopes = [
794-
self::SCOPE_PRIVATE,
795-
self::SCOPE_LOCAL,
796-
self::SCOPE_FEDERATED,
797-
self::SCOPE_PUBLISHED,
798-
self::VISIBILITY_PRIVATE,
799-
self::VISIBILITY_CONTACTS_ONLY,
800-
self::VISIBILITY_PUBLIC,
801-
];
802814
foreach ($account->getAllProperties() as $property) {
803-
$this->testPropertyScope($property, $allowedScopes, true);
815+
$this->testPropertyScope($property, self::ALLOWED_SCOPES, true);
804816
}
805817

806818
$oldData = $this->getUser($account->getUser(), false);

‎lib/public/Accounts/IAccountManager.php

+36
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
* @author Christoph Wurst <christoph@winzerhof-wurst.at>
99
* @author Joas Schilling <coding@schilljs.com>
1010
* @author Julius Härtl <jus@bitgrid.net>
11+
* @author Thomas Citharel <nextcloud@tcit.fr>
1112
* @author Vincent Petry <vincent@nextcloud.com>
1213
*
1314
* @license GNU AGPL version 3 or any later version
@@ -89,6 +90,21 @@ interface IAccountManager {
8990
*/
9091
public const VISIBILITY_PUBLIC = 'public';
9192

93+
/**
94+
* The list of allowed scopes
95+
*
96+
* @since 25.0.0
97+
*/
98+
public const ALLOWED_SCOPES = [
99+
self::SCOPE_PRIVATE,
100+
self::SCOPE_LOCAL,
101+
self::SCOPE_FEDERATED,
102+
self::SCOPE_PUBLISHED,
103+
self::VISIBILITY_PRIVATE,
104+
self::VISIBILITY_CONTACTS_ONLY,
105+
self::VISIBILITY_PUBLIC,
106+
];
107+
92108
public const PROPERTY_AVATAR = 'avatar';
93109
public const PROPERTY_DISPLAYNAME = 'displayname';
94110
public const PROPERTY_PHONE = 'phone';
@@ -122,6 +138,26 @@ interface IAccountManager {
122138
*/
123139
public const PROPERTY_PROFILE_ENABLED = 'profile_enabled';
124140

141+
/**
142+
* The list of allowed properties
143+
*
144+
* @since 25.0.0
145+
*/
146+
public const ALLOWED_PROPERTIES = [
147+
self::PROPERTY_AVATAR,
148+
self::PROPERTY_DISPLAYNAME,
149+
self::PROPERTY_PHONE,
150+
self::PROPERTY_EMAIL,
151+
self::PROPERTY_WEBSITE,
152+
self::PROPERTY_ADDRESS,
153+
self::PROPERTY_TWITTER,
154+
self::PROPERTY_ORGANISATION,
155+
self::PROPERTY_ROLE,
156+
self::PROPERTY_HEADLINE,
157+
self::PROPERTY_BIOGRAPHY,
158+
self::PROPERTY_PROFILE_ENABLED,
159+
];
160+
125161
public const COLLECTION_EMAIL = 'additional_mail';
126162

127163
public const NOT_VERIFIED = '0';

‎tests/lib/Accounts/AccountManagerTest.php

+71-33
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
<?php
22

33
/**
4+
* @copyright Copyright (c) 2016, ownCloud, Inc.
5+
*
46
* @author Björn Schießle <schiessle@owncloud.com>
7+
* @author Thomas Citharel <nextcloud@tcit.fr>
58
*
6-
* @copyright Copyright (c) 2016, ownCloud, Inc.
79
* @license AGPL-3.0
810
*
911
* This code is free software: you can redistribute it and/or modify
@@ -62,26 +64,25 @@ class AccountManagerTest extends TestCase {
6264
/** @var IFactory|MockObject */
6365
protected $l10nFactory;
6466

65-
/** @var \OCP\IDBConnection */
67+
/** @var IDBConnection */
6668
private $connection;
6769

68-
/** @var IConfig|MockObject */
70+
/** @var IConfig|MockObject */
6971
private $config;
7072

7173
/** @var EventDispatcherInterface|MockObject */
7274
private $eventDispatcher;
7375

74-
/** @var IJobList|MockObject */
76+
/** @var IJobList|MockObject */
7577
private $jobList;
7678

77-
/** @var string accounts table name */
78-
private $table = 'accounts';
79+
/** accounts table name */
80+
private string $table = 'accounts';
7981

8082
/** @var LoggerInterface|MockObject */
8183
private $logger;
8284

83-
/** @var AccountManager */
84-
private $accountManager;
85+
private AccountManager $accountManager;
8586

8687
protected function setUp(): void {
8788
parent::setUp();
@@ -115,7 +116,7 @@ protected function setUp(): void {
115116
protected function tearDown(): void {
116117
parent::tearDown();
117118
$query = $this->connection->getQueryBuilder();
118-
$query->delete($this->table)->execute();
119+
$query->delete($this->table)->executeStatement();
119120
}
120121

121122
protected function makeUser(string $uid, string $name, string $email = null): IUser {
@@ -423,6 +424,7 @@ protected function populateOrUpdate(): void {
423424
],
424425
],
425426
];
427+
$this->config->expects($this->exactly(count($users)))->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]);
426428
foreach ($users as $userInfo) {
427429
$this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], null, false]);
428430
}
@@ -431,10 +433,9 @@ protected function populateOrUpdate(): void {
431433
/**
432434
* get a instance of the accountManager
433435
*
434-
* @param array $mockedMethods list of methods which should be mocked
435436
* @return MockObject | AccountManager
436437
*/
437-
public function getInstance($mockedMethods = null) {
438+
public function getInstance(?array $mockedMethods = null) {
438439
return $this->getMockBuilder(AccountManager::class)
439440
->setConstructorArgs([
440441
$this->connection,
@@ -449,19 +450,15 @@ public function getInstance($mockedMethods = null) {
449450
$this->urlGenerator,
450451
$this->crypto
451452
])
452-
->setMethods($mockedMethods)
453+
->onlyMethods($mockedMethods)
453454
->getMock();
454455
}
455456

456457
/**
457458
* @dataProvider dataTrueFalse
458459
*
459-
* @param array $newData
460-
* @param array $oldData
461-
* @param bool $insertNew
462-
* @param bool $updateExisting
463460
*/
464-
public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) {
461+
public function testUpdateUser(array $newData, array $oldData, bool $insertNew, bool $updateExisting) {
465462
$accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser']);
466463
/** @var IUser $user */
467464
$user = $this->createMock(IUser::class);
@@ -487,7 +484,6 @@ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting)
487484
function ($eventName, $event) use ($user, $newData) {
488485
$this->assertSame('OC\AccountManager::userUpdated', $eventName);
489486
$this->assertInstanceOf(GenericEvent::class, $event);
490-
/** @var GenericEvent $event */
491487
$this->assertSame($user, $event->getSubject());
492488
$this->assertSame($newData, $event->getArguments());
493489
}
@@ -603,25 +599,14 @@ public function testAddMissingDefaults() {
603599
'value' => '1',
604600
],
605601
];
602+
$this->config->expects($this->once())->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]);
606603

607604
$defaultUserRecord = $this->invokePrivate($this->accountManager, 'buildDefaultUserRecord', [$user]);
608605
$result = $this->invokePrivate($this->accountManager, 'addMissingDefaultValues', [$input, $defaultUserRecord]);
609606

610607
$this->assertSame($expected, $result);
611608
}
612609

613-
private function addDummyValuesToTable($uid, $data) {
614-
$query = $this->connection->getQueryBuilder();
615-
$query->insert($this->table)
616-
->values(
617-
[
618-
'uid' => $query->createNamedParameter($uid),
619-
'data' => $query->createNamedParameter(json_encode($data)),
620-
]
621-
)
622-
->execute();
623-
}
624-
625610
public function testGetAccount() {
626611
$accountManager = $this->getInstance(['getUser']);
627612
/** @var IUser $user */
@@ -668,9 +653,6 @@ public function dataParsePhoneNumber(): array {
668653

669654
/**
670655
* @dataProvider dataParsePhoneNumber
671-
* @param string $phoneInput
672-
* @param string $defaultRegion
673-
* @param string|null $phoneNumber
674656
*/
675657
public function testParsePhoneNumber(string $phoneInput, string $defaultRegion, ?string $phoneNumber): void {
676658
$this->config->method('getSystemValueString')
@@ -790,6 +772,8 @@ public function dataCheckEmailVerification(): array {
790772
* @dataProvider dataCheckEmailVerification
791773
*/
792774
public function testCheckEmailVerification(IUser $user, ?string $newEmail): void {
775+
// Once because of getAccount, once because of getUser
776+
$this->config->expects($this->exactly(2))->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn([]);
793777
$account = $this->accountManager->getAccount($user);
794778
$emailUpdated = false;
795779

@@ -812,4 +796,58 @@ public function testCheckEmailVerification(IUser $user, ?string $newEmail): void
812796
$oldData = $this->invokePrivate($this->accountManager, 'getUser', [$user, false]);
813797
$this->invokePrivate($this->accountManager, 'checkEmailVerification', [$account, $oldData]);
814798
}
799+
800+
public function dataSetDefaultPropertyScopes(): array {
801+
return [
802+
[
803+
[],
804+
[
805+
IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED,
806+
IAccountManager::PROPERTY_ADDRESS => IAccountManager::SCOPE_LOCAL,
807+
IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_FEDERATED,
808+
IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_LOCAL,
809+
]
810+
],
811+
[
812+
[
813+
IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED,
814+
IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_LOCAL,
815+
IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE,
816+
], [
817+
IAccountManager::PROPERTY_DISPLAYNAME => IAccountManager::SCOPE_FEDERATED,
818+
IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_LOCAL,
819+
IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE,
820+
]
821+
],
822+
[
823+
[
824+
IAccountManager::PROPERTY_ADDRESS => 'invalid scope',
825+
'invalid property' => IAccountManager::SCOPE_LOCAL,
826+
IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE,
827+
],
828+
[
829+
IAccountManager::PROPERTY_ADDRESS => IAccountManager::SCOPE_LOCAL,
830+
IAccountManager::PROPERTY_EMAIL => IAccountManager::SCOPE_FEDERATED,
831+
IAccountManager::PROPERTY_ROLE => IAccountManager::SCOPE_PRIVATE,
832+
]
833+
],
834+
];
835+
}
836+
837+
/**
838+
* @dataProvider dataSetDefaultPropertyScopes
839+
*/
840+
public function testSetDefaultPropertyScopes(array $propertyScopes, array $expectedResultScopes): void {
841+
$user = $this->makeUser('steve', 'Steve Smith', 'steve@steve.steve');
842+
$this->config->expects($this->once())->method('getSystemValue')->with('account_manager.default_property_scope', [])->willReturn($propertyScopes);
843+
844+
$result = $this->invokePrivate($this->accountManager, 'buildDefaultUserRecord', [$user]);
845+
$resultProperties = array_column($result, 'name');
846+
847+
$this->assertEmpty(array_diff($resultProperties, IAccountManager::ALLOWED_PROPERTIES), "Building default user record returned non-allowed properties");
848+
foreach ($expectedResultScopes as $expectedResultScopeKey => $expectedResultScopeValue) {
849+
$resultScope = $result[array_search($expectedResultScopeKey, $resultProperties)]['scope'];
850+
$this->assertEquals($expectedResultScopeValue, $resultScope, "The result scope doesn't follow the value set into the config or defaults correctly.");
851+
}
852+
}
815853
}

0 commit comments

Comments
 (0)
Please sign in to comment.