From c7a2b855df8da1afa4377494dd8565679c286dd7 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 25 Nov 2021 12:38:44 +0100 Subject: [PATCH] Fix encryption issues after the impersonator logs out --- appinfo/info.xml | 1 + lib/Util.php | 45 +++++++++++++++++---- tests/lib/UtilTest.php | 90 ++++++++++++++++++++++++++++-------------- 3 files changed, 100 insertions(+), 36 deletions(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index bc5db15..989aa95 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -13,6 +13,7 @@ As a security measure, the application lets ownCloud administrators restrict the AGPL Jörn Friedrich Dreyer, Sujith Haridasan 0.5.0 + Impersonate https://doc.owncloud.org/server/latest/admin_manual/issues/impersonate_users.html diff --git a/lib/Util.php b/lib/Util.php index 9bd8f1d..526e33c 100644 --- a/lib/Util.php +++ b/lib/Util.php @@ -1,8 +1,9 @@ + * @author Jannik Stehle * - * @copyright Copyright (c) 2017, ownCloud GmbH + * @copyright Copyright (c) 2021, ownCloud GmbH * @license AGPL-3.0 * * This code is free software: you can redistribute it and/or modify @@ -58,7 +59,10 @@ public function __construct( } /** - * Switch from admin/subAdmin $impersonator to $user + * Switch from admin/subAdmin $impersonator to $user (and vice versa). + * $impersonator is empty in case the impersonator switches back to their profile. + * The impersonator must be given via the $user attribute then. + * * @param IUser $user * @param mixed $impersonator */ @@ -66,15 +70,42 @@ public function switchUser(IUser $user, $impersonator) { $this->tokenProvider->invalidateToken($this->session->getId()); $this->userSession->setUser($user); $this->userSession->createSessionToken($this->request, $user->getUID(), $user->getUID()); - //Store the session var impersonator with the impersonator value - if (($this->session->get('impersonator') === null) && - ($impersonator !== null)) { - $this->session->set('impersonator', $impersonator); + + if ($impersonator !== null) { + // Store the session var impersonator with the impersonator value + if ($this->session->get('impersonator') === null) { + $this->session->set('impersonator', $impersonator); + } + + // Save the impersonator's encryption key in another session variable to reset it again later. + // This is neccessary because the "privateKey" var gets removed on logout. + $encryptionInitialized = $this->session->get('encryptionInitialized'); + if ($encryptionInitialized !== null) { + $this->session->set('impersonatorEncryptionInitialized', $encryptionInitialized); + } + + $privateKey = $this->session->get('privateKey'); + if ($privateKey !== null) { + $this->session->set('impersonatorPrivateKey', $privateKey); + } } - //Remove the session variable impersonator set as it is a logout if (($impersonator === '') || ($impersonator === null)) { + // Remove the session variable impersonator set as it is a logout $this->session->remove('impersonator'); + + // Reset encryption key for the impersonator. + $impersonatorEncryptionInitialized = $this->session->get('impersonatorEncryptionInitialized'); + if ($impersonatorEncryptionInitialized !== null) { + $this->session->set('encryptionInitialized', $impersonatorEncryptionInitialized); + $this->session->remove('impersonatorEncryptionInitialized'); + } + + $impersonatorPrivateKey = $this->session->get('impersonatorPrivateKey'); + if ($impersonatorPrivateKey !== null) { + $this->session->set('privateKey', $impersonatorPrivateKey); + $this->session->remove('impersonatorPrivateKey'); + } } } } diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 1aa28a5..321bc5d 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -1,8 +1,9 @@ + * @author Jannik Stehle * - * @copyright Copyright (c) 2017, ownCloud GmbH + * @copyright Copyright (c) 2021, ownCloud GmbH * @license AGPL-3.0 * * This code is free software: you can redistribute it and/or modify @@ -30,48 +31,79 @@ use Test\TestCase; class UtilTest extends TestCase { - /** @var ISession | \PHPUnit\Framework\MockObject\MockObject */ + /** @var ISession | \PHPUnit\Framework\MockObject\MockObject */ private $session; - /** @var DefaultTokenProvider | \PHPUnit\Framework\MockObject\MockObject */ + /** @var DefaultTokenProvider | \PHPUnit\Framework\MockObject\MockObject */ private $tokenProvider; - /** @var Session | \PHPUnit\Framework\MockObject\MockObject */ + /** @var Session | \PHPUnit\Framework\MockObject\MockObject */ private $userSession; - /** @var IRequest | \PHPUnit\Framework\MockObject\MockObject */ + /** @var IRequest | \PHPUnit\Framework\MockObject\MockObject */ private $request; - /** @var Util */ + /** @var Util */ private $util; + /** @var IUser | \PHPUnit\Framework\MockObject\MockObject */ + private $user; + public function setUp(): void { $this->session = $this->createMock(ISession::class); $this->tokenProvider = $this->createMock(DefaultTokenProvider::class); $this->userSession = $this->createMock(Session::class); $this->request = $this->createMock(IRequest::class); + $this->util = new Util($this->session, $this->userSession, $this->request, $this->tokenProvider); + $this->user = $this->createMock(IUser::class); parent::setUp(); } - public function impersonator() { - return [ - [null], - ['admin'] - ]; + public function testSwitchUserWithImpersonator() { + $impersonator = 'admin'; + $encryptionInitialized = 1; + $privateKey = 'privateKey'; + $this->session + ->expects($this->exactly(3)) + ->method('get') + ->withConsecutive( + ['impersonator'], + ['encryptionInitialized'], + ['privateKey'], + ) + ->willReturn(null, $encryptionInitialized, $privateKey); + $this->session + ->expects($this->exactly(3)) + ->method('set') + ->withConsecutive( + ['impersonator', $impersonator], + ['impersonatorEncryptionInitialized', $encryptionInitialized], + ['impersonatorPrivateKey', $privateKey], + ); + $this->util->switchUser($this->user, $impersonator); } - /** - * @dataProvider impersonator - * @param $impersonator - */ - public function testSwitchUser($impersonator) { - $user = $this->createMock(IUser::class); - $this->util = new Util($this->session, $this->userSession, $this->request, $this->tokenProvider); - if ($impersonator !== null) { - $this->session - ->method('get') - ->willReturn($impersonator); - } - $this->util->switchUser($user, $impersonator); - if ($impersonator === null) { - $this->assertEquals($this->session->exists('impersonator'), false); - } else { - $this->assertEquals($this->session->get('impersonator'), $impersonator); - } + public function testSwitchUserWithoutImpersonator() { + $encryptionInitialized = 1; + $privateKey = 'privateKey'; + $this->session + ->expects($this->exactly(2)) + ->method('get') + ->withConsecutive( + ['impersonatorEncryptionInitialized'], + ['impersonatorPrivateKey'], + ) + ->willReturn($encryptionInitialized, $privateKey); + $this->session + ->expects($this->exactly(2)) + ->method('set') + ->withConsecutive( + ['encryptionInitialized', $encryptionInitialized], + ['privateKey', $privateKey], + ); + $this->session + ->expects($this->exactly(3)) + ->method('remove') + ->withConsecutive( + ['impersonator'], + ['impersonatorEncryptionInitialized'], + ['impersonatorPrivateKey'], + ); + $this->util->switchUser($this->user, null); } }