Skip to content

A person can connect Google Drive, and only they choose which of their folders a base reads - #1386

Merged
WaylandYang merged 1 commit into
deeplethe:devfrom
re-cinq:feat/google-drive-source-backend
Oct 25, 2026
Merged

WaylandYang merged 1 commit into
deeplethe:devfrom
re-cinq:feat/google-drive-source-backend

Conversation

@gedaiu

@gedaiu gedaiu commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

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.

@gedaiu

gedaiu commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

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.

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. When another editor reconnects, please revoke the previous person's grant, or note in the ADR why not.
  4. Make create refuse google_drive when the OAuth client isn't configured, as decision 2 says.
  5. Please rebase: CURRENT_SCHEMA_VERSION must 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.

@gedaiu

gedaiu commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

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:

  1. A 401 now gets a fresh access token and the request is tried once more.
  2. You were right that revoking ends the whole grant. Disconnect now revokes only when no other Drive source uses the same account, and it checks the response status too. When the email is unverified we can't tell, so we leave the grant alone. Decision 11 explains it.
  3. Only the person who connected can reconnect now. Anyone else disconnects first, which revokes as in 2, and a callback that finds someone else connected in the meantime is refused. Decision 5 says why.
  4. Creating a google_drive source is refused with google_drive_not_configured when the client isn't set.
  5. Rebased, and CURRENT_SCHEMA_VERSION is 106.

Each has a test that fails without it. Merging both together sounds good. Thanks again for the careful read!

@gedaiu

gedaiu commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

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 WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gedaiu, all five points are in and the tests read well. Two small things from the security pass before we merge:

  1. 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 in q at :581) and the file ids used in the export/download paths (:707, :721). Please skip any item whose id fails valid_id, as you already do for shortcut targets.
  2. If the connector reconnects with a different Google account, the callback keeps the old folders and leaves the old account's grant unrevoked. When the account changes, please clear folders and revoke the old token unless grant_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.

@gedaiu

gedaiu commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @WaylandYang, both are in, plus the optional ones:

  1. Every id Drive returns (child folders, files, shortcut targets) now has to pass valid_id, or the item is skipped, and content() refuses a bad id too.
  2. When the person who connected comes back with a different Google account, the old folders are dropped and the old grant is revoked unless another source still uses it. It's never revoked for the same account. If either email is unknown we treat it as a change, so the folders go but nothing is revoked.

The ID token's exp is checked now, accounts match ignoring case, and decision 11 says deleting a source doesn't revoke. Thanks again for such a careful pass!

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…r folders a base reads

Co-authored-by: muemich <michaelmuellerrw@gmail.com>
Signed-off-by: Bogdan Szabo <bogdan@re-cinq.com>
@gedaiu
gedaiu force-pushed the feat/google-drive-source-backend branch from d530a4d to c247de1 Compare October 24, 2026 20:30
@gedaiu

gedaiu commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @WaylandYang, good catch! The revoke now needs both emails known and different (revokes_previous), so a grant with no email never revokes the old one, as decision 5 says. There's a small test for it. I also rebased both PRs onto the latest dev and restacked #1388 on top, sorry if that dismissed your approval once more. And thanks for the heads-up about #1409, happy to move CURRENT_SCHEMA_VERSION to 107 if it lands first.

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the revoke now needs both emails known and different, exactly as decision 5 says, and the test pins all four cases. Approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants