Skip to content

fix: expand oauth scopes for observer verification if needed - #52

Open
maxwellpeterson wants to merge 1 commit into
mainfrom
mpeterson/fix-observer-google-scopes
Open

fix: expand oauth scopes for observer verification if needed#52
maxwellpeterson wants to merge 1 commit into
mainfrom
mpeterson/fix-observer-google-scopes

Conversation

@maxwellpeterson

@maxwellpeterson maxwellpeterson commented Aug 6, 2026

Copy link
Copy Markdown
Member

Ensures that the OAuth scopes requested during observer verification cover the resource types that need to be verified.

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. Observer 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 observed resource

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
@ndisidore

Copy link
Copy Markdown
Contributor

/bonk review this

@ndisidore
ndisidore self-requested a review August 6, 2026 23:20
return account?.description.uniqueName || account?.description.displayName || `Account ${accountId}`
}

/**

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.

super nit: (i've gotten called out here) inconsistent comment style with the rest of this file

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/missingResourceUrlPatterns mirror the same logic already in AccountChooser.tsx, GatekeeperModal.tsx, and gatekeepers.tsx, and they route through the existing connectAccount(vendorId, resourceUrlPatterns?) / ensureAccountResources(accountId, patterns) RPCs rather than inventing a new path. resolveRequestedResource is the shared source of truth, so the modal can't diverge from the backend's resolution.
  • Correctly consumes data the backend already sends. user.ts populates supportedResources in the subscriber.add(...) call; the modal previously discarded it as _supportedResources. Widening the vendor map from VendorDescription to GatekeeperVendorInfo is the right call since resource resolution needs supportedResources.
  • granted === undefined treated as fully granted matches the documented AccountDescription.grantedResourceUrlPatterns semantics (legacy/full-scope accounts) and the other call sites.
  • setGranting cleared in add() on a valid-credentials update mirrors the existing reconnecting handling, 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)

  1. Fallback scoping in requiredResourceUrlPatterns. resolveRequestedResource falls back to the https://* catch-all or the sole resource when need.resourceUrl doesn't match a specific pattern. If a vendor's catch-all (or sole resource) is grantable, the modal will request/verify that scope even for an unmatched resourceUrl. This matches how connectAccount already behaves, so it's not a regression — just worth being aware of that the "required" scope can be broader than the literal observed resource.
  2. 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 since ensureAccountResources runs 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

  • handleGrantResourceAccess and accountSatisfies wrap their requiredResourceUrlPatterns(...) call oddly across lines (need, vendorsById.get(...) on the next line). Cosmetic; the formatter presumably left it.

No changes requested. LGTM.

@ask-bonk

ask-bonk Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review posted to PR #52.

Summary

I reviewed PR #52 (expand OAuth scopes for observer verification). I read both changed files, verified the shared API/gatekeeper types the change relies on (GatekeeperVendorInfo, resolveRequestedResource, connectAccount, ensureAccountResources, grantedResourceUrlPatterns), confirmed the backend actually sends supportedResources over subscribeConnectedAccounts, and ran the frontend build + tests — build is clean and all 118 tests pass.

Verdict: LGTM, no changes requested.

Key findings:

  • The fix is focused and reuses existing mechanisms — requiredResourceUrlPatterns/missingResourceUrlPatterns mirror logic already present in AccountChooser.tsx, GatekeeperModal.tsx, and gatekeepers.tsx, routing through the existing RPCs rather than adding parallel ones.
  • It correctly consumes supportedResources, which the backend was already sending but the modal previously discarded (_supportedResources).
  • granted === undefined → fully granted semantics and the setGranting clearing all match established conventions.
  • Solid test coverage of the three real branches.

Non-blocking observations I raised: (1) resolveRequestedResource's catch-all/sole-resource fallback can make the "required" scope broader than the literal observed resource (consistent with existing connectAccount behavior, not a regression); (2) the grant button now takes precedence over re-auth when an account is both expired and under-scoped, which is fine since ensureAccountResources also refreshes credentials; plus one cosmetic line-wrap nit.

github run

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.

3 participants