feat: add options to restrict certain namespaces to certain login methods#2876
Open
JivusAyrus wants to merge 10 commits into
Open
feat: add options to restrict certain namespaces to certain login methods#2876JivusAyrus wants to merge 10 commits into
JivusAyrus wants to merge 10 commits into
Conversation
…45-controlplane-different-auth-providers-for-different
JivusAyrus
requested review from
a team,
Aenimus,
StarpTech,
endigma,
thisisnithin and
wilsonrivera
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements IdP-aware namespace access gating: adds LoginMethod/OIDC provider and namespace-SSO proto types and RPCs, DB schema and repositories for namespace SSO mappings, resolves login method from identity_provider claims, computes per-session allowed namespaces, enforces an IdP allowlist in RBAC and queries, and adds admin UI for mappings. ChangesMulti-Provider SSO Namespace Access Gating
🎯 4 (Complex) | ⏱️ ~75 minutes
|
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2876 +/- ##
===========================================
- Coverage 64.47% 40.82% -23.66%
===========================================
Files 319 151 -168
Lines 45359 14630 -30729
Branches 4927 661 -4266
===========================================
- Hits 29245 5972 -23273
+ Misses 16089 8541 -7548
- Partials 25 117 +92 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/bufservices/user/updateOrgMemberGroup.ts (1)
63-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeycloak client authentication happens after the SSO user checks.
The loop at lines 68-85 calls
opts.keycloakClient.client.users.find()for each provider, butopts.keycloakClient.authenticateClient()is only called at line 88, after the loop. This may cause the Keycloak calls inside the loop to fail if the client token isn't already valid.Consider moving the authentication call before the loop:
🔧 Proposed fix
// Ensure that the organization member has not signed in with SSO via any // of the org's configured providers. + await opts.keycloakClient.authenticateClient(); + const providers = await oidcRepo.listOidcProvidersByOrganizationId({ organizationId: authContext.organizationId, }); for (const provider of providers) { // checking if the user has logged in using the sso const ssoUser = await opts.keycloakClient.client.users.find({ realm: opts.keycloakRealm, email: orgMember.email, exact: true, idpAlias: provider.alias, }); if (ssoUser.length > 0) { return { response: { code: EnumStatusCode.ERR, details: 'User has logged in using the OIDC provider. Please update the group using the provider.', }, }; } } // Retrieve the Keycloak user - await opts.keycloakClient.authenticateClient(); const users = await opts.keycloakClient.client.users.find({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/bufservices/user/updateOrgMemberGroup.ts` around lines 63 - 88, The Keycloak client is authenticated after you call opts.keycloakClient.client.users.find() inside the providers loop (after listOidcProvidersByOrganizationId), which can cause those calls to fail; fix it by calling and awaiting opts.keycloakClient.authenticateClient() before entering the for (const provider of providers) loop so the client has a valid token, and keep the existing find calls (client.users.find) unchanged; also propagate or handle any authentication errors from authenticateClient() before proceeding.
🧹 Nitpick comments (2)
controlplane/src/core/services/RBACEvaluator.ts (1)
115-117: 💤 Low valueMinor: Redundant IdP gate check in
canCreateContract.
canCreateFederatedGraphalready callsisAllowedByIdpGate, so the additional check on line 116 is redundant. This doesn't cause incorrect behavior but adds unnecessary computation.♻️ Suggested simplification
canCreateContract(namespace: Namespace) { - return this.canCreateFederatedGraph(namespace) && this.isAllowedByIdpGate(namespace.id); + return this.canCreateFederatedGraph(namespace); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/services/RBACEvaluator.ts` around lines 115 - 117, canCreateContract currently performs an extra isAllowedByIdpGate check despite canCreateFederatedGraph already invoking isAllowedByIdpGate; remove the redundant check from canCreateContract so it simply returns the result of canCreateFederatedGraph(namespace) to avoid duplicate computation while preserving behavior (refer to the canCreateContract, canCreateFederatedGraph, and isAllowedByIdpGate symbols and the Namespace parameter).controlplane/src/core/bufservices/organization/whoAmI.ts (1)
4-9: 💤 Low valueStatic analysis false positive for generated protobuf types.
The linter flags
LoginMethod_Typefor not being in camelCase, but this identifier is auto-generated from the protobuf definitions and cannot be renamed here. Consider adding a lint suppression comment if this causes CI failures, or configure the linter to ignore generated import identifiers.+// eslint-disable-next-line `@typescript-eslint/naming-convention` import { LoginMethod, LoginMethod_Type,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/bufservices/organization/whoAmI.ts` around lines 4 - 9, The import brings in auto-generated protobuf identifiers like LoginMethod_Type which trigger the linter's camelCase rule; add a local lint suppression so the generated name is allowed: place an ESLint/TSLint disable comment immediately above the import line (targeting the naming-convention/camelcase rule) or add a one-line disable for the specific rule, referencing the imported symbol LoginMethod_Type to make it clear why the rule is disabled; ensure the comment is minimal and scoped (not file-wide) so only the generated import is exempted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/src/core/bufservices/sso/deleteOIDCProvider.ts`:
- Around line 46-53: Move the request validation for req.id to run before any
Keycloak client authentication/initialization so invalid input returns
ERR_BAD_REQUEST deterministically; specifically, in deleteOIDCProvider (and
before any calls to getKeycloakClient / keycloakAdminClient.authenticate or
usage of variables like keycloakClient/keycloakAdminClient), check if (!req.id)
and return the ERR_BAD_REQUEST response immediately, then proceed to
create/authenticate the Keycloak client only after validation succeeds.
In `@controlplane/src/core/repositories/NamespaceSsoMappingRepository.ts`:
- Around line 70-81: The getMapping method is declared async but doesn't await
the database promise; remove the unnecessary async keyword from the getMapping
declaration (or alternatively add an await before
this.db.select(...).from(...).where(...).execute()) so the function either
returns the promise directly or properly awaits it; update the getMapping
signature and keep the existing query body referencing namespaceSsoProviders and
eq(namespaceSsoProviders.namespaceId, input.namespaceId).
In `@controlplane/src/core/repositories/OidcRepository.ts`:
- Around line 32-39: The method listOidcProvidersByOrganizationId is declared
async but does not use await; remove the unnecessary async modifier (or
alternatively await the DB promise) to fix the lint warning—update the method
signature in OidcRepository (and mirror the same fix pattern used in
NamespaceSsoMappingRepository) so it returns the DB query promise directly
without async, or add an await before this.db.select(...).execute() if you
prefer an awaited result.
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 1896-1898: Update the GetOIDCProviderRequest (and analogous
messages at the other locations) to make the id field optional (i.e., mark it as
optional/allow empty) in the proto definition for message GetOIDCProviderRequest
and the similar messages at lines noted, then change the server handlers
GetOIDCProvider, DeleteOIDCProvider, and UpdateIDPMappers to tolerate a missing
id by implementing fallback resolution: if request.id is empty, try to infer the
provider id from the authenticated organization context (or from a single
default provider for that org) and only return a clear ERR_BAD_REQUEST when both
the id is missing and no unique provider can be resolved; ensure validation
error messages explicitly state when an id is required versus when a provider
could not be inferred.
- Around line 3100-3104: Change the UpdateNamespaceSSOMappingRequest proto so
allow_password_login is declared as "optional bool allow_password_login = 3;"
instead of plain bool, and then update the handler that processes
UpdateNamespaceSSOMappingRequest to check the presence of allow_password_login
(i.e., whether it is set/defined) before calling setMapping(); only pass a value
to setMapping() when the field is present (treat absent as “no change”), and
preserve existing behavior for explicit true/false inputs so setMapping()
continues to be called with explicit values when provided.
In `@studio/src/components/dashboard/namespace-selector.tsx`:
- Around line 91-100: The trigger hides the namespace label when
isViewingGraphOrSubgraph is true, which leaves only the caret visible even when
hasNoAccess is true; update the render logic in namespace-selector.tsx so that
when hasNoAccess is true you still render displayName (e.g., "No access" or the
existing displayName) even if isViewingGraphOrSubgraph is true—adjust the
conditional around the span that uses truncateNamespace, hasNoAccess and
displayName so it renders whenever hasNoAccess is true OR
isViewingGraphOrSubgraph is false, and ensure the className still applies
hasNoAccess styling.
In `@studio/src/components/user-menu.tsx`:
- Line 85: The SSO label currently uses loginMethod.ssoProviderName ||
loginMethod.ssoAlias || '' which can produce an empty string; update the logic
that sets ssoLabel (and the similar logic around lines 108-111) to provide a
fallback like 'SSO' instead of ''. For example, change the expression that
defines ssoLabel in component user-menu.tsx to use loginMethod.ssoProviderName
|| loginMethod.ssoAlias || 'SSO' (and apply the same fallback where the other
SSO display label is computed) so the UI never renders "Logged in via " with a
blank provider.
---
Outside diff comments:
In `@controlplane/src/core/bufservices/user/updateOrgMemberGroup.ts`:
- Around line 63-88: The Keycloak client is authenticated after you call
opts.keycloakClient.client.users.find() inside the providers loop (after
listOidcProvidersByOrganizationId), which can cause those calls to fail; fix it
by calling and awaiting opts.keycloakClient.authenticateClient() before entering
the for (const provider of providers) loop so the client has a valid token, and
keep the existing find calls (client.users.find) unchanged; also propagate or
handle any authentication errors from authenticateClient() before proceeding.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/organization/whoAmI.ts`:
- Around line 4-9: The import brings in auto-generated protobuf identifiers like
LoginMethod_Type which trigger the linter's camelCase rule; add a local lint
suppression so the generated name is allowed: place an ESLint/TSLint disable
comment immediately above the import line (targeting the
naming-convention/camelcase rule) or add a one-line disable for the specific
rule, referencing the imported symbol LoginMethod_Type to make it clear why the
rule is disabled; ensure the comment is minimal and scoped (not file-wide) so
only the generated import is exempted.
In `@controlplane/src/core/services/RBACEvaluator.ts`:
- Around line 115-117: canCreateContract currently performs an extra
isAllowedByIdpGate check despite canCreateFederatedGraph already invoking
isAllowedByIdpGate; remove the redundant check from canCreateContract so it
simply returns the result of canCreateFederatedGraph(namespace) to avoid
duplicate computation while preserving behavior (refer to the canCreateContract,
canCreateFederatedGraph, and isAllowedByIdpGate symbols and the Namespace
parameter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 300c6e7a-cd0b-4e45-bdb0-6e81f7d163be
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (71)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0139_nappy_fallen_one.sqlcontrolplane/migrations/meta/0139_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/auth-utils.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/analytics/getAnalyticsView.tscontrolplane/src/core/bufservices/analytics/getDashboardAnalyticsView.tscontrolplane/src/core/bufservices/analytics/getFieldUsage.tscontrolplane/src/core/bufservices/analytics/getGraphMetrics.tscontrolplane/src/core/bufservices/analytics/getMetricsErrorRate.tscontrolplane/src/core/bufservices/analytics/getOperationClients.tscontrolplane/src/core/bufservices/analytics/getOperationContent.tscontrolplane/src/core/bufservices/analytics/getOperationDeprecatedFields.tscontrolplane/src/core/bufservices/analytics/getOperations.tscontrolplane/src/core/bufservices/analytics/getTrace.tscontrolplane/src/core/bufservices/cache-warmer/computeCacheWarmerOperations.tscontrolplane/src/core/bufservices/cache-warmer/configureCacheWarmer.tscontrolplane/src/core/bufservices/feature-flag/getFeatureFlags.tscontrolplane/src/core/bufservices/federated-graph/getClientsFromAnalytics.tscontrolplane/src/core/bufservices/federated-graph/getCompositionDetails.tscontrolplane/src/core/bufservices/federated-graph/migrateFromApollo.tscontrolplane/src/core/bufservices/federated-graph/moveFederatedGraph.tscontrolplane/src/core/bufservices/namespace/getNamespaceSSOMapping.tscontrolplane/src/core/bufservices/namespace/updateNamespaceSSOMapping.tscontrolplane/src/core/bufservices/organization/deleteOrganizationGroup.tscontrolplane/src/core/bufservices/organization/getOrganizationGroups.tscontrolplane/src/core/bufservices/organization/whoAmI.tscontrolplane/src/core/bufservices/persisted-operation/getClients.tscontrolplane/src/core/bufservices/persisted-operation/getPersistedOperations.tscontrolplane/src/core/bufservices/proposal/getNamespaceProposalConfig.tscontrolplane/src/core/bufservices/proposal/getProposal.tscontrolplane/src/core/bufservices/proposal/getProposalChecks.tscontrolplane/src/core/bufservices/schema-version/getChangelogBySchemaVersion.tscontrolplane/src/core/bufservices/sso/createOIDCProvider.tscontrolplane/src/core/bufservices/sso/deleteOIDCProvider.tscontrolplane/src/core/bufservices/sso/getOIDCProvider.tscontrolplane/src/core/bufservices/sso/listOIDCProviders.tscontrolplane/src/core/bufservices/sso/updateIDPMappers.tscontrolplane/src/core/bufservices/user/updateOrgMemberGroup.tscontrolplane/src/core/build-server.tscontrolplane/src/core/controllers/auth.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/GraphCompositionRepository.tscontrolplane/src/core/repositories/NamespaceRepository.tscontrolplane/src/core/repositories/NamespaceSsoMappingRepository.tscontrolplane/src/core/repositories/OidcRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/repositories/UserRepository.tscontrolplane/src/core/services/AccessTokenAuthenticator.tscontrolplane/src/core/services/ApiKeyAuthenticator.tscontrolplane/src/core/services/Authentication.tscontrolplane/src/core/services/Authorization.tscontrolplane/src/core/services/RBACEvaluator.tscontrolplane/src/core/services/WebSessionAuthenticator.tscontrolplane/src/core/workers/DeleteOrganizationWorker.tscontrolplane/src/db/models.tscontrolplane/src/db/schema.tscontrolplane/src/types/index.tsdocker/keycloak/realm.jsonproto/wg/cosmo/platform/v1/platform.protostudio/src/components/app-provider.tsxstudio/src/components/dashboard/namespace-gate-empty-state.tsxstudio/src/components/dashboard/namespace-selector.tsxstudio/src/components/layout/dashboard-layout.tsxstudio/src/components/user-menu.tsxstudio/src/pages/[organizationSlug]/namespace-sso.tsxstudio/src/pages/[organizationSlug]/settings.tsx
💤 Files with no reviewable changes (1)
- controlplane/src/core/bufservices/sso/createOIDCProvider.ts
Comment on lines
+46
to
+53
| if (!req.id) { | ||
| return { | ||
| response: { | ||
| code: EnumStatusCode.ERR_BAD_REQUEST, | ||
| details: 'Provider id is required', | ||
| }, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Move request validation before Keycloak client authentication.
The new req.id validation is good, but it currently executes after external Keycloak auth. On invalid input, this can fail as an infra error instead of returning deterministic ERR_BAD_REQUEST.
Suggested patch
- await opts.keycloakClient.authenticateClient();
-
- const organization = await orgRepo.byId(authContext.organizationId);
- if (!organization) {
- return {
- response: {
- code: EnumStatusCode.ERR_NOT_FOUND,
- details: `Organization not found`,
- },
- };
- }
-
if (!req.id) {
return {
response: {
code: EnumStatusCode.ERR_BAD_REQUEST,
details: 'Provider id is required',
},
};
}
+
+ await opts.keycloakClient.authenticateClient();
+
+ const organization = await orgRepo.byId(authContext.organizationId);
+ if (!organization) {
+ return {
+ response: {
+ code: EnumStatusCode.ERR_NOT_FOUND,
+ details: `Organization not found`,
+ },
+ };
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controlplane/src/core/bufservices/sso/deleteOIDCProvider.ts` around lines 46
- 53, Move the request validation for req.id to run before any Keycloak client
authentication/initialization so invalid input returns ERR_BAD_REQUEST
deterministically; specifically, in deleteOIDCProvider (and before any calls to
getKeycloakClient / keycloakAdminClient.authenticate or usage of variables like
keycloakClient/keycloakAdminClient), check if (!req.id) and return the
ERR_BAD_REQUEST response immediately, then proceed to create/authenticate the
Keycloak client only after validation succeeds.
Comment on lines
+3100
to
+3104
| message UpdateNamespaceSSOMappingRequest { | ||
| string namespace_id = 1; | ||
| repeated string allowed_sso_provider_ids = 2; | ||
| bool allow_password_login = 3; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify update semantics in handler/repository for omitted allow_password_login.
rg -n -C3 'UpdateNamespaceSSOMapping|allow_password_login' controlplane/src/core/bufservices/namespace controlplane/src/core/repositories controlplane/src/core/servicesRepository: wundergraph/cosmo
Length of output: 2492
🏁 Script executed:
cat -n controlplane/src/core/bufservices/namespace/updateNamespaceSSOMapping.tsRepository: wundergraph/cosmo
Length of output: 4351
🏁 Script executed:
rg -n -A20 'allow_password_login' controlplane/src/core/repositoriesRepository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
cat -n proto/wg/cosmo/platform/v1/platform.proto | sed -n '3095,3110p'Repository: wundergraph/cosmo
Length of output: 533
🏁 Script executed:
rg -n -A15 'setMapping' controlplane/src/core/repositories/NamespaceSsoMappingRepository.tsRepository: wundergraph/cosmo
Length of output: 739
🏁 Script executed:
rg -n 'class NamespaceSsoMappingRepository' controlplane/src/core/repositories/Repository: wundergraph/cosmo
Length of output: 176
🏁 Script executed:
rg -n -A30 'async setMapping' controlplane/src/core/repositories/NamespaceSsoMappingRepository.tsRepository: wundergraph/cosmo
Length of output: 1000
Use optional bool to prevent unintended password login disabling on updates.
Currently, when a client sends an UpdateNamespaceSSOMappingRequest without specifying allow_password_login, proto3 deserializes the field to false (default), and the handler unconditionally passes this to setMapping(). The repository then deletes all existing SSO provider mappings and does not add a password login row, silently disabling password login even when the client never intended to change that setting.
With optional bool, omitted fields will be undefined, allowing the handler to distinguish "not specified" (no change) from "explicitly false" (disable password login). This is essential for safe update semantics.
Proposed proto adjustment
message UpdateNamespaceSSOMappingRequest {
string namespace_id = 1;
repeated string allowed_sso_provider_ids = 2;
- bool allow_password_login = 3;
+ optional bool allow_password_login = 3;
}The handler must also be updated to check field presence before passing the value to setMapping().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| message UpdateNamespaceSSOMappingRequest { | |
| string namespace_id = 1; | |
| repeated string allowed_sso_provider_ids = 2; | |
| bool allow_password_login = 3; | |
| } | |
| message UpdateNamespaceSSOMappingRequest { | |
| string namespace_id = 1; | |
| repeated string allowed_sso_provider_ids = 2; | |
| optional bool allow_password_login = 3; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 3100 - 3104, Change
the UpdateNamespaceSSOMappingRequest proto so allow_password_login is declared
as "optional bool allow_password_login = 3;" instead of plain bool, and then
update the handler that processes UpdateNamespaceSSOMappingRequest to check the
presence of allow_password_login (i.e., whether it is set/defined) before
calling setMapping(); only pass a value to setMapping() when the field is
present (treat absent as “no change”), and preserve existing behavior for
explicit true/false inputs so setMapping() continues to be called with explicit
values when provided.
Member
Author
There was a problem hiding this comment.
that is correct by default it has to be false. We create an entry only when its true so it correct right? check updateNamespaceSSOMapping.ts
Comment on lines
91
to
100
| {!isViewingGraphOrSubgraph && ( | ||
| <span className={cn(truncateNamespace && 'max-w-[180px] truncate lg:max-w-xs')}>{namespace.name}</span> | ||
| <span | ||
| className={cn( | ||
| truncateNamespace && 'max-w-[180px] truncate lg:max-w-xs', | ||
| hasNoAccess && 'text-muted-foreground', | ||
| )} | ||
| > | ||
| {displayName} | ||
| </span> | ||
| )} |
Contributor
There was a problem hiding this comment.
Show a visible label when access is gated in graph/subgraph view.
When isViewingGraphOrSubgraph is true and hasNoAccess is true, the trigger renders only the caret icon. Please render displayName in that case too (e.g., “No access”) so users understand the state immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@studio/src/components/dashboard/namespace-selector.tsx` around lines 91 -
100, The trigger hides the namespace label when isViewingGraphOrSubgraph is
true, which leaves only the caret visible even when hasNoAccess is true; update
the render logic in namespace-selector.tsx so that when hasNoAccess is true you
still render displayName (e.g., "No access" or the existing displayName) even if
isViewingGraphOrSubgraph is true—adjust the conditional around the span that
uses truncateNamespace, hasNoAccess and displayName so it renders whenever
hasNoAccess is true OR isViewingGraphOrSubgraph is false, and ensure the
className still applies hasNoAccess styling.
|
|
||
| const loginMethod = user.loginMethod; | ||
| const hasInvitations = user.invitations.length > 0; | ||
| const ssoLabel = loginMethod?.type === 'sso' ? loginMethod.ssoProviderName || loginMethod.ssoAlias || '' : ''; |
Contributor
There was a problem hiding this comment.
Add a fallback label for SSO display text.
If both ssoProviderName and ssoAlias are missing, this renders as “Logged in via ”. Use a fallback (e.g., SSO) to avoid an empty label.
Also applies to: 108-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@studio/src/components/user-menu.tsx` at line 85, The SSO label currently uses
loginMethod.ssoProviderName || loginMethod.ssoAlias || '' which can produce an
empty string; update the logic that sets ssoLabel (and the similar logic around
lines 108-111) to provide a fallback like 'SSO' instead of ''. For example,
change the expression that defines ssoLabel in component user-menu.tsx to use
loginMethod.ssoProviderName || loginMethod.ssoAlias || 'SSO' (and apply the same
fallback where the other SSO display label is computed) so the UI never renders
"Logged in via " with a blank provider.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/bufservices/organization/whoAmI.ts (1)
5-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ESLint camelcase error for protobuf-generated import.
The pipeline is failing because
LoginMethod_Typeviolates the camelcase rule. Since this is a protobuf-generated identifier that cannot be renamed, add an ESLint disable comment for this import.🔧 Proposed fix
import { LoginMethod, + // eslint-disable-next-line camelcase LoginMethod_Type, WhoAmIRequest, WhoAmIResponse, } from '`@wundergraph/cosmo-connect/dist/platform/v1/platform_pb`';Alternatively, disable inline at each usage site if the import-level disable doesn't work:
// eslint-disable-next-line camelcase type: LoginMethod_Type.LOGIN_METHOD_TYPE_SSO,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/bufservices/organization/whoAmI.ts` around lines 5 - 6, The import of the protobuf-generated identifier LoginMethod_Type violates ESLint's camelcase rule; fix it by adding an inline ESLint disable comment for camelcase on the import line that brings in LoginMethod_Type (i.e., adjust the import that includes LoginMethod and LoginMethod_Type to include "// eslint-disable-next-line camelcase" immediately above it) so the generated identifier is allowed without renaming; if that import-level disable doesn't resolve linting, alternatively add "// eslint-disable-next-line camelcase" at the specific usage sites where LoginMethod_Type is referenced (e.g., where type: LoginMethod_Type.LOGIN_METHOD_TYPE_SSO is used).
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/organization/whoAmI.ts (1)
44-88: 💤 Low valueInconsistent formatting in switch cases.
The switch statement has unusual blank lines before
break;statements (lines 57-58, 67-68, 77-78). This appears to be inconsistent formatting that should be cleaned up for readability.✨ Suggested formatting
case 'sso': { const oidcRepo = new OidcRepository(opts.db); const provider = await oidcRepo.getOidcProviderById({ id: lm.ssoProviderId, organizationId: authContext.organizationId, }); loginMethod = { type: LoginMethod_Type.LOGIN_METHOD_TYPE_SSO, ssoProviderId: lm.ssoProviderId, ssoProviderName: provider?.name ?? '', ssoAlias: lm.alias, }; - break; } case 'password': { loginMethod = { type: LoginMethod_Type.LOGIN_METHOD_TYPE_PASSWORD, ssoProviderId: '', ssoProviderName: '', ssoAlias: '', }; - break; } case 'api-key': { loginMethod = { type: LoginMethod_Type.LOGIN_METHOD_TYPE_API_KEY, ssoProviderId: '', ssoProviderName: '', ssoAlias: '', }; - break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/bufservices/organization/whoAmI.ts` around lines 44 - 88, The switch on lm?.type in whoAmI.ts (cases handling 'sso', 'password', 'api-key', and default that set loginMethod using LoginMethod_Type and OidcRepository/provider) contains stray blank lines immediately before each break; remove those blank lines so each case ends directly with the break statement, ensure consistent spacing/indentation within each case (no extra vertical gaps), and run the project formatter to normalize the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controlplane/src/core/bufservices/organization/whoAmI.ts`:
- Around line 5-6: The import of the protobuf-generated identifier
LoginMethod_Type violates ESLint's camelcase rule; fix it by adding an inline
ESLint disable comment for camelcase on the import line that brings in
LoginMethod_Type (i.e., adjust the import that includes LoginMethod and
LoginMethod_Type to include "// eslint-disable-next-line camelcase" immediately
above it) so the generated identifier is allowed without renaming; if that
import-level disable doesn't resolve linting, alternatively add "//
eslint-disable-next-line camelcase" at the specific usage sites where
LoginMethod_Type is referenced (e.g., where type:
LoginMethod_Type.LOGIN_METHOD_TYPE_SSO is used).
---
Nitpick comments:
In `@controlplane/src/core/bufservices/organization/whoAmI.ts`:
- Around line 44-88: The switch on lm?.type in whoAmI.ts (cases handling 'sso',
'password', 'api-key', and default that set loginMethod using LoginMethod_Type
and OidcRepository/provider) contains stray blank lines immediately before each
break; remove those blank lines so each case ends directly with the break
statement, ensure consistent spacing/indentation within each case (no extra
vertical gaps), and run the project formatter to normalize the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3529a86f-cc01-4aa6-934b-722a4f739841
📒 Files selected for processing (4)
controlplane/src/core/bufservices/organization/whoAmI.tsstudio/src/components/user-menu.tsxstudio/src/pages/[organizationSlug]/namespace-sso.tsxstudio/src/pages/[organizationSlug]/settings.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/src/core/services/RBACEvaluator.ts (1)
115-117: 💤 Low valueRedundant IdP gate check in
canCreateContract.
canCreateFederatedGraphalready includes theisAllowedByIdpGatecheck, making the additional call here redundant.♻️ Suggested simplification
canCreateContract(namespace: Namespace) { - return this.canCreateFederatedGraph(namespace) && this.isAllowedByIdpGate(namespace.id); + return this.canCreateFederatedGraph(namespace); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/services/RBACEvaluator.ts` around lines 115 - 117, The canCreateContract method redundantly calls isAllowedByIdpGate because canCreateFederatedGraph already performs that check; simplify canCreateContract by returning only the result of canCreateFederatedGraph(namespace) (keep the Namespace parameter) and remove the extra isAllowedByIdpGate(namespace.id) invocation so the logic isn’t duplicated in canCreateContract, referencing the canCreateContract, canCreateFederatedGraph, and isAllowedByIdpGate symbols for location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controlplane/src/core/services/RBACEvaluator.ts`:
- Around line 115-117: The canCreateContract method redundantly calls
isAllowedByIdpGate because canCreateFederatedGraph already performs that check;
simplify canCreateContract by returning only the result of
canCreateFederatedGraph(namespace) (keep the Namespace parameter) and remove the
extra isAllowedByIdpGate(namespace.id) invocation so the logic isn’t duplicated
in canCreateContract, referencing the canCreateContract,
canCreateFederatedGraph, and isAllowedByIdpGate symbols for location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b408b318-6432-4fd9-9105-909569ce3f6e
📒 Files selected for processing (5)
controlplane/src/core/bufservices/federated-graph/moveFederatedGraph.tscontrolplane/src/core/bufservices/organization/whoAmI.tscontrolplane/src/core/bufservices/sso/listOIDCProviders.tscontrolplane/src/core/services/RBACEvaluator.tscontrolplane/src/db/schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/test/oidc-provider.test.ts`:
- Around line 42-45: Replace the non-deterministic providers[0] selection with a
deterministic lookup: after calling client.listOIDCProviders use
Array.prototype.find to locate the provider by a known unique attribute (e.g.,
provider.name or createdProvider.id) and assert it exists, then pass that
provider.id into client.getOIDCProvider; update the same pattern for the other
occurrences (the blocks that call client.listOIDCProviders and then index
providers) so each test selects by name or by the id returned when creating the
provider rather than using providers[0].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c54a7ace-fd6a-4e38-9e0c-6a8b77a832aa
📒 Files selected for processing (1)
controlplane/test/oidc-provider.test.ts
Comment on lines
+42
to
+45
| const { providers } = await client.listOIDCProviders({}); | ||
| const providerId = providers[0].id; | ||
|
|
||
| const getOIDCProviderResponse = await client.getOIDCProvider({ id: providerId }); |
Contributor
There was a problem hiding this comment.
Use deterministic provider selection instead of providers[0].
At Line 42 (and the other listed ranges), indexing the first result assumes stable ordering and non-empty data; this can make tests flaky as data/setup evolves. Prefer selecting by known provider name (or created id if returned) and assert it exists.
Suggested pattern
const { providers } = await client.listOIDCProviders({});
-const providerId = providers[0].id;
+const providerId = providers.find((p) => p.name === 'okta')?.id;
+expect(providerId).toBeTruthy();Also applies to: 144-147, 221-224, 271-274, 329-332, 427-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controlplane/test/oidc-provider.test.ts` around lines 42 - 45, Replace the
non-deterministic providers[0] selection with a deterministic lookup: after
calling client.listOIDCProviders use Array.prototype.find to locate the provider
by a known unique attribute (e.g., provider.name or createdProvider.id) and
assert it exists, then pass that provider.id into client.getOIDCProvider; update
the same pattern for the other occurrences (the blocks that call
client.listOIDCProviders and then index providers) so each test selects by name
or by the id returned when creating the provider rather than using providers[0].
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
1 participant
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 by CodeRabbit
New Features
Security Improvements
UI/UX Enhancements
Chores
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.