From 4cbc35e79f159b89731e3bbffa6fef91046b3241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?= <81782+aureliensibiril@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:00:28 +0200 Subject: [PATCH] Fix the Segment region input and invite duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The region is a two-value allow-list the server resolves to an API host, but it rendered as a free-text field: only PostHog is special-cased in the API-key dialog, everything else falls through to a generic Field. Typing "EU1" — the region Segment's own UI shows for the EU workspace — passed the non-empty check, then failed the mutation, and the dialog's generic error blamed the API key. It is a select now, so the label no longer has to spell the accepted values out. An invite that has already been accepted can still be listed, and the member and the invite were keyed differently (user ID vs email), so the same person surfaced as two rows — one active with roles, one inactive without. Invites for an email already seen among members are dropped. Per-user permission errors now name the user, and the probe URL builds its query with url.Values rather than a hand-written string. The region-to-host mapping is the only API-key setting that derives a value instead of storing input verbatim, and it had no test; a typo in either host would only have surfaced as a live 404. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- .../_components/APIKeyConnectorDialog.tsx | 24 ++++- pkg/accessreview/drivers/segment.go | 16 ++- pkg/connector/provider/probe.go | 5 +- pkg/connector/provider/segment.go | 2 +- pkg/server/api/console/v1/segment_test.go | 99 +++++++++++++++++++ 5 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 pkg/server/api/console/v1/segment_test.go diff --git a/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx b/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx index 8e852ab94..d94a0b884 100644 --- a/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx +++ b/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx @@ -26,6 +26,8 @@ import { DialogContent, DialogFooter, Field, + Option, + Select, useDialogRef, useToast, } from "@probo/ui"; @@ -248,7 +250,8 @@ export function APIKeyConnectorDialog({ }; // PostHog renders a dedicated deployment selector (Cloud region or - // self-hosted URL); every other provider falls back to generic fields. + // self-hosted URL) and Segment a region selector; every other provider falls + // back to generic fields. const renderAPIKeyExtraSettings = () => { if (!provider) { return null; @@ -263,6 +266,25 @@ export function APIKeyConnectorDialog({ ); } + // The server maps this to a regional API host and rejects anything outside + // the allow-list, so it must not be typed by hand. + if (provider.provider === "SEGMENT") { + return ( +
+ + +
+ ); + } + return provider.extraSettings.map((setting) => { const value = extraSettingValues[setting.key] ?? ""; return ( diff --git a/pkg/accessreview/drivers/segment.go b/pkg/accessreview/drivers/segment.go index 99e71acfb..6fb7584b5 100644 --- a/pkg/accessreview/drivers/segment.go +++ b/pkg/accessreview/drivers/segment.go @@ -116,6 +116,7 @@ func (d *SegmentDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro } records := make([]AccountRecord, 0, len(users)) + seen := make(map[string]struct{}, len(users)) for _, u := range users { email := strings.TrimSpace(u.Email) @@ -123,9 +124,11 @@ func (d *SegmentDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro continue } + seen[strings.ToLower(email)] = struct{}{} + perms, err := d.userPermissions(ctx, base, u.ID) if err != nil { - return nil, err + return nil, fmt.Errorf("cannot list segment permissions for user %q: %w", u.ID, err) } roles, isAdmin := segmentRolesAndAdmin(perms) @@ -157,15 +160,22 @@ func (d *SegmentDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro continue } + // An invite that has already been accepted can still be listed; the + // member record above is the authoritative one, so skip the duplicate + // rather than emit two rows for the same person. + if _, ok := seen[strings.ToLower(email)]; ok { + continue + } + // A pending invite carries no role and no stable id at the workspace // level, so it is surfaced as an inactive member keyed by email. - inactive := false + active := false records = append(records, AccountRecord{ Email: email, FullName: email, Roles: []string{}, - Active: &inactive, + Active: &active, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, diff --git a/pkg/connector/provider/probe.go b/pkg/connector/provider/probe.go index 28783362b..db49ef0ef 100644 --- a/pkg/connector/provider/probe.go +++ b/pkg/connector/provider/probe.go @@ -636,8 +636,11 @@ func buildSegmentProbeURL(conn *coredata.Connector) (string, error) { return "", fmt.Errorf("cannot parse segment base URL: %w", err) } + q := url.Values{} + q.Set("pagination.count", "1") + u.Path = "/users" - u.RawQuery = "pagination.count=1" + u.RawQuery = q.Encode() return u.String(), nil } diff --git a/pkg/connector/provider/segment.go b/pkg/connector/provider/segment.go index 7cda2ba95..5ad243d54 100644 --- a/pkg/connector/provider/segment.go +++ b/pkg/connector/provider/segment.go @@ -43,7 +43,7 @@ func segmentRegistration() *Registration { // as an extra setting and resolved to a base URL (Pattern 3 + region); // there is nothing to pick. ExtraSettings: []ExtraSetting{ - {Key: "region", Label: "Region (US or EU)", Required: true}, + {Key: "region", Label: "Region", Required: true}, }, BuildProbeURL: buildSegmentProbeURL, // No NewNameResolver: the Public API exposes no read-only workspace-name diff --git a/pkg/server/api/console/v1/segment_test.go b/pkg/server/api/console/v1/segment_test.go new file mode 100644 index 000000000..0b2017905 --- /dev/null +++ b/pkg/server/api/console/v1/segment_test.go @@ -0,0 +1,99 @@ +// 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 console_v1 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/server/api/console/v1/types" +) + +// The Segment region is the only API-key setting resolved to a derived value +// rather than stored verbatim, so the region -> host mapping is pinned here: +// a typo in either host would otherwise only surface as a live 404. +func TestApiKeyConnectorSettings_SegmentRegion(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + region string + baseURL string + }{ + {name: "US", region: "US", baseURL: "https://api.segmentapis.com"}, + {name: "EU", region: "EU", baseURL: "https://eu1.api.segmentapis.com"}, + {name: "lowercase", region: "eu", baseURL: "https://eu1.api.segmentapis.com"}, + {name: "padded", region: " us ", baseURL: "https://api.segmentapis.com"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + raw, err := apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderSegment, + SegmentRegion: &tc.region, + }) + require.NoError(t, err) + + var settings coredata.SegmentConnectorSettings + require.NoError(t, json.Unmarshal(raw, &settings)) + assert.Equal(t, tc.baseURL, settings.BaseURL) + }) + } +} + +func TestApiKeyConnectorSettings_SegmentRejectsUnknownRegion(t *testing.T) { + t.Parallel() + + for _, name := range []string{"empty", "whitespace", "apac", "eu1", "host"} { + region := map[string]string{ + "empty": "", + "whitespace": " ", + "apac": "APAC", + // The region Segment's own UI shows for the EU workspace; it must + // not silently fall through to a wrong host. + "eu1": "EU1", + "host": "https://eu1.api.segmentapis.com", + }[name] + + t.Run(name, func(t *testing.T) { + t.Parallel() + + _, err := apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderSegment, + SegmentRegion: ®ion, + }) + require.Error(t, err) + }) + } +} + +func TestApiKeyConnectorSettings_SegmentRequiresRegion(t *testing.T) { + t.Parallel() + + _, err := apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderSegment, + }) + require.Error(t, err) +}