Merge pull request #20807 from owncloud/dont-append-redirect-url-if-user-is-already-logged-in

Don't append redirect URL if user is logged-in

lib/private/appframework/middleware/security/corsmiddleware.php

@@ -23,6 +23,7 @@
 
 namespace OC\AppFramework\Middleware\Security;
 
+use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;

lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\Appframework\Middleware\Security\Exceptions;
+
+use OCP\AppFramework\Http;
+
+/**
+ * Class AppNotEnabledException is thrown when a resource for an application is
+ * requested that is not enabled.
+ *
+ * @package OC\Appframework\Middleware\Security\Exceptions
+ */
+class AppNotEnabledException extends SecurityException {
+	public function __construct() {
+		parent::__construct('App is not enabled', Http::STATUS_PRECONDITION_FAILED);
+	}
+}

lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\Appframework\Middleware\Security\Exceptions;
+
+use OCP\AppFramework\Http;
+
+/**
+ * Class CrossSiteRequestForgeryException is thrown when a CSRF exception has
+ * been encountered.
+ *
+ * @package OC\Appframework\Middleware\Security\Exceptions
+ */
+class CrossSiteRequestForgeryException extends SecurityException {
+	public function __construct() {
+		parent::__construct('CSRF check failed', Http::STATUS_PRECONDITION_FAILED);
+	}
+}

lib/private/appframework/middleware/security/exceptions/notadminexception.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\Appframework\Middleware\Security\Exceptions;
+
+use OCP\AppFramework\Http;
+
+/**
+ * Class NotAdminException is thrown when a resource has been requested by a
+ * non-admin user that is not accessible to non-admin users.
+ *
+ * @package OC\Appframework\Middleware\Security\Exceptions
+ */
+class NotAdminException extends SecurityException {
+	public function __construct() {
+		parent::__construct('Logged in user must be an admin', Http::STATUS_FORBIDDEN);
+	}
+}

lib/private/appframework/middleware/security/exceptions/notloggedinexception.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\Appframework\Middleware\Security\Exceptions;
+
+use OCP\AppFramework\Http;
+
+/**
+ * Class NotLoggedInException is thrown when a resource has been requested by a
+ * guest user that is not accessible to the public.
+ *
+ * @package OC\Appframework\Middleware\Security\Exceptions
+ */
+class NotLoggedInException extends SecurityException {
+	public function __construct() {
+		parent::__construct('Current user is not logged in', Http::STATUS_UNAUTHORIZED);
+	}
+}

lib/private/appframework/middleware/security/exceptions/securityexception.php

@@ -1,7 +1,6 @@
 <?php
 /**
- * @author Morris Jobke <hey@morrisjobke.de>
- * @author Thomas Müller <thomas.mueller@tmit.eu>
+ * @author Lukas Reschke <lukas@owncloud.com>
  *
  * @copyright Copyright (c) 2015, ownCloud, Inc.
  * @license AGPL-3.0
@@ -20,20 +19,12 @@
  *
  */
 
-
-namespace OC\AppFramework\Middleware\Security;
-
+namespace OC\AppFramework\Middleware\Security\Exceptions;
 
 /**
- * Thrown when the security middleware encounters a security problem
+ * Class SecurityException is the base class for security exceptions thrown by
+ * the security middleware.
+ *
+ * @package OC\AppFramework\Middleware\Security\Exceptions
  */
-class SecurityException extends \Exception {
-
-	/**
-	 * @param string $msg the security error message
-	 */
-	public function __construct($msg, $code = 0) {
-		parent::__construct($msg, $code);
-	}
-
-}
+class SecurityException extends \Exception {}

lib/private/appframework/middleware/security/securitymiddleware.php

@@ -28,8 +28,13 @@
 namespace OC\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Http;
+use OC\Appframework\Middleware\Security\Exceptions\AppNotEnabledException;
+use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException;
+use OC\Appframework\Middleware\Security\Exceptions\NotAdminException;
+use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCP\AppFramework\Http\RedirectResponse;
+use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Middleware;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http\JSONResponse;
@@ -39,7 +44,7 @@ use OCP\IRequest;
 use OCP\ILogger;
 use OCP\AppFramework\Controller;
 use OCP\Util;
-
+use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 
 /**
  * Used to do all the authentication and checking stuff for a controller method
@@ -75,7 +80,7 @@ class SecurityMiddleware extends Middleware {
 								ILogger $logger,
 								$appName,
 								$isLoggedIn,
-								$isAdminUser){
+								$isAdminUser) {
 		$this->navigationManager = $navigationManager;
 		$this->request = $request;
 		$this->reflector = $reflector;
@@ -95,7 +100,7 @@ class SecurityMiddleware extends Middleware {
 	 * @param string $methodName the name of the method
 	 * @throws SecurityException when a security check fails
 	 */
-	public function beforeController($controller, $methodName){
+	public function beforeController($controller, $methodName) {
 
 		// this will set the current navigation entry of the app, use this only
 		// for normal HTML requests and not for AJAX requests
@@ -105,12 +110,12 @@ class SecurityMiddleware extends Middleware {
 		$isPublicPage = $this->reflector->hasAnnotation('PublicPage');
 		if(!$isPublicPage) {
 			if(!$this->isLoggedIn) {
-				throw new SecurityException('Current user is not logged in', Http::STATUS_UNAUTHORIZED);
+				throw new NotLoggedInException();
 			}
 
 			if(!$this->reflector->hasAnnotation('NoAdminRequired')) {
 				if(!$this->isAdminUser) {
-					throw new SecurityException('Logged in user must be an admin', Http::STATUS_FORBIDDEN);
+					throw new NotAdminException();
 				}
 			}
 		}
@@ -119,18 +124,18 @@ class SecurityMiddleware extends Middleware {
 		Util::callRegister();
 		if(!$this->reflector->hasAnnotation('NoCSRFRequired')) {
 			if(!$this->request->passesCSRFCheck()) {
-				throw new SecurityException('CSRF check failed', Http::STATUS_PRECONDITION_FAILED);
+				throw new CrossSiteRequestForgeryException();
 			}
 		}
 
 		/**
 		 * FIXME: Use DI once available
-		 * Checks if app is enabled (also inclues a check whether user is allowed to access the resource)
+		 * Checks if app is enabled (also includes a check whether user is allowed to access the resource)
 		 * The getAppPath() check is here since components such as settings also use the AppFramework and
 		 * therefore won't pass this check.
 		 */
 		if(\OC_App::getAppPath($this->appName) !== false && !\OC_App::isEnabled($this->appName)) {
-			throw new SecurityException('App is not enabled', Http::STATUS_PRECONDITION_FAILED);
+			throw new AppNotEnabledException();
 		}
 
 	}
@@ -146,28 +151,28 @@ class SecurityMiddleware extends Middleware {
 	 * @throws \Exception the passed in exception if it cant handle it
 	 * @return Response a Response object or null in case that the exception could not be handled
 	 */
-	public function afterException($controller, $methodName, \Exception $exception){
-		if($exception instanceof SecurityException){
-
-			if (stripos($this->request->getHeader('Accept'),'html')===false) {
+	public function afterException($controller, $methodName, \Exception $exception) {
+		if($exception instanceof SecurityException) {
 
+			if (stripos($this->request->getHeader('Accept'),'html') === false) {
 				$response = new JSONResponse(
 					array('message' => $exception->getMessage()),
 					$exception->getCode()
 				);
-				$this->logger->debug($exception->getMessage());
 			} else {
-
-				// TODO: replace with link to route
-				$url = $this->urlGenerator->getAbsoluteURL('index.php');
-				// add redirect URL to redirect to the previous page after login
-				$url .= '?redirect_url=' . urlencode($this->request->server['REQUEST_URI']);
-				$response = new RedirectResponse($url);
-				$this->logger->debug($exception->getMessage());
+				if($exception instanceof NotLoggedInException) {
+					// TODO: replace with link to route
+					$url = $this->urlGenerator->getAbsoluteURL('index.php');
+					$url .= '?redirect_url=' . urlencode($this->request->server['REQUEST_URI']);
+					$response = new RedirectResponse($url);
+				} else {
+					$response = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest');
+					$response->setStatus($exception->getCode());
+				}
 			}
 
+			$this->logger->debug($exception->getMessage());
 			return $response;
-
 		}
 
 		throw $exception;

tests/lib/appframework/middleware/security/CORSMiddlewareTest.php

@@ -14,7 +14,7 @@ namespace OC\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Http\Request;
 use OC\AppFramework\Utility\ControllerMethodReflector;
-
+use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\Response;
@@ -91,7 +91,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 
 	/**
 	 * @CORS
-	 * @expectedException \OC\AppFramework\Middleware\Security\SecurityException
+	 * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\SecurityException
 	 */
 	public function testCorsIgnoredIfWithCredentialsHeaderPresent() {
 		$request = new Request(
@@ -160,7 +160,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 
 	/**
 	 * @CORS
-	 * @expectedException \OC\AppFramework\Middleware\Security\SecurityException
+	 * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\SecurityException
 	 */
 	public function testCORSShouldNotAllowCookieAuth() {
 		$request = new Request(

tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php

@@ -1,34 +1,40 @@
 <?php
-
 /**
- * ownCloud - App Framework
+ * @author Bernhard Posselt <dev@bernhard-posselt.com>
+ * @author Lukas Reschke <lukas@owncloud.com>
  *
- * @author Bernhard Posselt
- * @copyright 2012 Bernhard Posselt <dev@bernhard-posselt.com>
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
  *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
- * License as published by the Free Software Foundation; either
- * version 3 of the License, or any later version.
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
  *
- * This library is distributed in the hope that it will be useful,
+ * 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.
+ * 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 library.  If not, see <http://www.gnu.org/licenses/>.
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
  *
  */
 
 
+
 namespace OC\AppFramework\Middleware\Security;
 
 use OC\AppFramework\Http;
 use OC\AppFramework\Http\Request;
+use OC\Appframework\Middleware\Security\Exceptions\AppNotEnabledException;
+use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException;
+use OC\Appframework\Middleware\Security\Exceptions\NotAdminException;
+use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException;
+use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\JSONResponse;
+use OCP\AppFramework\Http\TemplateResponse;
 
 
 class SecurityMiddlewareTest extends \Test\TestCase {
@@ -71,8 +77,12 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->secAjaxException = new SecurityException('hey', true);
 	}
 
-
-	private function getMiddleware($isLoggedIn, $isAdminUser){
+	/**
+	 * @param bool $isLoggedIn
+	 * @param bool $isAdminUser
+	 * @return SecurityMiddleware
+	 */
+	private function getMiddleware($isLoggedIn, $isAdminUser) {
 		return new SecurityMiddleware(
 			$this->request,
 			$this->reader,
@@ -219,8 +229,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 
 		$sec = $this->getMiddleware($isLoggedIn, $isAdminUser);
 
-		if($shouldFail){
-			$this->setExpectedException('\OC\AppFramework\Middleware\Security\SecurityException');
+		if($shouldFail) {
+			$this->setExpectedException('\OC\AppFramework\Middleware\Security\Exceptions\SecurityException');
 		} else {
 			$this->assertTrue(true);
 		}
@@ -232,7 +242,7 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 
 	/**
 	 * @PublicPage
-	 * @expectedException \OC\AppFramework\Middleware\Security\SecurityException
+	 * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException
 	 */
 	public function testCsrfCheck(){
 		$this->request->expects($this->once())
@@ -311,25 +321,85 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->middleware->afterException($this->controller, 'test', $ex);
 	}
 
-
-	public function testAfterExceptionReturnsRedirect(){
+	public function testAfterExceptionReturnsRedirectForNotLoggedInUser() {
 		$this->request = new Request(
-			[
-				'server' =>
 				[
-					'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
-					'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
-				]
+						'server' =>
+								[
+										'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
+										'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
+								]
+				],
+				$this->getMock('\OCP\Security\ISecureRandom'),
+				$this->getMock('\OCP\IConfig')
+		);
+		$this->middleware = $this->getMiddleware(false, false);
+		$this->urlGenerator
+				->expects($this->once())
+				->method('getAbsoluteURL')
+				->with('index.php')
+				->will($this->returnValue('http://localhost/index.php'));
+		$this->logger
+				->expects($this->once())
+				->method('debug')
+				->with('Current user is not logged in');
+		$response = $this->middleware->afterException(
+				$this->controller,
+				'test',
+				new NotLoggedInException()
+		);
+
+		$expected = new RedirectResponse('http://localhost/index.php?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp');
+		$this->assertEquals($expected , $response);
+	}
+
+	/**
+	 * @return array
+	 */
+	public function exceptionProvider() {
+		return [
+			[
+				new AppNotEnabledException(),
+			],
+			[
+				new CrossSiteRequestForgeryException(),
 			],
-			$this->getMock('\OCP\Security\ISecureRandom'),
-			$this->getMock('\OCP\IConfig')
+			[
+				new NotAdminException(),
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider exceptionProvider
+	 * @param SecurityException $exception
+	 */
+	public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception) {
+		$this->request = new Request(
+				[
+						'server' =>
+								[
+										'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
+										'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
+								]
+				],
+				$this->getMock('\OCP\Security\ISecureRandom'),
+				$this->getMock('\OCP\IConfig')
+		);
+		$this->middleware = $this->getMiddleware(false, false);
+		$this->logger
+				->expects($this->once())
+				->method('debug')
+				->with($exception->getMessage());
+		$response = $this->middleware->afterException(
+				$this->controller,
+				'test',
+				$exception
 		);
-		$this->middleware = $this->getMiddleware(true, true);
-		$response = $this->middleware->afterException($this->controller, 'test',
-				$this->secException);
 
-		$this->assertTrue($response instanceof RedirectResponse);
-		$this->assertEquals('?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp', $response->getRedirectURL());
+		$expected = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest');
+		$expected->setStatus($exception->getCode());
+		$this->assertEquals($expected , $response);
 	}