fix(dataverse): request the per-environment OAuth scope - #6709
fix(dataverse): request the per-environment OAuth scope#6709waleedlatif1 wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview OAuth service config now supports optional Fail-closed where a tenant URL cannot be supplied: desktop connect/authorize, copilot Reconnect on the credential detail page reads the environment back from that credential's granted scopes (not the viewer's OAuth connections) and passes it into the connect mutation. Reviewed by Cursor Bugbot for commit 9e86421. Configure here. |
Greptile SummaryThe PR makes Dataverse OAuth request the environment-specific resource scope and consistently validates that environment for OAuth and API use.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/oauth/resource-url.ts | Introduces shared validation and recovery for per-environment OAuth resource origins, including the prior port-qualified audience fix. |
| apps/sim/hooks/queries/oauth/oauth-connections.ts | Builds Dataverse link scopes from the validated environment and refuses unsupported desktop initiation. |
| apps/sim/app/workspace/[workspaceId]/integrations/connected/[credentialId]/connected-credential-detail.tsx | Recovers the environment from the target credential’s granted scopes so reconnect works for owned and shared credentials. |
| apps/sim/lib/oauth/token-resolution.ts | Adds the validated resource origin to OAuth token payloads and fails closed when the required resource scope is absent. |
| apps/sim/tools/microsoft_dataverse/utils.ts | Reuses the OAuth resource policy for every Dataverse API base URL, preventing audience and request-host policy drift. |
| apps/sim/app/api/auth/oauth2/authorize/route.ts | Refuses resource-scoped providers on an initiation route that cannot carry the required environment URL. |
Sequence Diagram
sequenceDiagram
participant User
participant UI as Connect UI
participant Auth as Better Auth
participant Entra as Microsoft Entra
participant Token as Token Resolution
participant Dataverse
User->>UI: Enter Dataverse environment URL
UI->>UI: Validate HTTPS host and normalize origin
UI->>Auth: Link with static scopes plus origin/user_impersonation
Auth->>Entra: Request environment-specific consent
Entra-->>Auth: Account and granted scopes
Token->>Token: Recover resource origin from granted scope
Token->>Dataverse: Send token only to validated environment origin
Reviews (5): Last reviewed commit: "fix(oauth): read reconnect scopes from t..." | Re-trigger Greptile
0bff854 to
91e5914
Compare
|
@cursor review |
Dataverse's API resource is the customer's own environment host. The provider declared a static `https://dynamics.microsoft.com/user_impersonation`, which is not an Entra Application ID URI — the Dataverse first-party app (00000007-0000-0000-c000-000000000000) publishes `admin.services.crm.dynamics.com`, its regional siblings, and `*.crm.dynamics.com` wildcards, and nothing matching what we sent. Entra rejects it at /authorize, so this integration has never been able to complete consent. There is no tenant-agnostic alternative: the data-API resource *is* the org URL, which is why the scope has to be built per connection rather than declared. A service can now declare `resourceUrl`, and the connect modal collects that host, validates it against the service's allowed domains, and sends `origin + scopeSuffix` in the link request — Better Auth's link route replaces the registered scopes with the body's, which is what makes this possible without a second provider registration. Only Microsoft Dataverse declares it; the other 60 services take an unchanged path. The three initiation surfaces that cannot collect a tenant host fail closed rather than minting a token with no API audience: the desktop hand-off (its scope is id-shaped, validated against ID_PATTERN in a separately released Electron binary, so an installed shell would drop a new field), the Copilot auth-link tool (its schema is generated from the Mothership catalog), and the authorize route the desktop path redirects through. `getDataverseBaseUrl` now shares one host list with the scope builder. Those had drifted into separate copies, and the failure mode is quiet: a new Microsoft sovereign cloud added on one side only produces a token whose audience the tools then refuse to send to. Not fixed here, and the integration stays non-functional until it is: the shared Entra app registration needs the Dynamics CRM delegated permission before any of these scopes can be consented to.
…ut a host - resolve: an explicit port survived into the origin, which becomes both the OAuth audience and the API base. Providers publish their resource on the default port, so a port-qualified origin names a resource nobody serves. `URL` drops the port only when it is the scheme default, so anything left is explicit and now rejected. - reconnect: the credential-detail Reconnect action calls the connect mutation directly with no environment URL, so a resource-scoped service failed with a validator error about a field that surface never shows. It now refuses up front with the route that works. The check runs before the draft is written, so a refusal leaves nothing behind. `WorkspaceCredential` carries no granted scopes, so the origin cannot be read back off the credential here — that is the follow-up already noted on the PR.
`check:tool-registry-boundary` compares each route's module graph against a recorded count and fails past max(25, 2%) growth. The knowledge page had drifted to 2133 against a baseline of 2091 — exactly the +42 ceiling — so the next change to reach that graph pays for the accumulated drift. This branch adds 2 modules to it: the connect modal is used by the knowledge connectors section, and it now imports the resource-URL validator and the shared Dataverse config. That tips 2133 to 2135 and trips the gate. Re-recorded that one route rather than all 34, so the diff shows the number that actually moved. 42 of the 44 predate this branch.
91e5914 to
9f86672
Compare
|
Heads-up on the third commit, since the number in it is misleading at a glance. CI's
So 42 of the 44 predate this PR; my two are I re-recorded only that one route (a one-line change) rather than running Worth flagging separately: because staging is at its exact ceiling, the next PR that adds even one module to the knowledge page graph will fail the same way. Someone may want to look at what pushed it from 2091 to 2133. Three other audits — |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9f86672. Configure here.
The OAuth scope for a resource-scoped service names the tenant origin by construction, so the granted scope already records which environment a credential belongs to. Nothing read it back, which left two gaps. - Token resolution now projects that origin as `resourceUrl`, the way Salesforce's `instanceUrl` and Zoho's `apiDomain` already are. Unlike those two it is not smuggled into the scope column; it is recovered from the scope itself. - A resource-scoped credential whose granted scopes name no valid host holds a token no API will accept. Resolution now refuses it with that explanation instead of handing back a token that fails later as an opaque 401 — which reads like an expired credential rather than one never bound to an environment. Reconnect cannot repair it, since the origin it would recover is the missing part, so the message points at disconnect-and-connect. - Reconnect recovers the origin instead of refusing outright. It reads the scopes granted to *that account*, now projected per account on the connections response, rather than the connection's union across accounts — two accounts on one provider are two different environments, and the union cannot tell them apart. Picking the wrong one would rebind the credential to the wrong environment, which is the failure this is meant to prevent. The per-account scopes were already computed in the connections route and merged away, so this projects a value that was in hand rather than widening a query. Deriving it on the workspace-credential list would instead have meant a new join to `account` on a hot path shared by the workflow credential selector, the integrations page, and the v2 API.
|
Follow-up 1 is now in this PR (77eab6f) — with one design change worth flagging, and one deliberate omission. What landed. The OAuth scope names the tenant origin by construction, so the granted scope already records which environment a credential belongs to; nothing read it back. Token resolution now projects it as Where I diverged from the plan. I said I'd add scopes to the workspace-credential contract. Tracing it, That distinction turned out to matter for correctness, not just cost: reconnect reads the scopes granted to that account, not the connection's union. Two accounts on one provider are two different environments, and the union cannot tell them apart — recovering the wrong one would rebind a credential to the wrong environment, which is exactly the failure this work exists to prevent. The scope-satisfaction check moved. I had planned it in Not done, deliberately: making the block's 4 new tests on the recovery path, verified to fail when the host check is removed from it. 369 tests pass; type-check, biome, strict API audit, and the module-graph boundary all clean. |
|
@cursor review |
…'s connections The reconnect handler recovered a resource-scoped credential's environment from useOAuthConnections(), which lists the *viewer's* own accounts and falls back to a bare catalog on a failed fetch. An admin reconnecting a teammate's shared credential found no matching account, and a still-loading or failed query was indistinguishable from no scopes granted — both surfaced as "not bound to an Environment URL" on a perfectly valid credential. Read the scopes from the credential itself instead. The credential detail route already left-joins the backing account and authorizes by credential access rather than ownership, so it answers for a shared credential and carries its own load and error states: Reconnect stays disabled while they load, a failed read says so, and only an authoritative empty result gives the unbound refusal. The per-account scopes projection added for the previous approach is reverted with it.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9e86421. Configure here.
The problem
Microsoft Dataverse OAuth has never been able to complete consent.
The provider declares a static scope
https://dynamics.microsoft.com/user_impersonation. That is not an Entra Application ID URI. The Dataverse first-party app00000007-0000-0000-c000-000000000000publisheshttps://admin.services.crm.dynamics.com/, its regional siblings, and00000007-.../*.crm.dynamics.comwildcards as its Service Principal Names — nothing matching what we send. Microsoft's docs require<environment-url>/user_impersonation(public client) or<environment-url>/.default(confidential). Entra rejects an unprovisionable resource at/authorizewithAADSTS500011, before a code is issued.That same SPN list rules out a one-line fix: the data-API resource is the org URL, so no tenant-agnostic scope exists and the scope must be built per connection.
Evidence it was never exercised: added in a bulk tool-authoring PR (#3257) with zero tests; the later "align with live API docs" PR (#5481) touched no OAuth file.
The change
OAuthServiceConfiggains an optionalresourceUrl— data only, following the existingadditionalProviderIds/providerIdLabelsprecedent for per-service connect-modal declarations. When present, the connect modal renders a field for the tenant host, validates it against the service's allowed registrable domains, and the connect mutation sends[...staticScopes, origin + scopeSuffix]in the link request.That last part works because Better Auth's link route replaces the registered scopes with the request body's (
generic-oauth/routes.mjs:381), unlike the sign-in route which merges. No second provider registration is needed.Only
microsoft-dataversedeclares it. The other 60 services take a byte-identical path.Surfaces that fail closed
Three initiation paths cannot collect a tenant host, so they refuse rather than mint a token with no API audience:
DesktopOAuthConnectScopeis id-shaped and validated againstID_PATTERNin the Electron main process; the desktop app ships as a separately released binary, so installed shells would drop a new field even after the contract gained oneoauth_get_auth_linkschema is generated from the Mothership catalog, not hand-editable hereThe desktop landing page (
/desktop/connect) is guarded too — its no-workspaceIdbranch rendersConnectLauncher, a bareclient.oauth2.linkwith no scopes, which would otherwise mint an audience-less token.Shared host policy
getDataverseBaseUrl(which pins every Web API request) and the scope builder (which sets the token's audience) now read oneDATAVERSE_RESOURCE_URL. They had drifted into separate copies of the same five domains, and the failure mode is quiet: a new Microsoft sovereign cloud added on one side only yields a token whose audience the tools then refuse to send to.What this does NOT fix
The integration stays non-functional until the shared Entra app registration gains the Dynamics CRM delegated permission. That is an Azure-portal change, not a code change. This PR makes the request correct; it cannot make it consentable.
I could not execute the flow to confirm the
AADSTS500011prediction — there is noMICROSOFT_CLIENT_IDavailable here. The one-minute check is opening the authorize URL in a browser signed into any work tenant.Deliberate decisions worth reviewing
getDataverseBaseUrlat call time. Server-side validation of a value we don't trust would be theater; the fail-closed guards above are server-side where they matter./api/auth/oauth2/authorize(as trello/instagram/shopify are). That route requiresworkspaceId, which the modal's reauthorize mode does not have, and splitting connect/reauthorize across two transports would put the two modes on different validation paths.resolveResourceOrigininvalidateExternalUrlfor private-IP rejection. A host-suffix allowlist strictly dominates it — nothing internal ends with.dynamics.com— so it would only pull a heavier module into all 18 Dataverse tools' graph.Follow-ups (not in scope)
account.scopefor free. Reading it back inbuildOAuthTokenPayload— the way Salesforce'sinstanceUrland Zoho'sapiDomainalready do — would let the block'senvironmentUrlbe derived instead of hand-typed a second time, and would stop a credential minted for env A being pointed at env B.getMissingRequiredScopescan no longer flag a Dataverse credential as under-scoped, sincerequiredScopesis now OIDC-only. (Before this PR it flagged every Dataverse credential, because the static scope could never be granted — so neither state is right.) A scope-satisfaction predicate onOAuthResourceUrlConfigwould close it.canConnectFromSurface(providerId, surface)predicate to replace the scattered guards — and to fold in the existing hand-mirrored trello/shopify reconnect rules in the same two files.Testing
bun run type-checkcleanbiome checkcleanbun run check:api-validation:strictpassesRan
/cleanup(8 passes) and/simplify(4 angles) over the diff. Notable catches:resourceUrlnever reset across modal close/provider-switch; the authorize-route lookup was strictly weaker thangetServiceConfigByProviderId(it missed provider alias ids, which for a fail-closed guard is the wrong direction); and the host-policy duplication above.