From f45f826b5262aefdeb97eb127ff65b647b19578b 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 170aec15a8..1c854a7ee5 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 9cefaf132f..293989baa0 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 5102396224..03f3d89e5f 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -36,6 +36,7 @@ namespace OC\Avatar; 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\IAvatarManager; 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 @@ class AvatarManager implements IAvatarManager { // 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 c0d6afbaaf..93ad3b3899 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -720,11 +720,13 @@ class Server extends ServerContainer implements IServerContainer { $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 ae70d8963b..9d720ba9e5 100644 --- a/lib/public/Accounts/IAccountManager.php +++ b/lib/public/Accounts/IAccountManager.php @@ -38,11 +38,54 @@ use OCP\IUser; */ 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 5a061cd10e..0ce0e75256 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 @@ class AvatarManagerTest extends \Test\TestCase { } 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 @@ class AvatarManagerTest extends \Test\TestCase { $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 e2ab530ee37c6bcb363c64c33f983ee13d1b8d4f 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 1c854a7ee5..b94e1686a4 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 293989baa0..759bf85c3e 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 a6055fd7a9..e71f484012 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 84198b3c0c..60db8c8533 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -34,8 +34,8 @@ script('settings', [ ]); ?> -
-
+
+

@@ -68,9 +68,7 @@ script('settings', [

- -
@@ -125,7 +123,7 @@ script('settings', [ - +
@@ -198,9 +196,7 @@ script('settings', [ autocomplete="on" autocapitalize="none" autocorrect="off" /> - -
@@ -223,9 +219,7 @@ script('settings', [ autocomplete="on" autocapitalize="none" autocorrect="off" /> - -
@@ -279,9 +273,7 @@ script('settings', [ /> - -
@@ -335,9 +327,7 @@ script('settings', [ /> - - From 5d76574a81197e00cf4d0e781d41a311a76f37fc 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 59e5401d05..95ac43aba3 100644 --- a/apps/dav/lib/CardDAV/Converter.php +++ b/apps/dav/lib/CardDAV/Converter.php @@ -71,8 +71,8 @@ class Converter { 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 aef5cf8ef1..4334445143 100644 --- a/apps/dav/tests/unit/CardDAV/ConverterTest.php +++ b/apps/dav/tests/unit/CardDAV/ConverterTest.php @@ -53,36 +53,36 @@ class ConverterTest extends TestCase { 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 eb8186807c..724670bc98 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -136,36 +136,36 @@ class SyncServiceTest extends TestCase { 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 31f13ee275..7e83ffaa7d 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -343,7 +343,7 @@ class ShareController extends AuthPublicShareController { $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 270f38a114..e00d6bc8c6 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -234,7 +234,7 @@ class ShareControllerTest extends \Test\TestCase { $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 @@ class ShareControllerTest extends \Test\TestCase { $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 @@ class ShareControllerTest extends \Test\TestCase { $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 @@ class ShareControllerTest extends \Test\TestCase { $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 889fcfd627..c462eeedb4 100644 --- a/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php +++ b/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php @@ -193,7 +193,7 @@ class RetryJob extends Job { $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 b94e1686a4..617a8e3d41 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 759bf85c3e..cf7f464890 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 a853846fad..7a0253d2be 100644 --- a/apps/settings/lib/Settings/Personal/PersonalInfo.php +++ b/apps/settings/lib/Settings/Personal/PersonalInfo.php @@ -37,6 +37,7 @@ namespace OCA\Settings\Settings\Personal; 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 @@ class PersonalInfo implements ISettings { $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 @@ class PersonalInfo implements ISettings { $languageParameters = $this->getLanguages($user); $localeParameters = $this->getLocales($user); - $messageParameters = $this->getMessageParameters($userData); + $messageParameters = $this->getMessageParameters($account); $parameters = [ 'total_space' => $totalSpace, @@ -119,23 +120,23 @@ class PersonalInfo implements ISettings { '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 @@ class PersonalInfo implements ISettings { } /** - * @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 b14e8d00d6..2daf383410 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -208,41 +208,41 @@ class UsersControllerTest extends \Test\TestCase { 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 @@ class UsersControllerTest extends \Test\TestCase { } $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 c5a0f21319..ff3b04d839 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -421,41 +421,41 @@ class AccountManager implements IAccountManager { 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 @@ class AccountManager implements IAccountManager { 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 97f9b1c356..4c75ad8541 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -26,6 +26,7 @@ declare(strict_types=1); 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 @@ class AccountProperty implements IAccountProperty { * @return IAccountProperty */ public function setScope(string $scope): IAccountProperty { - $this->scope = $scope; + $this->scope = $this->mapScopeToV2($scope); return $this; } @@ -127,6 +128,23 @@ class AccountProperty implements IAccountProperty { 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 9d720ba9e5..e88fd32f67 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 fcd1a78add..62da1cbc1d 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -278,26 +278,26 @@ class AccountManagerTest extends TestCase { 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 afd807a44b..f99abc21f8 100644 --- a/tests/lib/Accounts/AccountPropertyTest.php +++ b/tests/lib/Accounts/AccountPropertyTest.php @@ -37,12 +37,12 @@ class AccountPropertyTest extends TestCase { $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 @@ class AccountPropertyTest extends TestCase { $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 @@ class AccountPropertyTest extends TestCase { $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 @@ class AccountPropertyTest extends TestCase { $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 11b13637bd..8afcc44afd 100644 --- a/tests/lib/Accounts/AccountTest.php +++ b/tests/lib/Accounts/AccountTest.php @@ -43,21 +43,21 @@ class AccountTest extends TestCase { 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 @@ class AccountTest extends TestCase { 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 @@ class AccountTest extends TestCase { 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 @@ class AccountTest extends TestCase { ); $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 5c854ba13291c4eecb87ac7a5235304711e5cbbe 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 db4138054e..1c9b63a01c 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -50,7 +50,6 @@ use OC\Accounts\AccountManager; 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; @@ -84,8 +83,6 @@ class UsersController extends AUserData { protected $l10nFactory; /** @var NewUserMailHelper */ private $newUserMailHelper; - /** @var FederatedShareProviderFactory */ - private $federatedShareProviderFactory; /** @var ISecureRandom */ private $secureRandom; /** @var RemoteWipe */ @@ -107,7 +104,6 @@ class UsersController extends AUserData { ILogger $logger, IFactory $l10nFactory, NewUserMailHelper $newUserMailHelper, - FederatedShareProviderFactory $federatedShareProviderFactory, ISecureRandom $secureRandom, RemoteWipe $remoteWipe, KnownUserService $knownUserService, @@ -126,7 +122,6 @@ class UsersController extends AUserData { $this->logger = $logger; $this->l10nFactory = $l10nFactory; $this->newUserMailHelper = $newUserMailHelper; - $this->federatedShareProviderFactory = $federatedShareProviderFactory; $this->secureRandom = $secureRandom; $this->remoteWipe = $remoteWipe; $this->knownUserService = $knownUserService; @@ -519,15 +514,10 @@ class UsersController extends AUserData { $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); } @@ -573,15 +563,10 @@ class UsersController extends AUserData { $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 39743579b7..8f4e8395f2 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -46,9 +46,7 @@ use OC\Authentication\Token\RemoteWipe; 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 @@ class UsersControllerTest extends TestCase { $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 @@ class UsersControllerTest extends TestCase { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -407,7 +401,6 @@ class UsersControllerTest extends TestCase { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3246,7 +3239,6 @@ class UsersControllerTest extends TestCase { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3313,7 +3305,6 @@ class UsersControllerTest extends TestCase { $this->logger, $this->l10nFactory, $this->newUserMailHelper, - $this->federatedShareProviderFactory, $this->secureRandom, $this->remoteWipe, $this->knownUserService, @@ -3638,18 +3629,13 @@ class UsersControllerTest extends TestCase { 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, @@ -3664,27 +3650,15 @@ class UsersControllerTest extends TestCase { * @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 02a0cda139..c59c16666f 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -46,7 +46,6 @@ use OC\KnownUser\KnownUserService; 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 @@ class UsersController extends Controller { $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 60db8c8533..f2e3a51aad 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -177,7 +177,6 @@ script('settings', [ -

@@ -188,9 +187,7 @@ script('settings', [

- + @@ -199,8 +196,6 @@ script('settings', [ - -

@@ -211,9 +206,7 @@ script('settings', [

- + @@ -222,8 +215,6 @@ script('settings', [ - -

@@ -267,17 +258,12 @@ script('settings', [ />

- -

@@ -321,16 +307,12 @@ script('settings', [ />

-
diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 2daf383410..f9652053de 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -190,6 +190,7 @@ class UsersControllerTest extends \Test\TestCase { 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 b9d59e2994710bbf7818738ef21ba040a143f42c 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 b7b31b18b5..73909a319f 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -50,6 +50,7 @@ use OCP\User\Backend\ISetDisplayNameBackend; use OCP\User\Backend\ISetPasswordBackend; abstract class AUserData extends OCSController { + public const SCOPE_SUFFIX = 'Scope'; /** @var IUserManager */ protected $userManager; @@ -86,12 +87,13 @@ abstract class AUserData extends OCSController { * 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 = []; @@ -114,7 +116,7 @@ abstract class AUserData extends OCSController { } // Get groups data - $userAccount = $this->accountManager->getUser($targetUserObject); + $userAccount = $this->accountManager->getAccount($targetUserObject); $groups = $this->groupManager->getUserGroups($targetUserObject); $gids = []; foreach ($groups as $group) { @@ -137,11 +139,26 @@ abstract class AUserData extends OCSController { $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 1c9b63a01c..eb0bf978df 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -470,7 +470,13 @@ class UsersController extends AUserData { * @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('', \OCP\API::RESPOND_UNAUTHORISED); @@ -490,7 +496,7 @@ class UsersController extends AUserData { 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']; @@ -552,6 +558,9 @@ class UsersController extends AUserData { $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())) { @@ -567,6 +576,10 @@ class UsersController extends AUserData { $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())) { @@ -671,6 +684,23 @@ class UsersController extends AUserData { } } 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 ff3b04d839..74ba53737c 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -144,6 +144,37 @@ class AccountManager implements IAccountManager { } } + $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 @@ class AccountManager implements IAccountManager { } $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 4c75ad8541..850f39df9e 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -128,7 +128,7 @@ class AccountProperty implements IAccountProperty { return $this->scope; } - private function mapScopeToV2($scope) { + public static function mapScopeToV2($scope) { if (strpos($scope, 'v2-') === 0) { return $scope; } From ad402ffc969138efaa979390492b3b3ded5d6f17 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 73909a319f..1280ef2136 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -138,6 +138,11 @@ abstract class AUserData extends OCSController { $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(); @@ -159,6 +164,7 @@ abstract class AUserData extends OCSController { $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 eb0bf978df..f933cd4cd7 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -581,6 +581,8 @@ class UsersController extends AUserData { $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'; @@ -690,6 +692,7 @@ class UsersController extends AUserData { 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 2fd62b4f0dc71129ad39c44648fa63301608e455 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 8f4e8395f2..ca60304465 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -48,7 +48,9 @@ use OC\KnownUser\KnownUserService; 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; @@ -926,7 +928,6 @@ class UsersControllerTest extends TestCase { ->disableOriginalConstructor() ->getMock(); $this->userSession - ->expects($this->once()) ->method('getUser') ->willReturn($loggedInUser); $this->userManager @@ -996,16 +997,13 @@ class UsersControllerTest extends TestCase { $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') @@ -1165,16 +1163,13 @@ class UsersControllerTest extends TestCase { $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()) @@ -1217,14 +1212,13 @@ class UsersControllerTest extends TestCase { ->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 @@ -1336,16 +1330,12 @@ class UsersControllerTest extends TestCase { ->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()) @@ -1530,6 +1520,88 @@ class UsersControllerTest extends TestCase { $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() @@ -3247,7 +3319,7 @@ class UsersControllerTest extends TestCase { ->setMethods(['getUserData']) ->getMock(); - $api->expects($this->once())->method('getUserData')->with('UID') + $api->expects($this->once())->method('getUserData')->with('UID', true) ->willReturn( [ 'id' => 'UID', @@ -3288,8 +3360,15 @@ class UsersControllerTest extends TestCase { $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([ @@ -3325,11 +3404,16 @@ class UsersControllerTest extends TestCase { '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()); } @@ -3663,4 +3747,23 @@ class UsersControllerTest extends TestCase { $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 73ec32d19bb1bc58f856e4f5bb4034204680fd36 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 ca60304465..2b4ca78062 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -1563,6 +1563,9 @@ class UsersControllerTest extends TestCase { 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 74ba53737c..6198f8dbdd 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -165,13 +165,18 @@ class AccountManager implements IAccountManager { $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 @@ class AccountManager implements IAccountManager { * * @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 62da1cbc1d..27ebed6979 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -153,6 +153,129 @@ class AccountManagerTest extends TestCase { ]; } + 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 2613826fccb9ad9e6571a07141391601b28d513f 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 e94a97c194..22927806e6 100644 --- a/apps/provisioning_api/composer/composer/autoload_classmap.php +++ b/apps/provisioning_api/composer/composer/autoload_classmap.php @@ -8,6 +8,7 @@ $baseDir = $vendorDir; 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 b982f20321..f5a4b73f4f 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 7ec21c3329..af6b2b3371 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 @@ class Application extends App implements IBootstrap { ); }); $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 0000000000..eaec844313 --- /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 ab22999eb9d7eb684e31d6e5618241eadfd7e370 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 03f3d89e5f..92cd502dac 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -122,7 +122,11 @@ class AvatarManager implements IAvatarManager { $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 @@ class AvatarManager implements IAvatarManager { // 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 0000000000..5883fe531a --- /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 f7ace429f7..f47809425e 100644 --- a/lib/private/Avatar/UserAvatar.php +++ b/lib/private/Avatar/UserAvatar.php @@ -270,6 +270,7 @@ class UserAvatar extends Avatar { 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 0ce0e75256..1b06e786fb 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 @@ class AvatarManagerTest extends \Test\TestCase { ->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 @@ class AvatarManagerTest extends \Test\TestCase { ->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 ecae7141577f60aa337533c26314bb1afeff43c4 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 eaec844313..398502ae31 100644 --- a/apps/provisioning_api/lib/Capabilities.php +++ b/apps/provisioning_api/lib/Capabilities.php @@ -44,7 +44,7 @@ class Capabilities implements ICapability { return [ 'provisioning_api' => [ 'version' => $this->appManager->getAppVersion('provisioning_api'), - 'hasAccountPropertyScopes' => true, + 'AccountPropertyScopesVersion' => 2, ] ]; } From 92ff94083bc546e714f9447907c877a137476c13 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 de3c0e83cc..65a317495b 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3155,6 +3155,14 @@ InMemoryFile + + + ISimpleFile + + + $data + + ISimpleFile From ec492eadfa157a765e6ddf7fb9b64963cf64ef54 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 | 33 ++++-- lib/private/KnownUser/KnownUserService.php | 4 + lib/private/Server.php | 4 +- tests/lib/Avatar/AvatarManagerTest.php | 111 +++++++++++++++++---- 4 files changed, 118 insertions(+), 34 deletions(-) diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 92cd502dac..04d3a72102 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -34,6 +34,7 @@ declare(strict_types=1); 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 @@ class AvatarManager implements IAvatarManager { 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 @@ class AvatarManager implements IAvatarManager { $this->logger = $logger; $this->config = $config; $this->accountManager = $accountManager; + $this->knownUserService = $knownUserService; } /** @@ -128,17 +135,21 @@ class AvatarManager implements IAvatarManager { $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(); + $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); - } + 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 96af21c836..1f300a9f8e 100644 --- a/lib/private/KnownUser/KnownUserService.php +++ b/lib/private/KnownUser/KnownUserService.php @@ -74,6 +74,10 @@ class KnownUserService { * @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 93ad3b3899..26c76125e5 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -104,6 +104,7 @@ use OC\IntegrityCheck\Checker; 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 @@ class Server extends ServerContainer implements IServerContainer { $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 1b06e786fb..d3bc60efb5 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -27,6 +27,7 @@ namespace Test\Avatar; 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 @@ class AvatarManagerTest extends \Test\TestCase { $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 @@ class AvatarManagerTest extends \Test\TestCase { $this->l10n, $this->logger, $this->config, - $this->accountManager + $this->accountManager, + $this->knownUserService ); } @@ -95,26 +100,45 @@ class AvatarManagerTest extends \Test\TestCase { $this->avatarManager->getAvatar('invalidUser'); } - public function testGetAvatarValidUser() { + public function testGetAvatarForSelf() { + $user = $this->createMock(IUser::class); + $user + ->expects($this->any()) + ->method('getUID') + ->willReturn('valid-user'); + // requesting user $this->userSession->expects($this->once()) ->method('getUser') - ->willReturn($this->createMock(IUser::class)); + ->willReturn($user); - // 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()) - ->method('getUID') - ->willReturn('valid-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 @@ class AvatarManagerTest extends \Test\TestCase { $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 @@ class AvatarManagerTest extends \Test\TestCase { ->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 @@ class AvatarManagerTest extends \Test\TestCase { $property->expects($this->once()) ->method('getScope') - ->willReturn(IAccountManager::SCOPE_PRIVATE); + ->willReturn($avatarScope); - $expected = new PlaceholderAvatar($folder, $user, $this->createMock(ILogger::class)); + $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'); + } + + 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 9ca91c15db5b6049b66477ed46d56d7d9332bd48 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 0ec19f27c6..cbe11403ba 100644 --- a/build/integration/features/bootstrap/Provisioning.php +++ b/build/integration/features/bootstrap/Provisioning.php @@ -157,7 +157,11 @@ trait Provisioning { $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 717aa04e4b..03aaad4b85 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 6cb5f36cf4264f548e646f843a7c8169268de54a 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 3c538a2a59..da9e5ffa6b 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -738,6 +738,7 @@ return array( '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 0b1967e91d..c0e023b9f8 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -767,6 +767,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 15e52fa7cbc1bce648b325f7873d7366adab8c03 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 398502ae31..d355e4db4c 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 @@ class Capabilities implements ICapability { * @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 0000000000..89a4215d27 --- /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()); + } +}