fix: expand oauth scopes for observer verification if needed - #52
fix: expand oauth scopes for observer verification if needed#52maxwellpeterson wants to merge 1 commit into
Conversation
Fixes situations like these: 1. A user receives a shared Gadget that has observed data from a Google service such as Sheets or Docs 2. The user reaches the observer verification page without an existing Google authentication grant 3. The authentication flow requests only the Gmail scope 4. Access verification attempts to check the Sheets, Docs, or other relevant Google resource using that insufficient grant 5. The user is rejected as having no access, even though their Google account does have access to the resource
|
/bonk review this |
| return account?.description.uniqueName || account?.description.displayName || `Account ${accountId}` | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
super nit: (i've gotten called out here) inconsistent comment style with the rest of this file
There was a problem hiding this comment.
Review: expand OAuth scopes for observer verification
I read both files, checked the shared API/gatekeeper types the change relies on, and ran the frontend build + tests (build clean, 118/118 tests pass). This is a focused, correct fix and it's consistent with how the rest of the frontend already handles resource grants.
What it does well
- Reuses the established mechanism.
requiredResourceUrlPatterns/missingResourceUrlPatternsmirror the same logic already inAccountChooser.tsx,GatekeeperModal.tsx, andgatekeepers.tsx, and they route through the existingconnectAccount(vendorId, resourceUrlPatterns?)/ensureAccountResources(accountId, patterns)RPCs rather than inventing a new path.resolveRequestedResourceis the shared source of truth, so the modal can't diverge from the backend's resolution. - Correctly consumes data the backend already sends.
user.tspopulatessupportedResourcesin thesubscriber.add(...)call; the modal previously discarded it as_supportedResources. Widening the vendor map fromVendorDescriptiontoGatekeeperVendorInfois the right call since resource resolution needssupportedResources. granted === undefinedtreated as fully granted matches the documentedAccountDescription.grantedResourceUrlPatternssemantics (legacy/full-scope accounts) and the other call sites.setGrantingcleared inadd()on a valid-credentials update mirrors the existingreconnectinghandling, so the "Waiting for access…" spinner resolves when the expanded grant arrives over the subscription.- Good test coverage for the three real branches: fresh connect requests the scope, an under-scoped existing account is blocked and offered a grant, and an already-granted account verifies immediately.
Observations (non-blocking)
- Fallback scoping in
requiredResourceUrlPatterns.resolveRequestedResourcefalls back to thehttps://*catch-all or the sole resource whenneed.resourceUrldoesn't match a specific pattern. If a vendor's catch-all (or sole resource) isgrantable, the modal will request/verify that scope even for an unmatchedresourceUrl. This matches howconnectAccountalready behaves, so it's not a regression — just worth being aware of that the "required" scope can be broader than the literal observed resource. - Grant vs. re-auth precedence when both apply. The re-authenticate button now only renders when
missing.length === 0, so an account that is both expired and under-scoped shows only "Grant the access needed…". That's fine in practice sinceensureAccountResourcesruns an OAuth flow that also refreshes credentials — the comment already explains the re-auth affordance's purpose, and it might be worth a one-line note that the grant path subsumes re-auth here, but not required.
Nits
handleGrantResourceAccessandaccountSatisfieswrap theirrequiredResourceUrlPatterns(...)call oddly across lines (need, vendorsById.get(...)on the next line). Cosmetic; the formatter presumably left it.
No changes requested. LGTM.
|
Review posted to PR #52. SummaryI reviewed PR #52 (expand OAuth scopes for observer verification). I read both changed files, verified the shared API/gatekeeper types the change relies on ( Verdict: LGTM, no changes requested. Key findings:
Non-blocking observations I raised: (1) |
Ensures that the OAuth scopes requested during observer verification cover the resource types that need to be verified.
Fixes situations like these: