reducing controller annotations to:

@PublicPage - No user logon is expected
@NoAdminRequired - the login user requires no admin rights
@NoCSRFRequired - the incoming request will not check for CSRF token

diff --git a/lib/appframework/middleware/security/securitymiddleware.php b/lib/appframework/middleware/security/securitymiddleware.php
index 7a715f309a0c4bac433c9bfc6b7e1c7bc2ca4252..52818b1b53ebbce131f9141c45360d5f3c63c65c 100644 (file)
--- a/lib/appframework/middleware/security/securitymiddleware.php
+++ b/lib/appframework/middleware/security/securitymiddleware.php
@@ -77,25 +77,20 @@ class SecurityMiddleware extends Middleware {
                $this->api->activateNavigationEntry();
 
                // security checks
-               if(!$annotationReader->hasAnnotation('IsLoggedInExemption')) {
+               $isPublicPage = $annotationReader->hasAnnotation('PublicPage');
+               if(!$isPublicPage) {
                        if(!$this->api->isLoggedIn()) {
                                throw new SecurityException('Current user is not logged in', Http::STATUS_UNAUTHORIZED);
                        }
-               }
-
-               if(!$annotationReader->hasAnnotation('IsAdminExemption')) {
-                       if(!$this->api->isAdminUser($this->api->getUserId())) {
-                               throw new SecurityException('Logged in user must be an admin', Http::STATUS_FORBIDDEN);
-                       }
-               }
 
-               if(!$annotationReader->hasAnnotation('IsSubAdminExemption')) {
-                       if(!$this->api->isSubAdminUser($this->api->getUserId())) {
-                               throw new SecurityException('Logged in user must be a subadmin', Http::STATUS_FORBIDDEN);
+                       if(!$annotationReader->hasAnnotation('NoAdminRequired')) {
+                               if(!$this->api->isAdminUser($this->api->getUserId())) {
+                                       throw new SecurityException('Logged in user must be an admin', Http::STATUS_FORBIDDEN);
+                               }
                        }
                }
 
-               if(!$annotationReader->hasAnnotation('CSRFExemption')) {
+               if(!$annotationReader->hasAnnotation('NoCSRFRequired')) {
                        if(!$this->api->passesCSRFCheck()) {
                                throw new SecurityException('CSRF check failed', Http::STATUS_PRECONDITION_FAILED);
                        }

diff --git a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php
index 0b2103564e4300e560751a9b016bb79cceaa7254..90a19c9999dbe157673591890d96cf42b516ec9c 100644 (file)
--- a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php
+++ b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php
@@ -80,67 +80,27 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
+        * @NoCSRFRequired
         */
        public function testSetNavigationEntry(){
                $this->checkNavEntry('testSetNavigationEntry', true);
        }
 
 
-       private function ajaxExceptionCheck($method, $shouldBeAjax=false){
-               $api = $this->getAPI();
-               $api->expects($this->any())
-                               ->method('passesCSRFCheck')
-                               ->will($this->returnValue(false));
-
-               $sec = new SecurityMiddleware($api, $this->request);
-
-               try {
-                       $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest',
-                                       $method);
-               } catch (SecurityException $ex){
-                       if($shouldBeAjax){
-                               $this->assertTrue($ex->isAjax());
-                       } else {
-                               $this->assertFalse($ex->isAjax());
-                       }
-
-               }
-       }
-
-
-       /**
-        * @Ajax
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
-        */
-       public function testAjaxException(){
-               $this->ajaxExceptionCheck('testAjaxException');
-       }
-
-
-       /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
-        */
-       public function testNoAjaxException(){
-               $this->ajaxExceptionCheck('testNoAjaxException');
-       }
-
-
        private function ajaxExceptionStatus($method, $test, $status) {
                $api = $this->getAPI();
                $api->expects($this->any())
                                ->method($test)
                                ->will($this->returnValue(false));
 
+               // isAdminUser requires isLoggedIn call to return true
+               if ($test === 'isAdminUser') {
+                       $api->expects($this->any())
+                               ->method('isLoggedIn')
+                               ->will($this->returnValue(true));
+               }
+
                $sec = new SecurityMiddleware($api, $this->request);
 
                try {
@@ -151,9 +111,6 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
                }
        }
 
-       /**
-        * @Ajax
-        */
        public function testAjaxStatusLoggedInCheck() {
                $this->ajaxExceptionStatus(
                        'testAjaxStatusLoggedInCheck',
@@ -163,8 +120,8 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
        }
 
        /**
-        * @Ajax
-        * @IsLoggedInExemption
+        * @NoCSRFRequired
+        * @NoAdminRequired
         */
        public function testAjaxNotAdminCheck() {
                $this->ajaxExceptionStatus(
@@ -175,23 +132,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
        }
 
        /**
-        * @Ajax
-        * @IsLoggedInExemption
-        * @IsAdminExemption
-        */
-       public function testAjaxNotSubAdminCheck() {
-               $this->ajaxExceptionStatus(
-                       'testAjaxNotSubAdminCheck',
-                       'isSubAdminUser',
-                       Http::STATUS_FORBIDDEN
-               );
-       }
-
-       /**
-        * @Ajax
-        * @IsLoggedInExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
         */
        public function testAjaxStatusCSRFCheck() {
                $this->ajaxExceptionStatus(
@@ -202,11 +143,8 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
        }
 
        /**
-        * @Ajax
-        * @CSRFExemption
-        * @IsLoggedInExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
+        * @NoCSRFRequired
         */
        public function testAjaxStatusAllGood() {
                $this->ajaxExceptionStatus(
@@ -231,11 +169,10 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
                );
        }
 
+
        /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
+        * @NoCSRFRequired
         */
        public function testNoChecks(){
                $api = $this->getAPI();
@@ -245,9 +182,6 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
                $api->expects($this->never())
                                ->method('isAdminUser')
                                ->will($this->returnValue(true));
-               $api->expects($this->never())
-                               ->method('isSubAdminUser')
-                               ->will($this->returnValue(true));
                $api->expects($this->never())
                                ->method('isLoggedIn')
                                ->will($this->returnValue(true));
@@ -264,10 +198,19 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
                                ->method($expects)
                                ->will($this->returnValue(!$shouldFail));
 
+               // admin check requires login
+               if ($expects === 'isAdminUser') {
+                       $api->expects($this->once())
+                               ->method('isLoggedIn')
+                               ->will($this->returnValue(true));
+               }
+
                $sec = new SecurityMiddleware($api, $this->request);
 
                if($shouldFail){
                        $this->setExpectedException('\OC\AppFramework\Middleware\Security\SecurityException');
+               } else {
+                       $this->setExpectedException(null);
                }
 
                $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $method);
@@ -275,9 +218,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @IsLoggedInExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
         */
        public function testCsrfCheck(){
                $this->securityCheck('testCsrfCheck', 'passesCSRFCheck');
@@ -285,9 +226,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @IsLoggedInExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @PublicPage
         */
        public function testFailCsrfCheck(){
                $this->securityCheck('testFailCsrfCheck', 'passesCSRFCheck', true);
@@ -295,9 +234,8 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @NoCSRFRequired
+        * @NoAdminRequired
         */
        public function testLoggedInCheck(){
                $this->securityCheck('testLoggedInCheck', 'isLoggedIn');
@@ -305,9 +243,8 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @CSRFExemption
-        * @IsAdminExemption
-        * @IsSubAdminExemption
+        * @NoCSRFRequired
+        * @NoAdminRequired
         */
        public function testFailLoggedInCheck(){
                $this->securityCheck('testFailLoggedInCheck', 'isLoggedIn', true);
@@ -315,9 +252,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsSubAdminExemption
+        * @NoCSRFRequired
         */
        public function testIsAdminCheck(){
                $this->securityCheck('testIsAdminCheck', 'isAdminUser');
@@ -325,36 +260,13 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 
 
        /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsSubAdminExemption
+        * @NoCSRFRequired
         */
        public function testFailIsAdminCheck(){
                $this->securityCheck('testFailIsAdminCheck', 'isAdminUser', true);
        }
 
 
-       /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        */
-       public function testIsSubAdminCheck(){
-               $this->securityCheck('testIsSubAdminCheck', 'isSubAdminUser');
-       }
-
-
-       /**
-        * @IsLoggedInExemption
-        * @CSRFExemption
-        * @IsAdminExemption
-        */
-       public function testFailIsSubAdminCheck(){
-               $this->securityCheck('testFailIsSubAdminCheck', 'isSubAdminUser', true);
-       }
-
-
-
        public function testAfterExceptionNotCaughtThrowsItAgain(){
                $ex = new \Exception();
                $this->setExpectedException('\Exception');