Reject deletion while a campaign is fetching sources so workers are
not racing a removed record. All other statuses remain deletable.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Drop the backend status gate on campaign delete and show delete in
the console whenever the user has delete permission, regardless of
whether the campaign is draft, in progress, or completed.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Completed campaigns were immutable because delete only accepted draft
or cancelled status. Extend deletion to completed records and surface
delete in the console for that status.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
MergeByCampaignID joined access_review_sources directly from coredata
to resolve live sources, so unrecognized or out-of-scope IDs were
silently dropped instead of erroring, and in the worst case (every ID
invalid) the NOT MATCHED BY SOURCE clause deleted every existing
campaign source. The syncCampaignSources ErrResourceNotFound check
was therefore unreachable dead code.
Add AccessReviewSources.LoadByIDs, matching the existing scoped
LoadByIDs pattern (id = ANY(@ids) plus a resolved-count check), and
have CreateCampaign, AddCampaignSource, and syncCampaignSources
resolve and validate sources up front. MergeByCampaignID now takes
the already-loaded sources and builds its desired-state CTE from an
unnest() of their values instead of joining access_review_sources,
keeping the merge inside the campaign-source entity boundary.
Signed-off-by: Bryan Frimin <bryan@probo.com>
Declare AccessReviewSource before LoadByID so the package compiles
after RemoveCampaignSource cleanup.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Add AccessReviewCampaignSources.MergeByCampaignID and use it from
syncCampaignSources instead of per-row load, upsert, and delete.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Apply draft and lifecycle checks on loaded coredata records
directly in the campaign service.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Introduce accessreview.Campaign with draft/deletable predicates and
operation-specific client errors. Drop errUnlessDraftCampaign and
status-to-sentinel switches in the service layer.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Business-layer errors carry the access review campaign GID in
Error() and match GraphQL sentinels via Is().
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Remove status-to-error helpers and map loaded campaign data to
accessreview sentinels at each business rule instead of wrapping
coredata status values.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Drop CampaignStatusError from errors.go and map coredata status to
client errors in the service layer only. Cancel on terminal statuses
uses the same sentinels instead of fmt status strings.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
List campaign status sentinels explicitly at resolver AnyOf
call sites instead of a shared slice.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Centralize errors.Is OR chains in access review campaign resolvers
and expose CampaignStatusErrors for the four non-draft status
sentinels.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Replace ErrCampaignCannotStart and ErrCampaignCannotUpdate with
sentinels per campaign status, mapped through CampaignStatusError
and wrapped with operation-specific fmt.Errorf prefixes.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Use ErrCampaignCannotStart and ErrCampaignCannotUpdate with clear
actionable messages instead of a generic draft-status sentinel
wrapped in noisy fmt.Errorf chains.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
The failure is about unconfigured campaign sources, not an empty UI
selection.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Drop ErrCampaignSourceOrganizationMismatch. When a source ID is
missing or belongs to another organization, return
coredata.ErrResourceNotFound so clients get a generic not-found
response instead of leaking cross-organization details.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Align the sentinel name with the validation it represents.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Follow the cookiebanner pattern: grouped var Err* sentinels in the
service package, wrapped with fmt.Errorf where context is needed, and
explicit errors.Is checks in GraphQL resolvers.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Introduce sentinel and structured errors for access review campaign
validation failures, and map them to INVALID in GraphQL resolvers via
errors.Is rather than matching error message prefixes.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
Replace the separate Add Source operation with multi-select source
fields on create and update. Create passes accessReviewSourceIds to
the existing GraphQL input; update gains the same omittable field and
backend source sync so workflows can configure sources in one step.
Load source options from the organization in the n8n UI, and keep
surfacing INVALID errors when start fails for missing sources.
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Bryan FRIMIN <bryan@frimin.fr>
account/list marks the contract's primary account "main" on the live
API, not "mymain" as the published docs example shows, so matching the
documented spelling reported every account as a non-admin, including
the contract owner. Roles cannot stand in: a main account carries the
same technical/billing values a sub-account can hold. Classify off
"sub" instead, the one value the docs and the API agree on.
The fixture copied the docs example, so the test passed on the same
wrong assumption. Its bodies now mirror a live capture, anonymized: the
main account carries no main_account or allow_gui, sub-accounts add
them plus the access lists, and the primary account's type is "main".
A table test pins both spellings.
A review keys accounts on email plus external ID. Email came only from
account/details, and any failure blanked it while still emitting the
record, so a transient 5xx moved an account to a different key and
surfaced it as one account removed and another added. Only the stable
answers now degrade: UpCloud returns 403 ACCOUNT_FORBIDDEN, not 404,
for an account outside the token's reach, and both keep the list-only
fields. Anything else aborts the run.
A blank username no longer discards every account already collected,
matching the sibling drivers.
Resolve the source name from GET /1.3/account so sources read
"UpCloud <username>" rather than staying generic, and link the
connector to its documentation page.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Two corrections, both settled from Segment's published OpenAPI document
and their own client code rather than guessed.
The registration claimed the Public API exposes no workspace-name
endpoint on the token's scope. That is wrong: Get Workspace is the API
root, GET /, returning data.workspace.name for the workspace the token
is bound to — the base URL already encodes the US/EU region, so the URL
is the whole request. Without a resolver an organization running a prod
and a staging workspace saw two rows both named "Segment".
The per-user GET /users/{id} is what makes a large workspace exceed the
per-source budget, and it exists only to read permissions[].roleName.
Both endpoints return the same UserV1 schema, on which permissions is
declared but optional, so whether the list populates it is a server
behaviour no specification settles. Rather than assume, the list
response is now decoded for permissions and the per-user request is
issued only when the field is absent. Today Segment omits it — their own
Terraform provider's mock returns /users without permissions and
/users/{id} with them — so behaviour is unchanged; if that ever changes
the extra round trip disappears on its own. An empty-but-present array
is authoritative, meaning a user with no roles, not a missing field.
Page size stays at 200: the 1-1000 range is prose in the pagination
guide, the schema sets no maximum, and the migration guide says 200.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
A fetch runs under a 30-second per-source budget, but the retry transport
slept without reference to it, so backoff could convert a reportable
provider status into an opaque "context deadline exceeded".
Three changes. The final attempt no longer sleeps: nothing follows it, so
the wait only spent the caller's deadline to return a response already in
hand — up to a second per failed request, across sixteen drivers.
Retry-After is now honoured, in both the delta-seconds and HTTP-date
forms; ignoring it meant retrying a 429 after 250ms and earning another
429, spending the whole retry budget in under a second. And a wait is
skipped entirely when it exceeds the remaining deadline or a 5s cap,
because a retry that lands after the deadline cannot succeed — the
throttled response is surfaced instead so the caller reports what the
provider actually said.
The type moves from google_workspace.go to driver.go, which is where the
other shared driver machinery lives; sixteen drivers construct it and
none of them are Google Workspace. It had no tests, so it has them now.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The cassettes cannot reach these branches. Every fixture email is
already lowercase, so nothing exercised the trim-and-lowercase that is
the Google Analytics driver's whole merge key; likewise the Dotfile role
casing and the Segment role dedup and case-insensitive owner match.
The Dotfile and Square tests never asserted CreatedAt, which
parseRFC3339Ptr silently nils on any format mismatch — both cassettes
carry usable timestamps, so a dropped field looked like a clean pass.
Also gives the Square OAuth variables their own comment in the example
env file; they sat under the Google Analytics one.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
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>
Three defects, all found reviewing the rebased branch.
The connection probe hit /v1alpha/accounts, which any analytics.readonly
grant can call, while the driver's first request is the account's
accessBindings — that additionally needs Administrator on the account
and the manage.users.readonly scope. An Editor connecting, or a user
declining the second scope on Google's granular consent screen, probed
green and then 403'd on every campaign fetch, leaving the source
permanently "Connected" with no rows. The probe now targets the same
accessBindings collection the driver reads.
A single unreadable property aborted the whole account. A property the
token cannot see, or one deleted between the list and the read, threw
away every binding already collected; 49 of 50 readable properties are
still worth reviewing, so 403 and 404 now skip that property. Anything
else still fails the fetch.
Fan-out errors named no resource: the account call, the property list
and each per-property call all returned the same "unexpected status"
string, so a 403 on one subproperty out of forty was unattributable.
Errors now carry the account or property ID.
The cassette gains a subproperty parented to another property (only
reachable through the ancestor filter, so it pins the hierarchy walk
that the filter change claimed) and a property returning 403.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The four connectors landed with the ISC header. contrib/claude/license.md
mandates MIT for every source file, and these were the only ISC files
left in pkg/accessreview/drivers and pkg/connector/provider — the two
"Make license declarations consistently MIT" commits had already cleared
both packages, so this re-opened a closed cleanup.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
ExternalID is normally a stable provider-side ID, not an email. GA4 access
bindings identify a user only by email — no per-user ID and no display name
are exposed — so email is the only stable key available. Document that on
googleAnalyticsRecords so the choice reads as deliberate.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The Square, Segment and Google Analytics drivers all wrap their transport
with retryRoundTripper for flaky 5xx responses, but the Dotfile driver used
the client directly. Wrap it the same way so all four connectors handle
transient upstream failures consistently.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
ListGoogleAnalyticsOrganizations decoded the response body into the success
struct before inspecting the HTTP status, unlike every other lister in the
file. Check the status first so a non-2xx no longer wastes a decode against
an error body and the ordering matches the sibling functions.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Segment's /users API exposes no active/suspended field, so reporting every
confirmed member as Active=true fabricated a status the source never
provides, contrary to the AccountRecord contract (nil = no explicit signal).
Leave Active nil for confirmed members; pending invites keep Active=false,
which is a real signal from /invites.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
listProperties filtered properties with parent:accounts/{id}, which returns
only properties whose direct parent is the account and silently drops
subproperties and roll-up properties (parented to another property). A member
holding a binding only on such a subproperty was omitted from the review.
Switch to the ancestor:accounts/{id} filter, which walks the whole account
hierarchy and is a strict superset, so no property is lost.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Cover the 2xx business/display-name path, the terminal non-2xx branches
(401/403/404/500 keep the generic source name), the Square-Version header,
and Google Analytics' empty-account-id short-circuit, matching the existing
resolver tests.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The shared VCR BeforeSave hook scrubs every other header-auth provider's key
(X-Api-Key, Api-Key, Signoz-Api-Key, X-Auth-Token) but was not updated for
Dotfile, so re-recording testdata/dotfile.yaml with a real X-DOTFILE-API-KEY
would persist the key into the committed cassette. Delete the canonicalized
X-Dotfile-Api-Key header alongside the others.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The GA4 driver passed the account and property IDs to url.JoinPath as raw
segments, and the Segment driver built its per-user endpoint by
concatenating the user ID into url.URL.Path — both bypass the url.PathEscape
rule that every sibling driver (and the matching name resolvers) already
follow. The IDs are numeric today so there is no behaviour change, but this
keeps the drivers consistent and safe if a provider ever returns a segment
with a reserved character.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Two OAuth2 and two API-key connectors:
- Google Analytics (GA4): OAuth2 with both analytics.readonly and
analytics.manage.users.readonly (readonly alone 403s on the accounts
list); v1alpha accessBindings enumerated at account and property level
and merged by email; manual account picker (Pattern 1) with a
per-connection probe and name resolver; distinct from Google Workspace.
- Dotfile: API key in the X-DOTFILE-API-KEY header (Pattern 3); GET
/v1/users (owner/admin, suspended_at) with a static probe.
- Segment (Twilio): Public API token as Bearer with a required Region
setting (US or EU) mapped to the regional host; GET /users plus per-user
GET /users/{id} for roles and /invites for pending members; per-connection
BuildProbeURL.
- Square: OAuth2 (EMPLOYEES_READ) or a personal access token (Pattern 3);
POST /v2/team-members/search returns email/status/is_owner directly, so no
role resolution; custom probe and name resolver.
Google Analytics and Square are confidential OAuth clients, wired into the
bootstrap OAuth provider list and .env.example. Segment carries a required
extra setting, so the console add-source dialog maps region onto its
segmentRegion API-key input; without that mapping the value is silently
dropped and the create is rejected.
Cassette-backed driver tests plus unit tests for the Segment probe URL and
the bootstrap OAuth provider list.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The connection-status probe and the access-review driver each had
their own copy of the "try us.posthog.com, then eu.posthog.com"
region-discovery loop, and they drifted. The driver skips a region
that rejects the token (wrong region) and uses the one that answers
2xx; the probe instead returned "credential rejected" on the first
region's 401/403, before ever trying the second.
PostHog Cloud US and EU are separate deployments, so an EU OAuth
token is a 401 on us.posthog.com, which is probed first. The probe
bailed there and marked the source disconnected, while access-review
campaigns -- which use the driver -- kept working.
Delete the probe's copy and delegate to the driver's now-exported
ResolvePostHogRegion, the single resolver the campaign also uses. It
flags a credential every region rejected (ErrPostHogCredentialRejected)
apart from a transient failure on the token's own region, so the probe
marks a source disconnected only for a genuinely dead token and does
not flap on a passing 5xx.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Cut verbose inline comments across the access-review connector changes:
the mechanical fact stays at the read site, while incident backstory
("millions of error logs in prod") and provider-specific rationale (why
Clerk reviews the wrong population) move to the commit history where they
belong. Also tighten a loose "a 4xx" to "an auth/not-found 4xx" so the
terminal-classification contract is not overstated. No behavior change.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Note that Cloudflare requires per_page in 5..50 so nobody optimizes
it back to per_page=1, which returned 400 and, before terminal
classification, produced a 400 storm. Also correct a stale team_id
reference in the Vercel user-fetch comment: the OAuth callback
surfaces the team as teamId.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Org-defaulting for picker providers only ran in the GraphQL resolver,
so a picker-provider source created or updated through the MCP API
connected fine but resolved no users until the org was picked. Move
the defaulting into the accessreview service as
AutoSelectDefaultOrganization and call it from both surfaces, moving
the providerOrgConfigs picker dispatch alongside it (the three console
picker resolvers now dispatch through service accessors, behavior
unchanged).
Also harden the moved path: resolve the provider from cheap connector
metadata before building the authenticated HTTP client, so the ~50
non-picker providers no longer pay a decrypt/refresh/DB-write on every
create/update; bound the outbound ListOrgs call with a 10s timeout so
a hung provider cannot stall the mutation; and re-check inside the
ConfigureAccessReviewSource tx (OnlyIfUnset) so an org the user picks
while ListOrgs is in flight is not overwritten by the first listed
org.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Once the source-name worker sets name_synced_at, nothing cleared it,
so a source that hit a terminal failure (Brex 403, wrong Sentry org
slug) kept its generic name forever — even after the user reconnected
with the right scope or picked the correct org, contradicting the
Brex guidance to reconnect.
Clear name_synced_at whenever the connection changes: on UpdateSource
when a connector is (re)set, inside ConfigureAccessReviewSource when
the org is (re)selected, and on the OAuth reconnect path via the new
ResetSourceNameSyncForConnector service method. The worker then
re-claims the row and re-resolves the display name.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The source-name worker drains claims until ErrNoTask with no
inter-task delay. When connector load or HTTP-client setup failed the
worker logged an error and returned nil without marking the source
synced, leaving name_synced_at NULL. The row stayed claimable, so a
dead connector (notably a revoked OAuth refresh token failing the
eager token refresh) was re-claimed immediately every cycle and hot-
looped the vendor token endpoint.
Treat a setup failure like a terminal resolution failure: keep the
generic name and mark the source synced with a warning so it stops
re-claiming. A later reconnect or reconfigure clears name_synced_at
to retry.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The source-name worker keeps a generic name and marks the source
synced only when a resolver reports ErrTerminalNameResolution. The
Grafana and Metabase name resolvers, plus Tailscale via its shared
fetchUsers, returned a plain error on any non-2xx, so a revoked
credential (401/403) never reached the terminal path and the worker
re-claimed the row every drain cycle with no delay.
Route their non-2xx through nameStatusError so permanent 4xx wrap
ErrTerminalNameResolution while 5xx stay retryable. The sentinel is
inert on the ListAccounts sync path, which does not inspect it.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
