From b81a1c1bdbf7c7ca3e71a7c7cd7eb21edd0a3fb9 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 22 Mar 2021 12:08:53 +0100 Subject: [PATCH 01/16] Add new v2-private account scope Added new v2-private account manager scope that restricts the scope further by excluding public link access. Avatars with v2-private account scope are now showing the guest avatar instead of the real avatar. Signed-off-by: Vincent Petry --- apps/settings/js/federationscopemenu.js | 28 ++++++++-- apps/settings/js/federationsettingsview.js | 20 ++++++- lib/private/Avatar/AvatarManager.php | 36 ++++++++++++- lib/private/Server.php | 4 +- lib/public/Accounts/IAccountManager.php | 49 +++++++++++++++-- tests/lib/Avatar/AvatarManagerTest.php | 61 +++++++++++++++++++++- 6 files changed, 185 insertions(+), 13 deletions(-) diff --git a/apps/settings/js/federationscopemenu.js b/apps/settings/js/federationscopemenu.js index 170aec15a85f3..1c854a7ee574c 100644 --- a/apps/settings/js/federationscopemenu.js +++ b/apps/settings/js/federationscopemenu.js @@ -15,6 +15,8 @@ * Construct a new FederationScopeMenu instance * @constructs FederationScopeMenu * @memberof OC.Settings + * @param {object} options + * @param {array.} [options.excludedScopes] array of excluded scopes */ var FederationScopeMenu = OC.Backbone.View.extend({ tagName: 'div', @@ -26,8 +28,15 @@ this.field = options.field; this._scopes = [ { - name: 'private', + name: 'v2-private', displayName: t('settings', 'Private'), + tooltip: t('settings', "Don't show via public link"), + iconClass: 'icon-password', + active: false + }, + { + name: 'private', + displayName: t('settings', 'Local'), tooltip: t('settings', "Don't synchronize to servers"), iconClass: 'icon-password', active: false @@ -41,12 +50,18 @@ }, { name: 'public', - displayName: t('settings', 'Public'), + displayName: t('settings', 'Published'), tooltip: t('settings', 'Synchronize to trusted servers and the global and public address book'), iconClass: 'icon-link', active: false } ]; + + if (options.excludedScopes) { + this._scopes = this._scopes.filter(function(scopeEntry) { + return options.excludedScopes.indexOf(scopeEntry.name) === -1; + }) + } }, /** @@ -106,15 +121,18 @@ } switch (currentlyActiveValue) { - case 'private': + case 'v2-private': this._scopes[0].active = true; break; - case 'contacts': + case 'private': this._scopes[1].active = true; break; - case 'public': + case 'contacts': this._scopes[2].active = true; break; + case 'public': + this._scopes[3].active = true; + break; } this.render(); diff --git a/apps/settings/js/federationsettingsview.js b/apps/settings/js/federationsettingsview.js index 9cefaf132f2ab..293989baa0c3f 100644 --- a/apps/settings/js/federationsettingsview.js +++ b/apps/settings/js/federationsettingsview.js @@ -61,9 +61,26 @@ render: function() { var self = this; + var fieldsWithV2Private = [ + 'avatar', + 'phone', + 'twitter', + 'website', + 'address', + ]; + _.each(this._inputFields, function(field) { var $icon = self.$('#' + field + 'form h3 > .federation-menu'); - var scopeMenu = new OC.Settings.FederationScopeMenu({field: field}); + var excludedScopes = null + + if (fieldsWithV2Private.indexOf(field) === -1) { + excludedScopes = ['v2-private'] + } + + var scopeMenu = new OC.Settings.FederationScopeMenu({ + field: field, + excludedScopes: excludedScopes, + }); self.listenTo(scopeMenu, 'select:scope', function(scope) { self._onScopeChanged(field, scope); @@ -208,6 +225,7 @@ switch (scope) { case 'private': + case 'v2-private': $icon.addClass('icon-password'); $icon.removeClass('hidden'); break; diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 5102396224d87..03f3d89e5f6c6 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -36,6 +36,7 @@ use OC\User\Manager; use OC\User\NoUserException; +use OCP\Accounts\IAccountManager; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; @@ -44,12 +45,16 @@ use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; +use OCP\IUserSession; /** * This class implements methods to access Avatar functionality */ class AvatarManager implements IAvatarManager { + /** @var IUserSession */ + private $userSession; + /** @var Manager */ private $userManager; @@ -65,6 +70,9 @@ class AvatarManager implements IAvatarManager { /** @var IConfig */ private $config; + /** @var IAccountManager */ + private $accountManager; + /** * AvatarManager constructor. * @@ -73,18 +81,23 @@ class AvatarManager implements IAvatarManager { * @param IL10N $l * @param ILogger $logger * @param IConfig $config + * @param IUserSession $userSession */ public function __construct( + IUserSession $userSession, Manager $userManager, IAppData $appData, IL10N $l, ILogger $logger, - IConfig $config) { + IConfig $config, + IAccountManager $accountManager) { + $this->userSession = $userSession; $this->userManager = $userManager; $this->appData = $appData; $this->l = $l; $this->logger = $logger; $this->config = $config; + $this->accountManager = $accountManager; } /** @@ -104,6 +117,27 @@ public function getAvatar(string $userId) : IAvatar { // sanitize userID - fixes casing issue (needed for the filesystem stuff that is done below) $userId = $user->getUID(); + $requestingUser = null; + if ($this->userSession !== null) { + $requestingUser = $this->userSession->getUser(); + } + + $canShowRealAvatar = true; + + // requesting in public page + if ($requestingUser === null) { + $account = $this->accountManager->getAccount($user); + $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); + $avatarScope = $avatarProperties->getScope(); + + // v2-private scope hides the avatar from public access + if ($avatarScope === IAccountManager::SCOPE_PRIVATE) { + // FIXME: guest avatar is re-generated every time, use a cache instead + // see how UserAvatar caches the generated one + return $this->getGuestAvatar($userId); + } + } + try { $folder = $this->appData->getFolder($userId); } catch (NotFoundException $e) { diff --git a/lib/private/Server.php b/lib/private/Server.php index c0d6afbaaf6ef..93ad3b389972c 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -720,11 +720,13 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerService(AvatarManager::class, function (Server $c) { return new AvatarManager( + $c->get(IUserSession::class), $c->get(\OC\User\Manager::class), $c->getAppDataDir('avatar'), $c->getL10N('lib'), $c->get(ILogger::class), - $c->get(\OCP\IConfig::class) + $c->get(\OCP\IConfig::class), + $c->get(IAccountManager::class) ); }); $this->registerAlias(IAvatarManager::class, AvatarManager::class); diff --git a/lib/public/Accounts/IAccountManager.php b/lib/public/Accounts/IAccountManager.php index ae70d8963b47e..9d720ba9e5061 100644 --- a/lib/public/Accounts/IAccountManager.php +++ b/lib/public/Accounts/IAccountManager.php @@ -38,11 +38,54 @@ */ interface IAccountManager { - /** nobody can see my account details */ + /** + * Contact details visible locally only + * + * @since 21.0.1 + */ + public const SCOPE_PRIVATE = 'v2-private'; + + /** + * Contact details visible locally and through public link access on local instance + * + * @since 21.0.1 + */ + public const SCOPE_LOCAL = 'private'; + + /** + * Contact details visible locally, through public link access and on trusted federated servers. + * + * @since 21.0.1 + */ + public const SCOPE_FEDERATED = 'federated'; + + /** + * Contact details visible locally, through public link access, on trusted federated servers + * and published to the public lookup server. + * + * @since 21.0.1 + */ + public const SCOPE_PUBLISHED = 'public'; + + /** + * Contact details only visible locally + * + * @deprecated 21.0.1 + */ public const VISIBILITY_PRIVATE = 'private'; - /** only contacts, especially trusted servers can see my contact details */ + + /** + * Contact details visible on trusted federated servers. + * + * @deprecated 21.0.1 + */ public const VISIBILITY_CONTACTS_ONLY = 'contacts'; - /** every body ca see my contact detail, will be published to the lookup server */ + + /** + * Contact details visible on trusted federated servers and in the public lookup server. + * + * @deprecated 21.0.1 + */ public const VISIBILITY_PUBLIC = 'public'; public const PROPERTY_AVATAR = 'avatar'; diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 5a061cd10e9e4..0ce0e75256947 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -25,19 +25,26 @@ namespace Test\Avatar; use OC\Avatar\AvatarManager; +use OC\Avatar\GuestAvatar; use OC\Avatar\UserAvatar; use OC\User\Manager; +use OCP\Accounts\IAccount; +use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; use OCP\Files\IAppData; use OCP\Files\SimpleFS\ISimpleFolder; use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; use OCP\IUser; +use OCP\IUserSession; /** * Class AvatarManagerTest */ class AvatarManagerTest extends \Test\TestCase { + /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ + private $userSession; /** @var Manager|\PHPUnit\Framework\MockObject\MockObject */ private $userManager; /** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */ @@ -48,28 +55,33 @@ class AvatarManagerTest extends \Test\TestCase { private $logger; /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ private $config; + /** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */ + private $accountManager; /** @var AvatarManager | \PHPUnit\Framework\MockObject\MockObject */ private $avatarManager; protected function setUp(): void { parent::setUp(); + $this->userSession = $this->createMock(IUserSession::class); $this->userManager = $this->createMock(Manager::class); $this->appData = $this->createMock(IAppData::class); $this->l10n = $this->createMock(IL10N::class); $this->logger = $this->createMock(ILogger::class); $this->config = $this->createMock(IConfig::class); + $this->accountManager = $this->createMock(IAccountManager::class); $this->avatarManager = new AvatarManager( + $this->userSession, $this->userManager, $this->appData, $this->l10n, $this->logger, - $this->config + $this->config, + $this->accountManager ); } - public function testGetAvatarInvalidUser() { $this->expectException(\Exception::class); $this->expectExceptionMessage('user does not exist'); @@ -84,6 +96,15 @@ public function testGetAvatarInvalidUser() { } public function testGetAvatarValidUser() { + // requesting user + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($this->createMock(IUser::class)); + + // we skip account scope check for logged in user + $this->accountManager->expects($this->never()) + ->method('getAccount'); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) @@ -126,4 +147,40 @@ public function testGetAvatarValidUserDifferentCasing() { $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); $this->assertEquals($expected, $this->avatarManager->getAvatar('vaLid-USER')); } + + public function testGetAvatarPrivateScope() { + $user = $this->createMock(IUser::class); + $user + ->expects($this->once()) + ->method('getUID') + ->willReturn('valid-user'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('valid-user') + ->willReturn($user); + $folder = $this->createMock(ISimpleFolder::class); + $this->appData + ->expects($this->never()) + ->method('getFolder'); + + $account = $this->createMock(IAccount::class); + $this->accountManager->expects($this->once()) + ->method('getAccount') + ->with($user) + ->willReturn($account); + + $property = $this->createMock(IAccountProperty::class); + $account->expects($this->once()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_AVATAR) + ->willReturn($property); + + $property->expects($this->once()) + ->method('getScope') + ->willReturn(IAccountManager::SCOPE_PRIVATE); + + $expected = new GuestAvatar('valid-user', $this->createMock(ILogger::class)); + $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); + } } From 9fb447ea427428c86a8d6be77d0145f4fb0ed777 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 23 Mar 2021 11:38:25 +0100 Subject: [PATCH 02/16] Adjust scopes menu based on conditions Now not all fields have the "v2-private" option in place. Fix dropdown issue when a scope was stored that is not listed after disabling the lookup server. Whenever the lookup server upload is disabled, the scope menu is now displayed where it makes sense to allow switching between the two private scopes. Signed-off-by: Vincent Petry --- apps/settings/js/federationscopemenu.js | 23 +++++-------------- apps/settings/js/federationsettingsview.js | 17 ++++++++++++-- apps/settings/js/settings/personalInfo.js | 6 +++-- .../settings/personal/personal.info.php | 16 +++---------- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/apps/settings/js/federationscopemenu.js b/apps/settings/js/federationscopemenu.js index 1c854a7ee574c..b94e1686a4e8b 100644 --- a/apps/settings/js/federationscopemenu.js +++ b/apps/settings/js/federationscopemenu.js @@ -57,7 +57,7 @@ } ]; - if (options.excludedScopes) { + if (options.excludedScopes && options.excludedScopes.length) { this._scopes = this._scopes.filter(function(scopeEntry) { return options.excludedScopes.indexOf(scopeEntry.name) === -1; }) @@ -117,22 +117,11 @@ var currentlyActiveValue = $('#'+context.target.closest('form').id).find('input[type="hidden"]')[0].value; for(var i in this._scopes) { - this._scopes[i].active = false; - } - - switch (currentlyActiveValue) { - case 'v2-private': - this._scopes[0].active = true; - break; - case 'private': - this._scopes[1].active = true; - break; - case 'contacts': - this._scopes[2].active = true; - break; - case 'public': - this._scopes[3].active = true; - break; + if (this._scopes[i].name === currentlyActiveValue) { + this._scopes[i].active = true; + } else { + this._scopes[i].active = false; + } } this.render(); diff --git a/apps/settings/js/federationsettingsview.js b/apps/settings/js/federationsettingsview.js index 293989baa0c3f..759bf85c3e1b8 100644 --- a/apps/settings/js/federationsettingsview.js +++ b/apps/settings/js/federationsettingsview.js @@ -10,6 +10,13 @@ (function(_, $, OC) { 'use strict'; + /** + * Construct a new FederationScopeMenu instance + * @constructs FederationScopeMenu + * @memberof OC.Settings + * @param {object} options + * @param {bool} [options.lookupServerUploadEnabled=false] whether uploading to the lookup server is enabled + */ var FederationSettingsView = OC.Backbone.View.extend({ _inputFields: undefined, @@ -24,6 +31,7 @@ } else { this._config = new OC.Settings.UserSettings(); } + this.showFederationScopes = !!options.showFederationScopes; this._inputFields = [ 'displayname', @@ -71,10 +79,15 @@ _.each(this._inputFields, function(field) { var $icon = self.$('#' + field + 'form h3 > .federation-menu'); - var excludedScopes = null + var excludedScopes = [] if (fieldsWithV2Private.indexOf(field) === -1) { - excludedScopes = ['v2-private'] + excludedScopes.push('v2-private'); + } + + if (!self.showFederationScopes) { + excludedScopes.push('contacts'); + excludedScopes.push('public'); } var scopeMenu = new OC.Settings.FederationScopeMenu({ diff --git a/apps/settings/js/settings/personalInfo.js b/apps/settings/js/settings/personalInfo.js index a6055fd7a9490..e71f484012356 100644 --- a/apps/settings/js/settings/personalInfo.js +++ b/apps/settings/js/settings/personalInfo.js @@ -199,10 +199,12 @@ window.addEventListener('DOMContentLoaded', function () { }); + var settingsEl = $('#personal-settings') var userSettings = new OC.Settings.UserSettings(); var federationSettingsView = new OC.Settings.FederationSettingsView({ - el: '#personal-settings', - config: userSettings + el: settingsEl, + config: userSettings, + showFederationScopes: !!settingsEl.data('lookup-server-upload-enabled'), }); userSettings.on("sync", function() { diff --git a/apps/settings/templates/settings/personal/personal.info.php b/apps/settings/templates/settings/personal/personal.info.php index 84198b3c0c43e..60db8c8533343 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -34,8 +34,8 @@ ]); ?> -
-
+
+

@@ -68,9 +68,7 @@

- -
@@ -125,7 +123,7 @@ - +
@@ -198,9 +196,7 @@ autocomplete="on" autocapitalize="none" autocorrect="off" /> - -
@@ -223,9 +219,7 @@ autocomplete="on" autocapitalize="none" autocorrect="off" /> - -
@@ -279,9 +273,7 @@ /> - -
@@ -335,9 +327,7 @@ /> - - From 278a73789e777d2ba00fb7d8b311923a590f10fc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 23 Mar 2021 14:47:10 +0100 Subject: [PATCH 03/16] Map old account scope properties to new names Use new scope values in settings page. Adjust all consumers to use the new constants. Map old scope values to new ones in account property getter. Signed-off-by: Vincent Petry --- apps/dav/lib/CardDAV/Converter.php | 4 +- apps/dav/tests/unit/CardDAV/ConverterTest.php | 14 +++--- .../tests/unit/CardDAV/SyncServiceTest.php | 14 +++--- .../lib/Controller/ShareController.php | 2 +- .../tests/Controller/ShareControllerTest.php | 8 +-- .../lib/BackgroundJobs/RetryJob.php | 2 +- apps/settings/js/federationscopemenu.js | 8 +-- apps/settings/js/federationsettingsview.js | 10 ++-- .../lib/Settings/Personal/PersonalInfo.php | 43 ++++++++-------- .../tests/Controller/UsersControllerTest.php | 28 +++++------ lib/private/Accounts/AccountManager.php | 16 +++--- lib/private/Accounts/AccountProperty.php | 22 ++++++++- lib/public/Accounts/IAccountManager.php | 6 +-- tests/lib/Accounts/AccountManagerTest.php | 12 ++--- tests/lib/Accounts/AccountPropertyTest.php | 49 +++++++++++++++---- tests/lib/Accounts/AccountTest.php | 36 +++++++------- 16 files changed, 161 insertions(+), 113 deletions(-) diff --git a/apps/dav/lib/CardDAV/Converter.php b/apps/dav/lib/CardDAV/Converter.php index 59e5401d058a8..95ac43aba3649 100644 --- a/apps/dav/lib/CardDAV/Converter.php +++ b/apps/dav/lib/CardDAV/Converter.php @@ -71,8 +71,8 @@ public function createCardFromUser(IUser $user) { foreach ($userData as $property => $value) { $shareWithTrustedServers = - $value['scope'] === AccountManager::VISIBILITY_CONTACTS_ONLY || - $value['scope'] === AccountManager::VISIBILITY_PUBLIC; + $value['scope'] === AccountManager::SCOPE_FEDERATED || + $value['scope'] === AccountManager::SCOPE_PUBLISHED; $emptyValue = !isset($value['value']) || $value['value'] === ''; diff --git a/apps/dav/tests/unit/CardDAV/ConverterTest.php b/apps/dav/tests/unit/CardDAV/ConverterTest.php index aef5cf8ef1c42..43344451437c1 100644 --- a/apps/dav/tests/unit/CardDAV/ConverterTest.php +++ b/apps/dav/tests/unit/CardDAV/ConverterTest.php @@ -53,36 +53,36 @@ public function getAccountManager(IUser $user) { IAccountManager::PROPERTY_DISPLAYNAME => [ 'value' => $user->getDisplayName(), - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, ], IAccountManager::PROPERTY_ADDRESS => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_WEBSITE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_EMAIL => [ 'value' => $user->getEMailAddress(), - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, ], IAccountManager::PROPERTY_AVATAR => [ - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY + 'scope' => AccountManager::SCOPE_FEDERATED ], IAccountManager::PROPERTY_PHONE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_TWITTER => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], ] ); diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index eb8186807c68b..724670bc98687 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -136,36 +136,36 @@ public function testUpdateAndDeleteUser($activated, $createCalls, $updateCalls, IAccountManager::PROPERTY_DISPLAYNAME => [ 'value' => $user->getDisplayName(), - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, ], IAccountManager::PROPERTY_ADDRESS => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_WEBSITE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_EMAIL => [ 'value' => $user->getEMailAddress(), - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, ], IAccountManager::PROPERTY_AVATAR => [ - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY + 'scope' => AccountManager::SCOPE_FEDERATED ], IAccountManager::PROPERTY_PHONE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], IAccountManager::PROPERTY_TWITTER => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, ], ] ); diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 31f13ee275692..7e83ffaa7dc97 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -343,7 +343,7 @@ public function showShare($path = ''): TemplateResponse { $ownerAccount = $this->accountManager->getAccount($owner); $ownerName = $ownerAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME); - if ($ownerName->getScope() === IAccountManager::VISIBILITY_PUBLIC) { + if ($ownerName->getScope() === IAccountManager::SCOPE_PUBLISHED) { $shareTmpl['owner'] = $owner->getUID(); $shareTmpl['shareOwner'] = $owner->getDisplayName(); } diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index 270f38a114899..e00d6bc8c662e 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -234,7 +234,7 @@ public function testShowShare() { $accountName = $this->createMock(IAccountProperty::class); $accountName->method('getScope') - ->willReturn(IAccountManager::VISIBILITY_PUBLIC); + ->willReturn(IAccountManager::SCOPE_PUBLISHED); $account = $this->createMock(IAccount::class); $account->method('getProperty') ->with(IAccountManager::PROPERTY_DISPLAYNAME) @@ -381,7 +381,7 @@ public function testShowShareWithPrivateName() { $accountName = $this->createMock(IAccountProperty::class); $accountName->method('getScope') - ->willReturn(IAccountManager::VISIBILITY_PRIVATE); + ->willReturn(IAccountManager::SCOPE_LOCAL); $account = $this->createMock(IAccount::class); $account->method('getProperty') ->with(IAccountManager::PROPERTY_DISPLAYNAME) @@ -528,7 +528,7 @@ public function testShowShareHideDownload() { $accountName = $this->createMock(IAccountProperty::class); $accountName->method('getScope') - ->willReturn(IAccountManager::VISIBILITY_PUBLIC); + ->willReturn(IAccountManager::SCOPE_PUBLISHED); $account = $this->createMock(IAccount::class); $account->method('getProperty') ->with(IAccountManager::PROPERTY_DISPLAYNAME) @@ -688,7 +688,7 @@ public function testShareFileDrop() { $accountName = $this->createMock(IAccountProperty::class); $accountName->method('getScope') - ->willReturn(IAccountManager::VISIBILITY_PUBLIC); + ->willReturn(IAccountManager::SCOPE_PUBLISHED); $account = $this->createMock(IAccount::class); $account->method('getProperty') ->with(IAccountManager::PROPERTY_DISPLAYNAME) diff --git a/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php b/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php index 889fcfd627793..c462eeedb430a 100644 --- a/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php +++ b/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php @@ -193,7 +193,7 @@ protected function getUserAccountData(IUser $user): array { $publicData = []; foreach ($account->getProperties() as $property) { - if ($property->getScope() === IAccountManager::VISIBILITY_PUBLIC) { + if ($property->getScope() === IAccountManager::SCOPE_PUBLISHED) { $publicData[$property->getName()] = $property->getValue(); } } diff --git a/apps/settings/js/federationscopemenu.js b/apps/settings/js/federationscopemenu.js index b94e1686a4e8b..617a8e3d4121e 100644 --- a/apps/settings/js/federationscopemenu.js +++ b/apps/settings/js/federationscopemenu.js @@ -35,21 +35,21 @@ active: false }, { - name: 'private', + name: 'v2-local', displayName: t('settings', 'Local'), tooltip: t('settings', "Don't synchronize to servers"), iconClass: 'icon-password', active: false }, { - name: 'contacts', - displayName: t('settings', 'Trusted'), + name: 'v2-federated', + displayName: t('settings', 'Federated'), tooltip: t('settings', 'Only synchronize to trusted servers'), iconClass: 'icon-contacts-dark', active: false }, { - name: 'public', + name: 'v2-published', displayName: t('settings', 'Published'), tooltip: t('settings', 'Synchronize to trusted servers and the global and public address book'), iconClass: 'icon-link', diff --git a/apps/settings/js/federationsettingsview.js b/apps/settings/js/federationsettingsview.js index 759bf85c3e1b8..cf7f4648905fe 100644 --- a/apps/settings/js/federationsettingsview.js +++ b/apps/settings/js/federationsettingsview.js @@ -86,8 +86,8 @@ } if (!self.showFederationScopes) { - excludedScopes.push('contacts'); - excludedScopes.push('public'); + excludedScopes.push('v2-federated'); + excludedScopes.push('v2-published'); } var scopeMenu = new OC.Settings.FederationScopeMenu({ @@ -237,16 +237,16 @@ $icon.addClass('hidden'); switch (scope) { - case 'private': case 'v2-private': + case 'v2-local': $icon.addClass('icon-password'); $icon.removeClass('hidden'); break; - case 'contacts': + case 'v2-federated': $icon.addClass('icon-contacts-dark'); $icon.removeClass('hidden'); break; - case 'public': + case 'v2-published': $icon.addClass('icon-link'); $icon.removeClass('hidden'); break; diff --git a/apps/settings/lib/Settings/Personal/PersonalInfo.php b/apps/settings/lib/Settings/Personal/PersonalInfo.php index a853846fadd2e..7a0253d2be481 100644 --- a/apps/settings/lib/Settings/Personal/PersonalInfo.php +++ b/apps/settings/lib/Settings/Personal/PersonalInfo.php @@ -37,6 +37,7 @@ use OC\Accounts\AccountManager; use OCA\FederatedFileSharing\FederatedShareProvider; +use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\AppFramework\Http\TemplateResponse; @@ -96,7 +97,7 @@ public function getForm(): TemplateResponse { $uid = \OC_User::getUser(); $user = $this->userManager->get($uid); - $userData = $this->accountManager->getUser($user); + $account = $this->accountManager->getAccount($user); // make sure FS is setup before querying storage related stuff... \OC_Util::setupFS($user->getUID()); @@ -110,7 +111,7 @@ public function getForm(): TemplateResponse { $languageParameters = $this->getLanguages($user); $localeParameters = $this->getLocales($user); - $messageParameters = $this->getMessageParameters($userData); + $messageParameters = $this->getMessageParameters($account); $parameters = [ 'total_space' => $totalSpace, @@ -119,23 +120,23 @@ public function getForm(): TemplateResponse { 'quota' => $storageInfo['quota'], 'avatarChangeSupported' => $user->canChangeAvatar(), 'lookupServerUploadEnabled' => $lookupServerUploadEnabled, - 'avatarScope' => $userData[IAccountManager::PROPERTY_AVATAR]['scope'], + 'avatarScope' => $account->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(), 'displayNameChangeSupported' => $user->canChangeDisplayName(), - 'displayName' => $userData[IAccountManager::PROPERTY_DISPLAYNAME]['value'], - 'displayNameScope' => $userData[IAccountManager::PROPERTY_DISPLAYNAME]['scope'], - 'email' => $userData[IAccountManager::PROPERTY_EMAIL]['value'], - 'emailScope' => $userData[IAccountManager::PROPERTY_EMAIL]['scope'], - 'emailVerification' => $userData[IAccountManager::PROPERTY_EMAIL]['verified'], - 'phone' => $userData[IAccountManager::PROPERTY_PHONE]['value'], - 'phoneScope' => $userData[IAccountManager::PROPERTY_PHONE]['scope'], - 'address' => $userData[IAccountManager::PROPERTY_ADDRESS]['value'], - 'addressScope' => $userData[IAccountManager::PROPERTY_ADDRESS]['scope'], - 'website' => $userData[IAccountManager::PROPERTY_WEBSITE]['value'], - 'websiteScope' => $userData[IAccountManager::PROPERTY_WEBSITE]['scope'], - 'websiteVerification' => $userData[IAccountManager::PROPERTY_WEBSITE]['verified'], - 'twitter' => $userData[IAccountManager::PROPERTY_TWITTER]['value'], - 'twitterScope' => $userData[IAccountManager::PROPERTY_TWITTER]['scope'], - 'twitterVerification' => $userData[IAccountManager::PROPERTY_TWITTER]['verified'], + 'displayName' => $account->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getValue(), + 'displayNameScope' => $account->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(), + 'email' => $account->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue(), + 'emailScope' => $account->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(), + 'emailVerification' => $account->getProperty(IAccountManager::PROPERTY_EMAIL)->getVerified(), + 'phone' => $account->getProperty(IAccountManager::PROPERTY_PHONE)->getValue(), + 'phoneScope' => $account->getProperty(IAccountManager::PROPERTY_PHONE)->getScope(), + 'address' => $account->getProperty(IAccountManager::PROPERTY_ADDRESS)->getValue(), + 'addressScope' => $account->getProperty(IAccountManager::PROPERTY_ADDRESS)->getScope(), + 'website' => $account->getProperty(IAccountManager::PROPERTY_WEBSITE)->getValue(), + 'websiteScope' => $account->getProperty(IAccountManager::PROPERTY_WEBSITE)->getScope(), + 'websiteVerification' => $account->getProperty(IAccountManager::PROPERTY_WEBSITE)->getVerified(), + 'twitter' => $account->getProperty(IAccountManager::PROPERTY_TWITTER)->getValue(), + 'twitterScope' => $account->getProperty(IAccountManager::PROPERTY_TWITTER)->getScope(), + 'twitterVerification' => $account->getProperty(IAccountManager::PROPERTY_TWITTER)->getVerified(), 'groups' => $this->getGroups($user), ] + $messageParameters + $languageParameters + $localeParameters; @@ -263,14 +264,14 @@ private function getLocales(IUser $user): array { } /** - * @param array $userData + * @param IAccount $account * @return array */ - private function getMessageParameters(array $userData): array { + private function getMessageParameters(IAccount $account): array { $needVerifyMessage = [IAccountManager::PROPERTY_EMAIL, IAccountManager::PROPERTY_WEBSITE, IAccountManager::PROPERTY_TWITTER]; $messageParameters = []; foreach ($needVerifyMessage as $property) { - switch ($userData[$property]['verified']) { + switch ($account->getProperty($property)->getVerified()) { case AccountManager::VERIFIED: $message = $this->l->t('Verifying'); break; diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index b14e8d00d60bd..2daf383410eba 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -208,41 +208,41 @@ public function testSetUserSettings($email, $validEmail, $expectedStatus) { IAccountManager::PROPERTY_DISPLAYNAME => [ 'value' => 'Display name', - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, 'verified' => AccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_ADDRESS => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, 'verified' => AccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_WEBSITE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, 'verified' => AccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_EMAIL => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => AccountManager::SCOPE_FEDERATED, 'verified' => AccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_AVATAR => [ - 'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY + 'scope' => AccountManager::SCOPE_FEDERATED ], IAccountManager::PROPERTY_PHONE => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, 'verified' => AccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_TWITTER => [ 'value' => '', - 'scope' => AccountManager::VISIBILITY_PRIVATE, + 'scope' => AccountManager::SCOPE_LOCAL, 'verified' => AccountManager::NOT_VERIFIED, ], ]); @@ -255,19 +255,19 @@ public function testSetUserSettings($email, $validEmail, $expectedStatus) { } $result = $controller->setUserSettings(// - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, 'displayName', - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, '47658468', - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, $email, - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, 'nextcloud.com', - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, 'street and city', - AccountManager::VISIBILITY_CONTACTS_ONLY, + AccountManager::SCOPE_FEDERATED, '@nextclouders', - AccountManager::VISIBILITY_CONTACTS_ONLY + AccountManager::SCOPE_FEDERATED ); $this->assertSame($expectedStatus, $result->getStatus()); diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index c5a0f21319e89..ff3b04d83955c 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -421,41 +421,41 @@ protected function buildDefaultUserRecord(IUser $user) { self::PROPERTY_DISPLAYNAME => [ 'value' => $user->getDisplayName(), - 'scope' => self::VISIBILITY_CONTACTS_ONLY, + 'scope' => self::SCOPE_FEDERATED, 'verified' => self::NOT_VERIFIED, ], self::PROPERTY_ADDRESS => [ 'value' => '', - 'scope' => self::VISIBILITY_PRIVATE, + 'scope' => self::SCOPE_LOCAL, 'verified' => self::NOT_VERIFIED, ], self::PROPERTY_WEBSITE => [ 'value' => '', - 'scope' => self::VISIBILITY_PRIVATE, + 'scope' => self::SCOPE_LOCAL, 'verified' => self::NOT_VERIFIED, ], self::PROPERTY_EMAIL => [ 'value' => $user->getEMailAddress(), - 'scope' => self::VISIBILITY_CONTACTS_ONLY, + 'scope' => self::SCOPE_FEDERATED, 'verified' => self::NOT_VERIFIED, ], self::PROPERTY_AVATAR => [ - 'scope' => self::VISIBILITY_CONTACTS_ONLY + 'scope' => self::SCOPE_FEDERATED ], self::PROPERTY_PHONE => [ 'value' => '', - 'scope' => self::VISIBILITY_PRIVATE, + 'scope' => self::SCOPE_LOCAL, 'verified' => self::NOT_VERIFIED, ], self::PROPERTY_TWITTER => [ 'value' => '', - 'scope' => self::VISIBILITY_PRIVATE, + 'scope' => self::SCOPE_LOCAL, 'verified' => self::NOT_VERIFIED, ], ]; @@ -464,7 +464,7 @@ protected function buildDefaultUserRecord(IUser $user) { private function parseAccountData(IUser $user, $data): Account { $account = new Account($user); foreach ($data as $property => $accountData) { - $account->setProperty($property, $accountData['value'] ?? '', $accountData['scope'] ?? self::VISIBILITY_PRIVATE, $accountData['verified'] ?? self::NOT_VERIFIED); + $account->setProperty($property, $accountData['value'] ?? '', $accountData['scope'] ?? self::SCOPE_LOCAL, $accountData['verified'] ?? self::NOT_VERIFIED); } return $account; } diff --git a/lib/private/Accounts/AccountProperty.php b/lib/private/Accounts/AccountProperty.php index 97f9b1c356f17..4c75ad85414a5 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -26,6 +26,7 @@ namespace OC\Accounts; +use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; class AccountProperty implements IAccountProperty { @@ -42,7 +43,7 @@ class AccountProperty implements IAccountProperty { public function __construct(string $name, string $value, string $scope, string $verified) { $this->name = $name; $this->value = $value; - $this->scope = $scope; + $this->scope = $this->mapScopeToV2($scope); $this->verified = $verified; } @@ -77,7 +78,7 @@ public function setValue(string $value): IAccountProperty { * @return IAccountProperty */ public function setScope(string $scope): IAccountProperty { - $this->scope = $scope; + $this->scope = $this->mapScopeToV2($scope); return $this; } @@ -127,6 +128,23 @@ public function getScope(): string { return $this->scope; } + private function mapScopeToV2($scope) { + if (strpos($scope, 'v2-') === 0) { + return $scope; + } + + switch ($scope) { + case IAccountManager::VISIBILITY_PRIVATE: + return IAccountManager::SCOPE_LOCAL; + case IAccountManager::VISIBILITY_CONTACTS_ONLY: + return IAccountManager::SCOPE_FEDERATED; + case IAccountManager::VISIBILITY_PUBLIC: + return IAccountManager::SCOPE_PUBLISHED; + } + + return IAccountManager::SCOPE_LOCAL; + } + /** * Get the verification status of a property * diff --git a/lib/public/Accounts/IAccountManager.php b/lib/public/Accounts/IAccountManager.php index 9d720ba9e5061..e88fd32f6741e 100644 --- a/lib/public/Accounts/IAccountManager.php +++ b/lib/public/Accounts/IAccountManager.php @@ -50,14 +50,14 @@ interface IAccountManager { * * @since 21.0.1 */ - public const SCOPE_LOCAL = 'private'; + public const SCOPE_LOCAL = 'v2-local'; /** * Contact details visible locally, through public link access and on trusted federated servers. * * @since 21.0.1 */ - public const SCOPE_FEDERATED = 'federated'; + public const SCOPE_FEDERATED = 'v2-federated'; /** * Contact details visible locally, through public link access, on trusted federated servers @@ -65,7 +65,7 @@ interface IAccountManager { * * @since 21.0.1 */ - public const SCOPE_PUBLISHED = 'public'; + public const SCOPE_PUBLISHED = 'v2-published'; /** * Contact details only visible locally diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index fcd1a78add71a..62da1cbc1daae 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -278,26 +278,26 @@ public function testGetAccount() { IAccountManager::PROPERTY_TWITTER => [ 'value' => '@twitterhandle', - 'scope' => IAccountManager::VISIBILITY_PRIVATE, + 'scope' => IAccountManager::SCOPE_LOCAL, 'verified' => IAccountManager::NOT_VERIFIED, ], IAccountManager::PROPERTY_EMAIL => [ 'value' => 'test@example.com', - 'scope' => IAccountManager::VISIBILITY_PUBLIC, + 'scope' => IAccountManager::SCOPE_PUBLISHED, 'verified' => IAccountManager::VERIFICATION_IN_PROGRESS, ], IAccountManager::PROPERTY_WEBSITE => [ 'value' => 'https://example.com', - 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY, + 'scope' => IAccountManager::SCOPE_FEDERATED, 'verified' => IAccountManager::VERIFIED, ], ]; $expected = new Account($user); - $expected->setProperty(IAccountManager::PROPERTY_TWITTER, '@twitterhandle', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::NOT_VERIFIED); - $expected->setProperty(IAccountManager::PROPERTY_EMAIL, 'test@example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::VERIFICATION_IN_PROGRESS); - $expected->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_CONTACTS_ONLY, IAccountManager::VERIFIED); + $expected->setProperty(IAccountManager::PROPERTY_TWITTER, '@twitterhandle', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED); + $expected->setProperty(IAccountManager::PROPERTY_EMAIL, 'test@example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFICATION_IN_PROGRESS); + $expected->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_FEDERATED, IAccountManager::VERIFIED); $accountManager->expects($this->once()) ->method('getUser') diff --git a/tests/lib/Accounts/AccountPropertyTest.php b/tests/lib/Accounts/AccountPropertyTest.php index afd807a44b499..f99abc21f833a 100644 --- a/tests/lib/Accounts/AccountPropertyTest.php +++ b/tests/lib/Accounts/AccountPropertyTest.php @@ -37,12 +37,12 @@ public function testConstructor() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', - IAccountManager::VISIBILITY_PUBLIC, + IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED ); $this->assertEquals(IAccountManager::PROPERTY_WEBSITE, $accountProperty->getName()); $this->assertEquals('https://example.com', $accountProperty->getValue()); - $this->assertEquals(IAccountManager::VISIBILITY_PUBLIC, $accountProperty->getScope()); + $this->assertEquals(IAccountManager::SCOPE_PUBLISHED, $accountProperty->getScope()); $this->assertEquals(IAccountManager::VERIFIED, $accountProperty->getVerified()); } @@ -50,7 +50,7 @@ public function testSetValue() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', - IAccountManager::VISIBILITY_PUBLIC, + IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED ); $actualReturn = $accountProperty->setValue('https://example.org'); @@ -62,19 +62,48 @@ public function testSetScope() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', - IAccountManager::VISIBILITY_PUBLIC, + IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED ); - $actualReturn = $accountProperty->setScope(IAccountManager::VISIBILITY_PRIVATE); - $this->assertEquals(IAccountManager::VISIBILITY_PRIVATE, $accountProperty->getScope()); - $this->assertEquals(IAccountManager::VISIBILITY_PRIVATE, $actualReturn->getScope()); + $actualReturn = $accountProperty->setScope(IAccountManager::SCOPE_LOCAL); + $this->assertEquals(IAccountManager::SCOPE_LOCAL, $accountProperty->getScope()); + $this->assertEquals(IAccountManager::SCOPE_LOCAL, $actualReturn->getScope()); + } + + public function scopesProvider() { + return [ + // current values + [IAccountManager::SCOPE_PRIVATE, IAccountManager::SCOPE_PRIVATE], + [IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_LOCAL], + [IAccountManager::SCOPE_PUBLISHED, IAccountManager::SCOPE_PUBLISHED], + // legacy values + [IAccountManager::VISIBILITY_PRIVATE, IAccountManager::SCOPE_LOCAL], + [IAccountManager::VISIBILITY_CONTACTS_ONLY, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::VISIBILITY_PUBLIC, IAccountManager::SCOPE_PUBLISHED], + // fallback + ['', IAccountManager::SCOPE_LOCAL], + ['unknown', IAccountManager::SCOPE_LOCAL], + ]; + } + + /** + * @dataProvider scopesProvider + */ + public function testSetScopeMapping($storedScope, $returnedScope) { + $accountProperty = new AccountProperty( + IAccountManager::PROPERTY_WEBSITE, + 'https://example.com', + $storedScope, + IAccountManager::VERIFIED + ); + $this->assertEquals($returnedScope, $accountProperty->getScope()); } public function testSetVerified() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', - IAccountManager::VISIBILITY_PUBLIC, + IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED ); $actualReturn = $accountProperty->setVerified(IAccountManager::NOT_VERIFIED); @@ -86,13 +115,13 @@ public function testJsonSerialize() { $accountProperty = new AccountProperty( IAccountManager::PROPERTY_WEBSITE, 'https://example.com', - IAccountManager::VISIBILITY_PUBLIC, + IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED ); $this->assertEquals([ 'name' => IAccountManager::PROPERTY_WEBSITE, 'value' => 'https://example.com', - 'scope' => IAccountManager::VISIBILITY_PUBLIC, + 'scope' => IAccountManager::SCOPE_PUBLISHED, 'verified' => IAccountManager::VERIFIED ], $accountProperty->jsonSerialize()); } diff --git a/tests/lib/Accounts/AccountTest.php b/tests/lib/Accounts/AccountTest.php index 11b13637bd057..8afcc44afd198 100644 --- a/tests/lib/Accounts/AccountTest.php +++ b/tests/lib/Accounts/AccountTest.php @@ -43,21 +43,21 @@ public function testConstructor() { public function testSetProperty() { $user = $this->createMock(IUser::class); - $property = new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED); + $property = new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); $account = new Account($user); - $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); $this->assertEquals($property, $account->getProperty(IAccountManager::PROPERTY_WEBSITE)); } public function testGetProperties() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED) + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED) ]; $account = new Account($user); - $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED); - $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED); $this->assertEquals($properties, $account->getProperties()); } @@ -65,14 +65,14 @@ public function testGetProperties() { public function testGetFilteredProperties() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED), - IAccountManager::PROPERTY_PHONE => new AccountProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::VERIFIED), + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED), + IAccountManager::PROPERTY_PHONE => new AccountProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED), ]; $account = new Account($user); - $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED); - $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED); - $account->setProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED); $this->assertEquals( @@ -80,7 +80,7 @@ public function testGetFilteredProperties() { IAccountManager::PROPERTY_WEBSITE => $properties[IAccountManager::PROPERTY_WEBSITE], IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE], ], - $account->getFilteredProperties(IAccountManager::VISIBILITY_PUBLIC) + $account->getFilteredProperties(IAccountManager::SCOPE_PUBLISHED) ); $this->assertEquals( [ @@ -91,19 +91,19 @@ public function testGetFilteredProperties() { ); $this->assertEquals( [IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE]], - $account->getFilteredProperties(IAccountManager::VISIBILITY_PUBLIC, IAccountManager::VERIFIED) + $account->getFilteredProperties(IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED) ); } public function testJsonSerialize() { $user = $this->createMock(IUser::class); $properties = [ - IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED), - IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED) + IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED), + IAccountManager::PROPERTY_EMAIL => new AccountProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED) ]; $account = new Account($user); - $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::VISIBILITY_PUBLIC, IAccountManager::NOT_VERIFIED); - $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::VISIBILITY_PRIVATE, IAccountManager::VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); + $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED); $this->assertEquals($properties, $account->jsonSerialize()); } From 5d14fd4396b54134ce04809a10f46b084107003b Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 23 Mar 2021 16:59:05 +0100 Subject: [PATCH 04/16] Make extra user profile fields always editable The fields for phone number, address, website and twitter are now editable regardless whether federated sharing and the lookup server are enabled or not. Signed-off-by: Vincent Petry --- .../lib/Controller/UsersController.php | 31 +++++------------- .../tests/Controller/UsersControllerTest.php | 32 ++----------------- .../lib/Controller/UsersController.php | 15 +++------ .../settings/personal/personal.info.php | 22 ++----------- .../tests/Controller/UsersControllerTest.php | 1 + 5 files changed, 19 insertions(+), 82 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index d2f9b9e91c227..3a1dcd4f43a1d 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -50,7 +50,6 @@ use OC\Authentication\Token\RemoteWipe; use OC\HintException; use OC\KnownUser\KnownUserService; -use OCA\Provisioning_API\FederatedShareProviderFactory; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; @@ -85,8 +84,6 @@ class UsersController extends AUserData { protected $l10nFactory; /** @var NewUserMailHelper */ private $newUserMailHelper; - /** @var FederatedShareProviderFactory */ - private $federatedShareProviderFactory; /** @var ISecureRandom */ private $secureRandom; /** @var RemoteWipe */ @@ -108,7 +105,6 @@ public function __construct(string $appName, LoggerInterface $logger, IFactory $l10nFactory, NewUserMailHelper $newUserMailHelper, - FederatedShareProviderFactory $federatedShareProviderFactory, ISecureRandom $secureRandom, RemoteWipe $remoteWipe, KnownUserService $knownUserService, @@ -127,7 +123,6 @@ public function __construct(string $appName, $this->logger = $logger; $this->l10nFactory = $l10nFactory; $this->newUserMailHelper = $newUserMailHelper; - $this->federatedShareProviderFactory = $federatedShareProviderFactory; $this->secureRandom = $secureRandom; $this->remoteWipe = $remoteWipe; $this->knownUserService = $knownUserService; @@ -532,15 +527,10 @@ public function getEditableFields(): DataResponse { $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } - if ($this->appManager->isEnabledForUser('federatedfilesharing')) { - $shareProvider = $this->federatedShareProviderFactory->get(); - if ($shareProvider->isLookupServerUploadEnabled()) { - $permittedFields[] = IAccountManager::PROPERTY_PHONE; - $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; - $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; - $permittedFields[] = IAccountManager::PROPERTY_TWITTER; - } - } + $permittedFields[] = IAccountManager::PROPERTY_PHONE; + $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; + $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; + $permittedFields[] = IAccountManager::PROPERTY_TWITTER; return new DataResponse($permittedFields); } @@ -586,15 +576,10 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = 'locale'; } - if ($this->appManager->isEnabledForUser('federatedfilesharing')) { - $shareProvider = $this->federatedShareProviderFactory->get(); - if ($shareProvider->isLookupServerUploadEnabled()) { - $permittedFields[] = IAccountManager::PROPERTY_PHONE; - $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; - $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; - $permittedFields[] = IAccountManager::PROPERTY_TWITTER; - } - } + $permittedFields[] = IAccountManager::PROPERTY_PHONE; + $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; + $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; + $permittedFields[] = IAccountManager::PROPERTY_TWITTER; // If admin they can edit their own quota if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index d65e3d0791386..720eaaec9cf94 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -46,9 +46,7 @@ use OC\Group\Manager; use OC\KnownUser\KnownUserService; use OC\SubAdmin; -use OCA\FederatedFileSharing\FederatedShareProvider; use OCA\Provisioning_API\Controller\UsersController; -use OCA\Provisioning_API\FederatedShareProviderFactory; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; @@ -97,8 +95,6 @@ class UsersControllerTest extends TestCase { private $l10nFactory; /** @var NewUserMailHelper|MockObject */ private $newUserMailHelper; - /** @var FederatedShareProviderFactory|MockObject */ - private $federatedShareProviderFactory; /** @var ISecureRandom|MockObject */ private $secureRandom; /** @var RemoteWipe|MockObject */ @@ -122,7 +118,6 @@ protected function setUp(): void { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->newUserMailHelper = $this->createMock(NewUserMailHelper::class); - $this->federatedShareProviderFactory = $this->createMock(FederatedShareProviderFactory::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->remoteWipe = $this->createMock(RemoteWipe::class); $this->knownUserService = $this->createMock(KnownUserService::class); @@ -142,7 +137,6 @@ protected function setUp(): void { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -407,7 +401,6 @@ public function testAddUserSuccessfulWithDisplayName() { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3247,7 +3240,6 @@ public function testGetCurrentUserLoggedIn() { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3314,7 +3306,6 @@ public function testGetUser() { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3639,18 +3630,13 @@ public function testResendWelcomeMessageFailed() { public function dataGetEditableFields() { return [ - [false, false, []], - [false, true, [ + [false, [ IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, IAccountManager::PROPERTY_TWITTER, ]], - [ true, false, [ - IAccountManager::PROPERTY_DISPLAYNAME, - IAccountManager::PROPERTY_EMAIL, - ]], - [ true, true ,[ + [ true, [ IAccountManager::PROPERTY_DISPLAYNAME, IAccountManager::PROPERTY_EMAIL, IAccountManager::PROPERTY_PHONE, @@ -3665,27 +3651,15 @@ public function dataGetEditableFields() { * @dataProvider dataGetEditableFields * * @param bool $allowedToChangeDisplayName - * @param bool $federatedSharingEnabled * @param array $expected */ - public function testGetEditableFields(bool $allowedToChangeDisplayName, bool $federatedSharingEnabled, array $expected) { + public function testGetEditableFields(bool $allowedToChangeDisplayName, array $expected) { $this->config ->method('getSystemValue') ->with( $this->equalTo('allow_user_to_change_display_name'), $this->anything() )->willReturn($allowedToChangeDisplayName); - $this->appManager - ->method('isEnabledForUser') - ->with($this->equalTo('federatedfilesharing')) - ->willReturn($federatedSharingEnabled); - - $shareprovider = $this->createMock(FederatedShareProvider::class); - $shareprovider->method('isLookupServerUploadEnabled')->willReturn(true); - - $this->federatedShareProviderFactory - ->method('get') - ->willReturn($shareprovider); $expectedResp = new DataResponse($expected); $this->assertEquals($expectedResp, $this->api->getEditableFields()); diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index 46de0b4cd96a4..a9b72571de6be 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -46,7 +46,6 @@ use OC\L10N\Factory; use OC\Security\IdentityProof\Manager; use OC\User\Manager as UserManager; -use OCA\FederatedFileSharing\FederatedShareProvider; use OCA\Settings\BackgroundJobs\VerifyUserData; use OCA\Settings\Events\BeforeTemplateRenderedEvent; use OCA\User_LDAP\User_Proxy; @@ -401,15 +400,11 @@ public function setUserSettings(?string $avatarScope = null, $data[IAccountManager::PROPERTY_DISPLAYNAME] = ['value' => $displayname, 'scope' => $displaynameScope]; $data[IAccountManager::PROPERTY_EMAIL] = ['value' => $email, 'scope' => $emailScope]; } - if ($this->appManager->isEnabledForUser('federatedfilesharing')) { - $shareProvider = \OC::$server->query(FederatedShareProvider::class); - if ($shareProvider->isLookupServerUploadEnabled()) { - $data[IAccountManager::PROPERTY_WEBSITE] = ['value' => $website, 'scope' => $websiteScope]; - $data[IAccountManager::PROPERTY_ADDRESS] = ['value' => $address, 'scope' => $addressScope]; - $data[IAccountManager::PROPERTY_PHONE] = ['value' => $phone, 'scope' => $phoneScope]; - $data[IAccountManager::PROPERTY_TWITTER] = ['value' => $twitter, 'scope' => $twitterScope]; - } - } + $data[IAccountManager::PROPERTY_WEBSITE] = ['value' => $website, 'scope' => $websiteScope]; + $data[IAccountManager::PROPERTY_ADDRESS] = ['value' => $address, 'scope' => $addressScope]; + $data[IAccountManager::PROPERTY_PHONE] = ['value' => $phone, 'scope' => $phoneScope]; + $data[IAccountManager::PROPERTY_TWITTER] = ['value' => $twitter, 'scope' => $twitterScope]; + try { $data = $this->saveUserSettings($user, $data); if ($beforeData[IAccountManager::PROPERTY_PHONE]['value'] !== $data[IAccountManager::PROPERTY_PHONE]['value']) { diff --git a/apps/settings/templates/settings/personal/personal.info.php b/apps/settings/templates/settings/personal/personal.info.php index 60db8c8533343..f2e3a51aad70a 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -177,7 +177,6 @@ -

@@ -188,9 +187,7 @@

- + @@ -199,8 +196,6 @@ - -

@@ -211,9 +206,7 @@

- + @@ -222,8 +215,6 @@ - -

@@ -267,17 +258,12 @@ />

- -

@@ -321,16 +307,12 @@ />

-
diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 2daf383410eba..f9652053de891 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -190,6 +190,7 @@ protected function getController($isAdmin = false, $mockedMethods = []) { public function testSetUserSettings($email, $validEmail, $expectedStatus) { $controller = $this->getController(false, ['saveUserSettings']); $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('johndoe'); $this->userSession->method('getUser')->willReturn($user); From 266a6fb5f8a0ec0e89e2a96f7e2ebb4a5bbf44f5 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 24 Mar 2021 12:32:06 +0100 Subject: [PATCH 05/16] OCS allow reading and writing account property scopes Extends the provisioning API to allow a user to get and set their own account property scopes. Signed-off-by: Vincent Petry --- .../lib/Controller/AUserData.php | 29 ++++++++++++---- .../lib/Controller/UsersController.php | 34 +++++++++++++++++-- lib/private/Accounts/AccountManager.php | 33 +++++++++++++++++- lib/private/Accounts/AccountProperty.php | 2 +- 4 files changed, 88 insertions(+), 10 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index 5e6af27cf72d0..a1391e023b748 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -51,6 +51,7 @@ use OCP\User\Backend\ISetPasswordBackend; abstract class AUserData extends OCSController { + public const SCOPE_SUFFIX = 'Scope'; /** @var IUserManager */ protected $userManager; @@ -87,12 +88,13 @@ public function __construct(string $appName, * creates a array with all user data * * @param string $userId + * @param bool $includeScopes * @return array * @throws NotFoundException * @throws OCSException * @throws OCSNotFoundException */ - protected function getUserData(string $userId): array { + protected function getUserData(string $userId, bool $includeScopes = false): array { $currentLoggedInUser = $this->userSession->getUser(); $data = []; @@ -115,7 +117,7 @@ protected function getUserData(string $userId): array { } // Get groups data - $userAccount = $this->accountManager->getUser($targetUserObject); + $userAccount = $this->accountManager->getAccount($targetUserObject); $groups = $this->groupManager->getUserGroups($targetUserObject); $gids = []; foreach ($groups as $group) { @@ -138,11 +140,26 @@ protected function getUserData(string $userId): array { $data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID()); $data['quota'] = $this->fillStorageInfo($targetUserObject->getUID()); $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); + if ($includeScopes) { + $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); + } $data[IAccountManager::PROPERTY_DISPLAYNAME] = $targetUserObject->getDisplayName(); - $data[IAccountManager::PROPERTY_PHONE] = $userAccount[IAccountManager::PROPERTY_PHONE]['value']; - $data[IAccountManager::PROPERTY_ADDRESS] = $userAccount[IAccountManager::PROPERTY_ADDRESS]['value']; - $data[IAccountManager::PROPERTY_WEBSITE] = $userAccount[IAccountManager::PROPERTY_WEBSITE]['value']; - $data[IAccountManager::PROPERTY_TWITTER] = $userAccount[IAccountManager::PROPERTY_TWITTER]['value']; + if ($includeScopes) { + $data[IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(); + } + + foreach ([ + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + ] as $propertyName) { + $property = $userAccount->getProperty($propertyName); + $data[$propertyName] = $property->getValue(); + if ($includeScopes) { + $data[$propertyName . self::SCOPE_SUFFIX] = $property->getScope(); + } + } $data['groups'] = $gids; $data['language'] = $this->l10nFactory->getUserLanguage($targetUserObject); $data['locale'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'locale'); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 3a1dcd4f43a1d..968cf22e64762 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -483,7 +483,13 @@ public function addUser(string $userid, * @throws OCSException */ public function getUser(string $userId): DataResponse { - $data = $this->getUserData($userId); + $includeScopes = false; + $currentUser = $this->userSession->getUser(); + if ($currentUser && $currentUser->getUID() === $userId) { + $includeScopes = true; + } + + $data = $this->getUserData($userId, $includeScopes); // getUserData returns empty array if not enough permissions if (empty($data)) { throw new OCSException('', OCSController::RESPOND_UNAUTHORISED); @@ -503,7 +509,7 @@ public function getUser(string $userId): DataResponse { public function getCurrentUser(): DataResponse { $user = $this->userSession->getUser(); if ($user) { - $data = $this->getUserData($user->getUID()); + $data = $this->getUserData($user->getUID(), true); // rename "displayname" to "display-name" only for this call to keep // the API stable. $data['display-name'] = $data['displayname']; @@ -565,6 +571,9 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX; + $permittedFields[] = 'password'; if ($this->config->getSystemValue('force_language', false) === false || $this->groupManager->isAdmin($currentLoggedInUser->getUID())) { @@ -580,6 +589,10 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; $permittedFields[] = IAccountManager::PROPERTY_TWITTER; + $permittedFields[] = IAccountManager::PROPERTY_PHONE . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::PROPERTY_ADDRESS . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::PROPERTY_WEBSITE . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::PROPERTY_TWITTER . self::SCOPE_SUFFIX; // If admin they can edit their own quota if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { @@ -684,6 +697,23 @@ public function editUser(string $userId, string $key, string $value): DataRespon } } break; + case IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_PHONE . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_ADDRESS . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_WEBSITE . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_TWITTER . self::SCOPE_SUFFIX: + $propertyName = substr($key, 0, strlen($key) - strlen(self::SCOPE_SUFFIX)); + $userAccount = $this->accountManager->getUser($targetUser); + if ($userAccount[$propertyName]['scope'] !== $value) { + $userAccount[$propertyName]['scope'] = $value; + try { + $this->accountManager->updateUser($targetUser, $userAccount, true); + } catch (\InvalidArgumentException $e) { + throw new OCSException('Invalid ' . $e->getMessage(), 102); + } + } + break; default: throw new OCSException('', 103); } diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index ff3b04d83955c..74ba53737cac8 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -144,6 +144,37 @@ public function updateUser(IUser $user, array $data, bool $throwOnData = false): } } + $allowedScopes = [ + self::SCOPE_PRIVATE, + self::SCOPE_LOCAL, + self::SCOPE_FEDERATED, + self::SCOPE_PUBLISHED, + self::VISIBILITY_PRIVATE, + self::VISIBILITY_CONTACTS_ONLY, + self::VISIBILITY_PUBLIC, + ]; + + // validate and convert scope values + foreach ($data as $propertyName => $propertyData) { + if (isset($propertyData['scope'])) { + if ($throwOnData && !in_array($propertyData['scope'], $allowedScopes, true)) { + throw new \InvalidArgumentException('scope'); + } + + if ( + $propertyData['scope'] === self::SCOPE_PRIVATE + && ($propertyName === self::PROPERTY_DISPLAYNAME || $propertyName === self::PROPERTY_EMAIL) + ) { + // v2-private is not available for these fields + throw new \InvalidArgumentException('scope'); + } + + // migrate scope values to the new format + // invalid scopes are mapped to a default value + $data[$propertyName]['scope'] = AccountProperty::mapScopeToV2($propertyData['scope']); + } + } + if (empty($userData)) { $this->insertNewUser($user, $data); } elseif ($userData !== $data) { @@ -405,7 +436,7 @@ protected function writeUserData(IUser $user, array $data): void { } $query->setParameter('name', $propertyName) - ->setParameter('value', $property['value']); + ->setParameter('value', $property['value'] ?? ''); $query->execute(); } } diff --git a/lib/private/Accounts/AccountProperty.php b/lib/private/Accounts/AccountProperty.php index 4c75ad85414a5..850f39df9e3c7 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -128,7 +128,7 @@ public function getScope(): string { return $this->scope; } - private function mapScopeToV2($scope) { + public static function mapScopeToV2($scope) { if (strpos($scope, 'v2-') === 0) { return $scope; } From 2a312fe4e3a9c07bdc4b4e10bed66af48e07463a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 24 Mar 2021 15:32:51 +0100 Subject: [PATCH 06/16] Expose avatarScope through provisioning API Signed-off-by: Vincent Petry --- apps/provisioning_api/lib/Controller/AUserData.php | 6 ++++++ apps/provisioning_api/lib/Controller/UsersController.php | 3 +++ 2 files changed, 9 insertions(+) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index a1391e023b748..c26c4f9e2d0fe 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -139,6 +139,11 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr $data['backend'] = $targetUserObject->getBackendClassName(); $data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID()); $data['quota'] = $this->fillStorageInfo($targetUserObject->getUID()); + + if ($includeScopes) { + $data[IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(); + } + $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); if ($includeScopes) { $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); @@ -160,6 +165,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr $data[$propertyName . self::SCOPE_SUFFIX] = $property->getScope(); } } + $data['groups'] = $gids; $data['language'] = $this->l10nFactory->getUserLanguage($targetUserObject); $data['locale'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'locale'); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 968cf22e64762..0019472c8849e 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -594,6 +594,8 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_WEBSITE . self::SCOPE_SUFFIX; $permittedFields[] = IAccountManager::PROPERTY_TWITTER . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX; + // If admin they can edit their own quota if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { $permittedFields[] = 'quota'; @@ -703,6 +705,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon case IAccountManager::PROPERTY_ADDRESS . self::SCOPE_SUFFIX: case IAccountManager::PROPERTY_WEBSITE . self::SCOPE_SUFFIX: case IAccountManager::PROPERTY_TWITTER . self::SCOPE_SUFFIX: + case IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX: $propertyName = substr($key, 0, strlen($key) - strlen(self::SCOPE_SUFFIX)); $userAccount = $this->accountManager->getUser($targetUser); if ($userAccount[$propertyName]['scope'] !== $value) { From cb9d25c1629251e3b972582a97ed981b316a6d7e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 24 Mar 2021 16:17:28 +0100 Subject: [PATCH 07/16] Enhance UsersControllerTest of provisioning API with scopes Signed-off-by: Vincent Petry --- .../tests/Controller/UsersControllerTest.php | 177 ++++++++++++++---- 1 file changed, 140 insertions(+), 37 deletions(-) diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 720eaaec9cf94..bff1b06ae6360 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -48,7 +48,9 @@ use OC\SubAdmin; use OCA\Provisioning_API\Controller\UsersController; use OCA\Settings\Mailer\NewUserMailHelper; +use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; use OCP\EventDispatcher\IEventDispatcher; @@ -927,7 +929,6 @@ public function testGetUserTargetDoesNotExist() { ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->once()) ->method('getUser') ->willReturn($loggedInUser); $this->userManager @@ -997,16 +998,13 @@ public function testGetUserDataAsAdmin() { $group->expects($this->at(3)) ->method('getGID') ->willReturn('group3'); - $this->accountManager->expects($this->any())->method('getUser') - ->with($targetUser) - ->willReturn( - [ - IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], - IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], - IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], - ] - ); + + $this->mockAccount($targetUser, [ + IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], + IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], + IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], + ]); $this->config ->expects($this->at(0)) ->method('getUserValue') @@ -1166,16 +1164,13 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible() { $targetUser ->method('getUID') ->willReturn('UID'); - $this->accountManager->expects($this->any())->method('getUser') - ->with($targetUser) - ->willReturn( - [ - IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], - IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], - IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], - ] - ); + + $this->mockAccount($targetUser, [ + IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], + IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], + IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], + ]); $this->l10nFactory ->expects($this->once()) @@ -1218,14 +1213,13 @@ public function testGetUserDataAsSubAdminAndUserIsNotAccessible() { ->disableOriginalConstructor() ->getMock(); $loggedInUser - ->expects($this->exactly(2)) + ->expects($this->exactly(3)) ->method('getUID') ->willReturn('subadmin'); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->once()) ->method('getUser') ->willReturn($loggedInUser); $this->userManager @@ -1337,16 +1331,12 @@ public function testGetUserDataAsSubAdminSelfLookup() { ->expects($this->once()) ->method('getBackend') ->willReturn($backend); - $this->accountManager->expects($this->any())->method('getUser') - ->with($targetUser) - ->willReturn( - [ - IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], - IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], - IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], - ] - ); + $this->mockAccount($targetUser, [ + IAccountManager::PROPERTY_ADDRESS => ['value' => 'address'], + IAccountManager::PROPERTY_PHONE => ['value' => 'phone'], + IAccountManager::PROPERTY_TWITTER => ['value' => 'twitter'], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'website'], + ]); $this->l10nFactory ->expects($this->once()) @@ -1531,6 +1521,88 @@ public function testEditUserRegularUserSelfEditChangeEmailInvalid() { $this->api->editUser('UserToEdit', 'email', 'demo.org'); } + public function selfEditChangePropertyProvider() { + return [ + [IAccountManager::PROPERTY_TWITTER, '@oldtwitter', '@newtwitter'], + [IAccountManager::PROPERTY_PHONE, '1234', '12345'], + [IAccountManager::PROPERTY_ADDRESS, 'Something street 2', 'Another street 3'], + [IAccountManager::PROPERTY_WEBSITE, 'https://examplesite1', 'https://examplesite2'], + ]; + } + + /** + * @dataProvider selfEditChangePropertyProvider + */ + public function testEditUserRegularUserSelfEditChangeProperty($propertyName, $oldValue, $newValue) { + $loggedInUser = $this->getMockBuilder(IUser::class) + ->disableOriginalConstructor() + ->getMock(); + $loggedInUser + ->expects($this->any()) + ->method('getUID') + ->willReturn('UID'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('UserToEdit') + ->willReturn($loggedInUser); + + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($loggedInUser) + ->willReturn([$propertyName => ['value' => $oldValue, 'scope' => IAccountManager::SCOPE_LOCAL]]); + $this->accountManager->expects($this->once()) + ->method('updateUser') + ->with($loggedInUser, [$propertyName => ['value' => $newValue, 'scope' => IAccountManager::SCOPE_LOCAL]], true); + + $this->assertEquals([], $this->api->editUser('UserToEdit', $propertyName, $newValue)->getData()); + } + + public function selfEditChangePropertyScopeProvider() { + return [ + [IAccountManager::PROPERTY_TWITTER, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::PROPERTY_PHONE, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::PROPERTY_ADDRESS, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::PROPERTY_WEBSITE, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + ]; + } + + /** + * @dataProvider selfEditChangePropertyProvider + */ + public function testEditUserRegularUserSelfEditChangePropertyScope($propertyName, $oldScope, $newScope) { + $loggedInUser = $this->getMockBuilder(IUser::class) + ->disableOriginalConstructor() + ->getMock(); + $loggedInUser + ->expects($this->any()) + ->method('getUID') + ->willReturn('UID'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('UserToEdit') + ->willReturn($loggedInUser); + + $this->accountManager->expects($this->once()) + ->method('getUser') + ->with($loggedInUser) + ->willReturn([$propertyName => ['value' => 'somevalue', 'scope' => $oldScope]]); + $this->accountManager->expects($this->once()) + ->method('updateUser') + ->with($loggedInUser, [$propertyName => ['value' => 'somevalue', 'scope' => $newScope]], true); + + $this->assertEquals([], $this->api->editUser('UserToEdit', $propertyName . 'Scope', $newScope)->getData()); + } + public function testEditUserRegularUserSelfEditChangePassword() { $loggedInUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() @@ -3248,7 +3320,7 @@ public function testGetCurrentUserLoggedIn() { ->setMethods(['getUserData']) ->getMock(); - $api->expects($this->once())->method('getUserData')->with('UID') + $api->expects($this->once())->method('getUserData')->with('UID', true) ->willReturn( [ 'id' => 'UID', @@ -3289,8 +3361,15 @@ public function testGetCurrentUserNotLoggedIn() { $this->api->getCurrentUser(); } - public function testGetUser() { + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser + ->method('getUID') + ->willReturn('currentuser'); + $this->userSession + ->method('getUser') + ->willReturn($loggedInUser); + /** @var UsersController | MockObject $api */ $api = $this->getMockBuilder(UsersController::class) ->setConstructorArgs([ @@ -3326,11 +3405,16 @@ public function testGetUser() { 'displayname' => 'Demo User' ]; - $api->expects($this->once())->method('getUserData') - ->with('uid') + $api->expects($this->at(0))->method('getUserData') + ->with('uid', false) + ->willReturn($expected); + $api->expects($this->at(1))->method('getUserData') + ->with('currentuser', true) ->willReturn($expected); $this->assertSame($expected, $api->getUser('uid')->getData()); + + $this->assertSame($expected, $api->getUser('currentuser')->getData()); } @@ -3664,4 +3748,23 @@ public function testGetEditableFields(bool $allowedToChangeDisplayName, array $e $expectedResp = new DataResponse($expected); $this->assertEquals($expectedResp, $this->api->getEditableFields()); } + + private function mockAccount($targetUser, $accountProperties) { + $mockedProperties = []; + + foreach ($accountProperties as $propertyName => $data) { + $mockedProperty = $this->createMock(IAccountProperty::class); + $mockedProperty->method('getValue')->willReturn($data['value'] ?? ''); + $mockedProperty->method('getScope')->willReturn($data['scope'] ?? ''); + $mockedProperties[] = [$propertyName, $mockedProperty]; + } + + $account = $this->createMock(IAccount::class); + $account->method('getProperty') + ->will($this->returnValueMap($mockedProperties)); + + $this->accountManager->expects($this->any())->method('getAccount') + ->with($targetUser) + ->willReturn($account); + } } From b73df5846ce9c801e93e79e11e93a8cd73dabfdd Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 24 Mar 2021 17:06:04 +0100 Subject: [PATCH 08/16] Add property scope tests for AccountManager Signed-off-by: Vincent Petry --- .../tests/Controller/UsersControllerTest.php | 3 + lib/private/Accounts/AccountManager.php | 19 ++- tests/lib/Accounts/AccountManagerTest.php | 123 ++++++++++++++++++ 3 files changed, 139 insertions(+), 6 deletions(-) diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index bff1b06ae6360..4754c5a468d77 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -1564,6 +1564,9 @@ public function testEditUserRegularUserSelfEditChangeProperty($propertyName, $ol public function selfEditChangePropertyScopeProvider() { return [ + [IAccountManager::PROPERTY_AVATAR, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::PROPERTY_DISPLAYNAME, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], + [IAccountManager::PROPERTY_EMAIL, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], [IAccountManager::PROPERTY_TWITTER, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], [IAccountManager::PROPERTY_PHONE, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], [IAccountManager::PROPERTY_ADDRESS, IAccountManager::SCOPE_LOCAL, IAccountManager::SCOPE_FEDERATED], diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 74ba53737cac8..6198f8dbddd9a 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -165,13 +165,18 @@ public function updateUser(IUser $user, array $data, bool $throwOnData = false): $propertyData['scope'] === self::SCOPE_PRIVATE && ($propertyName === self::PROPERTY_DISPLAYNAME || $propertyName === self::PROPERTY_EMAIL) ) { - // v2-private is not available for these fields - throw new \InvalidArgumentException('scope'); + if ($throwOnData) { + // v2-private is not available for these fields + throw new \InvalidArgumentException('scope'); + } else { + // default to local + $data[$propertyName]['scope'] = self::SCOPE_LOCAL; + } + } else { + // migrate scope values to the new format + // invalid scopes are mapped to a default value + $data[$propertyName]['scope'] = AccountProperty::mapScopeToV2($propertyData['scope']); } - - // migrate scope values to the new format - // invalid scopes are mapped to a default value - $data[$propertyName]['scope'] = AccountProperty::mapScopeToV2($propertyData['scope']); } } @@ -229,6 +234,8 @@ public function deleteUserData(IUser $user): void { * * @param IUser $user * @return array + * + * @deprecated use getAccount instead to make sure migrated properties work correctly */ public function getUser(IUser $user) { $uid = $user->getUID(); diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 62da1cbc1daae..27ebed69793d4 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -153,6 +153,129 @@ public function dataTrueFalse() { ]; } + public function updateUserSetScopeProvider() { + return [ + // regular scope switching + [ + [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_AVATAR => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], + ], + [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PRIVATE], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + ], + [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PRIVATE], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + ], + ], + // legacy scope mapping, the given visibility values get converted to scopes + [ + [ + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], + ], + [ + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::VISIBILITY_PUBLIC], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::VISIBILITY_PRIVATE], + ], + [ + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_LOCAL], + ], + ], + // invalid or unsupported scope values get converted to SCOPE_LOCAL + [ + [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + ], + [ + // SCOPE_PRIVATE is not allowed for display name and email + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PRIVATE], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => 'invalid'], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => ''], + ], + [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], + ], + // don't throw but fall back + false, false, + ], + // invalid or unsupported scope values throw an exception when passing $throwOnData=true + [ + [IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED]], + [IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PRIVATE]], + null, + // throw exception + true, true, + ], + [ + [IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED]], + [IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PRIVATE]], + null, + // throw exception + true, true, + ], + [ + [IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED]], + [IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => 'invalid']], + null, + // throw exception + true, true, + ], + ]; + } + + /** + * @dataProvider updateUserSetScopeProvider + */ + public function testUpdateUserSetScope($oldData, $newData, $savedData, $throwOnData = true, $expectedThrow = false) { + $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser', 'updateVerifyStatus', 'checkEmailVerification']); + /** @var IUser $user */ + $user = $this->createMock(IUser::class); + + $accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($oldData); + + if ($expectedThrow) { + $accountManager->expects($this->never())->method('updateExistingUser'); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('scope'); + } else { + $accountManager->expects($this->once())->method('checkEmailVerification') + ->with($oldData, $savedData, $user)->willReturn($savedData); + $accountManager->expects($this->once())->method('updateVerifyStatus') + ->with($oldData, $savedData)->willReturn($savedData); + $accountManager->expects($this->once())->method('updateExistingUser') + ->with($user, $savedData); + $accountManager->expects($this->never())->method('insertNewUser'); + } + + $accountManager->updateUser($user, $newData, $throwOnData); + } /** * @dataProvider dataTestGetUser From b66af8a4329852ee599438c5be74e9246b4e0a68 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 11:16:21 +0100 Subject: [PATCH 09/16] Add capability for editable scopes in provisioning API Signed-off-by: Vincent Petry --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/AppInfo/Application.php | 2 + apps/provisioning_api/lib/Capabilities.php | 51 +++++++++++++++++++ 4 files changed, 55 insertions(+) create mode 100644 apps/provisioning_api/lib/Capabilities.php diff --git a/apps/provisioning_api/composer/composer/autoload_classmap.php b/apps/provisioning_api/composer/composer/autoload_classmap.php index e94a97c194911..22927806e656d 100644 --- a/apps/provisioning_api/composer/composer/autoload_classmap.php +++ b/apps/provisioning_api/composer/composer/autoload_classmap.php @@ -8,6 +8,7 @@ return array( 'Composer\\InstalledVersions' => $vendorDir . '/composer/InstalledVersions.php', 'OCA\\Provisioning_API\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', + 'OCA\\Provisioning_API\\Capabilities' => $baseDir . '/../lib/Capabilities.php', 'OCA\\Provisioning_API\\Controller\\AUserData' => $baseDir . '/../lib/Controller/AUserData.php', 'OCA\\Provisioning_API\\Controller\\AppConfigController' => $baseDir . '/../lib/Controller/AppConfigController.php', 'OCA\\Provisioning_API\\Controller\\AppsController' => $baseDir . '/../lib/Controller/AppsController.php', diff --git a/apps/provisioning_api/composer/composer/autoload_static.php b/apps/provisioning_api/composer/composer/autoload_static.php index b982f203211ae..f5a4b73f4f875 100644 --- a/apps/provisioning_api/composer/composer/autoload_static.php +++ b/apps/provisioning_api/composer/composer/autoload_static.php @@ -23,6 +23,7 @@ class ComposerStaticInitProvisioning_API public static $classMap = array ( 'Composer\\InstalledVersions' => __DIR__ . '/..' . '/composer/InstalledVersions.php', 'OCA\\Provisioning_API\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', + 'OCA\\Provisioning_API\\Capabilities' => __DIR__ . '/..' . '/../lib/Capabilities.php', 'OCA\\Provisioning_API\\Controller\\AUserData' => __DIR__ . '/..' . '/../lib/Controller/AUserData.php', 'OCA\\Provisioning_API\\Controller\\AppConfigController' => __DIR__ . '/..' . '/../lib/Controller/AppConfigController.php', 'OCA\\Provisioning_API\\Controller\\AppsController' => __DIR__ . '/..' . '/../lib/Controller/AppsController.php', diff --git a/apps/provisioning_api/lib/AppInfo/Application.php b/apps/provisioning_api/lib/AppInfo/Application.php index 7ec21c3329e13..af6b2b3371186 100644 --- a/apps/provisioning_api/lib/AppInfo/Application.php +++ b/apps/provisioning_api/lib/AppInfo/Application.php @@ -29,6 +29,7 @@ namespace OCA\Provisioning_API\AppInfo; use OC\Group\Manager as GroupManager; +use OCA\Provisioning_API\Capabilities; use OCA\Provisioning_API\Listener\UserDeletedListener; use OCA\Provisioning_API\Middleware\ProvisioningApiMiddleware; use OCA\Settings\Mailer\NewUserMailHelper; @@ -92,6 +93,7 @@ public function register(IRegistrationContext $context): void { ); }); $context->registerMiddleware(ProvisioningApiMiddleware::class); + $context->registerCapability(Capabilities::class); } public function boot(IBootContext $context): void { diff --git a/apps/provisioning_api/lib/Capabilities.php b/apps/provisioning_api/lib/Capabilities.php new file mode 100644 index 0000000000000..eaec844313f9a --- /dev/null +++ b/apps/provisioning_api/lib/Capabilities.php @@ -0,0 +1,51 @@ + + * + * @author Vincent Petry + * + * @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 . + * + */ + +namespace OCA\Provisioning_API; + +use OCP\App\IAppManager; +use OCP\Capabilities\ICapability; + +class Capabilities implements ICapability { + + /** @var IAppManager */ + private $appManager; + + public function __construct(IAppManager $appManager) { + $this->appManager = $appManager; + } + + /** + * Function an app uses to return the capabilities + * + * @return array Array containing the apps capabilities + */ + public function getCapabilities() { + return [ + 'provisioning_api' => [ + 'version' => $this->appManager->getAppVersion('provisioning_api'), + 'hasAccountPropertyScopes' => true, + ] + ]; + } +} From bb008be28af2e7379883543d73c18f63680915a6 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 12:21:03 +0100 Subject: [PATCH 10/16] Added PlaceholderAvatar with own cached images When avatar scope is private, the PlaceholderAvatar is used to deliver a placeholder avatar based on the user's initials. This was implemented as a separate class for now to avoid messing with the existing UserAvatar implementation and its generated vs non-generated logic. Signed-off-by: Vincent Petry --- lib/private/Avatar/AvatarManager.php | 17 +-- lib/private/Avatar/PlaceholderAvatar.php | 183 +++++++++++++++++++++++ lib/private/Avatar/UserAvatar.php | 1 + tests/lib/Avatar/AvatarManagerTest.php | 11 +- 4 files changed, 198 insertions(+), 14 deletions(-) create mode 100644 lib/private/Avatar/PlaceholderAvatar.php diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 03f3d89e5f6c6..92cd502dacb46 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -122,7 +122,11 @@ public function getAvatar(string $userId) : IAvatar { $requestingUser = $this->userSession->getUser(); } - $canShowRealAvatar = true; + try { + $folder = $this->appData->getFolder($userId); + } catch (NotFoundException $e) { + $folder = $this->appData->newFolder($userId); + } // requesting in public page if ($requestingUser === null) { @@ -132,18 +136,11 @@ public function getAvatar(string $userId) : IAvatar { // v2-private scope hides the avatar from public access if ($avatarScope === IAccountManager::SCOPE_PRIVATE) { - // FIXME: guest avatar is re-generated every time, use a cache instead - // see how UserAvatar caches the generated one - return $this->getGuestAvatar($userId); + // use a placeholder avatar which caches the generated images + return new PlaceholderAvatar($folder, $user, $this->logger); } } - try { - $folder = $this->appData->getFolder($userId); - } catch (NotFoundException $e) { - $folder = $this->appData->newFolder($userId); - } - return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config); } diff --git a/lib/private/Avatar/PlaceholderAvatar.php b/lib/private/Avatar/PlaceholderAvatar.php new file mode 100644 index 0000000000000..5883fe531a3fd --- /dev/null +++ b/lib/private/Avatar/PlaceholderAvatar.php @@ -0,0 +1,183 @@ + + * + * @author Arthur Schiwon + * @author Christoph Wurst + * @author Joas Schilling + * @author Michael Weimann + * + * @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 . + * + */ + +namespace OC\Avatar; + +use OC\NotSquareException; +use OC\User\User; +use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; +use OCP\Files\SimpleFS\ISimpleFile; +use OCP\Files\SimpleFS\ISimpleFolder; +use OCP\IConfig; +use OCP\IImage; +use OCP\IL10N; +use OCP\ILogger; + +/** + * This class represents a registered user's placeholder avatar. + * + * It generates an image based on the user's initials and caches it on storage + * for faster retrieval, unlike the GuestAvatar. + */ +class PlaceholderAvatar extends Avatar { + /** @var ISimpleFolder */ + private $folder; + + /** @var User */ + private $user; + + /** + * UserAvatar constructor. + * + * @param IConfig $config The configuration + * @param ISimpleFolder $folder The avatar files folder + * @param IL10N $l The localization helper + * @param User $user The user this class manages the avatar for + * @param ILogger $logger The logger + */ + public function __construct( + ISimpleFolder $folder, + $user, + ILogger $logger) { + parent::__construct($logger); + + $this->folder = $folder; + $this->user = $user; + } + + /** + * Check if an avatar exists for the user + * + * @return bool + */ + public function exists() { + return true; + } + + /** + * Sets the users avatar. + * + * @param IImage|resource|string $data An image object, imagedata or path to set a new avatar + * @throws \Exception if the provided file is not a jpg or png image + * @throws \Exception if the provided image is not valid + * @throws NotSquareException if the image is not square + * @return void + */ + public function set($data) { + // unimplemented for placeholder avatars + } + + /** + * Removes the users avatar. + */ + public function remove(bool $silent = false) { + $avatars = $this->folder->getDirectoryListing(); + + foreach ($avatars as $avatar) { + $avatar->delete(); + } + } + + /** + * Returns the avatar for an user. + * + * If there is no avatar file yet, one is generated. + * + * @param int $size + * @return ISimpleFile + * @throws NotFoundException + * @throws \OCP\Files\NotPermittedException + * @throws \OCP\PreConditionNotMetException + */ + public function getFile($size) { + $size = (int) $size; + + $ext = 'png'; + + if ($size === -1) { + $path = 'avatar-placeholder.' . $ext; + } else { + $path = 'avatar-placeholder.' . $size . '.' . $ext; + } + + try { + $file = $this->folder->getFile($path); + } catch (NotFoundException $e) { + if ($size <= 0) { + throw new NotFoundException; + } + + if (!$data = $this->generateAvatarFromSvg($size)) { + $data = $this->generateAvatar($this->getDisplayName(), $size); + } + + try { + $file = $this->folder->newFile($path); + $file->putContent($data); + } catch (NotPermittedException $e) { + $this->logger->error('Failed to save avatar placeholder for ' . $this->user->getUID()); + throw new NotFoundException(); + } + } + + return $file; + } + + /** + * Returns the user display name. + * + * @return string + */ + public function getDisplayName(): string { + return $this->user->getDisplayName(); + } + + /** + * Handles user changes. + * + * @param string $feature The changed feature + * @param mixed $oldValue The previous value + * @param mixed $newValue The new value + * @throws NotPermittedException + * @throws \OCP\PreConditionNotMetException + */ + public function userChanged($feature, $oldValue, $newValue) { + $this->remove(); + } + + /** + * Check if the avatar of a user is a custom uploaded one + * + * @return bool + */ + public function isCustomAvatar(): bool { + return false; + } +} diff --git a/lib/private/Avatar/UserAvatar.php b/lib/private/Avatar/UserAvatar.php index f7ace429f7d98..f47809425ed40 100644 --- a/lib/private/Avatar/UserAvatar.php +++ b/lib/private/Avatar/UserAvatar.php @@ -270,6 +270,7 @@ public function getFile($size) { throw new NotFoundException; } + // TODO: rework to integrate with the PlaceholderAvatar in a compatible way if ($this->folder->fileExists('generated')) { if (!$data = $this->generateAvatarFromSvg($size)) { $data = $this->generateAvatar($this->getDisplayName(), $size); diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 0ce0e75256947..1b06e786fb1c4 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -25,7 +25,7 @@ namespace Test\Avatar; use OC\Avatar\AvatarManager; -use OC\Avatar\GuestAvatar; +use OC\Avatar\PlaceholderAvatar; use OC\Avatar\UserAvatar; use OC\User\Manager; use OCP\Accounts\IAccount; @@ -159,10 +159,13 @@ public function testGetAvatarPrivateScope() { ->method('get') ->with('valid-user') ->willReturn($user); + $folder = $this->createMock(ISimpleFolder::class); $this->appData - ->expects($this->never()) - ->method('getFolder'); + ->expects($this->once()) + ->method('getFolder') + ->with('valid-user') + ->willReturn($folder); $account = $this->createMock(IAccount::class); $this->accountManager->expects($this->once()) @@ -180,7 +183,7 @@ public function testGetAvatarPrivateScope() { ->method('getScope') ->willReturn(IAccountManager::SCOPE_PRIVATE); - $expected = new GuestAvatar('valid-user', $this->createMock(ILogger::class)); + $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); } } From aac7b52c66e767efb5b0a3dfcca589fdb4214fff Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 14:16:20 +0100 Subject: [PATCH 11/16] Change account property capability Include version number in capability Signed-off-by: Vincent Petry Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> --- apps/provisioning_api/lib/Capabilities.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/provisioning_api/lib/Capabilities.php b/apps/provisioning_api/lib/Capabilities.php index eaec844313f9a..398502ae31c4c 100644 --- a/apps/provisioning_api/lib/Capabilities.php +++ b/apps/provisioning_api/lib/Capabilities.php @@ -44,7 +44,7 @@ public function getCapabilities() { return [ 'provisioning_api' => [ 'version' => $this->appManager->getAppVersion('provisioning_api'), - 'hasAccountPropertyScopes' => true, + 'AccountPropertyScopesVersion' => 2, ] ]; } From 5b7a94f84c34437170d1049f8562aa2785b7e68d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 13:04:39 +0100 Subject: [PATCH 12/16] Update psalm-baseline for Avatar API quirks Signed-off-by: Vincent Petry --- build/psalm-baseline.xml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 1a28e2e885e0a..5ae21f9787f97 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2981,6 +2981,14 @@ InMemoryFile + + + ISimpleFile + + + $data + + ISimpleFile From cc54f718f561c73cf5e91a2a42cd8b1d878d02d2 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 14:14:14 +0100 Subject: [PATCH 13/16] Add known user check in avatar when v2-private scope Signed-off-by: Vincent Petry --- lib/private/Avatar/AvatarManager.php | 35 ++++--- lib/private/KnownUser/KnownUserService.php | 4 + lib/private/Server.php | 4 +- tests/lib/Avatar/AvatarManagerTest.php | 111 +++++++++++++++++---- 4 files changed, 119 insertions(+), 35 deletions(-) diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 92cd502dacb46..04d3a72102263 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -34,6 +34,7 @@ namespace OC\Avatar; +use OC\KnownUser\KnownUserService; use OC\User\Manager; use OC\User\NoUserException; use OCP\Accounts\IAccountManager; @@ -73,6 +74,9 @@ class AvatarManager implements IAvatarManager { /** @var IAccountManager */ private $accountManager; + /** @var KnownUserService */ + private $knownUserService; + /** * AvatarManager constructor. * @@ -90,7 +94,9 @@ public function __construct( IL10N $l, ILogger $logger, IConfig $config, - IAccountManager $accountManager) { + IAccountManager $accountManager, + KnownUserService $knownUserService + ) { $this->userSession = $userSession; $this->userManager = $userManager; $this->appData = $appData; @@ -98,6 +104,7 @@ public function __construct( $this->logger = $logger; $this->config = $config; $this->accountManager = $accountManager; + $this->knownUserService = $knownUserService; } /** @@ -128,17 +135,21 @@ public function getAvatar(string $userId) : IAvatar { $folder = $this->appData->newFolder($userId); } - // requesting in public page - if ($requestingUser === null) { - $account = $this->accountManager->getAccount($user); - $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); - $avatarScope = $avatarProperties->getScope(); - - // v2-private scope hides the avatar from public access - if ($avatarScope === IAccountManager::SCOPE_PRIVATE) { - // use a placeholder avatar which caches the generated images - return new PlaceholderAvatar($folder, $user, $this->logger); - } + $account = $this->accountManager->getAccount($user); + $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); + $avatarScope = $avatarProperties->getScope(); + + if ( + // v2-private scope hides the avatar from public access and from unknown users + $avatarScope === IAccountManager::SCOPE_PRIVATE + && ( + // accessing from public link + $requestingUser === null + // logged in, but unknown to user + || !$this->knownUserService->isKnownToUser($requestingUser->getUID(), $userId) + )) { + // use a placeholder avatar which caches the generated images + return new PlaceholderAvatar($folder, $user, $this->logger); } return new UserAvatar($folder, $this->l, $user, $this->logger, $this->config); diff --git a/lib/private/KnownUser/KnownUserService.php b/lib/private/KnownUser/KnownUserService.php index 96af21c836f66..1f300a9f8e4c2 100644 --- a/lib/private/KnownUser/KnownUserService.php +++ b/lib/private/KnownUser/KnownUserService.php @@ -74,6 +74,10 @@ public function storeIsKnownToUser(string $knownTo, string $contactUserId): void * @return bool */ public function isKnownToUser(string $knownTo, string $contactUserId): bool { + if ($knownTo === $contactUserId) { + return true; + } + if (!isset($this->knownUsers[$knownTo])) { $entities = $this->mapper->getKnownUsers($knownTo); $this->knownUsers[$knownTo] = []; diff --git a/lib/private/Server.php b/lib/private/Server.php index 93ad3b389972c..26c76125e56ac 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -104,6 +104,7 @@ use OC\IntegrityCheck\Helpers\AppLocator; use OC\IntegrityCheck\Helpers\EnvironmentHelper; use OC\IntegrityCheck\Helpers\FileAccessHelper; +use OC\KnownUser\KnownUserService; use OC\Lock\DBLockingProvider; use OC\Lock\MemcacheLockingProvider; use OC\Lock\NoopLockingProvider; @@ -726,7 +727,8 @@ public function __construct($webRoot, \OC\Config $config) { $c->getL10N('lib'), $c->get(ILogger::class), $c->get(\OCP\IConfig::class), - $c->get(IAccountManager::class) + $c->get(IAccountManager::class), + $c->get(KnownUserService::class) ); }); $this->registerAlias(IAvatarManager::class, AvatarManager::class); diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 1b06e786fb1c4..d3bc60efb59a8 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -27,6 +27,7 @@ use OC\Avatar\AvatarManager; use OC\Avatar\PlaceholderAvatar; use OC\Avatar\UserAvatar; +use OC\KnownUser\KnownUserService; use OC\User\Manager; use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; @@ -59,6 +60,8 @@ class AvatarManagerTest extends \Test\TestCase { private $accountManager; /** @var AvatarManager | \PHPUnit\Framework\MockObject\MockObject */ private $avatarManager; + /** @var KnownUserService | \PHPUnit\Framework\MockObject\MockObject */ + private $knownUserService; protected function setUp(): void { parent::setUp(); @@ -70,6 +73,7 @@ protected function setUp(): void { $this->logger = $this->createMock(ILogger::class); $this->config = $this->createMock(IConfig::class); $this->accountManager = $this->createMock(IAccountManager::class); + $this->knownUserService = $this->createMock(KnownUserService::class); $this->avatarManager = new AvatarManager( $this->userSession, @@ -78,7 +82,8 @@ protected function setUp(): void { $this->l10n, $this->logger, $this->config, - $this->accountManager + $this->accountManager, + $this->knownUserService ); } @@ -95,26 +100,45 @@ public function testGetAvatarInvalidUser() { $this->avatarManager->getAvatar('invalidUser'); } - public function testGetAvatarValidUser() { - // requesting user - $this->userSession->expects($this->once()) - ->method('getUser') - ->willReturn($this->createMock(IUser::class)); - - // we skip account scope check for logged in user - $this->accountManager->expects($this->never()) - ->method('getAccount'); - + public function testGetAvatarForSelf() { $user = $this->createMock(IUser::class); $user - ->expects($this->once()) + ->expects($this->any()) ->method('getUID') ->willReturn('valid-user'); + + // requesting user + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $this->userManager ->expects($this->once()) ->method('get') ->with('valid-user') ->willReturn($user); + + $account = $this->createMock(IAccount::class); + $this->accountManager->expects($this->once()) + ->method('getAccount') + ->with($user) + ->willReturn($account); + + $property = $this->createMock(IAccountProperty::class); + $account->expects($this->once()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_AVATAR) + ->willReturn($property); + + $property->expects($this->once()) + ->method('getScope') + ->willReturn(IAccountManager::SCOPE_PRIVATE); + + $this->knownUserService->expects($this->any()) + ->method('isKnownToUser') + ->with('valid-user', 'valid-user') + ->willReturn(true); + $folder = $this->createMock(ISimpleFolder::class); $this->appData ->expects($this->once()) @@ -148,7 +172,36 @@ public function testGetAvatarValidUserDifferentCasing() { $this->assertEquals($expected, $this->avatarManager->getAvatar('vaLid-USER')); } - public function testGetAvatarPrivateScope() { + public function knownUnknownProvider() { + return [ + [IAccountManager::SCOPE_LOCAL, false, false, false], + [IAccountManager::SCOPE_LOCAL, true, false, false], + + // public access cannot see real avatar + [IAccountManager::SCOPE_PRIVATE, true, false, true], + // unknown users cannot see real avatar + [IAccountManager::SCOPE_PRIVATE, false, false, true], + // known users can see real avatar + [IAccountManager::SCOPE_PRIVATE, false, true, false], + ]; + } + + /** + * @dataProvider knownUnknownProvider + */ + public function testGetAvatarScopes($avatarScope, $isPublicCall, $isKnownUser, $expectedPlaceholder) { + if ($isPublicCall) { + $requestingUser = null; + } else { + $requestingUser = $this->createMock(IUser::class); + $requestingUser->method('getUID')->willReturn('requesting-user'); + } + + // requesting user + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($requestingUser); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) @@ -160,13 +213,6 @@ public function testGetAvatarPrivateScope() { ->with('valid-user') ->willReturn($user); - $folder = $this->createMock(ISimpleFolder::class); - $this->appData - ->expects($this->once()) - ->method('getFolder') - ->with('valid-user') - ->willReturn($folder); - $account = $this->createMock(IAccount::class); $this->accountManager->expects($this->once()) ->method('getAccount') @@ -181,9 +227,30 @@ public function testGetAvatarPrivateScope() { $property->expects($this->once()) ->method('getScope') - ->willReturn(IAccountManager::SCOPE_PRIVATE); + ->willReturn($avatarScope); + + $folder = $this->createMock(ISimpleFolder::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('valid-user') + ->willReturn($folder); + + if (!$isPublicCall) { + $this->knownUserService->expects($this->any()) + ->method('isKnownToUser') + ->with('requesting-user', 'valid-user') + ->willReturn($isKnownUser); + } else { + $this->knownUserService->expects($this->never()) + ->method('isKnownToUser'); + } - $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); + if ($expectedPlaceholder) { + $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); + } else { + $expected = new UserAvatar($folder, $this->l10n, $user, $this->logger, $this->config); + } $this->assertEquals($expected, $this->avatarManager->getAvatar('valid-user')); } } From 98e4ac39bc0de7dc02cc19f13f0d3b070577f249 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 15:30:41 +0100 Subject: [PATCH 14/16] Int tests for provisioning API scopes Added integration tests for the scope attributes in the provisioning API. Signed-off-by: Vincent Petry --- .../features/bootstrap/Provisioning.php | 6 +- .../features/provisioning-v1.feature | 77 ++++++++++++++++++- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/build/integration/features/bootstrap/Provisioning.php b/build/integration/features/bootstrap/Provisioning.php index 0ec19f27c6086..cbe11403ba820 100644 --- a/build/integration/features/bootstrap/Provisioning.php +++ b/build/integration/features/bootstrap/Provisioning.php @@ -157,7 +157,11 @@ public function userHasSetting($user, $settings) { $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user"; $client = new Client(); $options = []; - $options['auth'] = $this->adminUser; + if ($this->currentUser === 'admin') { + $options['auth'] = $this->adminUser; + } else { + $options['auth'] = [$this->currentUser, $this->regularUser]; + } $options['headers'] = [ 'OCS-APIREQUEST' => 'true', ]; diff --git a/build/integration/features/provisioning-v1.feature b/build/integration/features/provisioning-v1.feature index 717aa04e4bdbc..03aaad4b85724 100644 --- a/build/integration/features/provisioning-v1.feature +++ b/build/integration/features/provisioning-v1.feature @@ -103,6 +103,82 @@ Feature: provisioning | website | https://nextcloud.com | | twitter | Nextcloud | + Scenario: Edit a user account properties scopes + Given user "brand-new-user" exists + And As an "brand-new-user" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | phoneScope | + | value | v2-private | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | twitterScope | + | value | v2-local | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | addressScope | + | value | v2-federated | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | emailScope | + | value | v2-published | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | websiteScope | + | value | public | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | displaynameScope | + | value | contacts | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | avatarScope | + | value | private | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + Then user "brand-new-user" has + | id | brand-new-user | + | phoneScope | v2-private | + | twitterScope | v2-local | + | addressScope | v2-federated | + | emailScope | v2-published | + | websiteScope | v2-published | + | displaynameScope | v2-federated | + | avatarScope | v2-local | + + Scenario: Edit a user account properties scopes with invalid or unsupported value + Given user "brand-new-user" exists + And As an "brand-new-user" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | phoneScope | + | value | invalid | + Then the OCS status code should be "102" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | displaynameScope | + | value | v2-private | + Then the OCS status code should be "102" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | emailScope | + | value | v2-private | + Then the OCS status code should be "102" + And the HTTP status code should be "200" + + Scenario: An admin cannot edit user account property scopes + Given As an "admin" + And user "brand-new-user" exists + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | phoneScope | + | value | v2-private | + Then the OCS status code should be "997" + And the HTTP status code should be "401" + Scenario: Search by phone number Given As an "admin" And user "phone-user" exists @@ -612,4 +688,3 @@ Feature: provisioning And As an "user0" When sending "GET" with exact url to "/index.php/apps/files" And the HTTP status code should be "403" - From 5a8b7c1cb21a72c4d43f1e842f52a237d0abfd12 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 25 Mar 2021 15:57:24 +0100 Subject: [PATCH 15/16] Update autoloader for PlaceholderAvatar Signed-off-by: Vincent Petry --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index bf07152256d0f..9f4973e50f08e 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -734,6 +734,7 @@ 'OC\\Avatar\\Avatar' => $baseDir . '/lib/private/Avatar/Avatar.php', 'OC\\Avatar\\AvatarManager' => $baseDir . '/lib/private/Avatar/AvatarManager.php', 'OC\\Avatar\\GuestAvatar' => $baseDir . '/lib/private/Avatar/GuestAvatar.php', + 'OC\\Avatar\\PlaceholderAvatar' => $baseDir . '/lib/private/Avatar/PlaceholderAvatar.php', 'OC\\Avatar\\UserAvatar' => $baseDir . '/lib/private/Avatar/UserAvatar.php', 'OC\\BackgroundJob\\Job' => $baseDir . '/lib/private/BackgroundJob/Job.php', 'OC\\BackgroundJob\\JobList' => $baseDir . '/lib/private/BackgroundJob/JobList.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index e5fe0d55bfd87..9fcfb40b9bae6 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -763,6 +763,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Avatar\\Avatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/Avatar.php', 'OC\\Avatar\\AvatarManager' => __DIR__ . '/../../..' . '/lib/private/Avatar/AvatarManager.php', 'OC\\Avatar\\GuestAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/GuestAvatar.php', + 'OC\\Avatar\\PlaceholderAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/PlaceholderAvatar.php', 'OC\\Avatar\\UserAvatar' => __DIR__ . '/../../..' . '/lib/private/Avatar/UserAvatar.php', 'OC\\BackgroundJob\\Job' => __DIR__ . '/../../..' . '/lib/private/BackgroundJob/Job.php', 'OC\\BackgroundJob\\JobList' => __DIR__ . '/../../..' . '/lib/private/BackgroundJob/JobList.php', From 00d83a5db056a9c466831fd45347f414bd81f3ae Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 26 Mar 2021 16:54:47 +0100 Subject: [PATCH 16/16] Capability for federated scope Added additional capability in the provisioning API to signal whether the federation scope values can be used. This is based on whether the lookup server upload is enabled or not. Signed-off-by: Vincent Petry --- apps/provisioning_api/lib/Capabilities.php | 11 +++ .../tests/CapabilitiesTest.php | 92 +++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 apps/provisioning_api/tests/CapabilitiesTest.php diff --git a/apps/provisioning_api/lib/Capabilities.php b/apps/provisioning_api/lib/Capabilities.php index 398502ae31c4c..d355e4db4c2cb 100644 --- a/apps/provisioning_api/lib/Capabilities.php +++ b/apps/provisioning_api/lib/Capabilities.php @@ -23,6 +23,7 @@ namespace OCA\Provisioning_API; +use OCA\FederatedFileSharing\FederatedShareProvider; use OCP\App\IAppManager; use OCP\Capabilities\ICapability; @@ -41,10 +42,20 @@ public function __construct(IAppManager $appManager) { * @return array Array containing the apps capabilities */ public function getCapabilities() { + $federationScopesEnabled = false; + + $federatedFileSharingEnabled = $this->appManager->isEnabledForUser('federatedfilesharing'); + if ($federatedFileSharingEnabled) { + /** @var FederatedShareProvider $shareProvider */ + $shareProvider = \OC::$server->query(FederatedShareProvider::class); + $federationScopesEnabled = $shareProvider->isLookupServerUploadEnabled(); + } + return [ 'provisioning_api' => [ 'version' => $this->appManager->getAppVersion('provisioning_api'), 'AccountPropertyScopesVersion' => 2, + 'AccountPropertyScopesFederationEnabled' => $federationScopesEnabled, ] ]; } diff --git a/apps/provisioning_api/tests/CapabilitiesTest.php b/apps/provisioning_api/tests/CapabilitiesTest.php new file mode 100644 index 0000000000000..89a4215d27350 --- /dev/null +++ b/apps/provisioning_api/tests/CapabilitiesTest.php @@ -0,0 +1,92 @@ + + * + * @author Vincent Petry + * + * @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 . + * + */ + +namespace OCA\Provisioning_API\Tests\unit; + +use OCA\FederatedFileSharing\FederatedShareProvider; +use OCA\Provisioning_API\Capabilities; +use OCP\App\IAppManager; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +/** + * Capabilities test for provisioning API. + * + * Note: group DB needed because of usage of overwriteService() + * + * @package OCA\Provisioning_API\Tests + * @group DB + */ +class CapabilitiesTest extends TestCase { + + /** @var Capabilities */ + protected $capabilities; + + /** @var IAppManager|MockObject */ + protected $appManager; + + public function setUp(): void { + parent::setUp(); + $this->appManager = $this->createMock(IAppManager::class); + $this->capabilities = new Capabilities($this->appManager); + + $this->appManager->expects($this->once()) + ->method('getAppVersion') + ->with('provisioning_api') + ->willReturn('1.12'); + } + + public function getCapabilitiesProvider() { + return [ + [false, false, false], + [true, false, false], + [true, true, true], + ]; + } + + /** + * @dataProvider getCapabilitiesProvider + */ + public function testGetCapabilities($federationAppEnabled, $lookupServerEnabled, $expectedFederationScopesEnabled) { + $this->appManager->expects($this->once()) + ->method('isEnabledForUser') + ->with('federatedfilesharing') + ->willReturn($federationAppEnabled); + + $federatedShareProvider = $this->createMock(FederatedShareProvider::class); + $this->overwriteService(FederatedShareProvider::class, $federatedShareProvider); + + $federatedShareProvider->expects($this->any()) + ->method('isLookupServerUploadEnabled') + ->willReturn($lookupServerEnabled); + + $expected = [ + 'provisioning_api' => [ + 'version' => '1.12', + 'AccountPropertyScopesVersion' => 2, + 'AccountPropertyScopesFederationEnabled' => $expectedFederationScopesEnabled, + ], + ]; + $this->assertSame($expected, $this->capabilities->getCapabilities()); + } +}