From e8e5e9bf9d6e53b52e8fc76016c5ec2a0c68f7a7 Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Fri, 31 Jul 2026 15:32:45 +0200 Subject: [PATCH] Show missing OAuth scopes after connector reconnect Partial grants completed without feedback, leaving Reconnect required unexplained. Keep the token and toast the backend missing-scopes error after the OAuth callback redirect. Signed-off-by: Sacha Al Himdani --- .../sources/AccessReviewSourcesTab.tsx | 70 +++++++---- pkg/accessreview/errors.go | 23 ++++ pkg/accessreview/errors_test.go | 16 +++ .../source_missing_scopes_test.go | 112 ++++++++++++++++++ pkg/accessreview/source_service.go | 63 +++++++--- pkg/server/api/console/v1/resolver.go | 14 +++ 6 files changed, 255 insertions(+), 43 deletions(-) create mode 100644 pkg/accessreview/source_missing_scopes_test.go diff --git a/apps/console/src/pages/organizations/access-reviews/sources/AccessReviewSourcesTab.tsx b/apps/console/src/pages/organizations/access-reviews/sources/AccessReviewSourcesTab.tsx index 3a47bc6be..a3eb72b50 100644 --- a/apps/console/src/pages/organizations/access-reviews/sources/AccessReviewSourcesTab.tsx +++ b/apps/console/src/pages/organizations/access-reviews/sources/AccessReviewSourcesTab.tsx @@ -47,6 +47,13 @@ import { AccessReviewSourceRow } from "../_components/AccessReviewSourceRow"; import { createAccessReviewSourceMutation } from "../dialogs/accessReviewSourceMutations"; import { AddAccessReviewSourceDialog, addAccessReviewSourceDialogConnectorProviderInfoFragment } from "../dialogs/AddAccessReviewSourceDialog"; +function clearOAuthCallbackParams(params: URLSearchParams) { + params.delete("connector_id"); + params.delete("provider"); + params.delete("error"); + return params; +} + export const accessReviewSourcesTabQuery = graphql` query AccessReviewSourcesTabQuery($organizationId: ID!) { accessReviewDrivers { @@ -145,9 +152,11 @@ export default function AccessReviewSourcesTab({ queryRef }: Props) { ); // Handle OAuth callback: after the provider redirects back with connector_id, - // automatically create the access source for that connector. + // automatically create the access source for that connector. Missing scopes + // arrive as a backend error query param and are toasted like other errors. const callbackConnectorId = searchParams.get("connector_id"); const callbackProvider = searchParams.get("provider"); + const callbackError = searchParams.get("error"); const hasSourceForCallback = !!callbackConnectorId && accessReviewSources?.edges.some(edge => edge.node.connectorId === callbackConnectorId); @@ -155,11 +164,22 @@ export default function AccessReviewSourcesTab({ queryRef }: Props) { if (!callbackConnectorId) return; if (hasSourceForCallback) { - setSearchParams((params) => { - params.delete("connector_id"); - params.delete("provider"); - return params; - }, { replace: true }); + // Create sets processedConnectorIdRef before the mutation; when Relay + // inserts the edge mid-callback, skip toasting here so onCompleted is + // the only toast. Reconnect never sets that ref, so it still toasts. + const createInFlight + = processedConnectorIdRef.current === callbackConnectorId; + if (callbackError && !createInFlight) { + toast({ + title: t("accessReviewSourcesTab.messages.error"), + description: callbackError, + variant: "error", + }); + } + if (!createInFlight) { + processedConnectorIdRef.current = null; + setSearchParams(clearOAuthCallbackParams, { replace: true }); + } return; } @@ -186,11 +206,7 @@ export default function AccessReviewSourcesTab({ queryRef }: Props) { onCompleted(_, errors) { if (errors?.length) { processedConnectorIdRef.current = null; - setSearchParams((params) => { - params.delete("connector_id"); - params.delete("provider"); - return params; - }, { replace: true }); + setSearchParams(clearOAuthCallbackParams, { replace: true }); toast({ title: t("accessReviewSourcesTab.messages.error"), description: formatError( @@ -201,24 +217,25 @@ export default function AccessReviewSourcesTab({ queryRef }: Props) { }); return; } - toast({ - title: t("accessReviewSourcesTab.messages.success"), - description: t("accessReviewSourcesTab.messages.created"), - variant: "success", - }); - setSearchParams((params) => { - params.delete("connector_id"); - params.delete("provider"); - return params; - }, { replace: true }); + if (callbackError) { + toast({ + title: t("accessReviewSourcesTab.messages.error"), + description: callbackError, + variant: "error", + }); + } else { + toast({ + title: t("accessReviewSourcesTab.messages.success"), + description: t("accessReviewSourcesTab.messages.created"), + variant: "success", + }); + } + processedConnectorIdRef.current = null; + setSearchParams(clearOAuthCallbackParams, { replace: true }); }, onError(error) { processedConnectorIdRef.current = null; - setSearchParams((params) => { - params.delete("connector_id"); - params.delete("provider"); - return params; - }, { replace: true }); + setSearchParams(clearOAuthCallbackParams, { replace: true }); toast({ title: t("accessReviewSourcesTab.messages.error"), description: formatError( @@ -232,6 +249,7 @@ export default function AccessReviewSourcesTab({ queryRef }: Props) { }, [ callbackConnectorId, callbackProvider, + callbackError, connectorProviderInfos, createAccessReviewSource, hasSourceForCallback, diff --git a/pkg/accessreview/errors.go b/pkg/accessreview/errors.go index cdb40683b..5e3e8869b 100644 --- a/pkg/accessreview/errors.go +++ b/pkg/accessreview/errors.go @@ -23,6 +23,7 @@ package accessreview import ( "errors" "fmt" + "strings" "go.probo.inc/probo/pkg/gid" ) @@ -34,6 +35,7 @@ var ( ErrCampaignNotPendingActions = errors.New("access review campaign not pending actions") ErrCampaignCompleted = errors.New("access review campaign completed") ErrCampaignCancelled = errors.New("access review campaign cancelled") + ErrMissingOAuthScopes = errors.New("missing required OAuth scopes") ) type ( @@ -60,6 +62,10 @@ type ( CampaignCancelledError struct { CampaignID gid.GID } + + MissingOAuthScopesError struct { + Scopes []string + } ) func NewCampaignMissingSourcesError(campaignID gid.GID) error { @@ -142,3 +148,20 @@ func (e *CampaignCancelledError) Error() string { func (e *CampaignCancelledError) Is(target error) bool { return target == ErrCampaignCancelled } + +func NewMissingOAuthScopesError(scopes []string) error { + return &MissingOAuthScopesError{Scopes: append([]string(nil), scopes...)} +} + +func (e *MissingOAuthScopesError) Error() string { + display := make([]string, len(e.Scopes)) + for i, scope := range e.Scopes { + display[i] = strings.TrimPrefix(scope, "https://graph.microsoft.com/") + } + + return "Missing required OAuth scopes: " + strings.Join(display, ", ") +} + +func (e *MissingOAuthScopesError) Is(target error) bool { + return target == ErrMissingOAuthScopes +} diff --git a/pkg/accessreview/errors_test.go b/pkg/accessreview/errors_test.go index b26d541de..79d6ae8da 100644 --- a/pkg/accessreview/errors_test.go +++ b/pkg/accessreview/errors_test.go @@ -98,3 +98,19 @@ func TestCampaignClientErrors(t *testing.T) { }) } } + +func TestMissingOAuthScopesError(t *testing.T) { + t.Parallel() + + err := accessreview.NewMissingOAuthScopesError([]string{ + "https://graph.microsoft.com/AuditLog.Read.All", + "openid", + }) + + assert.Equal( + t, + "Missing required OAuth scopes: AuditLog.Read.All, openid", + err.Error(), + ) + assert.ErrorIs(t, err, accessreview.ErrMissingOAuthScopes) +} diff --git a/pkg/accessreview/source_missing_scopes_test.go b/pkg/accessreview/source_missing_scopes_test.go new file mode 100644 index 000000000..c10341743 --- /dev/null +++ b/pkg/accessreview/source_missing_scopes_test.go @@ -0,0 +1,112 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package accessreview + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.probo.inc/probo/pkg/connector" + "go.probo.inc/probo/pkg/coredata" +) + +func TestMissingOAuthScopesForConnector(t *testing.T) { + t.Parallel() + + required := []string{ + "openid", + "https://graph.microsoft.com/AuditLog.Read.All", + "https://graph.microsoft.com/User.Read.All", + } + + t.Run("non oauth protocol returns empty", func(t *testing.T) { + t.Parallel() + + dbConnector := coredata.Connector{ + Protocol: coredata.ConnectorProtocolAPIKey, + Connection: &connector.APIKeyConnection{APIKey: "k"}, + } + + assert.Empty(t, missingOAuthScopesForConnector(dbConnector, required)) + }) + + t.Run("empty required returns empty", func(t *testing.T) { + t.Parallel() + + dbConnector := coredata.Connector{ + Protocol: coredata.ConnectorProtocolOAuth2, + Connection: &connector.OAuth2Connection{ + Scope: "openid", + }, + } + + assert.Empty(t, missingOAuthScopesForConnector(dbConnector, nil)) + }) + + t.Run("nil connection treats grant as empty", func(t *testing.T) { + t.Parallel() + + dbConnector := coredata.Connector{ + Protocol: coredata.ConnectorProtocolOAuth2, + Connection: nil, + } + + assert.Equal( + t, + []string{ + "https://graph.microsoft.com/AuditLog.Read.All", + "https://graph.microsoft.com/User.Read.All", + "openid", + }, + missingOAuthScopesForConnector(dbConnector, required), + ) + }) + + t.Run("partial grant returns missing scopes", func(t *testing.T) { + t.Parallel() + + dbConnector := coredata.Connector{ + Protocol: coredata.ConnectorProtocolOAuth2, + Connection: &connector.OAuth2Connection{ + Scope: "openid User.Read.All", + }, + } + + assert.Equal( + t, + []string{"https://graph.microsoft.com/AuditLog.Read.All"}, + missingOAuthScopesForConnector(dbConnector, required), + ) + }) + + t.Run("full grant returns empty", func(t *testing.T) { + t.Parallel() + + dbConnector := coredata.Connector{ + Protocol: coredata.ConnectorProtocolOAuth2, + Connection: &connector.OAuth2Connection{ + Scope: "openid AuditLog.Read.All User.Read.All", + }, + } + + assert.Empty(t, missingOAuthScopesForConnector(dbConnector, required)) + }) +} diff --git a/pkg/accessreview/source_service.go b/pkg/accessreview/source_service.go index ed30f7def..f53b7d1bb 100644 --- a/pkg/accessreview/source_service.go +++ b/pkg/accessreview/source_service.go @@ -582,17 +582,17 @@ func (s *Service) SourceNeedsConfiguration( return cfg.SelectedSlug(dbConnector) == "", nil } -// SourceNeedsReconnect reports whether the connector is missing OAuth scopes -// required by the current provider registration. Only OAuth2 connectors are -// checked: API-key (and other non-OAuth) credentials have no grant scopes and -// cannot be repaired by an OAuth reconnect, even when the provider also -// advertises OAuth2Scopes for its dual-auth path. ErrResourceNotFound is -// propagated for a missing connector. -func (s *Service) SourceNeedsReconnect( +// SourceMissingOAuthScopes returns the OAuth scopes required by the current +// provider registration that are absent from the connector's stored grant. +// Only OAuth2 connectors are checked: API-key (and other non-OAuth) +// credentials have no grant scopes and return an empty slice, even when the +// provider also advertises OAuth2Scopes for its dual-auth path. +// ErrResourceNotFound is propagated for a missing connector. +func (s *Service) SourceMissingOAuthScopes( ctx context.Context, scope coredata.Scoper, connectorID gid.GID, -) (bool, error) { +) ([]string, error) { var dbConnector coredata.Connector err := s.pg.WithConn( @@ -606,23 +606,52 @@ func (s *Service) SourceNeedsReconnect( }, ) if err != nil { - return false, err - } - - if dbConnector.Protocol != coredata.ConnectorProtocolOAuth2 { - return false, nil + return nil, err } required := s.providerRegistry.ProviderOAuth2Scopes(dbConnector.Provider) + + return missingOAuthScopesForConnector(dbConnector, required), nil +} + +// SourceNeedsReconnect reports whether the connector is missing OAuth scopes +// required by the current provider registration. ErrResourceNotFound is +// propagated for a missing connector. +func (s *Service) SourceNeedsReconnect( + ctx context.Context, + scope coredata.Scoper, + connectorID gid.GID, +) (bool, error) { + missing, err := s.SourceMissingOAuthScopes(ctx, scope, connectorID) + if err != nil { + return false, err + } + + return len(missing) > 0, nil +} + +// missingOAuthScopesForConnector returns scopes in required that are absent +// from the connector's stored OAuth grant. Non-OAuth connectors and empty +// required lists yield an empty result. A nil Connection is treated as +// granting nothing. +func missingOAuthScopesForConnector( + dbConnector coredata.Connector, + required []string, +) []string { + if dbConnector.Protocol != coredata.ConnectorProtocolOAuth2 { + return []string{} + } + if len(required) == 0 { - return false, nil + return []string{} } - if dbConnector.Connection == nil { - return true, nil + var granted []string + if dbConnector.Connection != nil { + granted = dbConnector.Connection.Scopes() } - return len(connector.MissingScopes(required, dbConnector.Connection.Scopes())) > 0, nil + return connector.MissingScopes(required, granted) } // AutoSelectDefaultOrganization picks the first workspace/org a freshly linked diff --git a/pkg/server/api/console/v1/resolver.go b/pkg/server/api/console/v1/resolver.go index b79d34b88..4217d0d67 100644 --- a/pkg/server/api/console/v1/resolver.go +++ b/pkg/server/api/console/v1/resolver.go @@ -28,6 +28,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "github.com/go-chi/chi/v5" "go.gearno.de/kit/httpserver" @@ -431,6 +432,19 @@ func handleConnectorComplete( q := parsedURL.Query() q.Set("connector_id", cnnctr.ID.String()) q.Set("provider", string(connectorProvider)) + + // Access-review sources toast missing scopes after redirect. Other + // continue URLs (Slack compliance page, SCIM settings, …) must not + // get a false missing-scope error from this access-review check. + if strings.Contains(state.ContinueURL, "/access-reviews/sources") { + missing, err := accessReviewSvc.SourceMissingOAuthScopes(r.Context(), scope, cnnctr.ID) + if err != nil { + logger.WarnCtx(r.Context(), "cannot determine missing OAuth scopes after connector callback", log.Error(err)) + } else if len(missing) > 0 { + q.Set("error", accessreview.NewMissingOAuthScopesError(missing).Error()) + } + } + parsedURL.RawQuery = q.Encode() safeRedirect.Redirect(w, r, parsedURL.String(), "/", http.StatusSeeOther)