feat(photos): Add Google Photos import via Picker API#356
feat(photos): Add Google Photos import via Picker API#356AhsanIsEpic wants to merge 22 commits intonextcloud:mainfrom
Conversation
AhsanIsEpic
changed the title
|
This relates to #207 |
|
Undrafted to get review from Copilot, apologies code owners |
There was a problem hiding this comment.
Pull request overview
Replaces the deprecated Google Photos Library API integration with the Google Photos Picker API flow, adding a user-driven photo selection popup and a background-job-based import into Nextcloud Files.
Changes:
- Added a Photos section in personal settings with Picker-session creation/polling and import progress UI.
- Introduced a new
GooglePhotosAPIServiceplusImportPhotosJobto orchestrate downloads from Picker sessions. - Added new routes/controllers/config plumbing and a completion notification.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/PersonalSettings.vue |
Adds Photos picker/import UI, polling loops, and OAuth popup state refresh. |
src/components/AdminSettings.vue |
Updates admin setup instructions for required Google APIs. |
lib/Service/GooglePhotosAPIService.php |
Implements Picker session management and background import/download orchestration. |
lib/BackgroundJob/ImportPhotosJob.php |
Queued job wrapper that delegates to the Photos import service. |
lib/Controller/GoogleAPIController.php |
Adds endpoints for picker sessions, import start, and import progress info. |
lib/Controller/ConfigController.php |
Adds Photos scope, GET /config endpoint, and cancel-import wiring. |
lib/Notification/Notifier.php |
Adds import_photos_finished notification linking to the import directory. |
lib/Settings/Personal.php |
Adds photo_output_dir initial state defaulting to /Google Photos. |
appinfo/routes.php |
Registers the new config/picker/import routes. |
appinfo/info.xml |
Bumps app version to 4.3.2. |
package-lock.json |
Updates engines metadata (Node/NPM) to match package.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- popupSuccess.js: wrap BroadcastChannel send in try/finally so window.close() always runs and feature-detect BroadcastChannel - GooglePhotosAPIService.php: reset photo_import_running and photo_import_job_last_start before early return when importing_photos=0 - GooglePhotosAPIService.php: use is_array() guard when appending to json-decoded picker_session_queue in startImportPhotos - GoogleAPIController.php: use is_array() guard before count() on json-decoded picker_session_queue - PersonalSettings.vue: store BroadcastChannel on component as oauthBroadcastChannel, close previous instance before creating a new one, and close in beforeUnmount to prevent channel leaks Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On the error path in importPhotosJob(), picker_session_queue and photo_next_page_token were not cleared. Any queued sessions would remain stuck in config and never be processed. Now both are reset on error so the UI is consistent and no phantom queued sessions are left behind. Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
|
I'm leaving this in draft for now because I'd like maintainer input on direction before spending more time polishing this PR. I've addressed Copilot review comments across multiple rounds, and I think this is a reasonable point to stop and ask for guidance. The goal of this PR is to replace the deprecated Google Photos Library API flow with the Picker API flow, since the old approach no longer lets you import Photos with the previous API. Questions for maintainers:
If yes, I’m happy to continue iterating on feedback and move it out of draft afterward. |
Hi @AhsanIsEpic, I would be interested in merging this, but as this is a very large pr it will take a few rounds of reviews from both ends. I'll scan some of the files and mention some of the things that stand out to me before this weekend. For the failing CI you can run |
lukasdotcom
left a comment
lukasdotcom
left a comment
There was a problem hiding this comment.
Some initial feedback before this weekend.
| requestUrl, | ||
| t('integration_google', 'Sign in with Google'), | ||
| 'toolbar=no, menubar=no, width=600, height=700', | ||
| 'toolbar=no, menubar=no, width=600, height=700,noopener,noreferrer', |
There was a problem hiding this comment.
What is the reason for the changes here?
There was a problem hiding this comment.
Added noopener,noreferrer to the window.open() features string as recommended by Copilot code review - it prevents the popup from accessing window.opener, guarding against reverse tabnapping. The practical security impact is low here since these open known Google URLs so it pretty much would have no effect.
| $res = $this->googleApiService->simpleDownload($userId, $downloadUrl, $resource); | ||
| if (!isset($res['error'])) { | ||
| if (is_resource($resource)) { | ||
| fclose($resource); | ||
| } | ||
| if (isset($item['createTime'])) { | ||
| try { | ||
| $d = new DateTime($item['createTime']); | ||
| $savedFile->touch($d->getTimestamp()); | ||
| } catch (Exception $e) { | ||
| $this->logger->warning('Google Photo, invalid createTime, using current time: ' . $e->getMessage(), ['app' => Application::APP_ID]); | ||
| $savedFile->touch(); | ||
| } | ||
| } else { | ||
| $savedFile->touch(); | ||
| } | ||
| // Store the Google media ID in file metadata so future imports can | ||
| // identify this file by ID rather than filename alone. | ||
| if ($itemId !== '') { | ||
| try { | ||
| $meta = $this->metadataManager->getMetadata($savedFile->getId(), true); | ||
| $meta->setString(self::METADATA_KEY, $itemId); | ||
| $this->metadataManager->saveMetadata($meta); | ||
| } catch (\Throwable $e) { | ||
| $this->logger->warning('Google Photo, could not save file metadata: ' . $e->getMessage(), ['app' => Application::APP_ID]); | ||
| } | ||
| } | ||
| $stat = $savedFile->stat(); | ||
| return (int)($stat['size'] ?? 0); | ||
| } else { | ||
| $this->logger->warning('Google API error downloading photo: ' . $res['error'], ['app' => Application::APP_ID]); | ||
| if (is_resource($resource)) { | ||
| fclose($resource); | ||
| } | ||
| if ($savedFile->isDeletable()) { | ||
| $savedFile->unlock(ILockingProvider::LOCK_EXCLUSIVE); |
There was a problem hiding this comment.
This section seems very similar to downloadAndSaveFile in GoogleDriveApiService.php. It might be better to move that function into GoogleAPIService.php to reuse the code in both.
There was a problem hiding this comment.
moved the shared download logic into GoogleAPIService::downloadAndSaveFile(). Both GoogleDriveAPIService and GooglePhotosAPIService now call the shared method, and the private downloadAndSaveFile has been removed from GoogleDriveAPIService.
- Update copyright headers in new PHP files to 'Nextcloud GmbH and Nextcloud contributors 2025' with @author Ahsan Ahmed - Revert package-lock.json engine version bumps (these are auto-managed in separate PRs) - Refactor file download logic into GoogleAPIService::downloadAndSaveFile() so it can be reused by both GooglePhotosAPIService and GoogleDriveAPIService; remove the now-redundant private downloadAndSaveFile from GoogleDriveAPIService - Add ImportPhotosJob MissingOverrideAttribute to psalm-baseline.xml - Fix InvalidReturnType/InvalidReturnStatement in GooglePhotosAPIService via updated return-type docblocks and inline psalm-suppress annotations Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clear driveImportLoop in beforeUnmount() to prevent Drive polling timer leak when component is destroyed (nextcloud#92) - Move pickerPollTimer clearance into onImportPhotos() success handler; restart polling on import request failure so user is not stuck with an active session and no way to trigger import (nextcloud#93) - Delete newly created file in downloadAndSaveFile() when fopen throws LockedException or returns false, preventing zero-byte placeholder files from being left behind on early failure paths (nextcloud#94) Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mirror the same defensive pattern used in startImportPhotos(): if the stored picker_session_queue value cannot be decoded to an array (e.g. config corruption), fall back to an empty array rather than passing a non-array value to array_shift(). Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Cancel active Drive and Photos imports on user disconnect so background jobs don't re-queue against deleted tokens (nextcloud#106) - Add startingPhotoImport guard in pollPickerSession() to prevent concurrent /import-photos POST requests across poll ticks (nextcloud#107) Signed-off-by: Ahsan Ahmed <61637519+AhsanIsEpic@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pickerWindow.focus() | ||
| } | ||
| // Start polling for selection completion | ||
| const pollInterval = parseFloat(response.data.pollingConfig?.pollInterval ?? '5s') * 1000 |
There was a problem hiding this comment.
pollInterval is derived via parseFloat(...) * 1000. If pollingConfig.pollInterval is missing or has an unexpected format, this becomes NaN, and Math.max(NaN, 4000) also returns NaN (which browsers coerce to 0ms), potentially causing a very tight polling loop against /picker-session. Consider validating the parsed value (e.g., Number.isFinite) and falling back to a safe default (like 5000ms) before calling setInterval.
| const pollInterval = parseFloat(response.data.pollingConfig?.pollInterval ?? '5s') * 1000 | |
| const defaultPollInterval = 5000 | |
| const parsedPollInterval = parseFloat(response.data.pollingConfig?.pollInterval ?? '5s') * 1000 | |
| const pollInterval = Number.isFinite(parsedPollInterval) ? parsedPollInterval : defaultPollInterval |
| * Cancel an in-progress picker session or background import. | ||
| */ | ||
| onCancelPickerSession() { | ||
| clearInterval(this.pickerPollTimer) |
There was a problem hiding this comment.
onCancelPickerSession() clears the interval but does not set this.pickerPollTimer = null. Other code paths (e.g., the import failure handler) use !this.pickerPollTimer to decide whether polling should be restarted. Nulling the reference after clearInterval keeps this state consistent and avoids edge cases where polling isn’t restarted because the cleared interval ID is still truthy.
| clearInterval(this.pickerPollTimer) | |
| clearInterval(this.pickerPollTimer) | |
| this.pickerPollTimer = null |
| if (isset($values['user_name']) && $values['user_name'] === '') { | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'user_id'); | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'user_name'); | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'refresh_token'); | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'token_expires_at'); | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'token'); | ||
| $this->userConfig->deleteUserConfig($this->userId, Application::APP_ID, 'user_scopes'); | ||
| // Cancel any in-progress imports so background jobs don't keep re-queuing | ||
| // and failing against the now-deleted tokens | ||
| $this->googleDriveApiService->cancelImport($this->userId); | ||
| $this->googlePhotosApiService->cancelImport($this->userId); | ||
| $result['user_name'] = ''; |
There was a problem hiding this comment.
In the disconnect path (user_name === ''), tokens are deleted and cancelImport() is called, but neither Drive nor Photos cancelImport() resets the importing_* flags (Drive’s cancelImport() only removes the job; Photos’ cancelImport() clears the queue but also doesn’t flip importing_photos). If a background job is currently running while the user disconnects, it can re-queue itself based on the still-set importing_* flags and then fail repeatedly due to the deleted tokens. Consider explicitly setting importing_drive / importing_photos (and their *_import_running guards) to 0 in this disconnect branch before/alongside cancelling jobs.
Closes #357
Summary
The Google Photos Library API is deprecated and returns a
403 Forbiddenerror for new OAuth clients. This PR replaces it entirely with the Google Photos Picker API, which requires users to explicitly select photos in a Google-hosted popup before Nextcloud can access any data.This replaces
photoslibrary.readonly. Existing connected users must disconnect and re-authenticate to grant the new scope. Thecan_access_photosflag is written to user config during the OAuth redirect and controls visibility of the Photos section in personal settings.Admin setup: In the Google Cloud Console, enable the "Google Photos Picker API" (not the deprecated Photos Library API).
Changed Files
appinfo/routes.phpSix new routes:
GET/configconfig#getConfigPOST/picker-sessiongoogleAPI#createPickerSessionGET/picker-sessiongoogleAPI#getPickerSessionDELETE/picker-sessiongoogleAPI#deletePickerSessionPOST/import-photosgoogleAPI#importPhotosGET/import-photos-infogoogleAPI#getImportPhotosInformationlib/BackgroundJob/ImportPhotosJob.php(new)A
QueuedJobthat delegates toGooglePhotosAPIService::importPhotosJob(). Re-queued by the service until all picked items are downloaded.lib/Controller/ConfigController.phpPHOTOS_SCOPEconstantINT_CONFIGSextended withnb_imported_photos,last_import_timestamp,photo_import_job_last_startgetConfig()endpoint (GET /config): returnsuser_nameanduser_scopes— used by the OAuth popup redirect to refresh the parent settings page state without a full reloadoauthRedirect(): writescan_access_photosto the storeduser_scopesarraysetConfig(): disconnect path now deletesuser_scopes; cancel-import path routes throughGooglePhotosAPIService::cancelImport()lib/Controller/GoogleAPIController.phpFive new controller actions:
createPickerSession()—POST /picker-session: creates a Picker session, returnsid,pickerUri(with/autocloseappended), andpollingConfiggetPickerSession()—GET /picker-session?sessionId=: polls session state, returnsmediaItemsSetdeletePickerSession()—DELETE /picker-session?sessionId=: deletes session; only clears the storedpicker_session_idconfig key when it matches the deleted session, so cancelling a UI session cannot corrupt an active importimportPhotos()—POST /import-photos: callsstartImportPhotos(), returnstargetPath(andqueued: trueif an import is already running)getImportPhotosInformation()—GET /import-photos-info: returnsimporting_photos,nb_imported_photos,last_import_timestamp,nb_queued_sessionslib/Notification/Notifier.phpNew
import_photos_finishednotification: correctly pluralised via$l->n(), links to the import folder in Files, uses the app's dark icon.lib/Service/GooglePhotosAPIService.php(new)Core Picker API client and import orchestrator:
Session management
createPickerSession()—POST /v1/sessions; does not writepicker_session_id(onlystartImportPhotos()does, to avoid overwriting an active import session when the user queues a second picker)getPickerSession()—GET /v1/sessions/{id}deletePickerSession()—DELETE /v1/sessions/{id}; only clears storedpicker_session_idwhen it matchesImport orchestration
startImportPhotos(): validates$sessionId, creates the output folder, stores session ID, resets counters, enqueuesImportPhotosJob. If an import is already running, appends topicker_session_queueand returnsqueued: trueinsteadimportPhotosJob(): runs under user + filesystem scope; concurrent-run guard viaphoto_import_running+photo_import_job_last_start; delegates toimportFromPickerSession(); on finish sends notification + deletes session; on partial run re-queues itself. On successful completion with a non-empty queue, transitions atomically to the next queued session (never writesimporting_photos=0transiently)importFromPickerSession(): paginatesGET /v1/mediaItems(100/page); per-item dedup vianodeExists+IFilesMetadataManagermetadata (stores Google item ID asintegration_google_photo_id); honours a 500 MB per-run cap and persistsphoto_next_page_tokenso subsequent job runs resume where they left offcancelImport(): removes the job, deletes the active picker session, clears the queueDownload (
downloadPickerItem()→GoogleAPIService::downloadAndSaveFile())=dfor images,=dvfor video); mtime set fromcreateTimeGoogleAPIService::downloadAndSaveFile()lib/Settings/Personal.phpReads
photo_output_dirfrom user config (defaulting to/Google Photos) and injects it into theuser-configinitial state.src/components/AdminSettings.vueUpdates setup instructions to reference the Google Photos Picker API in the Google Cloud Console.
src/components/PersonalSettings.vueNew Photos section:
lib/Service/GoogleAPIService.phpNew
downloadAndSaveFile()public method: creates a file in a folder, streams a download viasimpleDownload(), sets mtime, and cleans up (deletes the empty file) on all failure paths includingLockedExceptionandfopen()returningfalse. Shared by bothGoogleDriveAPIServiceandGooglePhotosAPIService.Picker flow
POST /picker-sessionand opens it in a popup (noopener,noreferrer)GET /picker-sessionevery ~4 s; whenmediaItemsSetbecomestrue, the import is triggered automaticallyImport progress UI
NcLoadingIconspinnernb_imported_photos === 0and cron hasn't run yetnb_queued_sessions > 0Other
/Google Photos)BroadcastChannel('integration_google_oauth')exclusively for the success handshake;popupSuccess.jsposts to the channel and closes. All popup windows usenoopener,noreferrersrc/popupSuccess.jsUses
BroadcastChanneldirectly (no opener fallback): creates the channel, posts{ username }, closes the channel, then closes the window unconditionally.Even though this implementation currently works. It was developed with AI assistance (GitHub Copilot / Claude Sonnet) and should be treated as draft quality. It works end-to-end in a local Docker environment but has not been through a thorough human review or extensive testing.
Feedback welcome on anything including: