Add Snyk, Ramp, Lever, Deel access-review connectors#1195
Open
aureliensibiril wants to merge 10 commits into
Open
Add Snyk, Ramp, Lever, Deel access-review connectors#1195aureliensibiril wants to merge 10 commits into
aureliensibiril wants to merge 10 commits into
Conversation
Contributor
There was a problem hiding this comment.
3 issues found across 41 files
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/accessreview/drivers/organizations.go">
<violation number="1" location="pkg/accessreview/drivers/organizations.go:40">
P2: The Snyk org picker only fetches a single page (`limit=100`), so users in more than 100 organizations will not see all orgs and may be unable to select the intended one.</violation>
</file>
<file name="pkg/server/api/console/v1/provider_organizations.go">
<violation number="1" location="pkg/server/api/console/v1/provider_organizations.go:118">
P2: Snyk organization loading only fetches one page (`limit=100`) and does not paginate, so tenants with more orgs can get an incomplete org picker.</violation>
</file>
<file name="pkg/accessreview/drivers/ramp.go">
<violation number="1" location="pkg/accessreview/drivers/ramp.go:89">
P2: `IsAdmin` is derived from Ramp `is_manager`, which overstates privilege and can misclassify non-admin users as admins.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| req, err := http.NewRequestWithContext( | ||
| ctx, | ||
| http.MethodGet, | ||
| "https://api.snyk.io/rest/orgs?version=2024-10-15&limit=100", |
Contributor
There was a problem hiding this comment.
P2: The Snyk org picker only fetches a single page (limit=100), so users in more than 100 organizations will not see all orgs and may be unable to select the intended one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/organizations.go, line 40:
<comment>The Snyk org picker only fetches a single page (`limit=100`), so users in more than 100 organizations will not see all orgs and may be unable to select the intended one.</comment>
<file context>
@@ -0,0 +1,83 @@
+ req, err := http.NewRequestWithContext(
+ ctx,
+ http.MethodGet,
+ "https://api.snyk.io/rest/orgs?version=2024-10-15&limit=100",
+ nil,
+ )
</file context>
| req, err := http.NewRequestWithContext( | ||
| ctx, | ||
| http.MethodGet, | ||
| "https://api.snyk.io/rest/orgs?version=2024-10-15&limit=100", |
Contributor
There was a problem hiding this comment.
P2: Snyk organization loading only fetches one page (limit=100) and does not paginate, so tenants with more orgs can get an incomplete org picker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/server/api/console/v1/provider_organizations.go, line 118:
<comment>Snyk organization loading only fetches one page (`limit=100`) and does not paginate, so tenants with more orgs can get an incomplete org picker.</comment>
<file context>
@@ -108,6 +108,62 @@ func fetchSentryOrganizations(ctx context.Context, httpClient *http.Client) ([]*
+ req, err := http.NewRequestWithContext(
+ ctx,
+ http.MethodGet,
+ "https://api.snyk.io/rest/orgs?version=2024-10-15&limit=100",
+ nil,
+ )
</file context>
| FullName: fullName, | ||
| Role: u.Role, | ||
| Active: &active, | ||
| IsAdmin: u.IsManager, |
Contributor
There was a problem hiding this comment.
P2: IsAdmin is derived from Ramp is_manager, which overstates privilege and can misclassify non-admin users as admins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/ramp.go, line 89:
<comment>`IsAdmin` is derived from Ramp `is_manager`, which overstates privilege and can misclassify non-admin users as admins.</comment>
<file context>
@@ -0,0 +1,137 @@
+ FullName: fullName,
+ Role: u.Role,
+ Active: &active,
+ IsAdmin: u.IsManager,
+ ExternalID: u.ID,
+ MFAStatus: coredata.MFAStatusUnknown,
</file context>
PKCE (RFC 7636, S256) is required by some providers added in follow-up commits (Snyk). InitiateWithState generates a verifier when RequiresPKCE is set, embeds it in the signed state, adds code_challenge / code_challenge_method to the authorize URL, and CompleteWithState replays the verifier on the token exchange. TokenExtraParams are merged into the token-exchange body across all three TokenEndpointAuth branches (post-form, basic-form, basic-json). Lever requires an `audience` parameter in both the authorize URL and the token POST; this avoids per-provider branching in the OAuth2 core. The basic-form branch also sets the x-client-id header. Deel rejects token-exchange requests without it even when credentials are correctly Base64-encoded in the Authorization header. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Extend the connector_provider enum with the four new access-review providers, surface them through the GraphQL schema, and migrate the Postgres enum with IF NOT EXISTS so the migration is idempotent on re-runs. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # pkg/coredata/connector_provider.go # pkg/coredata/migrations/20260508T729406Z.sql # pkg/server/api/console/v1/graphql/connector.graphql
Wire the static OAuth2 definitions, probe URLs, and access-review scopes for the four new providers: - Snyk: PKCE-mandatory; scopes target the org and membership reads needed by the access-review driver, plus offline_access. - Ramp: basic-form token auth against api.ramp.com; the `users:read` scope is enough to enumerate users. - Lever: Auth0-backed; requires `audience` in both the authorize URL and the token-exchange body. Deep-copy ExtraAuthParams and TokenExtraParams in ApplyProviderDefaults so per-connector mutations cannot alias back into the shared map. - Deel: distinct auth and API hosts; the token endpoint path is the plural `/oauth2/tokens`. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # pkg/accessreview/drivers/oauth2_scopes.go # pkg/connector/providers.go # pkg/connector/registry.go
Snyk's REST API is JSON:API; users belong to one or more organizations and the operator picks one at connect time. The chosen org id is persisted in SnykConnectorSettings so the driver and the name resolver can scope subsequent calls. The console exposes the picker via fetchSnykOrganizations and the provider extra-settings expose the `orgId` field so the configure flow knows to prompt for an organization. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # pkg/accessreview/access_source_service.go # pkg/accessreview/drivers/organizations.go # pkg/coredata/connector_settings.go # pkg/server/api/console/v1/access_review_campaign_resolvers.go # pkg/server/api/console/v1/provider_organizations.go
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Plug the four new drivers into ReviewEngine.resolveDriver and sourceNameHandler.buildResolver, and register their display names so the source-name worker can prefix instance names consistently. Snyk's case reads its connector settings to surface a clear error when org_id was not configured at connect time. Lever has no dedicated org-name endpoint, so its NameResolver returns an empty string and the operator renames the source manually. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # pkg/accessreview/drivers/name_resolver.go # pkg/accessreview/review_engine.go # pkg/accessreview/source_name_worker.go
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # packages/ui/src/Atoms/ThirdParties/ThirdPartyLogo.tsx # packages/ui/src/Atoms/ThirdParties/index.ts
Read CONNECTOR_<PROVIDER>_CLIENT_ID and CLIENT_SECRET from the environment in probod-bootstrap, surface the variables in the .env.example, and project them through the Helm chart so the deployment template wires up the new access-review connectors the same way the existing ones are wired. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> # Conflicts: # .env.example # pkg/bootstrap/builder.go # pkg/bootstrap/builder_test.go
60d6f7d to
cb20a71
Compare
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
audienceparameter. The basic-form branch also sets thex-client-idheader that Deel requires alongside Basic auth.SnykConnectorSettingsand replayed by the driver and name resolver.NameResolverreturns an empty string and the operator renames the source manually.CONNECTOR_{SNYK,RAMP,LEVER,DEEL}_{CLIENT_ID,CLIENT_SECRET}throughprobod-bootstrap,.env.example, and the Helm chart.Commits
Sequenced so each commit is self-contained:
Add PKCE and TokenExtraParams to OAuth2 connectorAdd SNYK, RAMP, LEVER, DEEL connector providers(coredata enum + Postgres migration + GraphQL schema)Register Snyk, Ramp, Lever, Deel OAuth2 defaults(provider definitions, probe URLs, scopes)Add Snyk access-review driver(driver + settings + console picker + extra-settings)Add Ramp access-review driverAdd Lever access-review driverAdd Deel access-review driverWire Snyk, Ramp, Lever, Deel into review engine and workerAdd Snyk, Ramp, Lever, Deel vendor logosProvision Snyk, Ramp, Lever, Deel connector secretsTest plan
make test MODULE=./pkg/accessreview/drivers/...make test MODULE=./pkg/connector/...make test MODULE=./pkg/bootstrap/...connector_providerenum contains the four new valuesSummary by cubic
Adds four new OAuth2 access-review connectors: Snyk, Ramp, Lever, and Deel. Extends OAuth2 with PKCE (Snyk), token extra params for Lever’s
audience, and thex-client-idheader for Deel; registers provider defaults, scopes, and probe URLs.New Features
SnykConnectorSettingsand used by the driver and name resolver.audiencein both authorize URL and token POST; no org-name endpoint so the resolver returns an empty string.x-client-id.Migration
SNYK,RAMP,LEVER, andDEEL.CONNECTOR_{SNYK,RAMP,LEVER,DEEL}_{CLIENT_ID,CLIENT_SECRET}(Helm templates and.env.exampleupdated; optional_REDIRECT_URIsupported) and re-deploy.Written for commit cb20a71. Summary will update on new commits. Review in cubic