From eabfb2dca1acbe8ed30a36f3a66127571d22969e Mon Sep 17 00:00:00 2001 From: Andrey Borysenko Date: Wed, 9 Aug 2023 18:51:48 +0300 Subject: [PATCH] ExApp version check (#29) - [x] Resolve conflicts - [ ] Merge into auth throttling PR first --------- Signed-off-by: Alexander Piskun Co-authored-by: Alexander Piskun --- .github/workflows/tests-special.yml | 145 +++++++++++++++++++ docs/authentication.rst | 1 + lib/AppInfo/Application.php | 2 + lib/Db/ExAppMapper.php | 12 ++ lib/Notifications/ExAppAdminNotifier.php | 62 ++++++++ lib/Notifications/ExAppNotifier.php | 5 +- lib/Notifications/ExNotificationsManager.php | 30 +++- lib/Service/AppEcosystemV2Service.php | 89 ++++++++++-- lib/Settings/Admin.php | 2 +- lib/Settings/AdminSection.php | 2 +- tests/app_version_higher.py | 26 ++++ 11 files changed, 352 insertions(+), 24 deletions(-) create mode 100644 .github/workflows/tests-special.yml create mode 100644 lib/Notifications/ExAppAdminNotifier.php create mode 100644 tests/app_version_higher.py diff --git a/.github/workflows/tests-special.yml b/.github/workflows/tests-special.yml new file mode 100644 index 00000000..d5ef570a --- /dev/null +++ b/.github/workflows/tests-special.yml @@ -0,0 +1,145 @@ +name: Tests Special + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: tests-special-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + app-version-higher: + runs-on: ubuntu-22.04 + name: ExApp version higher + env: + NEXTCLOUD_URL: "http://localhost:8080/" + APP_ID: "nc_py_api" + APP_PORT: 9009 + APP_VERSION: "1.0.0" + APP_SECRET: "tC6vkwPhcppjMykD1r0n9NlI95uJMBYjs5blpIcA1PAdoPDmc5qoAjaBAkyocZ6E" + + services: + postgres: + image: ghcr.io/nextcloud/continuous-integration-postgres-14:latest + ports: + - 4444:5432/tcp + env: + POSTGRES_USER: root + POSTGRES_PASSWORD: rootpassword + POSTGRES_DB: nextcloud + options: --health-cmd pg_isready --health-interval 5s --health-timeout 2s --health-retries 5 + + steps: + - uses: actions/setup-python@v4 + with: + python-version: '3.11' + + - name: Set app env + run: echo "APP_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV + + - name: Checkout server + uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 + with: + submodules: true + 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 AppEcosystemV2 + uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 + with: + path: apps/${{ env.APP_NAME }} + + - name: Set up php + uses: shivammathur/setup-php@4bd44f22a98a19e0950cbad5f31095157cc9621b # v2 + with: + php-version: '8.1' + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql + coverage: none + ini-file: development + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Check composer file existence + id: check_composer + uses: andstor/file-existence-action@20b4d2e596410855db8f9ca21e96fbe18e12930b # v2 + with: + files: apps/${{ env.APP_NAME }}/composer.json + + - name: Set up dependencies + if: steps.check_composer.outputs.files_exists == 'true' + working-directory: apps/${{ env.APP_NAME }} + run: composer i + + - name: Set up Nextcloud + env: + DB_PORT: 4444 + run: | + mkdir data + ./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 config:system:set allow_local_remote_servers --value true + ./occ app:enable notifications + ./occ app:enable --force ${{ env.APP_NAME }} + patch -p 1 -i apps/${{ env.APP_NAME }}/base_php.patch + + - name: Run Nextcloud + run: php -S 127.0.0.1:8080 & + + - name: Checkout NcPyApi + uses: actions/checkout@v3 + with: + path: nc_py_api + repository: cloud-py-api/nc_py_api + + - name: Install NcPyApi + working-directory: nc_py_api + run: python3 -m pip -v install ".[dev]" + + - name: Register NcPyApi + run: | + cd nc_py_api + python3 tests/_install.py & + echo $! > /tmp/_install.pid + cd .. + sleep 5s + php occ app_ecosystem_v2:daemon:register manual_install "Manual Install" manual-install 0 0 0 + php occ app_ecosystem_v2: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,\"protocol\":\"http\",\"system_app\":1}" \ + -e --force-scopes + kill -15 $(cat /tmp/_install.pid) + timeout 3m tail --pid=$(cat /tmp/_install.pid) -f /dev/null + + - name: Run Manual App Update test + working-directory: apps/${{ env.APP_NAME }} + run: python3 tests/app_version_higher.py + + - name: Upload NC logs + if: always() + uses: actions/upload-artifact@v3 + with: + name: app_version_higher_nextcloud.log + path: data/nextcloud.log + if-no-files-found: warn + + tests-success: + permissions: + contents: none + runs-on: ubuntu-22.04 + needs: [app-version-higher] + name: TestsSpecial-OK + steps: + - run: echo "Tests special passed successfully" diff --git a/docs/authentication.rst b/docs/authentication.rst index f484b195..4c7b14cd 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -99,6 +99,7 @@ Authentication flow in details Nextcloud->>+AppEcosystemV2: Validate request AppEcosystemV2-->>AppEcosystemV2: Check if ExApp exists and enabled AppEcosystemV2-->>Nextcloud: Reject if ExApp not exists or disabled + AppEcosystemV2-->>AppEcosystemV2: Check if ExApp version changed AppEcosystemV2-->>AppEcosystemV2: Validate AE-SIGN-TIME AppEcosystemV2-->>Nextcloud: Reject if sign time diff > 5 min AppEcosystemV2-->>AppEcosystemV2: Generate and validate AE-SIGNATURE diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 64e03f16..bea6a9bd 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -9,6 +9,7 @@ use OCA\AppEcosystemV2\Listener\LoadFilesPluginListener; use OCA\AppEcosystemV2\Listener\SabrePluginAuthInitListener; use OCA\AppEcosystemV2\Middleware\AppEcosystemAuthMiddleware; +use OCA\AppEcosystemV2\Notifications\ExAppAdminNotifier; use OCA\AppEcosystemV2\Notifications\ExAppNotifier; use OCA\AppEcosystemV2\Profiler\AEDataCollector; use OCA\AppEcosystemV2\PublicCapabilities; @@ -45,6 +46,7 @@ public function register(IRegistrationContext $context): void { $context->registerMiddleware(AppEcosystemAuthMiddleware::class); $context->registerEventListener(SabrePluginAuthInitEvent::class, SabrePluginAuthInitListener::class); $context->registerNotifierService(ExAppNotifier::class); + $context->registerNotifierService(ExAppAdminNotifier::class); } public function boot(IBootContext $context): void { diff --git a/lib/Db/ExAppMapper.php b/lib/Db/ExAppMapper.php index 55fe14bd..4d978764 100644 --- a/lib/Db/ExAppMapper.php +++ b/lib/Db/ExAppMapper.php @@ -102,4 +102,16 @@ public function updateLastCheckTime(ExApp $exApp): int { $qb->expr()->eq('appid', $qb->createNamedParameter($exApp->getAppid())) )->executeStatement(); } + + /** + * @throws Exception + */ + public function updateExAppVersion(ExApp $exApp): int { + $qb = $this->db->getQueryBuilder(); + return $qb->update($this->tableName) + ->set('version', $qb->createNamedParameter($exApp->getVersion(), IQueryBuilder::PARAM_STR)) + ->where( + $qb->expr()->eq('appid', $qb->createNamedParameter($exApp->getAppid())) + )->executeStatement(); + } } diff --git a/lib/Notifications/ExAppAdminNotifier.php b/lib/Notifications/ExAppAdminNotifier.php new file mode 100644 index 00000000..41ed7053 --- /dev/null +++ b/lib/Notifications/ExAppAdminNotifier.php @@ -0,0 +1,62 @@ +factory = $factory; + $this->url = $urlGenerator; + $this->service = $service; + } + + public function getID(): string { + return Application::APP_ID; + } + + public function getName(): string { + return $this->factory->get(Application::APP_ID)->t('AppEcosystemV2 ExApp version update notifier'); + } + + public function prepare(INotification $notification, string $languageCode): INotification { + $exApp = $this->service->getExApp($notification->getApp()); + // TODO: Think about another possible admin ExApp notifications, make them unified + // TODO: Think about ExApp rich objects + if ($exApp === null || $notification->getSubject() !== 'ex_app_version_update') { + throw new \InvalidArgumentException(); + } + if ($exApp->getEnabled()) { + throw new \InvalidArgumentException('ExApp is probably already re-enabled'); + } + + $parameters = $notification->getSubjectParameters(); + + $notification->setLink($this->url->getAbsoluteURL('/index.php/settings/admin/app_ecosystem_v2')); + $notification->setIcon($this->url->imagePath(Application::APP_ID, 'app-dark.svg')); + + if (isset($parameters['rich_subject']) && isset($parameters['rich_subject_params'])) { + $notification->setRichSubject($parameters['rich_subject'], $parameters['rich_subject_params']); + } + if (isset($parameters['rich_message']) && isset($parameters['rich_message_params'])) { + $notification->setRichMessage($parameters['rich_message'], $parameters['rich_message_params']); + } + + return $notification; + } +} diff --git a/lib/Notifications/ExAppNotifier.php b/lib/Notifications/ExAppNotifier.php index 3772add6..f1e8bcb7 100644 --- a/lib/Notifications/ExAppNotifier.php +++ b/lib/Notifications/ExAppNotifier.php @@ -39,8 +39,9 @@ public function prepare(INotification $notification, string $languageCode): INot if ($exApp === null) { throw new \InvalidArgumentException(); } - // Only enabled ExApps can render notifications - if (!$exApp->getEnabled()) { + if ($notification->getSubject() === 'ex_app_version_update' && $exApp->getEnabled()) { + throw new \InvalidArgumentException('ExApp is probably already re-enabled'); + } elseif (!$exApp->getEnabled()) { // Only enabled ExApps can render notifications throw new \InvalidArgumentException('ExApp is disabled'); } diff --git a/lib/Notifications/ExNotificationsManager.php b/lib/Notifications/ExNotificationsManager.php index c5b00b8e..19b36f79 100644 --- a/lib/Notifications/ExNotificationsManager.php +++ b/lib/Notifications/ExNotificationsManager.php @@ -4,14 +4,17 @@ namespace OCA\AppEcosystemV2\Notifications; +use OCP\IGroupManager; use OCP\Notification\IManager; use OCP\Notification\INotification; class ExNotificationsManager { - private IManager $manager; + private IManager $notificationManager; + private IGroupManager $groupManager; - public function __construct(IManager $manager) { - $this->manager = $manager; + public function __construct(IManager $manager, IGroupManager $groupManager) { + $this->notificationManager = $manager; + $this->groupManager = $groupManager; } /** @@ -24,14 +27,31 @@ public function __construct(IManager $manager) { * @return INotification */ public function sendNotification(string $appId, ?string $userId = null, array $params = []): INotification { - $notification = $this->manager->createNotification(); + $notification = $this->notificationManager->createNotification(); $notification ->setApp($appId) ->setUser($userId) ->setDateTime(new \DateTime()) ->setObject($params['object'], $params['object_id']) ->setSubject($params['subject_type'], $params['subject_params']); - $this->manager->notify($notification); + $this->notificationManager->notify($notification); return $notification; } + + public function sendAdminsNotification(string $appId, array $params = []): array { + $admins = $this->groupManager->get("admin")->getUsers(); + $notifications = []; + foreach ($admins as $adminUser) { + $notification = $this->notificationManager->createNotification(); + $notification + ->setApp($appId) + ->setUser($adminUser->getUID()) + ->setDateTime(new \DateTime()) + ->setObject($params['object'], $params['object_id']) + ->setSubject($params['subject_type'], $params['subject_params']); + $this->notificationManager->notify($notification); + $notifications[] = $notification; + } + return $notifications; + } } diff --git a/lib/Service/AppEcosystemV2Service.php b/lib/Service/AppEcosystemV2Service.php index 6d7b1c28..e90b3a5e 100644 --- a/lib/Service/AppEcosystemV2Service.php +++ b/lib/Service/AppEcosystemV2Service.php @@ -8,6 +8,7 @@ use OCA\AppEcosystemV2\Db\ExApp; use OCA\AppEcosystemV2\Db\ExAppMapper; +use OCA\AppEcosystemV2\Notifications\ExNotificationsManager; use OCP\App\IAppManager; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -45,22 +46,24 @@ class AppEcosystemV2Service { private ExAppUsersService $exAppUsersService; private ExAppScopesService $exAppScopesService; private ExAppConfigService $exAppConfigService; + private ExNotificationsManager $exNotificationsManager; public function __construct( - LoggerInterface $logger, - ILogFactory $logFactory, - ICacheFactory $cacheFactory, - IConfig $config, - IClientService $clientService, - ExAppMapper $exAppMapper, - IAppManager $appManager, - ExAppUsersService $exAppUserService, + LoggerInterface $logger, + ILogFactory $logFactory, + ICacheFactory $cacheFactory, + IConfig $config, + IClientService $clientService, + ExAppMapper $exAppMapper, + IAppManager $appManager, + ExAppUsersService $exAppUserService, ExAppApiScopeService $exAppApiScopeService, - ExAppScopesService $exAppScopesService, - ISecureRandom $random, - IUserSession $userSession, - IUserManager $userManager, - ExAppConfigService $exAppConfigService, + ExAppScopesService $exAppScopesService, + ISecureRandom $random, + IUserSession $userSession, + IUserManager $userManager, + ExAppConfigService $exAppConfigService, + ExNotificationsManager $exNotificationsManager, ) { $this->logger = $logger; $this->logFactory = $logFactory; @@ -76,6 +79,7 @@ public function __construct( $this->exAppApiScopeService = $exAppApiScopeService; $this->exAppScopesService = $exAppScopesService; $this->exAppConfigService = $exAppConfigService; + $this->exNotificationsManager = $exNotificationsManager; } public function getExApp(string $appId): ?ExApp { @@ -448,6 +452,7 @@ private function generateDataHash(string $data): string { /** * AppEcosystem authentication request validation for Nextcloud: * - checks if ExApp exists and is enabled + * - checks if ExApp version changed and updates it in database * - validates request sign time (if it's complies with set time window) * - builds and checks request signature * - checks if request data hash is valid @@ -498,6 +503,9 @@ public function validateExAppRequestToNC(IRequest $request, bool $isDav = false) $signatureValid = $signature === $requestSignature; if ($signatureValid) { + if (!$this->handleExAppVersionChange($request, $exApp)) { + return false; + } if (!$this->verifyDataHash($dataHash)) { $this->logger->error(sprintf('Data hash %s is not valid', $dataHash)); return false; @@ -605,14 +613,65 @@ public function updateExAppLastCheckTime(ExApp &$exApp): void { } } + public function updateExAppVersion(ExApp $exApp): bool { + try { + return $this->exAppMapper->updateExAppVersion($exApp) === 1; + } catch (Exception $e) { + $this->logger->error(sprintf('Failed to update ExApp %s version to %s', $exApp->getAppid(), $exApp->getVersion()), ['exception' => $e]); + return false; + } + } + + /** + * Check if ExApp version changed and update it in database. + * Immediately disable ExApp and send notifications to the administrators (users of admins group). + * This handling only intentional case of manual ExApp update + * so the administrator must re-enable ExApp in UI or CLI after that. + * + * Ref: https://github.com/cloud-py-api/app_ecosystem_v2/pull/29 + * TODO: Add link to docs with warning and mark as not-recommended + * + * @param IRequest $request + * @param ExApp $exApp + * + * @return bool + */ + public function handleExAppVersionChange(IRequest $request, ExApp &$exApp): bool { + $requestExAppVersion = $request->getHeader('EX-APP-VERSION'); + $versionValid = $exApp->getVersion() === $requestExAppVersion; + if (!$versionValid) { + // Update ExApp version + $oldVersion = $exApp->getVersion(); + $exApp->setVersion($requestExAppVersion); + if (!$this->updateExAppVersion($exApp)) { + return false; + } + if ($this->disableExApp($exApp)) { + $this->exNotificationsManager->sendAdminsNotification($exApp->getAppid(), [ + 'object' => 'ex_app_update', + 'object_id' => $exApp->getAppid(), + 'subject_type' => 'ex_app_version_update', + 'subject_params' => [ + 'rich_subject' => 'ExApp updated, action required!', + 'rich_subject_params' => [], + 'rich_message' => sprintf('ExApp %s disabled due to update from %s to %s. Manual re-enable required.', $exApp->getAppid(), $oldVersion, $exApp->getVersion()), + 'rich_message_params' => [], + ], + ]); + } + return false; + } + return true; + } + public function getExAppsList(string $list = 'enabled'): array { try { $exApps = $this->exAppMapper->findAll(); if ($list === 'enabled') { - $exApps = array_filter($exApps, function (ExApp $exApp) { + $exApps = array_values(array_filter($exApps, function (ExApp $exApp) { return $exApp->getEnabled() === 1; - }); + })); } $exApps = array_map(function (ExApp $exApp) { diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index a6bbbda5..2b96ee9b 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -38,7 +38,7 @@ public function getForm(): TemplateResponse { } public function getSection(): string { - return 'app-ecosystem-v2'; + return Application::APP_ID; } public function getPriority(): int { diff --git a/lib/Settings/AdminSection.php b/lib/Settings/AdminSection.php index d8d4b6e4..c4f9ebec 100644 --- a/lib/Settings/AdminSection.php +++ b/lib/Settings/AdminSection.php @@ -29,7 +29,7 @@ public function __construct( * @inheritDoc */ public function getID(): string { - return 'app-ecosystem-v2'; + return 'app_ecosystem_v2'; } /** diff --git a/tests/app_version_higher.py b/tests/app_version_higher.py new file mode 100644 index 00000000..5497d55e --- /dev/null +++ b/tests/app_version_higher.py @@ -0,0 +1,26 @@ +import pytest + +from nc_py_api import Nextcloud, NextcloudApp, NextcloudException + + +if __name__ == "__main__": + nc_client = Nextcloud(nc_auth_user="admin", nc_auth_pass="admin") + assert nc_client.apps.ex_app_is_disabled("nc_py_api") is False + nc_client.users.create("second_admin", password="2Very3Strong4", groups=["admin"]) + + nc_application = NextcloudApp(user="admin") + assert nc_application.users.get_details() # OCS call works + assert not nc_application.users.notifications.get_all() # there are no notifications + nc_application._session.adapter.headers.update({"EX-APP-VERSION": "99.0.0"}) # change ExApp version + with pytest.raises(NextcloudException) as exc_info: + nc_application.users.get_details() # this call should be rejected by AppEcosystem + assert exc_info.value.status_code == 401 + + assert nc_client.apps.ex_app_is_disabled("nc_py_api") is True + notifications = nc_client.users.notifications.get_all() + notification = [i for i in notifications if i.object_type == "ex_app_update"] + assert len(notification) == 1 # only one notification for each admin + nc_client = Nextcloud(nc_auth_user="second_admin", nc_auth_pass="2Very3Strong4") + notifications = nc_client.users.notifications.get_all() + notification = [i for i in notifications if i.object_type == "ex_app_update"] + assert len(notification) == 1 # only one notification for each admin