Merge pull request #15936 from nextcloud/bugfix/15539/wronguser-apptoken-impersonation

Disable app token creation for impersonated people, ref #15539
This commit is contained in:
Roeland Jago Douma 2019-09-15 19:34:07 +02:00 committed by GitHub
commit 8137616b4b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 175 additions and 113 deletions

View file

@ -68,7 +68,7 @@ class UserHooks implements IHook {
/**
* @var IUserSession
*/
private $user;
private $userSession;
/**
* @var Util
*/
@ -93,7 +93,7 @@ class UserHooks implements IHook {
* @param IUserManager $userManager
* @param ILogger $logger
* @param Setup $userSetup
* @param IUserSession $user
* @param IUserSession $userSession
* @param Util $util
* @param Session $session
* @param Crypt $crypt
@ -103,7 +103,7 @@ class UserHooks implements IHook {
IUserManager $userManager,
ILogger $logger,
Setup $userSetup,
IUserSession $user,
IUserSession $userSession,
Util $util,
Session $session,
Crypt $crypt,
@ -113,7 +113,7 @@ class UserHooks implements IHook {
$this->userManager = $userManager;
$this->logger = $logger;
$this->userSetup = $userSetup;
$this->user = $user;
$this->userSession = $userSession;
$this->util = $util;
$this->session = $session;
$this->recovery = $recovery;
@ -253,7 +253,7 @@ class UserHooks implements IHook {
}
// Get existing decrypted private key
$user = $this->user->getUser();
$user = $this->userSession->getUser();
// current logged in user changes his own password
if ($user && $params['uid'] === $user->getUID()) {
@ -265,7 +265,7 @@ class UserHooks implements IHook {
// Save private key
if ($encryptedPrivateKey) {
$this->keyManager->setPrivateKey($this->user->getUser()->getUID(),
$this->keyManager->setPrivateKey($user->getUID(),
$this->crypt->generateHeader() . $encryptedPrivateKey);
} else {
$this->logger->error('Encryption could not update users encryption password');
@ -275,8 +275,8 @@ class UserHooks implements IHook {
// private key has not changed, only the passphrase
// used to decrypt it has changed
} else { // admin changed the password for a different user, create new keys and re-encrypt file keys
$user = $params['uid'];
$this->initMountPoints($user);
$userId = $params['uid'];
$this->initMountPoints($userId);
$recoveryPassword = isset($params['recoveryPassword']) ? $params['recoveryPassword'] : null;
$recoveryKeyId = $this->keyManager->getRecoveryKeyId();
@ -296,9 +296,9 @@ class UserHooks implements IHook {
// ...encryption was activated for the first time (no keys exists)
// ...the user doesn't have any files
if (
($this->recovery->isRecoveryEnabledForUser($user) && $recoveryPassword)
|| !$this->keyManager->userHasKeys($user)
|| !$this->util->userHasFiles($user)
($this->recovery->isRecoveryEnabledForUser($userId) && $recoveryPassword)
|| !$this->keyManager->userHasKeys($userId)
|| !$this->util->userHasFiles($userId)
) {
// backup old keys
@ -309,16 +309,16 @@ class UserHooks implements IHook {
$keyPair = $this->crypt->createKeyPair();
// Save public key
$this->keyManager->setPublicKey($user, $keyPair['publicKey']);
$this->keyManager->setPublicKey($userId, $keyPair['publicKey']);
// Encrypt private key with new password
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $newUserPassword, $user);
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $newUserPassword, $userId);
if ($encryptedKey) {
$this->keyManager->setPrivateKey($user, $this->crypt->generateHeader() . $encryptedKey);
$this->keyManager->setPrivateKey($userId, $this->crypt->generateHeader() . $encryptedKey);
if ($recoveryPassword) { // if recovery key is set we can re-encrypt the key files
$this->recovery->recoverUsersFiles($recoveryPassword, $user);
$this->recovery->recoverUsersFiles($recoveryPassword, $userId);
}
} else {
$this->logger->error('Encryption Could not update users encryption password');

View file

@ -46,10 +46,6 @@ class Recovery {
* @var Crypt
*/
protected $crypt;
/**
* @var ISecureRandom
*/
private $random;
/**
* @var KeyManager
*/
@ -58,10 +54,6 @@ class Recovery {
* @var IConfig
*/
private $config;
/**
* @var IStorage
*/
private $keyStorage;
/**
* @var View
*/
@ -72,29 +64,23 @@ class Recovery {
private $file;
/**
* @param IUserSession $user
* @param IUserSession $userSession
* @param Crypt $crypt
* @param ISecureRandom $random
* @param KeyManager $keyManager
* @param IConfig $config
* @param IStorage $keyStorage
* @param IFile $file
* @param View $view
*/
public function __construct(IUserSession $user,
public function __construct(IUserSession $userSession,
Crypt $crypt,
ISecureRandom $random,
KeyManager $keyManager,
IConfig $config,
IStorage $keyStorage,
IFile $file,
View $view) {
$this->user = ($user && $user->isLoggedIn()) ? $user->getUser() : false;
$this->user = ($userSession && $userSession->isLoggedIn()) ? $userSession->getUser() : false;
$this->crypt = $crypt;
$this->random = $random;
$this->keyManager = $keyManager;
$this->config = $config;
$this->keyStorage = $keyStorage;
$this->view = $view;
$this->file = $file;
}
@ -169,7 +155,7 @@ class Recovery {
* @return bool
*/
public function isRecoveryEnabledForUser($user = '') {
$uid = empty($user) ? $this->user->getUID() : $user;
$uid = $user === '' ? $this->user->getUID() : $user;
$recoveryMode = $this->config->getUserValue($uid,
'encryption',
'recoveryEnabled',

View file

@ -34,8 +34,10 @@ use OCP\AppFramework\Http;
use OCP\IL10N;
use OCP\IRequest;
use OCP\ISession;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
class SettingsControllerTest extends TestCase {
@ -63,6 +65,8 @@ class SettingsControllerTest extends TestCase {
/** @var \OCA\Encryption\Session|\PHPUnit_Framework_MockObject_MockObject */
private $sessionMock;
/** @var MockObject|IUser */
private $user;
/** @var \OCP\ISession|\PHPUnit_Framework_MockObject_MockObject */
private $ocSessionMock;
@ -94,28 +98,17 @@ class SettingsControllerTest extends TestCase {
$this->cryptMock = $this->getMockBuilder(Crypt::class)
->disableOriginalConstructor()->getMock();
$this->userSessionMock = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->setMethods([
'isLoggedIn',
'getUID',
'login',
'logout',
'setUser',
'getUser',
'canChangePassword',
])
->getMock();
$this->ocSessionMock = $this->getMockBuilder(ISession::class)->disableOriginalConstructor()->getMock();
$this->userSessionMock->expects($this->any())
$this->user = $this->createMock(IUser::class);
$this->user->expects($this->any())
->method('getUID')
->willReturn('testUserUid');
$this->userSessionMock = $this->createMock(IUserSession::class);
$this->userSessionMock->expects($this->any())
->method($this->anything())
->will($this->returnSelf());
->method('getUser')
->willReturn($this->user);
$this->sessionMock = $this->getMockBuilder(Session::class)
->disableOriginalConstructor()->getMock();
@ -146,7 +139,9 @@ class SettingsControllerTest extends TestCase {
$oldPassword = 'old';
$newPassword = 'new';
$this->userSessionMock->expects($this->once())->method('getUID')->willReturn('uid');
$this->user->expects($this->any())
->method('getUID')
->willReturn('uid');
$this->userManagerMock
->expects($this->exactly(2))

View file

@ -41,6 +41,7 @@ use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
/**
@ -79,6 +80,10 @@ class UserHooksTest extends TestCase {
* @var \PHPUnit_Framework_MockObject_MockObject
*/
private $userSessionMock;
/**
* @var MockObject|IUser
*/
private $user;
/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
@ -343,24 +348,15 @@ class UserHooksTest extends TestCase {
->disableOriginalConstructor()
->getMock();
$this->userSessionMock = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->setMethods([
'isLoggedIn',
'getUID',
'login',
'logout',
'setUser',
'getUser',
'canChangePassword'
])
->getMock();
$this->userSessionMock->expects($this->any())->method('getUID')->will($this->returnValue('testUser'));
$this->user = $this->createMock(IUser::class);
$this->user->expects($this->any())
->method('getUID')
->willReturn('testUser');
$this->userSessionMock = $this->createMock(IUserSession::class);
$this->userSessionMock->expects($this->any())
->method($this->anything())
->will($this->returnSelf());
->method('getUser')
->willReturn($this->user);
$utilMock = $this->getMockBuilder(Util::class)
->disableOriginalConstructor()

View file

@ -36,8 +36,10 @@ use OCA\Encryption\Recovery;
use OCP\Encryption\IFile;
use OCP\Encryption\Keys\IStorage;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Security\ISecureRandom;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
class RecoveryTest extends TestCase {
@ -54,6 +56,10 @@ class RecoveryTest extends TestCase {
* @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject
*/
private $userSessionMock;
/**
* @var MockObject|IUser
*/
private $user;
/**
* @var \OCA\Encryption\KeyManager|\PHPUnit_Framework_MockObject_MockObject
*/
@ -257,32 +263,22 @@ class RecoveryTest extends TestCase {
protected function setUp() {
parent::setUp();
$this->user = $this->createMock(IUser::class);
$this->user->expects($this->any())
->method('getUID')
->willReturn('admin');
$this->userSessionMock = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->setMethods([
'isLoggedIn',
'getUID',
'login',
'logout',
'setUser',
'getUser'
])
->getMock();
$this->userSessionMock->expects($this->any())->method('getUID')->will($this->returnValue('admin'));
$this->userSessionMock = $this->createMock(IUserSession::class);
$this->userSessionMock->expects($this->any())
->method($this->anything())
->will($this->returnSelf());
->method('getUser')
->willReturn($this->user);
$this->userSessionMock->expects($this->any())
->method('isLoggedIn')
->willReturn(true);
$this->cryptMock = $this->getMockBuilder(Crypt::class)->disableOriginalConstructor()->getMock();
/** @var \OCP\Security\ISecureRandom $randomMock */
$randomMock = $this->createMock(ISecureRandom::class);
$this->keyManagerMock = $this->getMockBuilder(KeyManager::class)->disableOriginalConstructor()->getMock();
$this->configMock = $this->createMock(IConfig::class);
/** @var \OCP\Encryption\Keys\IStorage $keyStorageMock */
$keyStorageMock = $this->createMock(IStorage::class);
$this->fileMock = $this->createMock(IFile::class);
$this->viewMock = $this->createMock(View::class);
@ -296,10 +292,8 @@ class RecoveryTest extends TestCase {
$this->instance = new Recovery($this->userSessionMock,
$this->cryptMock,
$randomMock,
$this->keyManagerMock,
$this->configMock,
$keyStorageMock,
$this->fileMock,
$this->viewMock);
}

View file

@ -36,8 +36,10 @@ use OCP\Files\Mount\IMountPoint;
use OCP\Files\Storage;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
class UtilTest extends TestCase {
@ -91,27 +93,20 @@ class UtilTest extends TestCase {
->getMock();
/** @var \OCP\ILogger $loggerMock */
$loggerMock = $this->createMock(ILogger::class);
/** @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject $userSessionMock */
$userSessionMock = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->setMethods([
'isLoggedIn',
'getUID',
'login',
'logout',
'setUser',
'getUser'
])
->getMock();
$userSessionMock->method('isLoggedIn')->will($this->returnValue(true));
$userSessionMock->method('getUID')->will($this->returnValue('admin'));
$user = $this->createMock(IUser::class);
$user->expects($this->any())
->method('getUID')
->willReturn('admin');
/** @var IUserSession|MockObject $userSessionMock */
$userSessionMock = $this->createMock(IUserSession::class);
$userSessionMock->expects($this->any())
->method($this->anything())
->will($this->returnSelf());
->method('getUser')
->willReturn($user);
$userSessionMock->expects($this->any())
->method('isLoggedIn')
->willReturn(true);
$this->configMock = $this->createMock(IConfig::class);

View file

@ -50,4 +50,24 @@ class DummyUserSession implements IUserSession {
public function isLoggedIn() {
return !is_null($this->user);
}
/**
* get getImpersonatingUserID
*
* @return string|null
* @since 17.0.0
*/
public function getImpersonatingUserID() : ?string {
return null;
}
/**
* set setImpersonatingUserID
*
* @since 17.0.0
*/
public function setImpersonatingUserID(bool $useCurrentUser = true): void {
//no OP
}
}

View file

@ -314,6 +314,29 @@ class Session implements IUserSession, Emitter {
return null;
}
/**
* @return mixed
*/
public function getImpersonatingUserID(): ?string {
return $this->session->get('oldUserId');
}
public function setImpersonatingUserID(bool $useCurrentUser = true): void {
if ($useCurrentUser === false) {
$this->session->remove('oldUserId');
return;
}
$currentUser = $this->getUser();
if ($currentUser === null) {
throw new \OC\User\NoUserException();
}
$this->session->set('oldUserId', $currentUser->getUID());
}
/**
* set the token id
*

View file

@ -42,6 +42,7 @@ namespace OCP;
interface IUserSession {
/**
* Do a user login
*
* @param string $user the username
* @param string $password the password
* @return bool true if successful
@ -52,6 +53,7 @@ interface IUserSession {
/**
* Logs the user out including all the session data
* Logout, destroys session
*
* @return void
* @since 6.0.0
*/
@ -80,4 +82,19 @@ interface IUserSession {
* @since 8.0.0
*/
public function isLoggedIn();
/**
* get getImpersonatingUserID
*
* @return string|null
* @since 18.0.0
*/
public function getImpersonatingUserID(): ?string;
/**
* set setImpersonatingUserID
*
* @since 18.0.0
*/
public function setImpersonatingUserID(bool $useCurrentUser = true): void;
}

View file

@ -44,6 +44,7 @@ use OCP\AppFramework\Http\JSONResponse;
use OCP\ILogger;
use OCP\IRequest;
use OCP\ISession;
use OCP\IUserSession;
use OCP\Security\ISecureRandom;
use OCP\Session\Exceptions\SessionNotAvailableException;
@ -55,6 +56,9 @@ class AuthSettingsController extends Controller {
/** @var ISession */
private $session;
/** IUserSession */
private $userSession;
/** @var string */
private $uid;
@ -77,6 +81,7 @@ class AuthSettingsController extends Controller {
* @param ISession $session
* @param ISecureRandom $random
* @param string|null $userId
* @param IUserSession $userSession
* @param IManager $activityManager
* @param RemoteWipe $remoteWipe
* @param ILogger $logger
@ -87,12 +92,14 @@ class AuthSettingsController extends Controller {
ISession $session,
ISecureRandom $random,
?string $userId,
IUserSession $userSession,
IManager $activityManager,
RemoteWipe $remoteWipe,
ILogger $logger) {
parent::__construct($appName, $request);
$this->tokenProvider = $tokenProvider;
$this->uid = $userId;
$this->userSession = $userSession;
$this->session = $session;
$this->random = $random;
$this->activityManager = $activityManager;
@ -114,6 +121,10 @@ class AuthSettingsController extends Controller {
} catch (SessionNotAvailableException $ex) {
return $this->getServiceNotAvailableResponse();
}
if ($this->userSession->getImpersonatingUserID() !== null)
{
return $this->getServiceNotAvailableResponse();
}
try {
$sessionToken = $this->tokenProvider->getToken($sessionId);

View file

@ -46,6 +46,9 @@ use OCP\IConfig;
class Security implements ISettings {
/** @var IInitialStateService */
private $initialStateService;
/** @var IUserManager */
private $userManager;
@ -61,11 +64,13 @@ class Security implements ISettings {
/** @var IConfig */
private $config;
public function __construct(IUserManager $userManager,
public function __construct(IInitialStateService $initialStateService,
IUserManager $userManager,
ProviderLoader $providerLoader,
IUserSession $userSession,
IConfig $config,
?string $UserId) {
$this->initialStateService = $initialStateService;
$this->userManager = $userManager;
$this->providerLoader = $providerLoader;
$this->userSession = $userSession;
@ -80,11 +85,18 @@ class Security implements ISettings {
$passwordChangeSupported = $user->canChangePassword();
}
$this->initialStateService->provideInitialState(
'settings',
'can_create_app_token',
$this->userSession->getImpersonatingUserID() !== null
);
return new TemplateResponse('settings', 'settings/personal/security', [
'passwordChangeSupported' => $passwordChangeSupported,
'twoFactorProviderData' => $this->getTwoFactorProviderData(),
'themedark' => $this->config->getUserValue($this->uid, 'accessibility', 'theme', false)
]);
}
public function getSection(): string {

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

View file

@ -28,7 +28,7 @@
@rename="rename"
@delete="deleteToken"
@wipe="wipeToken" />
<AuthTokenSetupDialogue :add="addNewToken" />
<AuthTokenSetupDialogue v-if="canCreateToken" :add="addNewToken" />
</div>
</template>
@ -63,7 +63,7 @@
props: {
tokens: {
type: Array,
requried: true,
required: true,
},
},
components: {

View file

@ -35,5 +35,6 @@ const View = Vue.extend(AuthTokenSection);
new View({
propsData: {
tokens: OCP.InitialState.loadState('settings', 'app_tokens'),
canCreateToken: OCP.InitialState.loadState('settings', 'can_create_app_token'),
}
}).$mount('#security-authtokens');

View file

@ -34,6 +34,7 @@ use OCP\AppFramework\Http\JSONResponse;
use OCP\ILogger;
use OCP\IRequest;
use OCP\ISession;
use OCP\IUserSession;
use OCP\Security\ISecureRandom;
use OCP\Session\Exceptions\SessionNotAvailableException;
use PHPUnit\Framework\MockObject\MockObject;
@ -49,6 +50,8 @@ class AuthSettingsControllerTest extends TestCase {
private $tokenProvider;
/** @var ISession|MockObject */
private $session;
/**@var IUserSession|MockObject */
private $userSession;
/** @var ISecureRandom|MockObject */
private $secureRandom;
/** @var IManager|MockObject */
@ -63,6 +66,7 @@ class AuthSettingsControllerTest extends TestCase {
$this->request = $this->createMock(IRequest::class);
$this->tokenProvider = $this->createMock(IProvider::class);
$this->session = $this->createMock(ISession::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->secureRandom = $this->createMock(ISecureRandom::class);
$this->activityManager = $this->createMock(IManager::class);
$this->remoteWipe = $this->createMock(RemoteWipe::class);
@ -76,6 +80,7 @@ class AuthSettingsControllerTest extends TestCase {
$this->session,
$this->secureRandom,
$this->uid,
$this->userSession,
$this->activityManager,
$this->remoteWipe,
$logger

View file

@ -25,6 +25,8 @@ declare(strict_types=1);
namespace Test\Settings\Personal;
use OC\Authentication\TwoFactorAuth\ProviderLoader;
use OCP\IInitialStateService;
use OCP\InitialStateService;
use OC\Settings\Personal\Security;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IConfig;
@ -36,6 +38,9 @@ use Test\TestCase;
class SecurityTest extends TestCase {
/** @var InitialStateService|MockObject */
private $initialStateService;
/** @var IUserManager|MockObject */
private $userManager;
@ -57,6 +62,7 @@ class SecurityTest extends TestCase {
public function setUp() {
parent::setUp();
$this->initialStateService = $this->createMock(IInitialStateService::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->providerLoader = $this->createMock(ProviderLoader::class);
$this->userSession = $this->createMock(IUserSession::class);
@ -64,6 +70,7 @@ class SecurityTest extends TestCase {
$this->uid = 'test123';
$this->section = new Security(
$this->initialStateService,
$this->userManager,
$this->providerLoader,
$this->userSession,