diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml index 113e796..c3bf5e9 100644 --- a/.github/workflows/integration-test.yml +++ b/.github/workflows/integration-test.yml @@ -93,26 +93,33 @@ jobs: php occ integration_onedrive:setup-user admin "$(jq -r '.user_name' config.json)" "$(jq -r '.refresh_token' config.json)" "$(jq -r '.access_token' config.json)" rm -f config.json php occ integration_onedrive:start-import admin + # The drive behind the secrets holds more than 500 MB, so a full import takes several + # batches of minutes each. Import until a few files have arrived, then stop: the import + # stays in progress, which the second run below takes into account. + max_imported=0 for run in {1..9}; do date - echo "run $run starting" + echo "round $run starting" timeout 40 php cron.php || true - echo "run $run done" - nb=$(php occ user:setting admin integration_onedrive nb_imported_files 2>/dev/null | tail -1 || echo "0") - echo "imported files so far: $nb" - if [ "$nb" -gt "2" ] 2>/dev/null; then + imported=$(php occ user:setting admin integration_onedrive nb_imported_files 2>/dev/null | tail -1 || echo "0") + skipped=$(php occ user:setting admin integration_onedrive nb_skipped_files 2>/dev/null | tail -1 || echo "0") + failed=$(php occ user:setting admin integration_onedrive nb_failed_files 2>/dev/null | tail -1 || echo "0") + echo "round $run: imported=$imported skipped=$skipped failed=$failed" + if [ "$imported" -gt "$max_imported" ] 2>/dev/null; then max_imported=$imported; fi + if [ "$imported" -gt "2" ] 2>/dev/null; then echo "Files imported successfully, stopping early" break fi done date + echo "MAX_IMPORTED=$max_imported" >> "$GITHUB_ENV" - name: Check import result if: always() working-directory: server/ run: | echo "=== Import status ===" - for key in nb_imported_files imported_size importing_onedrive onedrive_import_running last_onedrive_import_timestamp; do + for key in nb_imported_files nb_skipped_files nb_failed_files failed_files imported_size importing_onedrive onedrive_import_running last_onedrive_import_timestamp; do val=$(php occ user:setting admin integration_onedrive "$key" 2>/dev/null | tail -1 || echo "n/a") echo " $key: $val" done @@ -124,16 +131,222 @@ jobs: - name: Verify import working-directory: server/ run: | - nb=$(php occ user:setting admin integration_onedrive nb_imported_files 2>/dev/null | tail -1 || echo "0") - if [ "$nb" -gt "2" ] 2>/dev/null; then - echo "Import verification passed: $nb files imported" + if [ "${MAX_IMPORTED:-0}" -gt "2" ] 2>/dev/null; then + echo "Import verification passed: $MAX_IMPORTED files imported" else echo "Import verification failed: no files were imported" exit 1 fi + failed=$(php occ user:setting admin integration_onedrive nb_failed_files 2>/dev/null | tail -1 || echo "0") + if [ "$failed" != "0" ]; then + echo "The import reported $failed failed download(s) against a healthy drive" + exit 1 + fi - name: Show log on failure working-directory: server/ if: always() run: | tail -100 data/nextcloud.log | sed 's/"access_token":"[^"]*"/"access_token":"***"/g; s/"refresh_token":"[^"]*"/"refresh_token":"***"/g' + + stub-integration: + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + include: + - php-versions: '8.2' + server-versions: 'stable33' + - php-versions: '8.3' + server-versions: 'master' + + name: Import against a stubbed Graph API + + steps: + - name: Checkout nextcloud + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: server + repository: nextcloud/server + ref: ${{ matrix.server-versions }} + persist-credentials: false + + - name: Set up php + uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2 + with: + php-version: ${{ matrix.php-versions }} + coverage: none + ini-file: development + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Install nextcloud + working-directory: server/ + run: | + git submodule update --init + php occ maintenance:install --verbose --admin-user admin --admin-pass admin + + - name: Checkout app + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: server/apps/${{ env.APP_NAME }} + persist-credentials: false + + - name: Checkout the notifications app + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: server/apps/notifications + repository: nextcloud/notifications + ref: ${{ matrix.server-versions }} + persist-credentials: false + + - name: Install dependencies + run: | + composer i --working-dir=server/apps/${{ env.APP_NAME }} + composer i --no-dev --working-dir=server/apps/notifications + + - name: Install apps + working-directory: server/ + run: | + php occ app:enable notifications + php occ app:enable ${{ env.APP_NAME }} -vvv + # the HTTP client refuses to talk to a local address unless this is allowed + php occ config:system:set allow_local_remote_servers --value true --type boolean + + - name: Start the Graph stub + working-directory: server/ + run: | + php -S 127.0.0.1:8099 apps/${{ env.APP_NAME }}/tests/integration/graph-stub.php > stub.log 2>&1 & + for _ in $(seq 1 20); do + curl -fs -H 'Authorization: bearer test' http://127.0.0.1:8099/v1.0/me/drive >/dev/null && break + sleep 0.5 + done + curl -fs -H 'Authorization: bearer test' http://127.0.0.1:8099/v1.0/me/drive + echo + # a request without an access token has to be refused, otherwise the test could pass + # while the app stopped sending the token + test "$(curl -s -o /dev/null -w '%{http_code}' http://127.0.0.1:8099/v1.0/me/drive)" = "401" + + - name: Run Nextcloud + working-directory: server/ + run: php -S localhost:8080 & + + - name: Point the app at the stub + working-directory: server/ + run: | + php occ ${{ env.APP_NAME }}:setup stub-client stub-secret + php occ ${{ env.APP_NAME }}:setup-user admin stub-user stub-refresh-token stub-access-token + # setup-user expires the token on purpose so that it gets refreshed; the stub has no token + # endpoint, so keep the one it stored valid + php occ user:setting admin ${{ env.APP_NAME }} token_expires_at "$(( $(date +%s) + 86400 ))" + php occ config:app:set ${{ env.APP_NAME }} api_base_url --value http://127.0.0.1:8099/v1.0/ + + - name: Put one of the files in the target folder beforehand + working-directory: server/ + run: | + mkdir -p "data/admin/files/OneDrive import" + printf 'already there' > "data/admin/files/OneDrive import/already.txt" + php occ files:scan admin -q + + - name: Import + working-directory: server/ + run: | + php occ ${{ env.APP_NAME }}:start-import admin + importing=1 + for run in {1..4}; do + php cron.php + importing=$(php occ user:setting admin ${{ env.APP_NAME }} importing_onedrive 2>/dev/null | tail -1 || echo "1") + echo "round $run: importing=$importing" + if [ "$importing" = "0" ]; then break; fi + done + if [ "$importing" != "0" ]; then + echo "The import did not finish" + exit 1 + fi + + - name: Verify the imported files + working-directory: server/ + run: | + target="data/admin/files/OneDrive import" + find "$target" -type f -printf '%P %s bytes\n' | sort + fail=0 + check() { # path, expected size + if [ ! -f "$target/$1" ]; then echo "missing: $1"; fail=1; return; fi + size=$(stat -c%s "$target/$1") + if [ "$size" != "$2" ]; then echo "$1 has $size bytes, expected $2"; fail=1; fi + } + check normal.txt 12 + check empty.txt 0 + check flaky.txt 9 + check sub/nested.txt 11 + check already.txt 13 + if [ -f "$target/broken.txt" ]; then + echo "broken.txt should not have been created, its download failed twice" + fail=1 + fi + exit $fail + + - name: Verify what the app asked the stub for + working-directory: server/ + run: | + grep -oE 'stub (GET|POST) [^ ]+ -> [0-9]+' stub.log | sort | uniq -c + fail=0 + expect() { # pattern, what it proves + if ! grep -q "$1" stub.log; then echo "missing request ($2): $1"; fail=1; fi + } + expect 'skiptoken=page2' 'the second listing page was fetched' + expect '/v1.0/me/drive/items/f3 -> 200' 'a fresh download URL was fetched after a failure' + expect '/download/f3?source=fresh -> 200' 'the retry used the fresh URL' + expect '/download/f4?source=fresh -> 403' 'the second attempt failed as well' + expect '/v1.0/me/drive/root:%2Fsub:/children -> 200' 'the subfolder was listed' + if grep -q '/download/f5' stub.log; then + echo "already.txt was downloaded although it was there, it should have been skipped" + fail=1 + fi + exit $fail + + - name: Verify the notification and the counters + working-directory: server/ + run: | + notification=$(curl -s -u admin:admin -H 'OCS-APIRequest: true' -H 'Accept: application/json' \ + 'http://localhost:8080/ocs/v2.php/apps/notifications/api/v2/notifications') + subject=$(php -r 'foreach (json_decode(stream_get_contents(STDIN), true)["ocs"]["data"] as $n) { if (($n["app"] ?? "") === "integration_onedrive") { echo $n["subject"]; break; } }' <<< "$notification") + message=$(php -r 'foreach (json_decode(stream_get_contents(STDIN), true)["ocs"]["data"] as $n) { if (($n["app"] ?? "") === "integration_onedrive") { echo $n["message"] ?? ""; break; } }' <<< "$notification") + echo "subject: $subject" + echo "message: $message" + expected_subject='4 files were imported from OneDrive storage. 1 file was already there. 1 file could not be downloaded, check the server logs for details.' + expected_message='Could not download: broken.txt' + fail=0 + if [ "$subject" != "$expected_subject" ]; then + echo "expected subject: $expected_subject" + fail=1 + fi + if [ "$message" != "$expected_message" ]; then + echo "expected message: $expected_message" + fail=1 + fi + # the counters are reported in the notification and reset afterwards + for key in nb_imported_files nb_skipped_files nb_failed_files; do + val=$(php occ user:setting admin ${{ env.APP_NAME }} "$key" 2>/dev/null | tail -1 || echo "n/a") + if [ "$val" != "0" ]; then + echo "$key should be 0 after the import finished, it is '$val'" + fail=1 + fi + done + # the failed download is in the log for the admin to look up + if ! grep -q 'broken.txt' data/nextcloud.log; then + echo "the log does not mention the file that could not be downloaded" + fail=1 + fi + exit $fail + + - name: Show the logs on failure + working-directory: server/ + if: failure() + run: | + echo "=== stub ===" + tail -60 stub.log || true + echo "=== nextcloud ===" + tail -60 data/nextcloud.log || true diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe17f2..9ab28c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Stop logging download URLs, they contain a short lived access token - Retry a failed file download once with a freshly fetched download URL, the one from the folder listing may have expired during a long import - Stop logging a deprecation warning every time another app sends a notification +- Report files that could not be downloaded in the import finished notification, with their names, which also no longer counts them as imported +- Mention files that were already there in the import finished notification, so re-running an import does not look like a failure +- Count imported empty files as imported +- Test the import against a stubbed Graph API, including failed downloads, empty files and paging ## [3.5.2] - 2026-07-28 diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 112fcf6..8ea772f 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -82,12 +82,38 @@ public function prepare(INotification $notification, string $languageCode): INot switch ($notification->getSubject()) { case 'import_onedrive_finished': - /** @var array{nbImported?: string, targetPath: string} $p */ + /** @var array{nbImported?: string, nbFailed?: string, nbSkipped?: string, failedFiles?: string[], targetPath: string} $p */ $p = $notification->getSubjectParameters(); $nbImported = (int)($p['nbImported'] ?? 0); + $nbFailed = (int)($p['nbFailed'] ?? 0); + $nbSkipped = (int)($p['nbSkipped'] ?? 0); + $failedFiles = is_array($p['failedFiles'] ?? null) ? $p['failedFiles'] : []; $targetPath = $p['targetPath']; $content = $l->n('%n file was imported from OneDrive storage.', '%n files were imported from OneDrive storage.', $nbImported); + if ($nbSkipped > 0) { + $content .= ' ' . $l->n( + '%n file was already there.', + '%n files were already there.', + $nbSkipped + ); + } + if ($nbFailed > 0) { + $content .= ' ' . $l->n( + '%n file could not be downloaded, check the server logs for details.', + '%n files could not be downloaded, check the server logs for details.', + $nbFailed + ); + } + if ($failedFiles !== []) { + $names = implode(', ', $failedFiles); + $nbMore = $nbFailed - count($failedFiles); + $notification->setParsedMessage( + $nbMore > 0 + ? $l->t('Could not download: %1$s, and %2$s more', [$names, (string)$nbMore]) + : $l->t('Could not download: %s', [$names]) + ); + } $notification->setParsedSubject($content) ->setIcon($this->url->getAbsoluteURL($this->url->imagePath(Application::APP_ID, 'app-dark.svg'))) ->setLink($this->url->linkToRouteAbsolute('files.view.index', ['dir' => $targetPath])); diff --git a/lib/Service/OnedriveAPIService.php b/lib/Service/OnedriveAPIService.php index 1e60a61..dac92f1 100644 --- a/lib/Service/OnedriveAPIService.php +++ b/lib/Service/OnedriveAPIService.php @@ -27,6 +27,11 @@ use Throwable; class OnedriveAPIService { + /** + * Microsoft Graph, overridable through the app config so that tests can point the app at a + * local stub instead of reaching out to Microsoft. + */ + private const API_BASE_URL = 'https://graph.microsoft.com/v1.0/'; /** * Give up on a file download that has transferred nothing for this many seconds. @@ -171,7 +176,7 @@ public function request(string $userId, string $endPoint, array $params = [], st $accessToken = $this->config->getUserValue($userId, Application::APP_ID, 'token'); $accessToken = $accessToken === '' ? '' : $this->crypto->decrypt($accessToken); try { - $url = 'https://graph.microsoft.com/v1.0/' . $endPoint; + $url = $this->getApiBaseUrl() . $endPoint; $options = [ 'headers' => [ 'Authorization' => 'bearer ' . $accessToken, @@ -290,6 +295,18 @@ public function requestOAuthAccessToken(array $params = [], string $method = 'PO } } + /** + * The Microsoft Graph base URL, with a trailing slash. The app config value is only meant for + * tests; when it is unset the real Graph endpoint is used. + */ + private function getApiBaseUrl(): string { + $baseUrl = (string)$this->config->getAppValue(Application::APP_ID, 'api_base_url', self::API_BASE_URL); + if ($baseUrl === '') { + $baseUrl = self::API_BASE_URL; + } + return rtrim($baseUrl, '/') . '/'; + } + private function checkTokenExpiration(string $userId): void { $refreshToken = $this->config->getUserValue($userId, Application::APP_ID, 'refresh_token'); $refreshToken = $refreshToken === '' ? '' : $this->crypto->decrypt($refreshToken); diff --git a/lib/Service/OnedriveStorageAPIService.php b/lib/Service/OnedriveStorageAPIService.php index bb35a5b..6a8b21f 100644 --- a/lib/Service/OnedriveStorageAPIService.php +++ b/lib/Service/OnedriveStorageAPIService.php @@ -39,6 +39,17 @@ */ class OnedriveStorageAPIService { + + private const FILE_DOWNLOADED = 'downloaded'; + private const FILE_ALREADY_THERE = 'already there'; + private const FILE_FAILED = 'failed'; + + /** + * How many failed file names are kept to be shown in the import finished + * notification. The full list is in the server log. + */ + private const MAX_REPORTED_FAILED_FILES = 10; + /** * @var string */ @@ -129,7 +140,10 @@ public function startImportOnedrive(string $userId): array { } $this->config->setUserValue($userId, Application::APP_ID, 'importing_onedrive', '1'); $this->config->setUserValue($userId, Application::APP_ID, 'imported_size', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_failed_files', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_skipped_files', '0'); $this->config->setUserValue($userId, Application::APP_ID, 'last_onedrive_import_timestamp', '0'); + $this->config->deleteUserValue($userId, Application::APP_ID, 'failed_files'); $this->config->deleteUserValue($userId, Application::APP_ID, 'import_tree'); $this->jobList->add(ImportOnedriveJob::class, ['user_id' => $userId]); @@ -195,14 +209,25 @@ public function importOnedriveJob(string $userId): void { ]; } if (isset($result['error']) || (isset($result['finished']) && $result['finished'])) { + // read the counters accumulated over all batches before resetting them + $nbImported = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_imported_files', '0'); + $nbFailed = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_failed_files', '0'); + $nbSkipped = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_skipped_files', '0'); + $failedFiles = json_decode($this->config->getUserValue($userId, Application::APP_ID, 'failed_files', '[]'), true) ?: []; $this->config->setUserValue($userId, Application::APP_ID, 'importing_onedrive', '0'); $this->config->setUserValue($userId, Application::APP_ID, 'imported_size', '0'); $this->config->setUserValue($userId, Application::APP_ID, 'nb_imported_files', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_failed_files', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_skipped_files', '0'); + $this->config->deleteUserValue($userId, Application::APP_ID, 'failed_files'); $this->config->setUserValue($userId, Application::APP_ID, 'last_onedrive_import_timestamp', '0'); if (isset($result['finished']) && $result['finished']) { $this->config->deleteUserValue($userId, Application::APP_ID, 'import_tree'); $this->onedriveApiService->sendNCNotification($userId, 'import_onedrive_finished', [ - 'nbImported' => $result['totalSeenNumber'], + 'nbImported' => $nbImported, + 'nbFailed' => $nbFailed, + 'nbSkipped' => $nbSkipped, + 'failedFiles' => $failedFiles, 'targetPath' => $targetPath, ]); } @@ -319,22 +344,34 @@ private function downloadDir( ]; } + $pageSkipped = 0; /** @var OneDriveItem $item */ foreach ($result['value'] as $item) { if (isset($item['file'])) { $newTotalSeenNumber++; - $size = $this->getFile($userId, $folder, $item); - if ($size !== null) { - $newDownloadedSize += $size; - if ($size > 0) { - $newNbDownloaded++; - $this->config->setUserValue($userId, Application::APP_ID, 'imported_size', (string)($alreadyImportedSize + $newDownloadedSize)); - $this->config->setUserValue($userId, Application::APP_ID, 'nb_imported_files', (string)($alreadyImportedNumber + $newNbDownloaded)); - $this->config->setUserValue($userId, Application::APP_ID, 'last_onedrive_import_timestamp', (string)(new \DateTime())->getTimestamp()); - } + $fileResult = $this->getFile($userId, $folder, $item); + if ($fileResult['status'] === self::FILE_DOWNLOADED) { + $newDownloadedSize += $fileResult['size']; + $newNbDownloaded++; + $this->config->setUserValue($userId, Application::APP_ID, 'imported_size', (string)($alreadyImportedSize + $newDownloadedSize)); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_imported_files', (string)($alreadyImportedNumber + $newNbDownloaded)); + $this->config->setUserValue($userId, Application::APP_ID, 'last_onedrive_import_timestamp', (string)(new \DateTime())->getTimestamp()); if ($maxDownloadSize !== null && $newDownloadedSize >= $maxDownloadSize) { throw new MaxDownloadSizeReachedException('Download size limit reached'); } + } elseif ($fileResult['status'] === self::FILE_ALREADY_THERE) { + $pageSkipped++; + } else { + // count files that could not be downloaded and remember the first + // few names, to report them in the notification sent when the + // import finishes + $nbFailed = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_failed_files', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_failed_files', (string)($nbFailed + 1)); + if ($nbFailed < self::MAX_REPORTED_FAILED_FILES) { + $failedFiles = json_decode($this->config->getUserValue($userId, Application::APP_ID, 'failed_files', '[]'), true) ?: []; + $failedFiles[] = $item['name']; + $this->config->setUserValue($userId, Application::APP_ID, 'failed_files', json_encode($failedFiles)); + } } } // folders: remember for recursion @@ -345,6 +382,11 @@ private function downloadDir( $importTree[$subPath] = 'todo'; } } + if ($pageSkipped > 0) { + // one write per listing page, skipped files are frequent on re-imports + $nbSkipped = (int)$this->config->getUserValue($userId, Application::APP_ID, 'nb_skipped_files', '0'); + $this->config->setUserValue($userId, Application::APP_ID, 'nb_skipped_files', (string)($nbSkipped + $pageSkipped)); + } // if this directory was marked unfinished, remove it now if (isset($importTree[$path])) { @@ -420,17 +462,18 @@ private function touchFolder(string $userId, Folder $folder, string $onedrivePat * @param string $userId * @param Folder $folder * @param array $fileItem - * @return ?float downloaded size, null if already existing or network error + * @return array{status: string, size: float} status is one of the FILE_* constants, + * size is only meaningful for FILE_DOWNLOADED */ - private function getFile(string $userId, Folder $folder, array $fileItem): ?float { + private function getFile(string $userId, Folder $folder, array $fileItem): array { $fileName = $fileItem['name']; try { $fileExists = $folder->nodeExists($fileName); } catch (ForbiddenException $e) { - return null; + return ['status' => self::FILE_FAILED, 'size' => 0.0]; } if ($fileExists) { - return 0; + return ['status' => self::FILE_ALREADY_THERE, 'size' => 0.0]; } $savedFile = $folder->newFile($fileName); @@ -451,7 +494,7 @@ private function getFile(string $userId, Folder $folder, array $fileItem): ?floa if ($savedFile->isDeletable()) { $savedFile->delete(); } - return null; + return ['status' => self::FILE_FAILED, 'size' => 0.0]; } if (isset($fileItem['lastModifiedDateTime'])) { @@ -462,7 +505,7 @@ private function getFile(string $userId, Folder $folder, array $fileItem): ?floa $savedFile->touch(); } $stat = $savedFile->stat(); - return (float)($stat['size'] ?? 0); + return ['status' => self::FILE_DOWNLOADED, 'size' => (float)($stat['size'] ?? 0)]; } /** diff --git a/tests/integration/graph-stub.php b/tests/integration/graph-stub.php new file mode 100644 index 0000000..8aa6453 --- /dev/null +++ b/tests/integration/graph-stub.php @@ -0,0 +1,160 @@ + ['name' => 'normal.txt', 'content' => 'hello import', 'download' => 'always'], + 'f2' => ['name' => 'empty.txt', 'content' => '', 'download' => 'always'], + 'f3' => ['name' => 'flaky.txt', 'content' => 'retry me!', 'download' => 'fresh-url-only'], + 'f4' => ['name' => 'broken.txt', 'content' => 'never arrives', 'download' => 'never'], + 'f5' => ['name' => 'already.txt', 'content' => 'already there', 'download' => 'never'], + 'f6' => ['name' => 'nested.txt', 'content' => 'in a folder', 'download' => 'always'], +]; + +/** + * @param string $id id of the file in FILES + * @param string $source 'listing' for the URL a listing hands out, 'fresh' for a refetched one + */ +function fileItem(string $id, string $source = 'listing'): array { + $file = FILES[$id]; + return [ + 'id' => $id, + 'name' => $file['name'], + 'size' => strlen($file['content']), + 'lastModifiedDateTime' => MODIFIED, + 'file' => ['mimeType' => 'text/plain'], + '@microsoft.graph.downloadUrl' => baseUrl() . '/download/' . $id . '?source=' . $source, + ]; +} + +function folderItem(string $name): array { + return [ + 'id' => 'folder-' . $name, + 'name' => $name, + 'lastModifiedDateTime' => MODIFIED, + 'folder' => ['childCount' => 1], + ]; +} + +function baseUrl(): string { + return 'http://' . ($_SERVER['HTTP_HOST'] ?? '127.0.0.1:8099'); +} + +function respond(array $body, int $status = 200): void { + http_response_code($status); + header('Content-Type: application/json'); + logRequest($status); + echo json_encode($body); +} + +/** The requests the app made, so that a failing test can be read from the server log. */ +function logRequest(int $status): void { + error_log(sprintf( + 'stub %s %s -> %d', + $_SERVER['REQUEST_METHOD'] ?? '?', + $_SERVER['REQUEST_URI'] ?? '?', + $status + )); +} + +function serveDownload(string $id): void { + if (!isset(FILES[$id])) { + respond(['error' => ['code' => 'itemNotFound']], 404); + return; + } + $file = FILES[$id]; + $source = $_GET['source'] ?? 'listing'; + $allowed = $file['download'] === 'always' + || ($file['download'] === 'fresh-url-only' && $source === 'fresh'); + if (!$allowed) { + // what an expired or revoked download URL looks like + respond(['error' => ['code' => 'accessDenied', 'message' => 'the download URL is no good']], 403); + return; + } + header('Content-Type: text/plain'); + header('Content-Length: ' . strlen($file['content'])); + logRequest(200); + echo $file['content']; +} + +$path = parse_url($_SERVER['REQUEST_URI'] ?? '/', PHP_URL_PATH) ?: '/'; +$path = rawurldecode($path); + +// downloads carry their own authorisation in the URL, everything else needs the access token +if (!str_starts_with($path, '/download/') && !preg_match('/^bearer .+/i', $_SERVER['HTTP_AUTHORIZATION'] ?? '')) { + respond(['error' => ['code' => 'unauthenticated', 'message' => 'no access token']], 401); + return; +} + +if (preg_match('#^/download/([^/?]+)$#', $path, $matches)) { + serveDownload($matches[1]); + return; +} + +if ($path === '/v1.0/me/drive') { + respond(['id' => 'stub-drive', 'quota' => ['total' => 1073741824, 'used' => 42, 'remaining' => 1073741782]]); + return; +} + +// the folder itself, asked for to copy its modification time +if ($path === '/v1.0/me/drive/root' || preg_match('#^/v1\.0/me/drive/root:(/[^:]*):$#', $path)) { + respond(['id' => 'root', 'name' => 'root', 'lastModifiedDateTime' => MODIFIED, 'folder' => ['childCount' => 6]]); + return; +} + +// the listing of a folder, the root one in two pages +if ($path === '/v1.0/me/drive/root/children') { + if (($_GET['$skiptoken'] ?? '') === 'page2') { + respond(['value' => [fileItem('f3'), fileItem('f4'), fileItem('f5'), folderItem('sub')]]); + return; + } + respond([ + 'value' => [fileItem('f1'), fileItem('f2')], + '@odata.nextLink' => baseUrl() . '/v1.0/me/drive/root/children?$skiptoken=page2', + ]); + return; +} + +if (preg_match('#^/v1\.0/me/drive/root:(/[^:]*):/children$#', $path, $matches)) { + respond(['value' => $matches[1] === '/sub' ? [fileItem('f6')] : []]); + return; +} + +// a single item, asked for when a download failed and a fresh URL is needed +if (preg_match('#^/v1\.0/me/drive/items/([^/]+)$#', $path, $matches)) { + if (!isset(FILES[$matches[1]])) { + respond(['error' => ['code' => 'itemNotFound']], 404); + return; + } + respond(fileItem($matches[1], 'fresh')); + return; +} + +respond(['error' => ['code' => 'unknownEndpoint', 'message' => $path]], 404); diff --git a/tests/unit/Notification/NotifierTest.php b/tests/unit/Notification/NotifierTest.php new file mode 100644 index 0000000..fe712d5 --- /dev/null +++ b/tests/unit/Notification/NotifierTest.php @@ -0,0 +1,144 @@ +createMock(IL10N::class); + $l->method('t')->willReturnCallback( + static fn (string $text, $parameters = []) => vsprintf($text, is_array($parameters) ? $parameters : [$parameters]) + ); + $l->method('n')->willReturnCallback( + static fn (string $singular, string $plural, int $count) => str_replace('%n', (string)$count, $count === 1 ? $singular : $plural) + ); + $factory = $this->createMock(IFactory::class); + $factory->method('get')->willReturn($l); + + $url = $this->createMock(IURLGenerator::class); + $url->method('imagePath')->willReturn('img/app-dark.svg'); + $url->method('getAbsoluteURL')->willReturn('http://nc.example.org/img/app-dark.svg'); + $url->method('linkToRouteAbsolute')->willReturn('http://nc.example.org/apps/files'); + + $this->notifier = new Notifier( + $factory, + $this->createMock(IUserManager::class), + $this->createMock(INotificationManager::class), + $url, + ); + } + + private function prepare(array $subjectParameters, string $expectedSubject, ?string $expectedMessage = null, string $subject = 'import_onedrive_finished', string $app = 'integration_onedrive'): void { + $notification = $this->createMock(INotification::class); + $notification->method('getApp')->willReturn($app); + $notification->method('getSubject')->willReturn($subject); + $notification->method('getSubjectParameters')->willReturn($subjectParameters); + $notification->expects($this->once()) + ->method('setParsedSubject') + ->with($expectedSubject) + ->willReturnSelf(); + if ($expectedMessage === null) { + $notification->expects($this->never())->method('setParsedMessage'); + } else { + $notification->expects($this->once()) + ->method('setParsedMessage') + ->with($expectedMessage) + ->willReturnSelf(); + } + $notification->method('setIcon')->willReturnSelf(); + $notification->method('setLink')->willReturnSelf(); + + $this->notifier->prepare($notification, 'en'); + } + + public function testFinishedWithoutFailures(): void { + $this->prepare( + ['nbImported' => 5, 'nbFailed' => 0, 'targetPath' => '/OneDrive import'], + '5 files were imported from OneDrive storage.' + ); + } + + public function testFinishedWithFailures(): void { + $this->prepare( + ['nbImported' => 5, 'nbFailed' => 2, 'failedFiles' => ['a.jpg', 'b.jpg'], 'targetPath' => '/OneDrive import'], + '5 files were imported from OneDrive storage.' + . ' 2 files could not be downloaded, check the server logs for details.', + 'Could not download: a.jpg, b.jpg' + ); + } + + public function testFinishedWithSingleImportAndSingleFailure(): void { + $this->prepare( + ['nbImported' => 1, 'nbFailed' => 1, 'failedFiles' => ['a.jpg'], 'targetPath' => '/OneDrive import'], + '1 file was imported from OneDrive storage.' + . ' 1 file could not be downloaded, check the server logs for details.', + 'Could not download: a.jpg' + ); + } + + public function testFinishedWithMoreFailuresThanReportedNames(): void { + $this->prepare( + ['nbImported' => 5, 'nbFailed' => 12, 'failedFiles' => ['a.jpg', 'b.jpg', 'c.jpg'], 'targetPath' => '/OneDrive import'], + '5 files were imported from OneDrive storage.' + . ' 12 files could not be downloaded, check the server logs for details.', + 'Could not download: a.jpg, b.jpg, c.jpg, and 9 more' + ); + } + + public function testFinishedWithSkippedFiles(): void { + $this->prepare( + ['nbImported' => 7, 'nbFailed' => 0, 'nbSkipped' => 1993, 'targetPath' => '/OneDrive import'], + '7 files were imported from OneDrive storage. 1993 files were already there.' + ); + } + + public function testRerunWithNothingNewExplainsItself(): void { + $this->prepare( + ['nbImported' => 0, 'nbFailed' => 0, 'nbSkipped' => 2000, 'targetPath' => '/OneDrive import'], + '0 files were imported from OneDrive storage. 2000 files were already there.' + ); + } + + public function testNotificationQueuedBeforeUpgradeHasNoFailedCount(): void { + $this->prepare( + ['nbImported' => 4, 'targetPath' => '/OneDrive import'], + '4 files were imported from OneDrive storage.' + ); + } + + public function testThrowsForOtherApp(): void { + $notification = $this->createMock(INotification::class); + $notification->method('getApp')->willReturn('some_other_app'); + + $this->expectException(UnknownNotificationException::class); + $this->notifier->prepare($notification, 'en'); + } + + public function testThrowsForUnknownSubject(): void { + $notification = $this->createMock(INotification::class); + $notification->method('getApp')->willReturn('integration_onedrive'); + $notification->method('getSubject')->willReturn('some_unknown_subject'); + + $this->expectException(UnknownNotificationException::class); + $this->notifier->prepare($notification, 'en'); + } +} diff --git a/tests/unit/Service/OnedriveAPIServiceTest.php b/tests/unit/Service/OnedriveAPIServiceTest.php index 83b73f0..7972ee5 100644 --- a/tests/unit/Service/OnedriveAPIServiceTest.php +++ b/tests/unit/Service/OnedriveAPIServiceTest.php @@ -111,4 +111,42 @@ public function testFileRequestKeepsUrlsOutOfErrorsAndLogs(): void { $this->assertStringNotContainsString('SECRETTOKEN', $result['error']); $this->assertStringContainsString('', $result['error']); } + + public function testRequestsGoToMicrosoftGraphByDefault(): void { + $this->assertSame( + 'https://graph.microsoft.com/v1.0/me/drive', + $this->urlOfRequest('me/drive') + ); + } + + public function testTheApiBaseUrlCanBeOverriddenForTests(): void { + $this->config->method('getAppValue')->willReturnCallback( + static fn (string $app, string $key, string $default = ''): string + => $key === 'api_base_url' ? 'http://127.0.0.1:8099/v1.0' : $default + ); + + $this->assertSame( + 'http://127.0.0.1:8099/v1.0/me/drive', + $this->urlOfRequest('me/drive') + ); + } + + /** + * The URL the service asks the HTTP client for when requesting an endpoint. + */ + private function urlOfRequest(string $endPoint): string { + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getBody')->willReturn('{}'); + $requested = ''; + $this->client->method('get')->willReturnCallback( + function (string $url) use (&$requested, $response): IResponse { + $requested = $url; + return $response; + } + ); + + $this->apiService->request('user1', $endPoint); + return $requested; + } } diff --git a/tests/unit/Service/OnedriveStorageAPIServiceTest.php b/tests/unit/Service/OnedriveStorageAPIServiceTest.php index 9145911..0cb5c85 100644 --- a/tests/unit/Service/OnedriveStorageAPIServiceTest.php +++ b/tests/unit/Service/OnedriveStorageAPIServiceTest.php @@ -14,6 +14,7 @@ use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\IUserFolder; use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -23,11 +24,17 @@ class OnedriveStorageAPIServiceTest extends TestCase { private OnedriveAPIService|MockObject $apiService; + private IRootFolder|MockObject $rootFolder; + private IConfig|MockObject $config; + private IJobList|MockObject $jobList; private Folder|MockObject $folder; private File|MockObject $file; private OnedriveStorageAPIService $service; + /** @var array */ + private array $configStore = []; + private const STALE_URL = 'https://stale.example.org/download?tempauth=old'; private const FRESH_URL = 'https://fresh.example.org/download?tempauth=new'; @@ -35,12 +42,15 @@ public function setUp(): void { parent::setUp(); $this->apiService = $this->createMock(OnedriveAPIService::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->config = $this->createMock(IConfig::class); + $this->jobList = $this->createMock(IJobList::class); $this->service = new OnedriveStorageAPIService( 'integration_onedrive', $this->createMock(LoggerInterface::class), - $this->createMock(IRootFolder::class), - $this->createMock(IConfig::class), - $this->createMock(IJobList::class), + $this->rootFolder, + $this->config, + $this->jobList, $this->createMock(UserScopeService::class), $this->apiService, ); @@ -52,7 +62,36 @@ public function setUp(): void { $this->folder->method('newFile')->willReturn($this->file); } - private function getFile(array $fileItem): ?float { + /** + * Back the IConfig mock with an array, so code doing read-increment-write + * on user settings behaves like it does against the real config. + */ + private function useStatefulConfig(array $initial): void { + $this->configStore = $initial; + $this->config->method('getUserValue')->willReturnCallback( + fn (string $userId, string $appName, string $key, $default = '') => $this->configStore[$key] ?? $default + ); + $this->config->method('setUserValue')->willReturnCallback( + function (string $userId, string $appName, string $key, $value): void { + $this->configStore[$key] = (string)$value; + } + ); + $this->config->method('deleteUserValue')->willReturnCallback( + function (string $userId, string $appName, string $key): void { + unset($this->configStore[$key]); + } + ); + } + + /** + * IRootFolder::getUserFolder() declares an IUserFolder return type since + * Nextcloud 36, older versions have no IUserFolder to mock. + */ + private function createUserFolderMock(): Folder|MockObject { + return $this->createMock(interface_exists(IUserFolder::class) ? IUserFolder::class : Folder::class); + } + + private function getFile(array $fileItem): array { $method = new ReflectionMethod(OnedriveStorageAPIService::class, 'getFile'); return $method->invoke($this->service, 'user1', $this->folder, $fileItem); } @@ -66,14 +105,14 @@ public function testDownloadSucceedsOnTheFirstAttempt(): void { ->willReturn(['success' => true]); $this->apiService->expects($this->never())->method('getDownloadUrl'); - $size = $this->getFile([ + $result = $this->getFile([ 'name' => 'photo.jpg', 'id' => 'item1', 'file' => [], '@microsoft.graph.downloadUrl' => self::STALE_URL, ]); - $this->assertSame(123.0, $size); + $this->assertSame(['status' => 'downloaded', 'size' => 123.0], $result); } public function testFailedDownloadIsRetriedWithAFreshUrl(): void { @@ -91,14 +130,14 @@ public function testFailedDownloadIsRetriedWithAFreshUrl(): void { ->with('user1', 'item1') ->willReturn(self::FRESH_URL); - $size = $this->getFile([ + $result = $this->getFile([ 'name' => 'photo.jpg', 'id' => 'item1', 'file' => [], '@microsoft.graph.downloadUrl' => self::STALE_URL, ]); - $this->assertSame(123.0, $size); + $this->assertSame(['status' => 'downloaded', 'size' => 123.0], $result); $this->assertSame([self::STALE_URL, self::FRESH_URL], $requestedUrls); } @@ -110,7 +149,7 @@ public function testFileIsDroppedWhenTheRetryFailsToo(): void { $this->file->method('isDeletable')->willReturn(true); $this->file->expects($this->once())->method('delete'); - $this->assertNull($this->getFile([ + $this->assertSame(['status' => 'failed', 'size' => 0.0], $this->getFile([ 'name' => 'photo.jpg', 'id' => 'item1', 'file' => [], @@ -124,7 +163,7 @@ public function testFileIsDroppedWhenNoFreshUrlCanBeFetched(): void { $this->file->method('isDeletable')->willReturn(true); $this->file->expects($this->once())->method('delete'); - $this->assertNull($this->getFile([ + $this->assertSame(['status' => 'failed', 'size' => 0.0], $this->getFile([ 'name' => 'photo.jpg', 'id' => 'item1', 'file' => [], @@ -143,13 +182,27 @@ public function testListingItemWithoutUrlIsDownloadedViaAFreshUrl(): void { ->with('user1', 'item1') ->willReturn(self::FRESH_URL); - $size = $this->getFile([ + $result = $this->getFile([ 'name' => 'note.one', 'id' => 'item1', 'file' => [], ]); - $this->assertSame(42.0, $size); + $this->assertSame(['status' => 'downloaded', 'size' => 42.0], $result); + } + + public function testDownloadedEmptyFileCountsAsDownloaded(): void { + $this->file->method('stat')->willReturn(['size' => 0]); + $this->apiService->method('fileRequest')->willReturn(['success' => true]); + + $result = $this->getFile([ + 'name' => 'empty.txt', + 'id' => 'item1', + 'file' => [], + '@microsoft.graph.downloadUrl' => self::STALE_URL, + ]); + + $this->assertSame(['status' => 'downloaded', 'size' => 0.0], $result); } public function testExistingFileIsNotDownloadedAgain(): void { @@ -159,8 +212,125 @@ public function testExistingFileIsNotDownloadedAgain(): void { $this->apiService->expects($this->never())->method('fileRequest'); $method = new ReflectionMethod(OnedriveStorageAPIService::class, 'getFile'); - $size = $method->invoke($this->service, 'user1', $folder, ['name' => 'photo.jpg']); + $result = $method->invoke($this->service, 'user1', $folder, ['name' => 'photo.jpg']); + + $this->assertSame(['status' => 'already there', 'size' => 0.0], $result); + } + + public function testImportCountsFailedSkippedAndEmptyDownloads(): void { + $this->useStatefulConfig([]); + + // remote drive root holds four files: a succeeds, b fails on the listing URL + // and on a fresh one, c is an empty file, d already exists locally + $this->apiService->method('request')->willReturnCallback( + static function (string $userId, string $endPoint) { + if ($endPoint === 'me/drive') { + return ['quota' => ['used' => 1000]]; + } + if ($endPoint === 'me/drive/root/children') { + return ['value' => array_map(static fn (string $name) => [ + 'name' => $name . '.jpg', + 'id' => 'id-' . $name, + 'file' => [], + '@microsoft.graph.downloadUrl' => 'https://listing.example.org/' . $name, + ], ['a', 'b', 'c', 'd'])]; + } + return []; + } + ); + $requestedUrls = []; + $this->apiService->method('fileRequest')->willReturnCallback( + static function (string $url) use (&$requestedUrls) { + $requestedUrls[] = $url; + return str_ends_with($url, '/b') ? ['error' => 'download error'] : ['success' => true]; + } + ); + $this->apiService->expects($this->once()) + ->method('getDownloadUrl') + ->with('user1', 'id-b') + ->willReturn('https://fresh.example.org/b'); + + $dirFolder = $this->createMock(Folder::class); + $dirFolder->method('nodeExists')->willReturnCallback( + static fn (string $name) => $name === 'd.jpg' + ); + $dirFolder->method('newFile')->willReturnCallback(function (string $name) { + $file = $this->createMock(File::class); + $file->method('fopen')->willReturnCallback(static fn () => fopen('php://temp', 'w+')); + $file->method('stat')->willReturn(['size' => $name === 'c.jpg' ? 0 : 10]); + $file->method('isDeletable')->willReturn(true); + return $file; + }); + $topFolder = $this->createMock(Folder::class); + $topFolder->method('nodeExists')->willReturn(true); + $topFolder->method('get')->willReturn($dirFolder); + $userFolder = $this->createUserFolderMock(); + $userFolder->method('nodeExists')->willReturn(true); + $userFolder->method('get')->willReturn($topFolder); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + + $result = $this->service->importFiles('user1', '/Import'); + + $this->assertTrue($result['finished']); + $this->assertSame('1', $this->configStore['nb_failed_files'] ?? null); + $this->assertSame('["b.jpg"]', $this->configStore['failed_files'] ?? null); + $this->assertSame('1', $this->configStore['nb_skipped_files'] ?? null); + // a and the empty file c are both imported + $this->assertSame('2', $this->configStore['nb_imported_files'] ?? null); + $this->assertSame('10', $this->configStore['imported_size'] ?? null); + $this->assertSame([ + 'https://listing.example.org/a', + 'https://listing.example.org/b', + 'https://fresh.example.org/b', + 'https://listing.example.org/c', + ], $requestedUrls); + } + + public function testFinishedNotificationReportsImportedAndFailedCounts(): void { + $this->useStatefulConfig([ + 'importing_onedrive' => '1', + 'onedrive_import_running' => '0', + 'nb_imported_files' => '7', + 'nb_failed_files' => '3', + 'nb_skipped_files' => '5', + 'failed_files' => '["x.jpg","y.jpg","z.jpg"]', + ]); + + // nothing left to download, the job finishes right away + $this->apiService->method('request')->willReturnCallback( + static function (string $userId, string $endPoint) { + if ($endPoint === 'me/drive') { + return ['quota' => ['used' => 1000]]; + } + if ($endPoint === 'me/drive/root/children') { + return ['value' => []]; + } + return []; + } + ); + $folder = $this->createUserFolderMock(); + $folder->method('isShared')->willReturn(false); + $folder->method('nodeExists')->willReturn(true); + $folder->method('get')->willReturnSelf(); + $this->rootFolder->method('getUserFolder')->willReturn($folder); + + $this->apiService->expects($this->once()) + ->method('sendNCNotification') + ->with('user1', 'import_onedrive_finished', [ + 'nbImported' => 7, + 'nbFailed' => 3, + 'nbSkipped' => 5, + 'failedFiles' => ['x.jpg', 'y.jpg', 'z.jpg'], + 'targetPath' => '/OneDrive import', + ]); + $this->jobList->expects($this->never())->method('add'); + + $this->service->importOnedriveJob('user1'); - $this->assertSame(0.0, $size); + $this->assertSame('0', $this->configStore['nb_imported_files']); + $this->assertSame('0', $this->configStore['nb_failed_files']); + $this->assertSame('0', $this->configStore['nb_skipped_files']); + $this->assertArrayNotHasKey('failed_files', $this->configStore); + $this->assertSame('0', $this->configStore['importing_onedrive']); } }