REST: basic read restrictions
authorTim Starling <tstarling@wikimedia.org>
Wed, 26 Jun 2019 02:33:35 +0000 (12:33 +1000)
committerTim Starling <tstarling@wikimedia.org>
Tue, 9 Jul 2019 05:23:20 +0000 (15:23 +1000)
Protect private wikis by providing basic read restrictions,
closely following the example of the action API.

The BasicAccess module provides a narrow interface for this
functionality, without exposing the whole session/user concept to the
router.

Also, add RouterTest and fix a bug in Router::getRelativePath() thus
discovered.

Change-Id: I82319d56f08b2eec4a585ff6dbd348ccdbadc5b5

15 files changed:
includes/Rest/BasicAccess/BasicAuthorizerBase.php [new file with mode: 0644]
includes/Rest/BasicAccess/BasicAuthorizerInterface.php [new file with mode: 0644]
includes/Rest/BasicAccess/BasicRequestAuthorizer.php [new file with mode: 0644]
includes/Rest/BasicAccess/MWBasicAuthorizer.php [new file with mode: 0644]
includes/Rest/BasicAccess/MWBasicRequestAuthorizer.php [new file with mode: 0644]
includes/Rest/BasicAccess/StaticBasicAuthorizer.php [new file with mode: 0644]
includes/Rest/EntryPoint.php
includes/Rest/Handler.php
includes/Rest/Handler/HelloHandler.php
includes/Rest/Router.php
tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/Rest/EntryPointTest.php
tests/phpunit/unit/includes/Rest/Handler/HelloHandlerTest.php
tests/phpunit/unit/includes/Rest/RouterTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/Rest/testRoutes.json

diff --git a/includes/Rest/BasicAccess/BasicAuthorizerBase.php b/includes/Rest/BasicAccess/BasicAuthorizerBase.php
new file mode 100644 (file)
index 0000000..7aefe25
--- /dev/null
@@ -0,0 +1,28 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * An implementation of BasicAuthorizerInterface which creates a request-local
+ * object (a request authorizer) to do the actual authorization.
+ *
+ * @internal
+ */
+abstract class BasicAuthorizerBase implements BasicAuthorizerInterface {
+       public function authorize( RequestInterface $request, Handler $handler ) {
+               return $this->createRequestAuthorizer( $request, $handler )->authorize();
+       }
+
+       /**
+        * Create a BasicRequestAuthorizer to authorize the request.
+        *
+        * @param RequestInterface $request
+        * @param Handler $handler
+        * @return BasicRequestAuthorizer
+        */
+       abstract protected function createRequestAuthorizer( RequestInterface $request,
+               Handler $handler ) : BasicRequestAuthorizer;
+}
diff --git a/includes/Rest/BasicAccess/BasicAuthorizerInterface.php b/includes/Rest/BasicAccess/BasicAuthorizerInterface.php
new file mode 100644 (file)
index 0000000..7eb7f3f
--- /dev/null
@@ -0,0 +1,28 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * An interface used by Router to ensure that the client has "basic" access,
+ * i.e. the ability to read or write to the wiki.
+ *
+ * @internal
+ */
+interface BasicAuthorizerInterface {
+       /**
+        * Determine whether a request should be permitted, given the handler's
+        * needsReadAccess().
+        *
+        * If the request should be permitted, return null. If the request should
+        * be denied, return a string error identifier.
+        *
+        * @param RequestInterface $request
+        * @param Handler $handler
+        * @return string|null If the request is denied, the string error code. If
+        *   the request is allowed, null.
+        */
+       public function authorize( RequestInterface $request, Handler $handler );
+}
diff --git a/includes/Rest/BasicAccess/BasicRequestAuthorizer.php b/includes/Rest/BasicAccess/BasicRequestAuthorizer.php
new file mode 100644 (file)
index 0000000..f940589
--- /dev/null
@@ -0,0 +1,46 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * A request authorizer which checks needsReadAccess() in the
+ * handler and calls isReadAllowed() in the subclass
+ * accordingly.
+ *
+ * @internal
+ */
+abstract class BasicRequestAuthorizer {
+       protected $request;
+       protected $handler;
+
+       /**
+        * @param RequestInterface $request
+        * @param Handler $handler
+        */
+       public function __construct( RequestInterface $request, Handler $handler ) {
+               $this->request = $request;
+               $this->handler = $handler;
+       }
+
+       /**
+        * @see BasicAuthorizerInterface::authorize()
+        * @return string|null If the request is denied, the string error code. If
+        *   the request is allowed, null.
+        */
+       public function authorize() {
+               if ( $this->handler->needsReadAccess() && !$this->isReadAllowed() ) {
+                       return 'rest-read-denied';
+               }
+               return null;
+       }
+
+       /**
+        * Check if the current user is allowed to read from the wiki
+        *
+        * @return bool
+        */
+       abstract protected function isReadAllowed();
+}
diff --git a/includes/Rest/BasicAccess/MWBasicAuthorizer.php b/includes/Rest/BasicAccess/MWBasicAuthorizer.php
new file mode 100644 (file)
index 0000000..43014f1
--- /dev/null
@@ -0,0 +1,33 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use User;
+use MediaWiki\Permissions\PermissionManager;
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * A factory for MWBasicRequestAuthorizer which passes through a User object
+ *
+ * @internal
+ */
+class MWBasicAuthorizer extends BasicAuthorizerBase {
+       /** @var User */
+       private $user;
+
+       /** @var PermissionManager */
+       private $permissionManager;
+
+       public function __construct( User $user, PermissionManager $permissionManager ) {
+               $this->user = $user;
+               $this->permissionManager = $permissionManager;
+       }
+
+       protected function createRequestAuthorizer( RequestInterface $request,
+               Handler $handler
+       ): BasicRequestAuthorizer {
+               return new MWBasicRequestAuthorizer( $request, $handler, $this->user,
+                       $this->permissionManager );
+       }
+}
diff --git a/includes/Rest/BasicAccess/MWBasicRequestAuthorizer.php b/includes/Rest/BasicAccess/MWBasicRequestAuthorizer.php
new file mode 100644 (file)
index 0000000..01367d1
--- /dev/null
@@ -0,0 +1,38 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use User;
+use MediaWiki\Permissions\PermissionManager;
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * The concrete implementation of basic read restrictions in MediaWiki
+ *
+ * @internal
+ */
+class MWBasicRequestAuthorizer extends BasicRequestAuthorizer {
+       /** @var User */
+       private $user;
+
+       /** @var PermissionManager */
+       private $permissionManager;
+
+       public function __construct( RequestInterface $request, Handler $handler,
+               User $user, PermissionManager $permissionManager
+       ) {
+               parent::__construct( $request, $handler );
+               $this->user = $user;
+               $this->permissionManager = $permissionManager;
+       }
+
+       protected function isReadAllowed() {
+               return $this->permissionManager->isEveryoneAllowed( 'read' )
+                  || $this->isAllowed( 'read' );
+       }
+
+       private function isAllowed( $action ) {
+               return $this->permissionManager->userHasRight( $this->user, $action );
+       }
+}
diff --git a/includes/Rest/BasicAccess/StaticBasicAuthorizer.php b/includes/Rest/BasicAccess/StaticBasicAuthorizer.php
new file mode 100644 (file)
index 0000000..c4dcda1
--- /dev/null
@@ -0,0 +1,30 @@
+<?php
+
+namespace MediaWiki\Rest\BasicAccess;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+
+/**
+ * An authorizer which returns a value from authorize() which is given in the constructor.
+ *
+ * @internal
+ */
+class StaticBasicAuthorizer implements BasicAuthorizerInterface {
+       private $value;
+
+       /**
+        * @see BasicAuthorizerInterface::authorize()
+        *
+        * @param string|null $value The value returned by authorize(). If the
+        *   request is denied, this is the string error code. If the request is
+        *   allowed, it is null.
+        */
+       public function __construct( $value = null ) {
+               $this->value = $value;
+       }
+
+       public function authorize( RequestInterface $request, Handler $handler ) {
+               return $this->value;
+       }
+}
index 795999a..40d0b4a 100644 (file)
@@ -4,6 +4,7 @@ namespace MediaWiki\Rest;
 
 use ExtensionRegistry;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer;
 use RequestContext;
 use Title;
 use WebResponse;
@@ -35,13 +36,17 @@ class EntryPoint {
                        'cookiePrefix' => $conf->get( 'CookiePrefix' )
                ] );
 
+               $authorizer = new MWBasicAuthorizer( RequestContext::getMain()->getUser(),
+                       $services->getPermissionManager() );
+
                global $IP;
                $router = new Router(
                        [ "$IP/includes/Rest/coreRoutes.json" ],
                        ExtensionRegistry::getInstance()->getAttribute( 'RestRoutes' ),
                        $conf->get( 'RestPath' ),
                        $services->getLocalServerObjectCache(),
-                       new ResponseFactory
+                       new ResponseFactory,
+                       $authorizer
                );
 
                $entryPoint = new self(
index cee403f..c05d8e7 100644 (file)
@@ -95,6 +95,35 @@ abstract class Handler {
                return null;
        }
 
+       /**
+        * Indicates whether this route requires read rights.
+        *
+        * The handler should override this if it does not need to read from the
+        * wiki. This is uncommon, but may be useful for login and other account
+        * management APIs.
+        *
+        * @return bool
+        */
+       public function needsReadAccess() {
+               return true;
+       }
+
+       /**
+        * Indicates whether this route requires write access.
+        *
+        * The handler should override this if the route does not need to write to
+        * the database.
+        *
+        * This should return true for routes that may require synchronous database writes.
+        * Modules that do not need such writes should also not rely on master database access,
+        * since only read queries are needed and each master DB is a single point of failure.
+        *
+        * @return bool
+        */
+       public function needsWriteAccess() {
+               return true;
+       }
+
        /**
         * Execute the handler. This is called after parameter validation. The
         * return value can either be a Response or any type accepted by
index 6e119dd..34faee2 100644 (file)
@@ -12,4 +12,8 @@ class HelloHandler extends SimpleHandler {
        public function run( $name ) {
                return [ 'message' => "Hello, $name!" ];
        }
+
+       public function needsWriteAccess() {
+               return false;
+       }
 }
index 5ba3d08..14b4c9c 100644 (file)
@@ -4,6 +4,7 @@ namespace MediaWiki\Rest;
 
 use AppendIterator;
 use BagOStuff;
+use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface;
 use MediaWiki\Rest\PathTemplateMatcher\PathMatcher;
 use Wikimedia\ObjectFactory;
 
@@ -40,21 +41,27 @@ class Router {
        /** @var ResponseFactory */
        private $responseFactory;
 
+       /** @var BasicAuthorizerInterface */
+       private $basicAuth;
+
        /**
         * @param string[] $routeFiles List of names of JSON files containing routes
         * @param array $extraRoutes Extension route array
         * @param string $rootPath The base URL path
         * @param BagOStuff $cacheBag A cache in which to store the matcher trees
         * @param ResponseFactory $responseFactory
+        * @param BasicAuthorizerInterface $basicAuth
         */
        public function __construct( $routeFiles, $extraRoutes, $rootPath,
-               BagOStuff $cacheBag, ResponseFactory $responseFactory
+               BagOStuff $cacheBag, ResponseFactory $responseFactory,
+               BasicAuthorizerInterface $basicAuth
        ) {
                $this->routeFiles = $routeFiles;
                $this->extraRoutes = $extraRoutes;
                $this->rootPath = $rootPath;
                $this->cacheBag = $cacheBag;
                $this->responseFactory = $responseFactory;
+               $this->basicAuth = $basicAuth;
        }
 
        /**
@@ -189,7 +196,9 @@ class Router {
         * @return false|string
         */
        private function getRelativePath( $path ) {
-               if ( substr_compare( $path, $this->rootPath, 0, strlen( $this->rootPath ) ) !== 0 ) {
+               if ( strlen( $this->rootPath ) > strlen( $path ) ||
+                       substr_compare( $path, $this->rootPath, 0, strlen( $this->rootPath ) ) !== 0
+               ) {
                        return false;
                }
                return substr( $path, strlen( $this->rootPath ) );
@@ -254,6 +263,10 @@ class Router {
         * @return ResponseInterface
         */
        private function executeHandler( $handler ): ResponseInterface {
+               $authResult = $this->basicAuth->authorize( $handler->getRequest(), $handler );
+               if ( $authResult ) {
+                       return $this->responseFactory->createHttpError( 403, [ 'error' => $authResult ] );
+               }
                $response = $handler->execute();
                if ( !( $response instanceof ResponseInterface ) ) {
                        $response = $this->responseFactory->createFromReturnValue( $response );
diff --git a/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php b/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php
new file mode 100644 (file)
index 0000000..5a16434
--- /dev/null
@@ -0,0 +1,73 @@
+<?php
+
+namespace MediaWiki\Tests\Rest\BasicAccess;
+
+use GuzzleHttp\Psr7\Uri;
+use MediaWiki\Permissions\PermissionManager;
+use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer;
+use MediaWiki\Rest\RequestData;
+use MediaWiki\Rest\ResponseFactory;
+use MediaWiki\Rest\Router;
+use MediaWiki\User\UserIdentity;
+use MediaWikiTestCase;
+use User;
+
+/**
+ * @group Database
+ *
+ * @covers \MediaWiki\Rest\BasicAccess\BasicAuthorizerBase
+ * @covers \MediaWiki\Rest\BasicAccess\MWBasicAuthorizer
+ * @covers \MediaWiki\Rest\BasicAccess\BasicRequestAuthorizer
+ * @covers \MediaWiki\Rest\BasicAccess\MWBasicRequestAuthorizer
+ */
+class MWBasicRequestAuthorizerTest extends MediaWikiTestCase {
+       private function createRouter( $userRights ) {
+               $user = User::newFromName( 'Test user' );
+
+               $pm = new class( $user, $userRights ) extends PermissionManager {
+                       private $testUser;
+                       private $testUserRights;
+
+                       public function __construct( $user, $userRights ) {
+                               $this->testUser = $user;
+                               $this->testUserRights = $userRights;
+                       }
+
+                       public function userHasRight( UserIdentity $user, $action = '' ) {
+                               if ( $user === $this->testUser ) {
+                                       return $this->testUserRights[$action] ?? false;
+                               }
+                               return parent::userHasRight( $user, $action );
+                       }
+               };
+
+               global $IP;
+
+               return new Router(
+                       [ "$IP/tests/phpunit/unit/includes/Rest/testRoutes.json" ],
+                       [],
+                       '/rest',
+                       new \EmptyBagOStuff(),
+                       new ResponseFactory(),
+                       new MWBasicAuthorizer( $user, $pm ) );
+       }
+
+       public function testReadDenied() {
+               $router = $this->createRouter( [ 'read' => false ] );
+               $request = new RequestData( [ 'uri' => new Uri( '/rest/user/joe/hello' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 403, $response->getStatusCode() );
+
+               $body = $response->getBody();
+               $body->rewind();
+               $data = json_decode( $body->getContents(), true );
+               $this->assertSame( 'rest-read-denied', $data['error'] );
+       }
+
+       public function testReadAllowed() {
+               $router = $this->createRouter( [ 'read' => true ] );
+               $request = new RequestData( [ 'uri' => new Uri( '/rest/user/joe/hello' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 200, $response->getStatusCode() );
+       }
+}
index e1f2c88..a74c0cb 100644 (file)
@@ -5,6 +5,7 @@ namespace MediaWiki\Tests\Rest;
 use EmptyBagOStuff;
 use GuzzleHttp\Psr7\Uri;
 use GuzzleHttp\Psr7\Stream;
+use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer;
 use MediaWiki\Rest\Handler;
 use MediaWiki\Rest\EntryPoint;
 use MediaWiki\Rest\RequestData;
@@ -25,7 +26,8 @@ class EntryPointTest extends \MediaWikiUnitTestCase {
                        [],
                        '/rest',
                        new EmptyBagOStuff(),
-                       new ResponseFactory() );
+                       new ResponseFactory(),
+                       new StaticBasicAuthorizer() );
        }
 
        private function createWebResponse() {
index c68273b..188629f 100644 (file)
@@ -4,6 +4,7 @@ namespace MediaWiki\Tests\Rest\Handler;
 
 use EmptyBagOStuff;
 use GuzzleHttp\Psr7\Uri;
+use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer;
 use MediaWiki\Rest\RequestData;
 use MediaWiki\Rest\ResponseFactory;
 use MediaWiki\Rest\Router;
@@ -52,7 +53,8 @@ class HelloHandlerTest extends \MediaWikiUnitTestCase {
                        [],
                        '/rest',
                        new EmptyBagOStuff(),
-                       new ResponseFactory() );
+                       new ResponseFactory(),
+                       new StaticBasicAuthorizer() );
                $request = new RequestData( $requestInfo );
                $response = $router->execute( $request );
                if ( isset( $responseInfo['statusCode'] ) ) {
diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php
new file mode 100644 (file)
index 0000000..6d8e794
--- /dev/null
@@ -0,0 +1,85 @@
+<?php
+
+namespace MediaWiki\Tests\Rest;
+
+use GuzzleHttp\Psr7\Uri;
+use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer;
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\HttpException;
+use MediaWiki\Rest\RequestData;
+use MediaWiki\Rest\ResponseFactory;
+use MediaWiki\Rest\Router;
+
+/**
+ * @covers \MediaWiki\Rest\Router
+ */
+class RouterTest extends \MediaWikiTestCase {
+       /** @return Router */
+       private function createRouter( $authError = null ) {
+               return new Router(
+                       [ __DIR__ . '/testRoutes.json' ],
+                       [],
+                       '/rest',
+                       new \EmptyBagOStuff(),
+                       new ResponseFactory(),
+                       new StaticBasicAuthorizer( $authError ) );
+       }
+
+       public function testPrefixMismatch() {
+               $router = $this->createRouter();
+               $request = new RequestData( [ 'uri' => new Uri( '/bogus' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 404, $response->getStatusCode() );
+       }
+
+       public function testWrongMethod() {
+               $router = $this->createRouter();
+               $request = new RequestData( [
+                       'uri' => new Uri( '/rest/user/joe/hello' ),
+                       'method' => 'OPTIONS'
+               ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 405, $response->getStatusCode() );
+               $this->assertSame( 'Method Not Allowed', $response->getReasonPhrase() );
+               $this->assertSame( 'GET', $response->getHeaderLine( 'Allow' ) );
+       }
+
+       public function testNoMatch() {
+               $router = $this->createRouter();
+               $request = new RequestData( [ 'uri' => new Uri( '/rest/bogus' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 404, $response->getStatusCode() );
+               // TODO: add more information to the response body and test for its presence here
+       }
+
+       public static function throwHandlerFactory() {
+               return new class extends Handler {
+                       public function execute() {
+                               throw new HttpException( 'Mock error', 555 );
+                       }
+               };
+       }
+
+       public function testException() {
+               $router = $this->createRouter();
+               $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throw' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 555, $response->getStatusCode() );
+               $body = $response->getBody();
+               $body->rewind();
+               $data = json_decode( $body->getContents(), true );
+               $this->assertSame( 'Mock error', $data['message'] );
+       }
+
+       public function testBasicAccess() {
+               $router = $this->createRouter( 'test-error' );
+               // Using the throwing handler is a way to assert that the handler is not executed
+               $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throw' ) ] );
+               $response = $router->execute( $request );
+               $this->assertSame( 403, $response->getStatusCode() );
+               $body = $response->getBody();
+               $body->rewind();
+               $data = json_decode( $body->getContents(), true );
+               $this->assertSame( 'test-error', $data['error'] );
+       }
+}
index 7e43bb0..65eda6b 100644 (file)
        {
                "path": "/mock/EntryPoint/bodyRewind",
                "factory": "MediaWiki\\Tests\\Rest\\EntryPointTest::mockHandlerBodyRewind"
+       },
+       {
+               "path": "/mock/RouterTest/throw",
+               "factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwHandlerFactory"
+       },
+       {
+               "path": "/mock/MWBasicRequestAuthorizerTest/write",
+               "factory": "MediaWiki\\Tests\\Rest\\BasicAccess\\MWBasicRequestAuthorizerTest::writeHandlerFactory"
        }
 ]