Show error template

Otherwise this leads to an endless redirection in case of a CSRF exception. Also sets user expectation right.

7 files changed, 178 insertions, 37 deletions

diff --git a/lib/private/appframework/middleware/security/corsmiddleware.php b/lib/private/appframework/middleware/security/corsmiddleware.php
index 74b0dd09974..0e37e81c5be 100644
--- a/lib/private/appframework/middleware/security/corsmiddleware.php
+++ b/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;
diff --git a/lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php b/lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php
new file mode 100644
index 00000000000..54cb08f4385
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php b/lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php
new file mode 100644
index 00000000000..c59c921f5eb
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/lib/private/appframework/middleware/security/exceptions/notadminexception.php b/lib/private/appframework/middleware/security/exceptions/notadminexception.php
new file mode 100644
index 00000000000..3fa03cae896
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/lib/private/appframework/middleware/security/exceptions/notloggedinexception.php b/lib/private/appframework/middleware/security/exceptions/notloggedinexception.php
new file mode 100644
index 00000000000..5f27625aa52
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/lib/private/appframework/middleware/security/securityexception.php b/lib/private/appframework/middleware/security/exceptions/securityexception.php
index 6f96b9a1d80..9b99282ce86 100644
--- a/lib/private/appframework/middleware/security/securityexception.php
+++ b/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 {}
diff --git a/lib/private/appframework/middleware/security/securitymiddleware.php b/lib/private/appframework/middleware/security/securitymiddleware.php
index 34c626ce8be..d0b7202a360 100644
--- a/lib/private/appframework/middleware/security/securitymiddleware.php
+++ b/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;