Refactor OAuth2 connectors: caller-owned scopes, error handling, simpler config#997
Merged
gearnode merged 17 commits intogetprobo:mainfrom Apr 8, 2026
Conversation
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/server/api/console/v1/resolver.go">
<violation number="1" location="pkg/server/api/console/v1/resolver.go:283">
P2: OAuth2 error callback always redirects to base URL and drops state continuation/organization context.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
9d44a32 to
b8ac0a0
Compare
gearnode
reviewed
Apr 8, 2026
gearnode
reviewed
Apr 8, 2026
gearnode
reviewed
Apr 8, 2026
8e20152 to
d509e9c
Compare
Centralise static OAuth2 properties (auth URL, token URL, scopes, extra params, token endpoint auth) per provider in a single map. This removes the need to duplicate these values in deployment config; only ClientID and ClientSecret remain configurable. Introduces ApplyProviderDefaults to set redirect URI and provider defaults onto an OAuth2Connector at wiring time. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Remove provider-default mutation from Register; defaults are now applied via ApplyProviderDefaults before registration. Rename receiver from cr to r. Fix error messages to follow the cannot convention. Wrap providerProbeURLs in var () block. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
All other OAuth2 properties (redirect URI, auth URL, token URL, scopes, extra params, token endpoint auth) now come from the connector package provider definitions at wiring time. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Compute the OAuth2 redirect URI from the base URL using the CallbackPath constant and apply provider defaults before registering each connector. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Drop RedirectURI, AuthURL, TokenURL, Scopes, ExtraAuthParams, and TokenEndpointAuth from all connector config blocks. Remove REDIRECT_URI from env var validation. Fix error wrapping in SAML credential helpers. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Reduce closure size in NewMux by extracting the /connectors/complete handler into a dedicated handleConnectorComplete function. Cache r.URL.Query() into a local variable to avoid repeated parsing. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
When a provider returns an error (e.g. user denies consent), the callback now logs the error with provider name and redirects to the base URL with error and error_description query parameters instead of falling through to the code exchange. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Use the ContinueURL from the state token so the user is redirected back to where they initiated the flow instead of the root URL. The redirect is safe because safeRedirect validates the host. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Add an InitiateOptions struct to the Connector interface so each caller can declare the scopes it needs instead of having them baked into the connector at registration. The HTTP handler reads repeated ?scope= query parameters from /connectors/initiate and forwards them. Also restore GOOGLE_WORKSPACE and LINEAR provider definitions which were silently dropped from the bootstrap config refactor. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Each module that initiates an OAuth2 flow now declares its scopes in its own package instead of duplicating them in the frontend or in shared connector config: - pkg/accessreview/drivers: per-provider scopes for the access review drivers - pkg/slack: scopes for the compliance page integration - pkg/iam/scim/bridge/provider/googleworkspace: scopes for the SCIM provisioning bridge These constants are surfaced to the frontend via GraphQL fields so the frontend never hardcodes scope strings. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Add per-context fields so the frontend can read scopes from the type that owns each connection: - ConnectorProviderInfo.oauth2Scopes: access review providers - AccessSource.oauth2Scopes: access review reconnect flow - Organization.slackOAuth2Scopes (console): compliance page Slack - Organization.googleWorkspaceOAuth2Scopes (connect): SCIM bridge Resolvers delegate to the constants declared in each owning Go module. The Google Workspace field lives on Organization, not on SCIMConfiguration, so the Connect button can read it before any SCIM configuration exists. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Each component that initiates an OAuth2 flow now reads its scopes from the colocated Relay fragment instead of a hardcoded TypeScript map. The five live call sites pass scopes to the backend via the new ?scope= query parameter. Drop the dead CreateAccessSourceDialog React component and rename the file to accessSourceMutations.ts since only the mutation export was used. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
eslint import-x/order sorts case-insensitively, so the lowercase accessSourceMutations imports must precede the AddAccessSourceDialog imports in both files. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The AccessSource.oauth2Scopes field duplicated knowledge that
naturally belongs on the Connector object that AccessSource
already exposes via its connector field. Move it to Connector so
every type that holds a connector (AccessSource, SCIMBridge, etc.)
can reach the scopes through the connector relationship.
AccessSourceRow now queries accessSource.connector { oauth2Scopes }
in its reconnect flow.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
A Google-Workspace-specific field on the generic Organization type was future-hostile: each new SCIM bridge type would need its own top-level field. Replace with a generic SCIMBridgeTypeInfo type queried through Organization.scimBridgeTypes, parallel to the ConnectorProviderInfo pattern in console/v1. ConnectorList looks up the Google Workspace entry from the list and passes its scopes to GoogleWorkspaceConnector as before. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
d509e9c to
22ccfaa
Compare
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR overhauls the OAuth2 connector layer in three coherent passes:
1. Simpler bootstrap config
ClientIDandClientSecretConnectorRegistryto a pure store; wire provider defaults at registration timeGOOGLE_WORKSPACEandLINEARprovider definitions which were silently dropped during the bootstrap simplification (both were broken on this branch before this PR)2. OAuth2 callback error handling
/connectors/completehandler fromNewMuxinto dedicated functionserroranderror_descriptionquery params instead of falling through to the code exchange3. Caller-owned OAuth2 scopes
The same provider may need different scopes in different contexts. Google Workspace is the canonical case: the SCIM bridge needs full directory + groups + customer + userschema (4 scopes); access review only needs to list users (2 scopes). Baking scopes into the connector at registration time made this impossible.
InitiateOptions{Scopes []string}to theConnectorinterface;/connectors/initiatereads repeated?scope=query parameters and forwards thempkg/accessreview/drivers/oauth2_scopes.go— per-provider scopes for access reviewpkg/slack/oauth2_scopes.go— Slack compliance page scopespkg/iam/scim/bridge/provider/googleworkspace/oauth2_scopes.go— Google Workspace SCIM scopesConnectorProviderInfo.oauth2Scopes(access review providers list)AccessSource.oauth2Scopes(reconnect flow)Organization.slackOAuth2Scopes(compliance page)Organization.googleWorkspaceOAuth2Scopes(SCIM bridge — on Organization, not SCIMConfiguration, since the Connect button shows when SCIM is unconfigured)CreateAccessSourceDialogReact component, rename file toaccessSourceMutations.tsPersistence note
OAuth2 refresh tokens do not need scopes (RFC 6749 §6) — the refreshed token inherits the originally granted scopes. Scopes only matter at the auth code exchange step, so they are not persisted on the connector record.
Test plan