From a8863dffeed9b690b2d1b2d7712f8ee250930ca2 Mon Sep 17 00:00:00 2001 From: Alexander Piskun <13381981+bigcat88@users.noreply.github.com> Date: Mon, 1 Jan 2024 16:51:33 +0300 Subject: [PATCH] `ALL` ApiScope (#190) Added `ALL` ApiScope to allow the use of OCS Apis that we have not yet defined in the AppAPI. --------- Signed-off-by: Andrey Borysenko Co-authored-by: Andrey Borysenko --- .github/workflows/tests-special.yml | 60 ++++++++++++++++++------- CHANGELOG.md | 7 ++- docs/tech_details/ApiScopes.rst | 25 ++++++----- lib/Middleware/AppAPIAuthMiddleware.php | 25 ++++------- lib/Service/AppAPIService.php | 25 ++++++----- lib/Service/ExAppApiScopeService.php | 5 +++ tests/auth_scopes_system.py | 25 +++++++++++ tests/psalm-baseline.xml | 11 ----- 8 files changed, 117 insertions(+), 66 deletions(-) create mode 100644 tests/auth_scopes_system.py diff --git a/.github/workflows/tests-special.yml b/.github/workflows/tests-special.yml index ca190230..eac57233 100644 --- a/.github/workflows/tests-special.yml +++ b/.github/workflows/tests-special.yml @@ -116,8 +116,8 @@ jobs: cd .. sleep 5s php occ app_api:daemon:register manual_install "Manual Install" manual-install 0 0 0 - php occ app_api:app:register nc_py_api manual_install --json-info \ - "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"SYSTEM\", \"FILES\", \"FILES_SHARING\"],\"optional\":[\"USER_INFO\", \"USER_STATUS\", \"NOTIFICATIONS\", \"WEATHER_STATUS\", \"TALK\"]},\"protocol\":\"http\",\"system_app\":1}" \ + php occ app_api:app:register $APP_ID manual_install --json-info \ + "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"SYSTEM\", \"NOTIFICATIONS\"],\"optional\":[\"USER_INFO\"]},\"protocol\":\"http\",\"system_app\":1}" \ --force-scopes --wait-finish kill -15 $(cat /tmp/_install.pid) timeout 3m tail --pid=$(cat /tmp/_install.pid) -f /dev/null @@ -134,9 +134,9 @@ jobs: path: data/nextcloud.log if-no-files-found: warn - no-init-endpoint: + auth-tests-no-init: runs-on: ubuntu-22.04 - name: Without Init Endpoint + name: Auth tests (no Init endpoint) services: postgres: @@ -164,13 +164,6 @@ jobs: repository: nextcloud/server ref: 'stable27' - - name: Checkout Notifications - uses: actions/checkout@v3 - with: - repository: nextcloud/notifications - ref: ${{ matrix.server-version }} - path: apps/notifications - - name: Checkout AppAPI uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 with: @@ -205,7 +198,6 @@ jobs: ./occ maintenance:install --verbose --database=pgsql --database-name=nextcloud --database-host=127.0.0.1 \ --database-port=$DB_PORT --database-user=root --database-pass=rootpassword \ --admin-user admin --admin-pass admin - ./occ app:enable notifications ./occ app:enable --force ${{ env.APP_NAME }} - name: Run Nextcloud @@ -221,18 +213,54 @@ jobs: working-directory: nc_py_api run: python3 -m pip -v install ".[dev]" - - name: Register NcPyApi + - name: Register App run: | python3 apps/${{ env.APP_NAME }}/tests/install_no_init.py & echo $! > /tmp/_install.pid sleep 5s php occ app_api:daemon:register manual_install "Manual Install" manual-install 0 0 0 - php occ app_api:app:register nc_py_api manual_install --json-info \ - "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"SYSTEM\", \"FILES\", \"FILES_SHARING\"],\"optional\":[\"USER_INFO\", \"USER_STATUS\", \"NOTIFICATIONS\", \"WEATHER_STATUS\", \"TALK\"]},\"protocol\":\"http\",\"system_app\":1}" \ + php occ app_api:app:register $APP_ID manual_install --json-info \ + "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"ALL\"],\"optional\":[]},\"protocol\":\"http\",\"system_app\":1}" \ --force-scopes --wait-finish kill -15 $(cat /tmp/_install.pid) timeout 3m tail --pid=$(cat /tmp/_install.pid) -f /dev/null + - name: Check logs + run: grep -q 'Hello from ' data/nextcloud.log || error + + - name: Test ALL Scope, System App + run: python3 apps/${{ env.APP_NAME }}/tests/auth_scopes_system.py 0 + + - name: Re-Register App + run: | + php occ app_api:app:unregister $APP_ID --silent || true + python3 apps/${{ env.APP_NAME }}/tests/install_no_init.py & + echo $! > /tmp/_install.pid + sleep 5s + php occ app_api:app:register $APP_ID manual_install --json-info \ + "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"SYSTEM\"],\"optional\":[]},\"protocol\":\"http\",\"system_app\":1}" \ + --force-scopes --wait-finish + kill -15 $(cat /tmp/_install.pid) + timeout 3m tail --pid=$(cat /tmp/_install.pid) -f /dev/null + + - name: Test NO ALL Scope, System App + run: python3 apps/${{ env.APP_NAME }}/tests/auth_scopes_system.py 1 + + - name: Re-Register App + run: | + php occ app_api:app:unregister $APP_ID --silent || true + python3 apps/${{ env.APP_NAME }}/tests/install_no_init.py & + echo $! > /tmp/_install.pid + sleep 5s + php occ app_api:app:register $APP_ID manual_install --json-info \ + "{\"appid\":\"$APP_ID\",\"name\":\"$APP_ID\",\"daemon_config_name\":\"manual_install\",\"version\":\"$APP_VERSION\",\"secret\":\"$APP_SECRET\",\"host\":\"localhost\",\"port\":$APP_PORT,\"scopes\":{\"required\":[\"ALL\"],\"optional\":[]},\"protocol\":\"http\",\"system_app\":0}" \ + --force-scopes --wait-finish + kill -15 $(cat /tmp/_install.pid) + timeout 3m tail --pid=$(cat /tmp/_install.pid) -f /dev/null + + - name: Test NO ALL Scope, NO System App + run: python3 apps/${{ env.APP_NAME }}/tests/auth_scopes_system.py 2 + - name: Upload NC logs if: always() uses: actions/upload-artifact@v3 @@ -245,7 +273,7 @@ jobs: permissions: contents: none runs-on: ubuntu-22.04 - needs: [app-version-higher, no-init-endpoint] + needs: [app-version-higher, auth-tests-no-init] name: TestsSpecial-OK steps: - run: echo "Tests special passed successfully" diff --git a/CHANGELOG.md b/CHANGELOG.md index 08fe0a43..efff1c9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] -## [1.4.5 - 202x-xx-xx] +## [1.4.5 - 2024-01-02] + +### Added + +- Support for `ALL` API scope, that allows to call anyNextcloud endpoints bypassing API Scope check. #190 ### Fixed - Fixed incorrect DeployConfig SSL params parsing. #188 (Thanks to @raudraido) +- Incorrect HTTP status during invalid auth. #190 ## [1.4.4 - 2023-12-21] diff --git a/docs/tech_details/ApiScopes.rst b/docs/tech_details/ApiScopes.rst index f915dd7f..f5a8fbbd 100644 --- a/docs/tech_details/ApiScopes.rst +++ b/docs/tech_details/ApiScopes.rst @@ -20,18 +20,19 @@ but a subsequent version does, you can effortlessly specify the new API groups i The following API groups are currently supported: -* ``2`` SYSTEM -* ``10`` FILES -* ``11`` FILES_SHARING -* ``30`` USER_INFO -* ``31`` USER_STATUS -* ``32`` NOTIFICATIONS -* ``33`` WEATHER_STATUS -* ``50`` TALK -* ``60`` TALK_BOT -* ``61`` AI_PROVIDERS -* ``110`` ACTIVITIES -* ``120`` NOTES +* ``2`` SYSTEM +* ``10`` FILES +* ``11`` FILES_SHARING +* ``30`` USER_INFO +* ``31`` USER_STATUS +* ``32`` NOTIFICATIONS +* ``33`` WEATHER_STATUS +* ``50`` TALK +* ``60`` TALK_BOT +* ``61`` AI_PROVIDERS +* ``110`` ACTIVITIES +* ``120`` NOTES +* ``9999`` ALL These groups are identified using names. As time progresses, the list will steadily expand, comprehensively encompassing all potential APIs provided by Nextcloud. diff --git a/lib/Middleware/AppAPIAuthMiddleware.php b/lib/Middleware/AppAPIAuthMiddleware.php index f4fe81b6..c70cb730 100644 --- a/lib/Middleware/AppAPIAuthMiddleware.php +++ b/lib/Middleware/AppAPIAuthMiddleware.php @@ -23,26 +23,29 @@ class AppAPIAuthMiddleware extends Middleware { public function __construct( private AppAPIService $service, - protected IRequest $request, + protected IRequest $request, private IL10N $l, private LoggerInterface $logger, ) { } + /** + * @throws AppAPIAuthNotValidException when a security check fails + * @throws \ReflectionException + */ public function beforeController($controller, $methodName) { $reflectionMethod = new ReflectionMethod($controller, $methodName); $isAppAPIAuth = !empty($reflectionMethod->getAttributes(AppAPIAuth::class)); - if ($isAppAPIAuth) { if (!$this->service->validateExAppRequestToNC($this->request)) { - throw new AppAPIAuthNotValidException($this->l->t('AppAPIAuth authentication failed'), Http::STATUS_UNAUTHORIZED); + throw new AppAPIAuthNotValidException($this->l->t('AppAPI authentication failed'), Http::STATUS_UNAUTHORIZED); } } } /** - * If an AEAuthNotValidException is being caught + * If an AppAPIAuthNotValidException is being caught * * @param Controller $controller the controller that is being called * @param string $methodName the name of the method that will be called on @@ -52,21 +55,11 @@ public function beforeController($controller, $methodName) { * @throws Exception the passed in exception if it can't handle it */ public function afterException($controller, $methodName, Exception $exception): Response { - if ($exception instanceof AppAPIAuth) { - $response = new JSONResponse([ - 'message' => $exception->getMessage(), - ]); - if (stripos($this->request->getHeader('Accept'), 'html') === false) { - $response = new JSONResponse( - ['message' => $exception->getMessage()], - $exception->getCode() - ); - } - + if ($exception instanceof AppAPIAuthNotValidException) { $this->logger->debug($exception->getMessage(), [ 'exception' => $exception, ]); - return $response; + return new JSONResponse(['message' => $exception->getMessage()], $exception->getCode()); } throw $exception; diff --git a/lib/Service/AppAPIService.php b/lib/Service/AppAPIService.php index e2419219..a88e85a1 100644 --- a/lib/Service/AppAPIService.php +++ b/lib/Service/AppAPIService.php @@ -38,7 +38,6 @@ use SimpleXMLElement; class AppAPIService { - public const BASIC_API_SCOPE = 1; public const CACHE_TTL = 60 * 60; // 1 hour private ICache $cache; @@ -737,21 +736,27 @@ public function validateExAppRequestToNC(IRequest $request, bool $isDav = false) } else { $path = '/dav/'; } + + $allScopesFlag = (bool)$this->exAppScopesService->getByScope($exApp, ExAppApiScopeService::ALL_API_SCOPE); $apiScope = $this->exAppApiScopeService->getApiScopeByRoute($path); - if ($apiScope === null) { - $this->logger->error(sprintf('Failed to check apiScope %s', $path)); - return false; - } - // BASIC ApiScope is granted to all ExApps (all Api routes with BASIC scope group). - if ($apiScope->getScopeGroup() !== self::BASIC_API_SCOPE) { - if (!$this->exAppScopesService->passesScopeCheck($exApp, $apiScope->getScopeGroup())) { - $this->logger->error(sprintf('ExApp %s not passed scope group check %s', $exApp->getAppid(), $path)); + if (!$allScopesFlag) { + if ($apiScope === null) { + $this->logger->error(sprintf('Failed to check apiScope %s', $path)); return false; } + + // BASIC ApiScope is granted to all ExApps (all API routes with BASIC scope group). + if ($apiScope->getScopeGroup() !== ExAppApiScopeService::BASIC_API_SCOPE) { + if (!$this->exAppScopesService->passesScopeCheck($exApp, $apiScope->getScopeGroup())) { + $this->logger->error(sprintf('ExApp %s not passed scope group check %s', $exApp->getAppid(), $path)); + return false; + } + } } + // For APIs that not assuming work under user context we do not check ExApp users - if ($apiScope->getUserCheck()) { + if (($apiScope === null) or ($apiScope->getUserCheck())) { try { if (!$this->exAppUsersService->exAppUserExists($exApp->getAppid(), $userId)) { $this->logger->error(sprintf('ExApp %s user %s does not exist', $exApp->getAppid(), $userId)); diff --git a/lib/Service/ExAppApiScopeService.php b/lib/Service/ExAppApiScopeService.php index b38b990f..3a1dacc6 100644 --- a/lib/Service/ExAppApiScopeService.php +++ b/lib/Service/ExAppApiScopeService.php @@ -16,6 +16,8 @@ use Psr\Log\LoggerInterface; class ExAppApiScopeService { + public const BASIC_API_SCOPE = 1; + public const ALL_API_SCOPE = 9999; private ICache $cache; public function __construct( @@ -115,6 +117,9 @@ public function registerInitScopes(): bool { ['api_route' => '/apps/spreed/api/', 'scope_group' => 50, 'name' => 'TALK', 'user_check' => 1], ['api_route' => '/apps/activity/api/', 'scope_group' => 110, 'name' => 'ACTIVITIES', 'user_check' => 1], ['api_route' => '/apps/notes/api/', 'scope_group' => 120, 'name' => 'NOTES', 'user_check' => 1], + + //ALL Scope + ['api_route' => 'non-exist-all-api-route', 'scope_group' => self::ALL_API_SCOPE, 'name' => 'ALL', 'user_check' => 1], ]; $this->cache->clear('/all_api_scopes'); diff --git a/tests/auth_scopes_system.py b/tests/auth_scopes_system.py new file mode 100644 index 00000000..ef21c54d --- /dev/null +++ b/tests/auth_scopes_system.py @@ -0,0 +1,25 @@ +import sys +import pytest +import nc_py_api + +# sys.argv[1] = 0 -> System App, ALL Scope +# sys.argv[1] = 1 -> System App, No ALL Scope +# sys.argv[1] = 2 -> No System App, ALL Scope + +if __name__ == "__main__": + nc = nc_py_api.NextcloudApp(user="admin") + assert nc.capabilities + if int(sys.argv[1]) == 0: + nc.ocs("GET", "/ocs/v2.php/core/whatsnew") + else: + with pytest.raises(nc_py_api.NextcloudException) as e: + nc.ocs("GET", "/ocs/v2.php/core/whatsnew") + assert e.value.status_code == 401 + + if int(sys.argv[1]) == 2: + # as NextcloudApp was initialized with `user="admin"` this will fail for non-system app. + with pytest.raises(nc_py_api.NextcloudException) as e: + nc.users_list() + assert e.value.status_code == 401 + else: + assert nc.users_list() diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 9bc4e2c0..eed90ead 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -96,17 +96,6 @@ IEventListener - - - $exception instanceof AppAPIAuth - - - getCode - getMessage - getMessage - getMessage - - Request