From c0ed865ab2e5166e71fd6046fc2c426dd5b7c6d4 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Aug 2016 21:14:50 +0200 Subject: [PATCH 1/3] UserController does not require Defaults --- core/Application.php | 8 -------- core/Controller/UserController.php | 12 +++--------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/core/Application.php b/core/Application.php index 0c69394c97..9a6d0878fe 100644 --- a/core/Application.php +++ b/core/Application.php @@ -73,14 +73,6 @@ class Application extends App { $c->query('TimeFactory') ); }); - $container->registerService('UserController', function(SimpleContainer $c) { - return new UserController( - $c->query('AppName'), - $c->query('Request'), - $c->query('UserManager'), - $c->query('Defaults') - ); - }); $container->registerService('LoginController', function(SimpleContainer $c) { return new LoginController( $c->query('AppName'), diff --git a/core/Controller/UserController.php b/core/Controller/UserController.php index 0cede94eb6..fc282e36d9 100644 --- a/core/Controller/UserController.php +++ b/core/Controller/UserController.php @@ -26,26 +26,20 @@ namespace OC\Core\Controller; use \OCP\AppFramework\Controller; use \OCP\AppFramework\Http\JSONResponse; use \OCP\IRequest; +use \OCP\IUserManager; class UserController extends Controller { /** - * @var \OCP\IUserManager + * @var IUserManager */ protected $userManager; - /** - * @var \OC_Defaults - */ - protected $defaults; - public function __construct($appName, IRequest $request, - $userManager, - $defaults + IUserManager $userManager ) { parent::__construct($appName, $request); $this->userManager = $userManager; - $this->defaults = $defaults; } /** From f6423f74e3ca925fd43c67f2669384994ccc55fe Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Aug 2016 21:17:16 +0200 Subject: [PATCH 2/3] Minor cleanup in core Controllers --- core/Controller/LoginController.php | 1 - core/Controller/LostController.php | 5 ++--- core/Controller/TokenController.php | 7 ++----- core/Controller/TwoFactorChallengeController.php | 2 +- tests/Core/Controller/TokenControllerTest.php | 16 +++++++++------- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 67e1e21528..f63d5cd8f2 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -25,7 +25,6 @@ namespace OC\Core\Controller; -use OC\AppFramework\Utility\TimeFactory; use OC\Authentication\TwoFactorAuth\Manager; use OC\Security\Bruteforce\Throttler; use OC\User\Session; diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index fe6be1e685..b1111559a6 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -40,7 +40,6 @@ use \OCP\IConfig; use OCP\IUserManager; use OCP\Mail\IMailer; use OCP\Security\ISecureRandom; -use OCP\Security\StringUtils; /** * Class LostController @@ -144,7 +143,7 @@ class LostController extends Controller { } /** - * @param string $userId + * @param string $token * @param string $userId * @throws \Exception */ @@ -161,7 +160,7 @@ class LostController extends Controller { throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is expired')); } - if (!StringUtils::equals($splittedToken[1], $token)) { + if (!hash_equals($splittedToken[1], $token)) { throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); } } diff --git a/core/Controller/TokenController.php b/core/Controller/TokenController.php index 9d4fd7c965..6e3ff50fa1 100644 --- a/core/Controller/TokenController.php +++ b/core/Controller/TokenController.php @@ -24,13 +24,10 @@ namespace OC\Core\Controller; use OC\AppFramework\Http; -use OC\AppFramework\Utility\TimeFactory; -use OC\Authentication\Token\DefaultTokenProvider; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; use OC\Authentication\TwoFactorAuth\Manager as TwoFactorAuthManager; use OC\User\Manager as UserManager; -use OCA\User_LDAP\User\Manager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; @@ -100,9 +97,9 @@ class TokenController extends Controller { $token = $this->secureRandom->generate(128); $this->tokenProvider->generateToken($token, $user->getUID(), $loginName, $password, $name, IToken::PERMANENT_TOKEN); - return [ + return new JSONResponse([ 'token' => $token, - ]; + ]); } } diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index b9e10b147c..c19cf52327 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -96,7 +96,7 @@ class TwoFactorChallengeController extends Controller { * * @param string $challengeProviderId * @param string $redirect_url - * @return TemplateResponse + * @return TemplateResponse|RedirectResponse */ public function showChallenge($challengeProviderId, $redirect_url) { $user = $this->userSession->getUser(); diff --git a/tests/Core/Controller/TokenControllerTest.php b/tests/Core/Controller/TokenControllerTest.php index b6b54b14fa..0e965aac2e 100644 --- a/tests/Core/Controller/TokenControllerTest.php +++ b/tests/Core/Controller/TokenControllerTest.php @@ -41,15 +41,17 @@ class TokenControllerTest extends TestCase { protected function setUp() { parent::setUp(); - $this->request = $this->getMock('\OCP\IRequest'); + $this->request = $this->getMockBuilder('\OCP\IRequest')->getMock(); $this->userManager = $this->getMockBuilder('\OC\User\Manager') ->disableOriginalConstructor() ->getMock(); - $this->tokenProvider = $this->getMock('\OC\Authentication\Token\IProvider'); + $this->tokenProvider = $this->getMockBuilder('\OC\Authentication\Token\IProvider') + ->getMock(); $this->twoFactorAuthManager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager') ->disableOriginalConstructor() ->getMock(); - $this->secureRandom = $this->getMock('\OCP\Security\ISecureRandom'); + $this->secureRandom = $this->getMockBuilder('\OCP\Security\ISecureRandom') + ->getMock(); $this->tokenController = new TokenController('core', $this->request, $this->userManager, $this->tokenProvider, $this->twoFactorAuthManager, $this->secureRandom); } @@ -77,7 +79,7 @@ class TokenControllerTest extends TestCase { } public function testWithValidCredentials() { - $user = $this->getMock('\OCP\IUser'); + $user = $this->getMockBuilder('\OCP\IUser')->getMock(); $this->userManager->expects($this->once()) ->method('checkPassword') ->with('john', '123456') @@ -96,9 +98,9 @@ class TokenControllerTest extends TestCase { $this->tokenProvider->expects($this->once()) ->method('generateToken') ->with('verysecurerandomtoken', 'john', 'john', '123456', 'unknown client', IToken::PERMANENT_TOKEN); - $expected = [ + $expected = new JSONResponse([ 'token' => 'verysecurerandomtoken' - ]; + ]); $actual = $this->tokenController->generateToken('john', '123456'); @@ -106,7 +108,7 @@ class TokenControllerTest extends TestCase { } public function testWithValidCredentialsBut2faEnabled() { - $user = $this->getMock('\OCP\IUser'); + $user = $this->getMockBuilder('\OCP\IUser')->getMock(); $this->userManager->expects($this->once()) ->method('checkPassword') ->with('john', '123456') From b1a090f3576895f526cd102c43aad56789e6e889 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Aug 2016 21:31:41 +0200 Subject: [PATCH 3/3] AvatarController use proper JSONResponse * Do not rely on DataResponse magic. We want JSON so use JSON * Fix tests --- core/Controller/AvatarController.php | 75 ++++++++----------- .../Core/Controller/AvatarControllerTest.php | 6 +- 2 files changed, 36 insertions(+), 45 deletions(-) diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 3aa002634d..5b64320948 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -29,8 +29,8 @@ namespace OC\Core\Controller; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; -use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\DataDisplayResponse; +use OCP\AppFramework\Http\JSONResponse; use OCP\Files\File; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; @@ -111,7 +111,7 @@ class AvatarController extends Controller { * * @param string $userId * @param int $size - * @return DataResponse|DataDisplayResponse + * @return JSONResponse|DataDisplayResponse */ public function getAvatar($userId, $size) { if ($size > 2048) { @@ -128,13 +128,13 @@ class AvatarController extends Controller { $resp->setETag($avatar->getEtag()); } catch (NotFoundException $e) { $user = $this->userManager->get($userId); - $resp = new DataResponse([ + $resp = new JSONResponse([ 'data' => [ 'displayname' => $user->getDisplayName(), ], ]); } catch (\Exception $e) { - $resp = new DataResponse([ + $resp = new JSONResponse([ 'data' => [ 'displayname' => '', ], @@ -152,25 +152,22 @@ class AvatarController extends Controller { * @NoAdminRequired * * @param string $path - * @return DataResponse + * @return JSONResponse */ public function postAvatar($path) { $files = $this->request->getUploadedFile('files'); - $headers = []; - if (isset($path)) { $path = stripslashes($path); $userFolder = $this->rootFolder->getUserFolder($this->userId); $node = $userFolder->get($path); if (!($node instanceof File)) { - return new DataResponse(['data' => ['message' => $this->l->t('Please select a file.')]], Http::STATUS_OK, $headers); + return new JSONResponse(['data' => ['message' => $this->l->t('Please select a file.')]]); } if ($node->getSize() > 20*1024*1024) { - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('File is too big')]], - Http::STATUS_BAD_REQUEST, - $headers + Http::STATUS_BAD_REQUEST ); } $content = $node->getContent(); @@ -181,28 +178,25 @@ class AvatarController extends Controller { !\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0]) ) { if ($files['size'][0] > 20*1024*1024) { - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('File is too big')]], - Http::STATUS_BAD_REQUEST, - $headers + Http::STATUS_BAD_REQUEST ); } $this->cache->set('avatar_upload', file_get_contents($files['tmp_name'][0]), 7200); $content = $this->cache->get('avatar_upload'); unlink($files['tmp_name'][0]); } else { - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('Invalid file provided')]], - Http::STATUS_BAD_REQUEST, - $headers + Http::STATUS_BAD_REQUEST ); } } else { //Add imgfile - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('No image or file provided')]], - Http::STATUS_BAD_REQUEST, - $headers + Http::STATUS_BAD_REQUEST ); } @@ -214,57 +208,54 @@ class AvatarController extends Controller { if ($image->valid()) { $mimeType = $image->mimeType(); if ($mimeType !== 'image/jpeg' && $mimeType !== 'image/png') { - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('Unknown filetype')]], - Http::STATUS_OK, - $headers + Http::STATUS_OK ); } $this->cache->set('tmpAvatar', $image->data(), 7200); - return new DataResponse( + return new JSONResponse( ['data' => 'notsquare'], - Http::STATUS_OK, - $headers + Http::STATUS_OK ); } else { - return new DataResponse( + return new JSONResponse( ['data' => ['message' => $this->l->t('Invalid image')]], - Http::STATUS_OK, - $headers + Http::STATUS_OK ); } } catch (\Exception $e) { $this->logger->logException($e, ['app' => 'core']); - return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_OK, $headers); + return new JSONResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_OK); } } /** * @NoAdminRequired * - * @return DataResponse + * @return JSONResponse */ public function deleteAvatar() { try { $avatar = $this->avatarManager->getAvatar($this->userId); $avatar->remove(); - return new DataResponse(); + return new JSONResponse(); } catch (\Exception $e) { $this->logger->logException($e, ['app' => 'core']); - return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); + return new JSONResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); } } /** * @NoAdminRequired * - * @return DataResponse|DataDisplayResponse + * @return JSONResponse|DataDisplayResponse */ public function getTmpAvatar() { $tmpAvatar = $this->cache->get('tmpAvatar'); if (is_null($tmpAvatar)) { - return new DataResponse(['data' => [ + return new JSONResponse(['data' => [ 'message' => $this->l->t("No temporary profile picture available, try again") ]], Http::STATUS_NOT_FOUND); @@ -286,22 +277,22 @@ class AvatarController extends Controller { * @NoAdminRequired * * @param array $crop - * @return DataResponse + * @return JSONResponse */ public function postCroppedAvatar($crop) { if (is_null($crop)) { - return new DataResponse(['data' => ['message' => $this->l->t("No crop data provided")]], + return new JSONResponse(['data' => ['message' => $this->l->t("No crop data provided")]], Http::STATUS_BAD_REQUEST); } if (!isset($crop['x'], $crop['y'], $crop['w'], $crop['h'])) { - return new DataResponse(['data' => ['message' => $this->l->t("No valid crop data provided")]], + return new JSONResponse(['data' => ['message' => $this->l->t("No valid crop data provided")]], Http::STATUS_BAD_REQUEST); } $tmpAvatar = $this->cache->get('tmpAvatar'); if (is_null($tmpAvatar)) { - return new DataResponse(['data' => [ + return new JSONResponse(['data' => [ 'message' => $this->l->t("No temporary profile picture available, try again") ]], Http::STATUS_BAD_REQUEST); @@ -314,13 +305,13 @@ class AvatarController extends Controller { $avatar->set($image); // Clean up $this->cache->remove('tmpAvatar'); - return new DataResponse(['status' => 'success']); + return new JSONResponse(['status' => 'success']); } catch (\OC\NotSquareException $e) { - return new DataResponse(['data' => ['message' => $this->l->t('Crop is not square')]], + return new JSONResponse(['data' => ['message' => $this->l->t('Crop is not square')]], Http::STATUS_BAD_REQUEST); } catch (\Exception $e) { $this->logger->logException($e, ['app' => 'core']); - return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); + return new JSONResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST); } } } diff --git a/tests/Core/Controller/AvatarControllerTest.php b/tests/Core/Controller/AvatarControllerTest.php index a275a8bd16..fe1a44b28a 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -228,7 +228,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->logger->expects($this->once()) ->method('logException') ->with(new \Exception("foo")); - $expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST); + $expectedResponse = new Http\JSONResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST); $this->assertEquals($expectedResponse, $this->avatarController->deleteAvatar()); } @@ -377,7 +377,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->logger->expects($this->once()) ->method('logException') ->with(new \Exception("foo")); - $expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_OK); + $expectedResponse = new Http\JSONResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_OK); $this->assertEquals($expectedResponse, $this->avatarController->postAvatar('avatar.jpg')); } @@ -437,7 +437,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->logger->expects($this->once()) ->method('logException') ->with(new \Exception('foo')); - $expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST); + $expectedResponse = new Http\JSONResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST); $this->assertEquals($expectedResponse, $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 11])); }