[2 of 2] ENG-3121: Google Workspace integration, identity resolution UI, dynamic consumer scopes#7832
[2 of 2] ENG-3121: Google Workspace integration, identity resolution UI, dynamic consumer scopes#7832galvana wants to merge 4 commits intoplatform-identity-resolutionfrom
Conversation
…ynamic consumer UI - New connection type: google_workspace with GoogleWorkspaceSchema (keyfile + delegation_subject + domain) - GoogleWorkspaceConnector for connection testing - Alembic migration adding google_workspace to ConnectionType enum - IDENTITY_RESOLUTION IntegrationFeature for BigQuery and Google Workspace integrations - Identity Resolution tab on integration pages (enable/disable + test connection) - Identity group provider RTK Query slice (CRUD + test) - Consumer form: dynamic type dropdown (grouped by platform), scope select from available-scopes API - Consumer table: scope column shows primary identifier, removed contact_email and tags columns - useConsumerTypeOptions hook for fetching and grouping dynamic consumer types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
galvana
requested review from
dsill-ethyca and
jpople
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
galvana
requested review from
lucanovera and
thabofletcher
and removed request for
dsill-ethyca and
jpople
galvana
changed the title
galvana
changed the title
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — PR #7832 (Google Workspace integration + Data Consumer scope)
Overall this is a well-structured PR that adds the Google Workspace connector and refactors data consumer types from a static enum to a dynamic API-driven model. The architecture is sound and the separation of concerns is clean. A few issues worth addressing before merge:
Must fix
test_connectionhas no exception handling — raw Google auth exceptions will leak out rather than returningConnectionTestStatus.failed. See inline comment.- Migration filename uses
xx_prefix — non-standard and may break Alembic discovery tooling. See inline comment. providesTags: ["DataConsumer"]ongetAvailableScopes— causes spurious refetches on every consumer mutation. See inline comment.
Should fix
- Lazy imports in
GoogleWorkspaceConnector—from google.oauth2 import service_accountandfrom google.auth.transport.requests import Requestinside method bodies should move to the module top level. - Magic string
"google_workspace"inIdentityResolutionTab.tsx— useConnectionType.GOOGLE_WORKSPACEfrom the generated types enum. - Duplicated scope-display logic —
getDisplayNameForScopein the form and the inlinescope.group_email ?? scope.role ?? ...in the table are the same logic. Extract to a shared util.
Minor
- Hidden
Form.Itemusing<Input />— the pattern works but is fragile; consider tracking scope value in local state and merging at submit instead. PROVIDER_TYPE_OPTIONSexport appears unused — remove or add a comment explaining intent.
|
|
||
| def create_client(self) -> Any: | ||
| config = GoogleWorkspaceSchema(**self.configuration.secrets) | ||
| from google.oauth2 import service_account |
There was a problem hiding this comment.
Lazy imports inside method bodies are discouraged — they make import errors harder to catch at startup and are inconsistent with the rest of the codebase. Move from google.oauth2 import service_account (and the google.auth import in test_connection) to the top of the module alongside the other imports.
| from google.auth.transport.requests import Request as AuthRequest | ||
|
|
||
| creds = self.create_client() | ||
| creds.refresh(AuthRequest()) |
There was a problem hiding this comment.
test_connection has no exception handling. If creds.refresh() raises a Google auth exception (e.g. google.auth.exceptions.TransportError, DefaultCredentialsError), it will propagate as an unhandled exception rather than returning ConnectionTestStatus.failed. Other connectors catch exceptions here and return the failed status. Wrap this in a try/except to conform to the expected interface:
def test_connection(self) -> Optional[ConnectionTestStatus]:
from google.auth.exceptions import GoogleAuthError
from google.auth.transport.requests import Request as AuthRequest
try:
creds = self.create_client()
creds.refresh(AuthRequest())
return ConnectionTestStatus.succeeded
except GoogleAuthError as e:
logger.error("Google Workspace connection test failed: %s", e)
return ConnectionTestStatus.failed| return "google_workspace"; | ||
| } | ||
| return "gcp"; | ||
| }, [integration.connection_type]); |
There was a problem hiding this comment.
Magic string — use ConnectionType.GOOGLE_WORKSPACE from ~/types/api instead of the literal "google_workspace". This avoids a silent mismatch if the value ever changes and makes the intent clear:
import { ConnectionType } from "~/types/api";
const providerType = useMemo(() => {
if (integration.connection_type === ConnectionType.GOOGLE_WORKSPACE) {
return "google_workspace";
}
return "gcp";
}, [integration.connection_type]);There was a problem hiding this comment.
The migration filename starts with xx_, which is non-standard. Other migrations in this repo use the revision hash prefix (e.g. b3c8d5e7f2a1_...). Please rename to match the convention so Alembic can discover it correctly and CI tooling doesn't skip it.
| ): string => { | ||
| if (type === "google_group") { | ||
| return scope.group_email ?? ""; | ||
| } |
There was a problem hiding this comment.
The getDisplayNameForScope function here and the inline scope label logic in useDataConsumersTable.tsx (the scope.group_email ?? scope.role ?? scope.email ?? ... pattern) are effectively the same logic duplicated in two places. Consider extracting this into a shared utility (e.g. in constants.ts or a scopeUtils.ts) so both the form and the table stay in sync if the set of scope key names ever changes.
| <Select | ||
| placeholder="Select scope" | ||
| options={allScopeOptions} | ||
| value={selectedScopeValue} |
There was a problem hiding this comment.
Using a visible <Input /> as the child of a hidden Form.Item whose value is a Record<string, string> is a fragile pattern. Ant Design's Form.Item expects its child to be a compatible control, but here the actual value is set externally via form.setFieldsValue({ scope }) — the <Input /> is a no-op placeholder. If anything ever reads form.getFieldValue("scope") expecting an Input value this will silently return the object. A cleaner approach is to use a custom controlled component or just track the scope value in local state and merge it into the payload at submit time.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (68.42%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## platform-identity-resolution #7832 +/- ##
================================================================
- Coverage 82.94% 82.44% -0.50%
================================================================
Files 624 615 -9
Lines 40498 40099 -399
Branches 4711 4667 -44
================================================================
- Hits 33592 33061 -531
- Misses 5827 5980 +153
+ Partials 1079 1058 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e to mock provider - Mock integration only shown when NEXT_PUBLIC_APP_ENV=development - Identity resolution tab maps test_datastore connection to mock provider type - Updated description text for mock provider Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Ticket ENG-3121
Description Of Changes
Adds Google Workspace as a new integration type for identity group resolution via Google Groups. Adds an "Identity resolution" tab to BigQuery, Google Workspace, and Mock integration pages. Replaces the hardcoded consumer type dropdown with dynamic types from configured identity group providers, and scope fields with selects populated from real groups/roles/service accounts. Mock integration is gated to development environments only.
Code Changes
google_workspaceconnection type withGoogleWorkspaceSchema(keyfile + delegation_subject + domain)GoogleWorkspaceConnectorfor connection testing (credential validation only, no DSR)google_workspaceto the PostgreSQLconnectiontypeenumIDENTITY_RESOLUTIONandQUERY_LOGGINGadded toIntegrationFeatureenumIdentityResolutionTab.tsx) — enable/disable toggle + test connectionGET /plus/pbac/available-scopesuseConsumerTypeOptionshook for fetching and grouping dynamic consumer typestest_datastore) gated toNEXT_PUBLIC_APP_ENV=development, maps tomockprovider typeSteps to Confirm
nox -s "dev(slim)" -- fides-pkg fides-admin-uiPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works