Repository navigation
A person can connect Google Drive, and only they choose which of their folders a base reads - #1386
Conversation
9ac90e4 to
ca68e2e
Compare
|
One small note: CI checks that the web's list of source kinds matches the backend, so this PR also carries the Google Drive kind's name, hint and icon. The connect panel itself is in the follow-up. |
Google Drive source: bring upstream deeplethe#1386 and deeplethe#1388 into re-dev
ca68e2e to
a739344
Compare
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks @gedaiu. This addresses every point from the 10-14 review, and the database tests are just what I asked for. A few things before merge:
- A sync uses one access token for the whole run. Past an hour, every download fails with 401 and is reported as "could not be read". Please get a new token once on a 401.
- Revoking the refresh token on disconnect probably cancels the person's whole grant to our client, which would stop their other Drive sources too. Please confirm, and either skip the revoke when the same account still backs another source, or record this in the ADR. Also check the revoke response status, not just network errors.
- When another editor reconnects, please revoke the previous person's grant, or note in the ADR why not.
- Make
createrefusegoogle_drivewhen the OAuth client isn't configured, as decision 2 says. - Please rebase:
CURRENT_SCHEMA_VERSIONmust be 106 now. ADR 0074 and migration 0120 are still free.
#1386 and #1388 should merge together, since #1386 makes the kind creatable before the panel exists. After that I'll do the security pass.
ebf51ba to
6c28f98
Compare
|
Thank you @WaylandYang, these were great catches, especially the one-hour token, which would have bitten the first big Drive. All five are in now:
Each has a test that fails without it. Merging both together sounds good. Thanks again for the careful read! |
6c28f98 to
2301694
Compare
|
One small addition since the review: a sync now follows shortcuts. Meeting notes and transcripts from calls someone else organised reach the person only as shortcuts, so a real Meet folder came in empty. A shortcut to a folder is walked, a shortcut to a file brings in its target under the target's id, and one the account can't open is named in the run's result. Two new tests cover it. Thanks! |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks @gedaiu, all five points are in and the tests read well. Two small things from the security pass before we merge:
- Ids that Drive returns aren't validated before they reach a query or URL: the child folder ids in
walk(google_drive.rs:608-617, used inqat :581) and the file ids used in the export/download paths (:707, :721). Please skip any item whose id failsvalid_id, as you already do for shortcut targets. - If the connector reconnects with a different Google account, the callback keeps the old
foldersand leaves the old account's grant unrevoked. When the account changes, please clearfoldersand revoke the old token unlessgrant_still_used. Never revoke when it's the same account, since that would kill the new token.
Optional: check the ID token's exp; match account_email case-insensitively (or store sub); and say in decision 11 that deleting a source doesn't revoke.
2301694 to
9add132
Compare
|
Thank you @WaylandYang, both are in, plus the optional ones:
The ID token's |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks @gedaiu, both points and the optional ones are in, and saving the new grant before revoking the old one is exactly right. One line before merge: when the old email is known but the new grant has none, finish still revokes the old token (google_drive_routes.rs:336), though decision 5 says nothing is revoked when either email is unknown. Please also require grant.account_email.is_some() there. With that, approved, and we'll merge #1386 and #1388 together. Heads-up: #1409 also bumps CURRENT_SCHEMA_VERSION to 106, so whichever lands second moves it to 107.
9add132 to
d530a4d
Compare
…r folders a base reads Co-authored-by: muemich <michaelmuellerrw@gmail.com> Signed-off-by: Bogdan Szabo <bogdan@re-cinq.com>
d530a4d to
c247de1
Compare
|
Thank you @WaylandYang, good catch! The revoke now needs both emails known and different ( |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks, the revoke now needs both emails known and different, exactly as decision 5 says, and the test pins all four cases. Approving.
Continues #1222, which I've taken over from my teammate @muemich. Thanks @WaylandYang for the careful review! As you suggested, it's split: this PR is the backend, and the web panel follows once it merges. Part of #1223.
Only the person who connected can now browse and choose folders. The general source settings no longer write a Drive source's config, so they can't get around that. A sync stops with a "reconnect" error once that person is no longer an editor. The cap is on downloads (2,000 per run) instead of the listing, and a run that hits it says how many files are waiting. Pending connections are capped at 5 per person. Connect, select and disconnect each change only their own keys, in one statement. The ADR is now 0074 and the migration 0120.
New database-backed tests cover a state used twice, a callback from another session, an editor removed before the callback, an expired flow, the per-person cap, a colleague trying to browse or choose, the settings route, and a sync after the person who connected stops being an editor. Sealing of patched keys is checked in the store's sealed-at-rest test.