Add SSO group mapping from IdP claims to ICP groups - #780
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis change adds SSO-only login controls, configurable SSO administrator claims, federated group mappings, effective membership storage and RBAC views, scoped mapping APIs, frontend mapping management, membership-source indicators, database upgrade scripts, v1-to-v2 migration scripts, and related tests. Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant AuthService
participant OIDCProvider
participant Database
User->>Frontend: Select SSO sign-in
Frontend->>AuthService: Start OIDC login
AuthService->>OIDCProvider: Authenticate and receive claims
AuthService->>Database: Reconcile federated memberships
Database-->>AuthService: Effective memberships
AuthService-->>Frontend: Issue authenticated session
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (14)
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql (1)
5-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider idempotent creates for consistency with the H2 migration script.
add_sso_group_mapping_tables_h2.sqlusesCREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS, while this script uses unguardedCREATE TABLE. A re-run or partial-failure retry aborts here. MySQL supportsIF NOT EXISTSonCREATE TABLE, so the two migration scripts can behave the same way.🤖 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 `@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql` around lines 5 - 48, Make the MySQL migration idempotent by adding IF NOT EXISTS to both CREATE TABLE statements, sso_group_mappings and federated_group_user_mapping, matching the H2 migration behavior and allowing safe reruns after partial failures.frontend/src/components/LoginForm.tsx (1)
153-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme tokens instead of hardcoded colors.
#1e1e1e,#333, and#cccare hardcoded in thesxprops of both buttons. As per coding guidelines, "Use theme tokens with thesxprop instead of hardcoded colors and spacing values".♻️ Proposed change
- sx={{ mt: 1, bgcolor: '`#1e1e1e`', '&:hover': { bgcolor: '`#333`' }, textTransform: 'none', py: 1.2 }} + sx={{ mt: 1, bgcolor: 'common.black', '&:hover': { bgcolor: 'grey.800' }, textTransform: 'none', py: 1.2 }}- sx={{ textTransform: 'none', py: 1.2, borderColor: '`#ccc`', color: 'text.primary' }} + sx={{ textTransform: 'none', py: 1.2, borderColor: 'divider', color: 'text.primary' }}🤖 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 `@frontend/src/components/LoginForm.tsx` around lines 153 - 176, Update the `sx` props on the password and SSO buttons in `LoginForm` to replace hardcoded colors `#1e1e1e`, `#333`, and `#ccc` with appropriate MUI theme palette tokens, preserving the existing button appearance and hover behavior.Source: Coding guidelines
frontend/src/pages/AccessControl.tsx (2)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile
slice(0, N)coupling to array order.
ORG_TABS.slice(0, 3)andPROJECT_TABS.slice(0, 2)assume'sso-mappings'stays the last element of each tuple; reordering either constant later would silently drop the wrong tab instead of failing to compile.♻️ Filter by name instead of position
- const orgTabs: readonly string[] = window.API_CONFIG.ssoEnabled ? ORG_TABS : ORG_TABS.slice(0, 3); + const orgTabs: readonly string[] = window.API_CONFIG.ssoEnabled ? ORG_TABS : ORG_TABS.filter((t) => t !== 'sso-mappings');Apply the equivalent change for
PROJECT_TABS.slice(0, 2).Also applies to: 54-54, 106-106, 167-168, 215-216
🤖 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 `@frontend/src/pages/AccessControl.tsx` around lines 33 - 34, Replace the positional ORG_TABS.slice(0, 3) and PROJECT_TABS.slice(0, 2) usage with name-based filtering that excludes 'sso-mappings'. Apply this consistently at all referenced usages while preserving the existing tab ordering and behavior for the remaining tabs.
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent scrollable-tabs behavior across scopes.
Org-level tabs (lines 71, 123) use
variant="scrollable" scrollButtons="auto", but project/component-level tabs (lines 184, 244) don't, despite both now carrying an extra SSO tab that can overflow on narrow viewports.Also applies to: 123-123, 184-184, 244-244
🤖 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 `@frontend/src/pages/AccessControl.tsx` at line 71, Make the tab configuration consistent across scopes by adding variant="scrollable" and scrollButtons="auto" to the project- and component-level Tabs instances near the existing org-level configurations. Update the Tabs elements at all four referenced locations so the extra SSO tab remains accessible on narrow viewports while preserving their current navigation and value behavior.frontend/src/pages/access-control/SSOMappingsTab.tsx (2)
71-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEditable Issuer field risks silent mapping failures.
The Issuer field defaults from
window.API_CONFIG.ssoIssuerbut remains freely editable; since mapping matching inoidc.balrequires an exact string match against the token'sissclaim, a typo here causes the mapping to silently never apply, with no immediate feedback. Please confirm whether multiple issuers are actually supported; if not, consider making this field read-only.Also applies to: 107-107
🤖 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 `@frontend/src/pages/access-control/SSOMappingsTab.tsx` around lines 71 - 76, Confirm whether SSO mappings support multiple issuers; if they do not, update the issuer input in SSOMappingsTab to be read-only while retaining the window.API_CONFIG.ssoIssuer default and submitted value. If multiple issuers are supported, preserve editability and add appropriate validation against accepted issuer values before submission.
178-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo search or pagination, unlike sibling list tabs.
UsersTab,GroupsTab, andRolesTaball provide search plus pagination for their lists; this table renders the full mapping list unbounded, which is inconsistent and may not scale for orgs with many mappings.🤖 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 `@frontend/src/pages/access-control/SSOMappingsTab.tsx` around lines 178 - 273, Update the SSOMappingsTab mapping list flow around the mappings rendering to add search and pagination, matching the established behavior and controls used by UsersTab, GroupsTab, and RolesTab. Filter mappings by the relevant claim, issuer, or group fields, paginate the filtered results, and render the paginated collection in both mobile cards and the desktop ListingTable while preserving empty-state and delete-action behavior.icp_server/modules/storage/user_repository.bal (1)
154-194: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce redundant EXISTS subqueries and duplicate invocation cost.
The new query evaluates the same manual/federated membership check twice per group row (once in the
CASE, once inWHERE) via correlatedEXISTS. ALEFT JOINcomputes both flags once and reuses them. This also compounds with the existing pattern elsewhere in this file wheregetGroupsForUseris called twice per user (once forgroups, once forgroupCount, e.g. ingetAllUsersV2), so the added subquery cost is paid twice per user in that list endpoint.⚡ Suggested query rewrite using LEFT JOIN
- stream<record {|string group_id; string group_name; string description?; string membership_source;|}, sql:Error?> groupStream = dbClient->query( - `SELECT g.group_id, g.group_name, g.description, - CASE - WHEN EXISTS ( - SELECT 1 FROM group_user_mapping gum - WHERE gum.user_uuid = ${userId} AND gum.group_id = g.group_id - ) AND EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) THEN 'manual_and_federated' - WHEN EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) THEN 'federated' - ELSE 'manual' - END AS membership_source - FROM user_groups g - WHERE EXISTS ( - SELECT 1 FROM group_user_mapping gum - WHERE gum.user_uuid = ${userId} AND gum.group_id = g.group_id - ) OR EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) - ORDER BY g.group_name ASC` - ); + stream<record {|string group_id; string group_name; string description?; string membership_source;|}, sql:Error?> groupStream = dbClient->query( + `SELECT g.group_id, g.group_name, g.description, + CASE + WHEN gum.user_uuid IS NOT NULL AND fgm.user_uuid IS NOT NULL THEN 'manual_and_federated' + WHEN fgm.user_uuid IS NOT NULL THEN 'federated' + ELSE 'manual' + END AS membership_source + FROM user_groups g + LEFT JOIN group_user_mapping gum ON gum.group_id = g.group_id AND gum.user_uuid = ${userId} + LEFT JOIN federated_group_user_mapping fgm ON fgm.group_id = g.group_id AND fgm.user_uuid = ${userId} + WHERE gum.user_uuid IS NOT NULL OR fgm.user_uuid IS NOT NULL + ORDER BY g.group_name ASC` + );🤖 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 `@icp_server/modules/storage/user_repository.bal` around lines 154 - 194, Update getGroupsForUser so each group’s manual and federated membership status is computed once through LEFT JOIN-derived flags, then reuse those flags for both filtering and membership_source classification instead of repeating correlated EXISTS subqueries. Preserve the current group results, ordering, and membership_source values.icp_server/tests/sso_config_validation_tests.bal (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also covering an empty
adminValueslist.
testSSOOnlyWithoutAdminClaimIsRejectedcovers an emptyadminClaim, but an SSO-only deployment configured withadminClaimset andadminValues = []would leave no path to an administrator. A parallel test would pin down whether validation rejects that combination.🤖 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 `@icp_server/tests/sso_config_validation_tests.bal` around lines 75 - 83, Add a parallel validation test alongside testSSOOnlyWithoutAdminClaimIsRejected that builds an SSO-only configuration with a valid adminClaim and an empty adminValues list, then asserts validateSSOConfig returns an error. Keep the test focused on ensuring SSO-only deployments cannot be configured without any administrator values.icp_server/tests/auth_tests_v2.bal (1)
253-292: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding an authorization test for the new SSO mapping endpoints.
The new suite covers happy path, validation, immutability, and deletion, but not access control. Since these endpoints govern group membership derived from IdP claims, a case asserting that a caller without the group-management permission is rejected would protect the endpoint contract.
🤖 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 `@icp_server/tests/auth_tests_v2.bal` around lines 253 - 292, Add an authorization test alongside testCreateSSOGroupMapping covering the SSO group-mapping endpoints, using a caller that lacks the group-management permission and asserting the request is rejected with the expected unauthorized status. Reuse the existing authV2Client, endpoint path, and test authentication setup, and ensure the test validates access control rather than mapping behavior.icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql (1)
5-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMigration is not re-runnable, unlike the Oracle sibling.
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_oracle.sqldocuments itself as idempotent and guards every object creation. This script errors on a second run or after a partially applied migration. PostgreSQL supportsIF NOT EXISTSfor tables and indexes, andCREATE OR REPLACE TRIGGER(PG 14+); aDROP TRIGGER IF EXISTSbefore creation works on older versions.♻️ Suggested guards
-CREATE TABLE sso_group_mappings ( +CREATE TABLE IF NOT EXISTS sso_group_mappings ( @@ -CREATE INDEX idx_sgm_org_uuid ON sso_group_mappings(org_uuid); -CREATE INDEX idx_sgm_issuer_claim ON sso_group_mappings(issuer, claim_name, claim_value); -CREATE INDEX idx_sgm_group_id ON sso_group_mappings(group_id); -CREATE INDEX idx_sgm_project_uuid ON sso_group_mappings(project_uuid); -CREATE INDEX idx_sgm_integration_uuid ON sso_group_mappings(integration_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_org_uuid ON sso_group_mappings(org_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_issuer_claim ON sso_group_mappings(issuer, claim_name, claim_value); +CREATE INDEX IF NOT EXISTS idx_sgm_group_id ON sso_group_mappings(group_id); +CREATE INDEX IF NOT EXISTS idx_sgm_project_uuid ON sso_group_mappings(project_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_integration_uuid ON sso_group_mappings(integration_uuid); +DROP TRIGGER IF EXISTS update_sso_group_mappings_updated_at ON sso_group_mappings; CREATE TRIGGER update_sso_group_mappings_updated_at BEFORE UPDATE ON sso_group_mappings FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); -CREATE TABLE federated_group_user_mapping ( +CREATE TABLE IF NOT EXISTS federated_group_user_mapping ( @@ -CREATE INDEX idx_fgum_user_uuid ON federated_group_user_mapping(user_uuid); -CREATE INDEX idx_fgum_group_id ON federated_group_user_mapping(group_id); -CREATE INDEX idx_fgum_issuer_claim ON federated_group_user_mapping(issuer, claim_name, claim_value); +CREATE INDEX IF NOT EXISTS idx_fgum_user_uuid ON federated_group_user_mapping(user_uuid); +CREATE INDEX IF NOT EXISTS idx_fgum_group_id ON federated_group_user_mapping(group_id); +CREATE INDEX IF NOT EXISTS idx_fgum_issuer_claim ON federated_group_user_mapping(issuer, claim_name, claim_value); +DROP TRIGGER IF EXISTS update_federated_group_user_mapping_updated_at ON federated_group_user_mapping; CREATE TRIGGER update_federated_group_user_mapping_updated_at BEFORE UPDATE ON federated_group_user_mapping FOR EACH ROW EXECUTE FUNCTION update_updated_at_column();🤖 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 `@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql` around lines 5 - 56, Make the PostgreSQL migration idempotent like the Oracle sibling: guard both CREATE TABLE statements with IF NOT EXISTS, guard all CREATE INDEX statements with IF NOT EXISTS, and make each update trigger safely replaceable using CREATE OR REPLACE TRIGGER or DROP TRIGGER IF EXISTS before creation, while preserving the existing definitions and constraints.icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql (2)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused variable.
@seed_admin_idis set but the literal UUID is used at Lines 153 and 170; either use the variable or drop it.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql` at line 148, Remove the unused `@seed_admin_id` assignment, or replace the repeated literal UUIDs at the migration statements around lines 153 and 170 with `@seed_admin_id`; ensure the migration uses one consistent UUID source.
90-133: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
INSERT IGNOREdowngrades all errors to warnings.Beyond duplicate keys, it also suppresses foreign-key failures and truncation, so a partially migrated user set would be reported as success. Since Step 1a already clears conflicting non-admin rows, an explicit
NOT EXISTSguard (as used in the SQL Server script) would keep genuine errors visible.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql` around lines 90 - 133, The user migration INSERT in the dynamic SQL should not use INSERT IGNORE, which suppresses non-duplicate errors. Remove INSERT IGNORE and add an explicit NOT EXISTS guard against conflicting non-admin rows, matching the SQL Server migration’s behavior while allowing genuine foreign-key and truncation errors to surface.frontend/src/pages/EditGroup.tsx (1)
322-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMembership-source label logic is duplicated across pages.
The same
membershipSource→ label mapping is repeated inEditUser.tsxandProfile.tsx. Extracting a small helper (e.g. inpages/access-control/utils) keeps the three views consistent if new source values are added later.🤖 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 `@frontend/src/pages/EditGroup.tsx` around lines 322 - 332, Extract the repeated membershipSource-to-label mapping into a shared helper under pages/access-control/utils, then update EditGroup, EditUser, and Profile to use it for their membership-source labels. Preserve the existing labels for federated, manual_and_federated, and local sources, and ensure all three views use the same helper.icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql (1)
9-23: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueQuote dynamically composed identifiers.
Database names supplied through the configuration variables are concatenated directly into the generated statements. Using
QUOTENAME(@old_db)(and the same for the two target databases) keeps identifier handling robust for names that are not plain identifiers.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql` around lines 9 - 23, The dynamically composed database identifiers in the migration SQL should be safely quoted. Update the generated statements using `@old_db`, `@new_main_db`, and `@new_creds_db` to wrap each database name with QUOTENAME rather than concatenating raw variable values, while preserving the existing query behavior.
🤖 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 `@frontend/RUNTIME_CONFIG.md`:
- Around line 24-27: Update the configuration keys documented in
RUNTIME_CONFIG.md to use VITE_PASSWORD_LOGIN_DISABLED, matching
frontend/public/config.json.example and frontend usage, and add the two SSO
configuration keys documented by the example config. Keep the documented values
and surrounding runtime configuration entries consistent with the example.
In `@frontend/src/pages/AccessControl.tsx`:
- Around line 36-45: Replace the duplicated responsive SSO label JSX in
AccessControl and OrgAccessControl with the existing SSO_TAB_LABEL constant,
matching its use in ProjectAccessControl and ComponentAccessControl.
In `@icp_server/auth_service.bal`:
- Around line 1295-1363: Constrain target-group validation in the mapping
creation flow around targetGroup so a caller authorized at project or
integration scope cannot select groups with role assignments outside that scope.
Either restrict eligible groups to the mapping’s administrative scope or require
org-level permission when the target group has broader-scope assignments, while
preserving the existing organization-membership check.
- Around line 3618-3652: Update grantSuperAdminFromSSOClaims and the surrounding
SSO reconciliation flow so claim-based Super Admin membership is managed as a
federated grant that is re-evaluated on every login, including removing the
membership when the configured admin claim no longer matches. Reuse the existing
federated reconciliation mechanism and preserve the current validation and error
handling.
- Around line 1457-1468: Update the deletion flow around
storage:deleteSSOGroupMapping to also remove matching rows from
federated_group_user_mapping using the mapping’s organization, issuer, claim,
and group identifiers. Perform this cleanup as part of the same operation, while
preserving the existing not-found response and internal-error handling.
- Around line 368-373: Gate the federated group synchronization in the flow
around `syncFederatedGroupsFromSSOClaims` with `federatedAccessControlEnabled`,
so the call and its error handling execute only when the feature is enabled.
Preserve the existing synchronization behavior and internal-server-error
response when the flag is enabled.
- Line 1757: Update the user-groups update flow around
storage:getUserManualGroups so the response distinguishes manual group IDs from
effective membership IDs: rename the current/final group ID fields to
manualGroupIds/addedGroupIds, or derive and return effective IDs including
federated memberships. Ensure federated groups are not reported as removed
solely because they are absent from the manual-group query.
In `@icp_server/Config.toml`:
- Around line 155-157: Update the commented configuration key in the SSO
settings block to use the declared passwordLoginDisabled name instead of
disablePasswordLogin, while preserving its default value and explanatory
comment.
In `@icp_server/custom_auth/OIDC_SETUP_GUIDE.md`:
- Around line 86-88: Replace disablePasswordLogin with passwordLoginDisabled in
the configuration table at icp_server/custom_auth/OIDC_SETUP_GUIDE.md lines
86-88 and in the FAQ instruction at lines 308-308, ensuring both documentation
references match the server configuration key.
In `@icp_server/modules/storage/auth_repository.bal`:
- Around line 758-782: The insert branch in the federated membership
reconciliation loop should treat duplicate-key failures as benign concurrency
outcomes. Update the INSERT execution in the flow around findFederatedMembership
to classify its SQL error with classifySqlError, suppress only the duplicate-key
result, and continue synchronization; propagate all other database errors
unchanged.
- Around line 702-716: Update the federated group membership insertion flow to
handle Oracle identity retrieval, alongside the existing integer and MSSQL
string parsing in the code returning the mapping ID. When Oracle returns a ROWID
instead of the generated ID, read back federated_group_user_mapping.id using the
inserted row’s unique mapping tuple, then log and return that integer ID;
preserve the existing failure path when no valid ID is found.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/README.md`:
- Around line 72-81: The README migration documentation should cover the DDL
performed by both scripts: creation of org_secrets, ensuring
mi_composite_app_artifacts, and adding runtimes.key_id with its foreign key.
Update the prerequisites to require CREATE and ALTER privileges in addition to
DML, and explicitly warn that existing non-admin accounts are deleted and
replaced, advising a backup before migration.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql`:
- Around line 154-166: The Step 2a credential merge must warn when no row is
updated despite the legacy admin user existing. Update the dynamic SQL execution
flow around `@sql` and sp_executesql to capture @@ROWCOUNT, then check for zero
updates alongside the presence of the legacy admin user and print a clear
warning; retain the existing success message for rows that are merged.
- Around line 271-299: Update the org_secrets DDL block to be re-runnable by
guarding CREATE TABLE and runtimes.key_id creation with existence checks
consistent with the later artifact block. Split the ALTER TABLE adding key_id
and the ALTER TABLE adding fk_runtime_key_id into separate sp_executesql
batches, with the foreign-key batch executed only after the column exists.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql`:
- Around line 318-372: Make the Step 5 DDL re-runnable by adding an existence
guard to the org_secrets CREATE TABLE statement and replacing the unconditional
runtimes ALTER TABLE additions with guarded checks for both the key_id column
and fk_runtime_key_id constraint. Preserve the existing schema definitions and
ensure each change can safely be executed after a partial migration.
In `@icp_server/resources/db/init-scripts/mssql_init.sql`:
- Around line 377-388: Update the trigger statements in
icp_server/resources/db/init-scripts/mssql_init.sql: within
trg_sso_group_mappings_updated_at, target the aliased table with UPDATE sgm, and
within the corresponding federated group mapping trigger, use UPDATE fgum. If
the same triggers exist in
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql,
apply the identical alias-target changes there.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_h2.sql`:
- Around line 33-48: Change the user_uuid column definition in
federated_group_user_mapping from VARCHAR(36) to CHAR(36), keeping it NOT NULL
and preserving the existing foreign key to users(user_id) and all other
constraints.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql`:
- Around line 5-78: The SQL Server SSO migration is not idempotent because both
tables and triggers are created unconditionally. In
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql:5-78,
wrap sso_group_mappings, federated_group_user_mapping,
trg_sso_group_mappings_updated_at, and
trg_federated_group_user_mapping_updated_at creation in IF OBJECT_ID(...) IS
NULL guards, matching the H2 variant. No direct change is needed in
icp_server/resources/db/migration-scripts/README.md:203-204 because the guards
preserve its idempotency statement.
In `@icp_server/resources/db/migration-scripts/README.md`:
- Around line 203-204: Correct the idempotency statement in the migration README
to exclude engines where add_sso_group_mapping_tables_mssql.sql lacks existence
guards, or update that script’s table and trigger creation to guard against
existing objects. Ensure rerunning the migration after partial failure succeeds
for the supported engines.
In `@icp_server/tests/sso_federated_mapping_tests.bal`:
- Around line 221-229: Ensure teardown always runs after failures in
testSSOGroupMappingStorage, testFederatedMembershipReconciliation, and
testDeletedMappingRemovesMembershipOnNextLogin by moving their cleanup calls
into an on fail block or shared `@test`:AfterEach/@test:AfterGroups hook. In
icp_server/tests/sso_federated_mapping_tests.bal#L221-L229, include
role-mapping, role, project, user, and group deletions; at `#L297-L298` and
`#L361-L362`, include the corresponding user and group deletions.
---
Nitpick comments:
In `@frontend/src/components/LoginForm.tsx`:
- Around line 153-176: Update the `sx` props on the password and SSO buttons in
`LoginForm` to replace hardcoded colors `#1e1e1e`, `#333`, and `#ccc` with
appropriate MUI theme palette tokens, preserving the existing button appearance
and hover behavior.
In `@frontend/src/pages/access-control/SSOMappingsTab.tsx`:
- Around line 71-76: Confirm whether SSO mappings support multiple issuers; if
they do not, update the issuer input in SSOMappingsTab to be read-only while
retaining the window.API_CONFIG.ssoIssuer default and submitted value. If
multiple issuers are supported, preserve editability and add appropriate
validation against accepted issuer values before submission.
- Around line 178-273: Update the SSOMappingsTab mapping list flow around the
mappings rendering to add search and pagination, matching the established
behavior and controls used by UsersTab, GroupsTab, and RolesTab. Filter mappings
by the relevant claim, issuer, or group fields, paginate the filtered results,
and render the paginated collection in both mobile cards and the desktop
ListingTable while preserving empty-state and delete-action behavior.
In `@frontend/src/pages/AccessControl.tsx`:
- Around line 33-34: Replace the positional ORG_TABS.slice(0, 3) and
PROJECT_TABS.slice(0, 2) usage with name-based filtering that excludes
'sso-mappings'. Apply this consistently at all referenced usages while
preserving the existing tab ordering and behavior for the remaining tabs.
- Line 71: Make the tab configuration consistent across scopes by adding
variant="scrollable" and scrollButtons="auto" to the project- and
component-level Tabs instances near the existing org-level configurations.
Update the Tabs elements at all four referenced locations so the extra SSO tab
remains accessible on narrow viewports while preserving their current navigation
and value behavior.
In `@frontend/src/pages/EditGroup.tsx`:
- Around line 322-332: Extract the repeated membershipSource-to-label mapping
into a shared helper under pages/access-control/utils, then update EditGroup,
EditUser, and Profile to use it for their membership-source labels. Preserve the
existing labels for federated, manual_and_federated, and local sources, and
ensure all three views use the same helper.
In `@icp_server/modules/storage/user_repository.bal`:
- Around line 154-194: Update getGroupsForUser so each group’s manual and
federated membership status is computed once through LEFT JOIN-derived flags,
then reuse those flags for both filtering and membership_source classification
instead of repeating correlated EXISTS subqueries. Preserve the current group
results, ordering, and membership_source values.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql`:
- Around line 9-23: The dynamically composed database identifiers in the
migration SQL should be safely quoted. Update the generated statements using
`@old_db`, `@new_main_db`, and `@new_creds_db` to wrap each database name with
QUOTENAME rather than concatenating raw variable values, while preserving the
existing query behavior.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql`:
- Line 148: Remove the unused `@seed_admin_id` assignment, or replace the repeated
literal UUIDs at the migration statements around lines 153 and 170 with
`@seed_admin_id`; ensure the migration uses one consistent UUID source.
- Around line 90-133: The user migration INSERT in the dynamic SQL should not
use INSERT IGNORE, which suppresses non-duplicate errors. Remove INSERT IGNORE
and add an explicit NOT EXISTS guard against conflicting non-admin rows,
matching the SQL Server migration’s behavior while allowing genuine foreign-key
and truncation errors to surface.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql`:
- Around line 5-48: Make the MySQL migration idempotent by adding IF NOT EXISTS
to both CREATE TABLE statements, sso_group_mappings and
federated_group_user_mapping, matching the H2 migration behavior and allowing
safe reruns after partial failures.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql`:
- Around line 5-56: Make the PostgreSQL migration idempotent like the Oracle
sibling: guard both CREATE TABLE statements with IF NOT EXISTS, guard all CREATE
INDEX statements with IF NOT EXISTS, and make each update trigger safely
replaceable using CREATE OR REPLACE TRIGGER or DROP TRIGGER IF EXISTS before
creation, while preserving the existing definitions and constraints.
In `@icp_server/tests/auth_tests_v2.bal`:
- Around line 253-292: Add an authorization test alongside
testCreateSSOGroupMapping covering the SSO group-mapping endpoints, using a
caller that lacks the group-management permission and asserting the request is
rejected with the expected unauthorized status. Reuse the existing authV2Client,
endpoint path, and test authentication setup, and ensure the test validates
access control rather than mapping behavior.
In `@icp_server/tests/sso_config_validation_tests.bal`:
- Around line 75-83: Add a parallel validation test alongside
testSSOOnlyWithoutAdminClaimIsRejected that builds an SSO-only configuration
with a valid adminClaim and an empty adminValues list, then asserts
validateSSOConfig returns an error. Keep the test focused on ensuring SSO-only
deployments cannot be configured without any administrator values.
🪄 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 Plus
Run ID: ae7ed87c-57df-4a1c-9166-102f8f119eab
📒 Files selected for processing (47)
frontend/RUNTIME_CONFIG.mdfrontend/public/config.jsonfrontend/public/config.json.examplefrontend/src/api/auth.tsfrontend/src/api/authQueries.tsfrontend/src/components/LoginForm.tsxfrontend/src/config/api.tsfrontend/src/pages/AccessControl.tsxfrontend/src/pages/CreateGroup.tsxfrontend/src/pages/EditGroup.tsxfrontend/src/pages/EditUser.tsxfrontend/src/pages/Profile.tsxfrontend/src/pages/access-control/SSOMappingsTab.tsxfrontend/src/pages/access-control/UsersTab.tsxfrontend/src/paths.tsicp_server/Config.tomlicp_server/Dependencies.tomlicp_server/auth_service.balicp_server/config.balicp_server/custom_auth/OIDC_SETUP_GUIDE.mdicp_server/init.balicp_server/modules/auth/oidc.balicp_server/modules/storage/auth_repository.balicp_server/modules/storage/user_repository.balicp_server/modules/types/auth_types.balicp_server/modules/types/types.balicp_server/resources/db/icp-1.2.x-to-2.x.x/README.mdicp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sqlicp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sqlicp_server/resources/db/init-scripts/h2_init.sqlicp_server/resources/db/init-scripts/mssql_init.sqlicp_server/resources/db/init-scripts/mysql_init.sqlicp_server/resources/db/init-scripts/oracle_init.sqlicp_server/resources/db/init-scripts/postgresql_init.sqlicp_server/resources/db/migration-scripts/README.mdicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_h2.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_oracle.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sqlicp_server/tests/Config.tomlicp_server/tests/auth_tests_v2.balicp_server/tests/mock_oidc_provider.balicp_server/tests/oidc_tests.balicp_server/tests/sso_config_validation_tests.balicp_server/tests/sso_federated_mapping_tests.balicp_server/webserver.bal
| "VITE_OBSERVABILITY_URL": "https://localhost:9446/icp/observability", | ||
| "VITE_SSO_ENABLED": false, | ||
| "VITE_DISABLE_PASSWORD_LOGIN": false, | ||
| "VITE_ICP_VERSION": "2.0.0-SNAPSHOT" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Documented key name does not match frontend/public/config.json.example.
The doc uses VITE_DISABLE_PASSWORD_LOGIN while the example config and frontend read VITE_PASSWORD_LOGIN_DISABLED. Operators following this doc would set a key that is never read. The two new SSO keys are also missing here.
📝 Proposed doc fix
"VITE_OBSERVABILITY_URL": "https://localhost:9446/icp/observability",
"VITE_SSO_ENABLED": false,
- "VITE_DISABLE_PASSWORD_LOGIN": false,
+ "VITE_SSO_ISSUER": "",
+ "VITE_PASSWORD_LOGIN_DISABLED": false,
+ "VITE_FEDERATED_ACCESS_CONTROL_ENABLED": false,
"VITE_ICP_VERSION": "2.0.0-SNAPSHOT"📝 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.
| "VITE_OBSERVABILITY_URL": "https://localhost:9446/icp/observability", | |
| "VITE_SSO_ENABLED": false, | |
| "VITE_DISABLE_PASSWORD_LOGIN": false, | |
| "VITE_ICP_VERSION": "2.0.0-SNAPSHOT" | |
| "VITE_OBSERVABILITY_URL": "https://localhost:9446/icp/observability", | |
| "VITE_SSO_ENABLED": false, | |
| "VITE_SSO_ISSUER": "", | |
| "VITE_PASSWORD_LOGIN_DISABLED": false, | |
| "VITE_FEDERATED_ACCESS_CONTROL_ENABLED": false, | |
| "VITE_ICP_VERSION": "2.0.0-SNAPSHOT" |
🤖 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 `@frontend/RUNTIME_CONFIG.md` around lines 24 - 27, Update the configuration
keys documented in RUNTIME_CONFIG.md to use VITE_PASSWORD_LOGIN_DISABLED,
matching frontend/public/config.json.example and frontend usage, and add the two
SSO configuration keys documented by the example config. Keep the documented
values and surrounding runtime configuration entries consistent with the
example.
| const SSO_TAB_LABEL = ( | ||
| <> | ||
| <Box component="span" sx={{ display: { xs: 'inline', sm: 'none' } }}> | ||
| SSO | ||
| </Box> | ||
| <Box component="span" sx={{ display: { xs: 'none', sm: 'inline' } }}> | ||
| SSO Mappings | ||
| </Box> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reuse SSO_TAB_LABEL instead of duplicating the JSX inline.
SSO_TAB_LABEL was introduced for exactly this purpose and is used correctly in ProjectAccessControl/ComponentAccessControl, but AccessControl and OrgAccessControl inline the identical JSX instead.
♻️ Reuse the shared label constant
- {window.API_CONFIG.ssoEnabled && (
- <Tab
- label={
- <>
- <Box component="span" sx={{ display: { xs: 'inline', sm: 'none' } }}>
- SSO
- </Box>
- <Box component="span" sx={{ display: { xs: 'none', sm: 'inline' } }}>
- SSO Mappings
- </Box>
- </>
- }
- />
- )}
+ {window.API_CONFIG.ssoEnabled && <Tab label={SSO_TAB_LABEL} />}Apply the same change in both AccessControl (lines 75-88) and OrgAccessControl (lines 127-140).
Also applies to: 75-88, 127-140
🤖 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 `@frontend/src/pages/AccessControl.tsx` around lines 36 - 45, Replace the
duplicated responsive SSO label JSX in AccessControl and OrgAccessControl with
the existing SSO_TAB_LABEL constant, matching its use in ProjectAccessControl
and ComponentAccessControl.
| error? federatedSyncResult = syncFederatedGroupsFromSSOClaims(userDetails.userId, userInfo.username, claims); | ||
| if federatedSyncResult is error { | ||
| log:printError("Error synchronizing SSO group memberships", federatedSyncResult, | ||
| username = userInfo.username); | ||
| return utils:createInternalServerError("Error synchronizing SSO group access"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Federated group sync is not gated on federatedAccessControlEnabled.
The flag gates manual membership edits (Lines 1518 and 1802) but not this sync. If mappings exist while the flag is off, federated rows are still written and become effective through v_effective_group_user_mapping, so the flag does not consistently control the feature. Consider gating the call, or documenting that mappings apply regardless of the flag.
🔧 Proposed gate
- error? federatedSyncResult = syncFederatedGroupsFromSSOClaims(userDetails.userId, userInfo.username, claims);
- if federatedSyncResult is error {
- log:printError("Error synchronizing SSO group memberships", federatedSyncResult,
- username = userInfo.username);
- return utils:createInternalServerError("Error synchronizing SSO group access");
+ if federatedAccessControlEnabled {
+ error? federatedSyncResult = syncFederatedGroupsFromSSOClaims(userDetails.userId, userInfo.username, claims);
+ if federatedSyncResult is error {
+ log:printError("Error synchronizing SSO group memberships", federatedSyncResult,
+ username = userInfo.username);
+ return utils:createInternalServerError("Error synchronizing SSO group access");
+ }
}📝 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.
| error? federatedSyncResult = syncFederatedGroupsFromSSOClaims(userDetails.userId, userInfo.username, claims); | |
| if federatedSyncResult is error { | |
| log:printError("Error synchronizing SSO group memberships", federatedSyncResult, | |
| username = userInfo.username); | |
| return utils:createInternalServerError("Error synchronizing SSO group access"); | |
| } | |
| if federatedAccessControlEnabled { | |
| error? federatedSyncResult = syncFederatedGroupsFromSSOClaims(userDetails.userId, userInfo.username, claims); | |
| if federatedSyncResult is error { | |
| log:printError("Error synchronizing SSO group memberships", federatedSyncResult, | |
| username = userInfo.username); | |
| return utils:createInternalServerError("Error synchronizing SSO group access"); | |
| } | |
| } |
🤖 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 `@icp_server/auth_service.bal` around lines 368 - 373, Gate the federated group
synchronization in the flow around `syncFederatedGroupsFromSSOClaims` with
`federatedAccessControlEnabled`, so the call and its error handling execute only
when the feature is enabled. Preserve the existing synchronization behavior and
internal-server-error response when the flag is enabled.
| // Authorize at the mapping's administrative scope so project/integration | ||
| // scoped admins can manage mappings at their level but not broader ones. | ||
| types:AccessScope mappingScope = {orgUuid: storage:DEFAULT_ORG_ID}; | ||
| if projectUuid is string { | ||
| mappingScope.projectUuid = projectUuid; | ||
| } | ||
| if integrationUuid is string { | ||
| mappingScope.integrationUuid = integrationUuid; | ||
| } | ||
| boolean|error hasPermission = auth:hasAnyPermission(userContext.userId, | ||
| [auth:PERMISSION_USER_MANAGE_GROUPS, auth:PERMISSION_USER_UPDATE_GROUP_ROLES], mappingScope); | ||
| if hasPermission is error { | ||
| log:printError("Error checking permissions", hasPermission, userId = userContext.userId); | ||
| return utils:createInternalServerError("Error checking permissions"); | ||
| } | ||
| if !hasPermission { | ||
| return <http:Forbidden>{ | ||
| body: { | ||
| message: "Insufficient permissions to create SSO group mappings at the requested scope" | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| string? projectName = (); | ||
| string? integrationName = (); | ||
| if projectUuid is string { | ||
| types:Project|error project = storage:getProjectById(projectUuid); | ||
| if project is error || project.orgId != storage:DEFAULT_ORG_ID { | ||
| return <http:NotFound>{ | ||
| body: { | ||
| message: "Target project not found" | ||
| } | ||
| }; | ||
| } | ||
| projectName = project.name; | ||
| } | ||
| if integrationUuid is string { | ||
| types:Component|error component = storage:getComponentById(integrationUuid); | ||
| if component is error || component.projectId != projectUuid { | ||
| return <http:NotFound>{ | ||
| body: { | ||
| message: "Target integration not found in the specified project" | ||
| } | ||
| }; | ||
| } | ||
| integrationName = component.displayName; | ||
| } | ||
|
|
||
| types:SSOGroupMappingInput inputWithOrg = { | ||
| issuer: mappingInput.issuer.trim(), | ||
| claimName: mappingInput.claimName.trim(), | ||
| claimValue: mappingInput.claimValue.trim(), | ||
| groupId: mappingInput.groupId.trim(), | ||
| orgUuid: storage:DEFAULT_ORG_ID | ||
| }; | ||
| if projectUuid is string { | ||
| inputWithOrg.projectUuid = projectUuid; | ||
| } | ||
| if integrationUuid is string { | ||
| inputWithOrg.integrationUuid = integrationUuid; | ||
| } | ||
| types:Group|error targetGroup = storage:getGroupById(inputWithOrg.groupId); | ||
| if targetGroup is error || targetGroup.orgUuid != storage:DEFAULT_ORG_ID { | ||
| return <http:NotFound>{ | ||
| body: { | ||
| message: "Target group not found" | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Target group is not constrained by the mapping's administrative scope.
Authorization is evaluated at the mapping's project/integration scope, but groupId is only validated for org membership (Line 1356). A caller authorized at a narrow scope can therefore create a mapping to any org group, including groups holding broader role assignments. Consider restricting selectable target groups, or requiring org-level permission when the target group carries role assignments outside the caller's scope.
🤖 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 `@icp_server/auth_service.bal` around lines 1295 - 1363, Constrain target-group
validation in the mapping creation flow around targetGroup so a caller
authorized at project or integration scope cannot select groups with role
assignments outside that scope. Either restrict eligible groups to the mapping’s
administrative scope or require org-level permission when the target group has
broader-scope assignments, while preserving the existing organization-membership
check.
Source: Path instructions
| error? deleteResult = storage:deleteSSOGroupMapping(mappingId, storage:DEFAULT_ORG_ID); | ||
| if deleteResult is error { | ||
| log:printError("Error deleting SSO group mapping", deleteResult, mappingId = mappingId); | ||
| if deleteResult.message().includes("not found") { | ||
| return <http:NotFound>{ | ||
| body: { | ||
| message: "SSO group mapping not found" | ||
| } | ||
| }; | ||
| } | ||
| return utils:createInternalServerError("Failed to delete SSO group mapping"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Deleting a mapping leaves derived federated memberships in place.
federated_group_user_mapping rows are reconciled per user at login, so memberships granted by this mapping persist until each affected user signs in again. Consider deleting the matching federated rows (same org/issuer/claim/group) as part of this operation so revocation takes effect promptly.
🤖 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 `@icp_server/auth_service.bal` around lines 1457 - 1468, Update the deletion
flow around storage:deleteSSOGroupMapping to also remove matching rows from
federated_group_user_mapping using the mapping’s organization, issuer, claim,
and group identifiers. Perform this cleanup as part of the same operation, while
preserving the existing not-found response and internal-error handling.
| CREATE TRIGGER trg_sso_group_mappings_updated_at | ||
| ON sso_group_mappings | ||
| AFTER UPDATE | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON; | ||
| UPDATE sso_group_mappings | ||
| SET updated_at = GETDATE() | ||
| FROM sso_group_mappings sgm | ||
| INNER JOIN inserted i ON sgm.mapping_id = i.mapping_id; | ||
| END; | ||
| GO |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Both new MSSQL triggers use an incorrect UPDATE ... FROM target. Each trigger aliases the target table in FROM but names the base table in UPDATE, which T-SQL rejects for duplicate exposed names. The same construction was applied twice.
icp_server/resources/db/init-scripts/mssql_init.sql#L377-L388: changeUPDATE sso_group_mappingstoUPDATE sgm.icp_server/resources/db/init-scripts/mssql_init.sql#L412-L423: changeUPDATE federated_group_user_mappingtoUPDATE fgum.
Please also mirror the fix in icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql if it carries the same triggers.
📍 Affects 1 file
icp_server/resources/db/init-scripts/mssql_init.sql#L377-L388(this comment)icp_server/resources/db/init-scripts/mssql_init.sql#L412-L423
🤖 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 `@icp_server/resources/db/init-scripts/mssql_init.sql` around lines 377 - 388,
Update the trigger statements in
icp_server/resources/db/init-scripts/mssql_init.sql: within
trg_sso_group_mappings_updated_at, target the aliased table with UPDATE sgm, and
within the corresponding federated group mapping trigger, use UPDATE fgum. If
the same triggers exist in
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql,
apply the identical alias-target changes there.
| CREATE TABLE IF NOT EXISTS federated_group_user_mapping ( | ||
| id BIGINT AUTO_INCREMENT PRIMARY KEY, | ||
| org_uuid INT NOT NULL DEFAULT 1, | ||
| issuer VARCHAR(255) NOT NULL, | ||
| user_uuid VARCHAR(36) NOT NULL, | ||
| group_id VARCHAR(36) NOT NULL, | ||
| claim_name VARCHAR(128) NOT NULL, | ||
| claim_value VARCHAR(255) NOT NULL, | ||
| last_seen_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| CONSTRAINT fk_fed_group_user_org FOREIGN KEY (org_uuid) REFERENCES organizations (org_id) ON DELETE CASCADE, | ||
| CONSTRAINT fk_fed_group_user_user FOREIGN KEY (user_uuid) REFERENCES users (user_id) ON DELETE CASCADE, | ||
| CONSTRAINT fk_fed_group_user_group FOREIGN KEY (group_id) REFERENCES user_groups (group_id) ON DELETE CASCADE, | ||
| CONSTRAINT unique_fed_group_user_claim UNIQUE (org_uuid, issuer, user_uuid, group_id, claim_name, claim_value) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'h2_init.sql|mssql_init.sql' -x rg -n 'user_id|user_uuid' {}Repository: wso2/integration-control-plane
Length of output: 6097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
init_files="$(fd -t f 'h2_init.sql|mssql_init.sql|-init.sql' icp_server/resources/db/migration-scripts | sort)"
printf 'Files:\n'
printf '%s\n' "$init_files"
printf '\nH2 relevant user/group columns:\n'
for f in "$init_files"; do
if [[ "$f" != *h2* ]]; then continue; fi
sed -n '200,240p;330,380p' "$f"
done
printf '\nMSSQL relevant user/group columns:\n'
for f in "$init_files"; do
if [[ "$f" != *mssql* ]]; then continue; fi
sed -n '320,350p;385,410p' "$f"
done
printf '\nMigration script relevant user_uuid columns:\n'
for f in $(fd -t f 'add_sso_group_mapping_tables_(.*).sql' icp_server/resources/db/migration-scripts | sort); do
echo "--- $f ---"
sed -n '1,90p' "$f"
doneRepository: wso2/integration-control-plane
Length of output: 21695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating init files:"
git ls-files 'icp_server/resources/db/migration-scripts/*init.sql' 'icp_server/resources/db/migration-scripts/*-init.sql'
echo
echo "Search matching files by exact name:"
fd -t f -p 'icp_server/resources/db/migration-scripts/.*init.sql$|icp_server/resources/db/migration-scripts/.*-init.sql$' || true
echo
echo "All relevant user/group mapping occurrences in migration scripts:"
rg -n 'CREATE TABLE (group_user_mapping|federated_group_user_mapping|users)|user_id|user_uuid|group_user_mapping\.|federated_group_user_mapping\.|v_effective_group_user_mapping' icp_server/resources/db/migration-scripts || trueRepository: wso2/integration-control-plane
Length of output: 15223
Align user_uuid with the FK target in H2.
users.user_id is CHAR(36) and the MS SQL migration uses CHAR(36) for federated_group_user_mapping.user_uuid, while this H2 script uses VARCHAR(36). Change the H2 federated_group_user_mapping.user_uuid column to CHAR(36) to keep FK/type behavior consistent across dialects.
🤖 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
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_h2.sql`
around lines 33 - 48, Change the user_uuid column definition in
federated_group_user_mapping from VARCHAR(36) to CHAR(36), keeping it NOT NULL
and preserving the existing foreign key to users(user_id) and all other
constraints.
| CREATE TABLE sso_group_mappings ( | ||
| mapping_id VARCHAR(36) PRIMARY KEY, | ||
| org_uuid INT NOT NULL DEFAULT 1, | ||
| issuer VARCHAR(255) NOT NULL, | ||
| claim_name VARCHAR(128) NOT NULL, | ||
| claim_value VARCHAR(255) NOT NULL, | ||
| group_id VARCHAR(36) NOT NULL, | ||
| project_uuid CHAR(36) NULL, | ||
| integration_uuid CHAR(36) NULL, | ||
| created_at DATETIME2 DEFAULT GETDATE (), | ||
| updated_at DATETIME2 DEFAULT GETDATE (), | ||
| CONSTRAINT fk_sso_group_mapping_org FOREIGN KEY (org_uuid) REFERENCES organizations (org_id) ON DELETE NO ACTION, | ||
| CONSTRAINT fk_sso_group_mapping_group FOREIGN KEY (group_id) REFERENCES user_groups (group_id) ON DELETE CASCADE, | ||
| CONSTRAINT fk_sso_group_mapping_project FOREIGN KEY (project_uuid) REFERENCES projects (project_id) ON DELETE NO ACTION, | ||
| CONSTRAINT fk_sso_group_mapping_integration FOREIGN KEY (integration_uuid) REFERENCES components (component_id) ON DELETE NO ACTION, | ||
| CONSTRAINT chk_sso_mapping_integration_requires_project CHECK ( | ||
| integration_uuid IS NULL | ||
| OR project_uuid IS NOT NULL | ||
| ), | ||
| CONSTRAINT unique_sso_group_mapping UNIQUE (org_uuid, issuer, claim_name, claim_value, group_id), | ||
| INDEX idx_sso_group_mapping_org (org_uuid), | ||
| INDEX idx_sso_group_mapping_issuer_claim (issuer, claim_name, claim_value), | ||
| INDEX idx_sso_group_mapping_group (group_id), | ||
| INDEX idx_sso_group_mapping_project (project_uuid), | ||
| INDEX idx_sso_group_mapping_integration (integration_uuid) | ||
| ); | ||
| GO | ||
|
|
||
| CREATE TRIGGER trg_sso_group_mappings_updated_at | ||
| ON sso_group_mappings | ||
| AFTER UPDATE | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON; | ||
| UPDATE sso_group_mappings | ||
| SET updated_at = GETDATE() | ||
| FROM sso_group_mappings sgm | ||
| INNER JOIN inserted i ON sgm.mapping_id = i.mapping_id; | ||
| END; | ||
| GO | ||
|
|
||
| CREATE TABLE federated_group_user_mapping ( | ||
| id BIGINT IDENTITY (1, 1) PRIMARY KEY, | ||
| org_uuid INT NOT NULL DEFAULT 1, | ||
| issuer VARCHAR(255) NOT NULL, | ||
| user_uuid CHAR(36) NOT NULL, | ||
| group_id VARCHAR(36) NOT NULL, | ||
| claim_name VARCHAR(128) NOT NULL, | ||
| claim_value VARCHAR(255) NOT NULL, | ||
| last_seen_at DATETIME2 DEFAULT GETDATE (), | ||
| created_at DATETIME2 DEFAULT GETDATE (), | ||
| updated_at DATETIME2 DEFAULT GETDATE (), | ||
| CONSTRAINT fk_fed_group_user_org FOREIGN KEY (org_uuid) REFERENCES organizations (org_id) ON DELETE NO ACTION, | ||
| CONSTRAINT fk_fed_group_user_user FOREIGN KEY (user_uuid) REFERENCES users (user_id) ON DELETE CASCADE, | ||
| CONSTRAINT fk_fed_group_user_group FOREIGN KEY (group_id) REFERENCES user_groups (group_id) ON DELETE CASCADE, | ||
| CONSTRAINT unique_fed_group_user_claim UNIQUE (org_uuid, issuer, user_uuid, group_id, claim_name, claim_value), | ||
| INDEX idx_fed_group_user_user (user_uuid), | ||
| INDEX idx_fed_group_user_group (group_id), | ||
| INDEX idx_fed_group_user_issuer_claim (issuer, claim_name, claim_value) | ||
| ); | ||
| GO | ||
|
|
||
| CREATE TRIGGER trg_federated_group_user_mapping_updated_at | ||
| ON federated_group_user_mapping | ||
| AFTER UPDATE | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON; | ||
| UPDATE federated_group_user_mapping | ||
| SET updated_at = GETDATE() | ||
| FROM federated_group_user_mapping fgum | ||
| INNER JOIN inserted i ON fgum.id = i.id; | ||
| END; | ||
| GO |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
SQL Server SSO upgrade script is not idempotent, contradicting the documented behaviour. The tables and triggers are created unconditionally, while the documentation states these upgrade scripts are safe to re-run after a partial failure.
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql#L5-L78: wrap bothCREATE TABLEstatements and bothCREATE TRIGGERstatements inIF OBJECT_ID(...) IS NULLguards, matching the H2 variant.icp_server/resources/db/migration-scripts/README.md#L203-L204: once the guards are added no doc change is needed; otherwise qualify the idempotency statement for SQL Server.
📍 Affects 2 files
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql#L5-L78(this comment)icp_server/resources/db/migration-scripts/README.md#L203-L204
🤖 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
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql`
around lines 5 - 78, The SQL Server SSO migration is not idempotent because both
tables and triggers are created unconditionally. In
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql:5-78,
wrap sso_group_mappings, federated_group_user_mapping,
trg_sso_group_mappings_updated_at, and
trg_federated_group_user_mapping_updated_at creation in IF OBJECT_ID(...) IS
NULL guards, matching the H2 variant. No direct change is needed in
icp_server/resources/db/migration-scripts/README.md:203-204 because the guards
preserve its idempotency statement.
| The scripts are **idempotent** — safe to re-run, including after a partial failure. No data | ||
| backfill is involved: federated memberships are recorded as users log in through the IdP. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Idempotency claim does not hold for all engines.
add_sso_group_mapping_tables_mssql.sql creates the two tables and their triggers without existence guards, so a re-run fails on the first statement. Either add the guards there or qualify this statement. See the related comment on that script.
🤖 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 `@icp_server/resources/db/migration-scripts/README.md` around lines 203 - 204,
Correct the idempotency statement in the migration README to exclude engines
where add_sso_group_mapping_tables_mssql.sql lacks existence guards, or update
that script’s table and trigger creation to guard against existing objects.
Ensure rerunning the migration after partial failure succeeds for the supported
engines.
| check storage:removeRoleFromGroup(scopedRoleMappingId); | ||
| check storage:removeRoleFromGroup(roleMappingId); | ||
| check storage:removePermissionsFromRole(scopedRoleId, [projectViewPermission.permissionId]); | ||
| check storage:removePermissionsFromRole(roleId, [userManageGroupsPermission.permissionId]); | ||
| check storage:deleteRoleV2(scopedRoleId); | ||
| check storage:deleteRoleV2(roleId); | ||
| check storage:deleteProject(projectId); | ||
| check storage:deleteUserV2(userId, "test-cleanup-user"); | ||
| check storage:deleteGroup(groupId); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test teardown is unreachable on assertion failure in all three tests. Each test performs cleanup as a trailing sequence of check calls in the function body, so any failing test:assert* short-circuits before teardown and leaves groups, roles, projects, users, and federated rows behind. Later tests in this module read org-wide state, and repeated local runs accumulate orphaned data. Move teardown into a construct that always runs — an on fail block or an @test:AfterEach/@test:AfterGroups hook.
icp_server/tests/sso_federated_mapping_tests.bal#L221-L229: move the role-mapping, role, project, user, and group deletions intestSSOGroupMappingStorageinto a guaranteed-teardown construct.icp_server/tests/sso_federated_mapping_tests.bal#L297-L298: move the user and group deletions intestFederatedMembershipReconciliationinto the same construct.icp_server/tests/sso_federated_mapping_tests.bal#L361-L362: move the user and group deletions intestDeletedMappingRemovesMembershipOnNextLogininto the same construct.
📍 Affects 1 file
icp_server/tests/sso_federated_mapping_tests.bal#L221-L229(this comment)icp_server/tests/sso_federated_mapping_tests.bal#L297-L298icp_server/tests/sso_federated_mapping_tests.bal#L361-L362
🤖 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 `@icp_server/tests/sso_federated_mapping_tests.bal` around lines 221 - 229,
Ensure teardown always runs after failures in testSSOGroupMappingStorage,
testFederatedMembershipReconciliation, and
testDeletedMappingRemovesMembershipOnNextLogin by moving their cleanup calls
into an on fail block or shared `@test`:AfterEach/@test:AfterGroups hook. In
icp_server/tests/sso_federated_mapping_tests.bal#L221-L229, include
role-mapping, role, project, user, and group deletions; at `#L297-L298` and
`#L361-L362`, include the corresponding user and group deletions.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 20
🧹 Nitpick comments (14)
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql (1)
5-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider idempotent creates for consistency with the H2 migration script.
add_sso_group_mapping_tables_h2.sqlusesCREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS, while this script uses unguardedCREATE TABLE. A re-run or partial-failure retry aborts here. MySQL supportsIF NOT EXISTSonCREATE TABLE, so the two migration scripts can behave the same way.🤖 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 `@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql` around lines 5 - 48, Make the MySQL migration idempotent by adding IF NOT EXISTS to both CREATE TABLE statements, sso_group_mappings and federated_group_user_mapping, matching the H2 migration behavior and allowing safe reruns after partial failures.frontend/src/components/LoginForm.tsx (1)
153-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme tokens instead of hardcoded colors.
#1e1e1e,#333, and#cccare hardcoded in thesxprops of both buttons. As per coding guidelines, "Use theme tokens with thesxprop instead of hardcoded colors and spacing values".♻️ Proposed change
- sx={{ mt: 1, bgcolor: '`#1e1e1e`', '&:hover': { bgcolor: '`#333`' }, textTransform: 'none', py: 1.2 }} + sx={{ mt: 1, bgcolor: 'common.black', '&:hover': { bgcolor: 'grey.800' }, textTransform: 'none', py: 1.2 }}- sx={{ textTransform: 'none', py: 1.2, borderColor: '`#ccc`', color: 'text.primary' }} + sx={{ textTransform: 'none', py: 1.2, borderColor: 'divider', color: 'text.primary' }}🤖 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 `@frontend/src/components/LoginForm.tsx` around lines 153 - 176, Update the `sx` props on the password and SSO buttons in `LoginForm` to replace hardcoded colors `#1e1e1e`, `#333`, and `#ccc` with appropriate MUI theme palette tokens, preserving the existing button appearance and hover behavior.Source: Coding guidelines
frontend/src/pages/AccessControl.tsx (2)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile
slice(0, N)coupling to array order.
ORG_TABS.slice(0, 3)andPROJECT_TABS.slice(0, 2)assume'sso-mappings'stays the last element of each tuple; reordering either constant later would silently drop the wrong tab instead of failing to compile.♻️ Filter by name instead of position
- const orgTabs: readonly string[] = window.API_CONFIG.ssoEnabled ? ORG_TABS : ORG_TABS.slice(0, 3); + const orgTabs: readonly string[] = window.API_CONFIG.ssoEnabled ? ORG_TABS : ORG_TABS.filter((t) => t !== 'sso-mappings');Apply the equivalent change for
PROJECT_TABS.slice(0, 2).Also applies to: 54-54, 106-106, 167-168, 215-216
🤖 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 `@frontend/src/pages/AccessControl.tsx` around lines 33 - 34, Replace the positional ORG_TABS.slice(0, 3) and PROJECT_TABS.slice(0, 2) usage with name-based filtering that excludes 'sso-mappings'. Apply this consistently at all referenced usages while preserving the existing tab ordering and behavior for the remaining tabs.
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent scrollable-tabs behavior across scopes.
Org-level tabs (lines 71, 123) use
variant="scrollable" scrollButtons="auto", but project/component-level tabs (lines 184, 244) don't, despite both now carrying an extra SSO tab that can overflow on narrow viewports.Also applies to: 123-123, 184-184, 244-244
🤖 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 `@frontend/src/pages/AccessControl.tsx` at line 71, Make the tab configuration consistent across scopes by adding variant="scrollable" and scrollButtons="auto" to the project- and component-level Tabs instances near the existing org-level configurations. Update the Tabs elements at all four referenced locations so the extra SSO tab remains accessible on narrow viewports while preserving their current navigation and value behavior.frontend/src/pages/access-control/SSOMappingsTab.tsx (2)
71-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEditable Issuer field risks silent mapping failures.
The Issuer field defaults from
window.API_CONFIG.ssoIssuerbut remains freely editable; since mapping matching inoidc.balrequires an exact string match against the token'sissclaim, a typo here causes the mapping to silently never apply, with no immediate feedback. Please confirm whether multiple issuers are actually supported; if not, consider making this field read-only.Also applies to: 107-107
🤖 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 `@frontend/src/pages/access-control/SSOMappingsTab.tsx` around lines 71 - 76, Confirm whether SSO mappings support multiple issuers; if they do not, update the issuer input in SSOMappingsTab to be read-only while retaining the window.API_CONFIG.ssoIssuer default and submitted value. If multiple issuers are supported, preserve editability and add appropriate validation against accepted issuer values before submission.
178-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo search or pagination, unlike sibling list tabs.
UsersTab,GroupsTab, andRolesTaball provide search plus pagination for their lists; this table renders the full mapping list unbounded, which is inconsistent and may not scale for orgs with many mappings.🤖 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 `@frontend/src/pages/access-control/SSOMappingsTab.tsx` around lines 178 - 273, Update the SSOMappingsTab mapping list flow around the mappings rendering to add search and pagination, matching the established behavior and controls used by UsersTab, GroupsTab, and RolesTab. Filter mappings by the relevant claim, issuer, or group fields, paginate the filtered results, and render the paginated collection in both mobile cards and the desktop ListingTable while preserving empty-state and delete-action behavior.icp_server/modules/storage/user_repository.bal (1)
154-194: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce redundant EXISTS subqueries and duplicate invocation cost.
The new query evaluates the same manual/federated membership check twice per group row (once in the
CASE, once inWHERE) via correlatedEXISTS. ALEFT JOINcomputes both flags once and reuses them. This also compounds with the existing pattern elsewhere in this file wheregetGroupsForUseris called twice per user (once forgroups, once forgroupCount, e.g. ingetAllUsersV2), so the added subquery cost is paid twice per user in that list endpoint.⚡ Suggested query rewrite using LEFT JOIN
- stream<record {|string group_id; string group_name; string description?; string membership_source;|}, sql:Error?> groupStream = dbClient->query( - `SELECT g.group_id, g.group_name, g.description, - CASE - WHEN EXISTS ( - SELECT 1 FROM group_user_mapping gum - WHERE gum.user_uuid = ${userId} AND gum.group_id = g.group_id - ) AND EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) THEN 'manual_and_federated' - WHEN EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) THEN 'federated' - ELSE 'manual' - END AS membership_source - FROM user_groups g - WHERE EXISTS ( - SELECT 1 FROM group_user_mapping gum - WHERE gum.user_uuid = ${userId} AND gum.group_id = g.group_id - ) OR EXISTS ( - SELECT 1 FROM federated_group_user_mapping fgm - WHERE fgm.user_uuid = ${userId} AND fgm.group_id = g.group_id - ) - ORDER BY g.group_name ASC` - ); + stream<record {|string group_id; string group_name; string description?; string membership_source;|}, sql:Error?> groupStream = dbClient->query( + `SELECT g.group_id, g.group_name, g.description, + CASE + WHEN gum.user_uuid IS NOT NULL AND fgm.user_uuid IS NOT NULL THEN 'manual_and_federated' + WHEN fgm.user_uuid IS NOT NULL THEN 'federated' + ELSE 'manual' + END AS membership_source + FROM user_groups g + LEFT JOIN group_user_mapping gum ON gum.group_id = g.group_id AND gum.user_uuid = ${userId} + LEFT JOIN federated_group_user_mapping fgm ON fgm.group_id = g.group_id AND fgm.user_uuid = ${userId} + WHERE gum.user_uuid IS NOT NULL OR fgm.user_uuid IS NOT NULL + ORDER BY g.group_name ASC` + );🤖 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 `@icp_server/modules/storage/user_repository.bal` around lines 154 - 194, Update getGroupsForUser so each group’s manual and federated membership status is computed once through LEFT JOIN-derived flags, then reuse those flags for both filtering and membership_source classification instead of repeating correlated EXISTS subqueries. Preserve the current group results, ordering, and membership_source values.icp_server/tests/sso_config_validation_tests.bal (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also covering an empty
adminValueslist.
testSSOOnlyWithoutAdminClaimIsRejectedcovers an emptyadminClaim, but an SSO-only deployment configured withadminClaimset andadminValues = []would leave no path to an administrator. A parallel test would pin down whether validation rejects that combination.🤖 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 `@icp_server/tests/sso_config_validation_tests.bal` around lines 75 - 83, Add a parallel validation test alongside testSSOOnlyWithoutAdminClaimIsRejected that builds an SSO-only configuration with a valid adminClaim and an empty adminValues list, then asserts validateSSOConfig returns an error. Keep the test focused on ensuring SSO-only deployments cannot be configured without any administrator values.icp_server/tests/auth_tests_v2.bal (1)
253-292: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding an authorization test for the new SSO mapping endpoints.
The new suite covers happy path, validation, immutability, and deletion, but not access control. Since these endpoints govern group membership derived from IdP claims, a case asserting that a caller without the group-management permission is rejected would protect the endpoint contract.
🤖 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 `@icp_server/tests/auth_tests_v2.bal` around lines 253 - 292, Add an authorization test alongside testCreateSSOGroupMapping covering the SSO group-mapping endpoints, using a caller that lacks the group-management permission and asserting the request is rejected with the expected unauthorized status. Reuse the existing authV2Client, endpoint path, and test authentication setup, and ensure the test validates access control rather than mapping behavior.icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql (1)
5-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMigration is not re-runnable, unlike the Oracle sibling.
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_oracle.sqldocuments itself as idempotent and guards every object creation. This script errors on a second run or after a partially applied migration. PostgreSQL supportsIF NOT EXISTSfor tables and indexes, andCREATE OR REPLACE TRIGGER(PG 14+); aDROP TRIGGER IF EXISTSbefore creation works on older versions.♻️ Suggested guards
-CREATE TABLE sso_group_mappings ( +CREATE TABLE IF NOT EXISTS sso_group_mappings ( @@ -CREATE INDEX idx_sgm_org_uuid ON sso_group_mappings(org_uuid); -CREATE INDEX idx_sgm_issuer_claim ON sso_group_mappings(issuer, claim_name, claim_value); -CREATE INDEX idx_sgm_group_id ON sso_group_mappings(group_id); -CREATE INDEX idx_sgm_project_uuid ON sso_group_mappings(project_uuid); -CREATE INDEX idx_sgm_integration_uuid ON sso_group_mappings(integration_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_org_uuid ON sso_group_mappings(org_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_issuer_claim ON sso_group_mappings(issuer, claim_name, claim_value); +CREATE INDEX IF NOT EXISTS idx_sgm_group_id ON sso_group_mappings(group_id); +CREATE INDEX IF NOT EXISTS idx_sgm_project_uuid ON sso_group_mappings(project_uuid); +CREATE INDEX IF NOT EXISTS idx_sgm_integration_uuid ON sso_group_mappings(integration_uuid); +DROP TRIGGER IF EXISTS update_sso_group_mappings_updated_at ON sso_group_mappings; CREATE TRIGGER update_sso_group_mappings_updated_at BEFORE UPDATE ON sso_group_mappings FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); -CREATE TABLE federated_group_user_mapping ( +CREATE TABLE IF NOT EXISTS federated_group_user_mapping ( @@ -CREATE INDEX idx_fgum_user_uuid ON federated_group_user_mapping(user_uuid); -CREATE INDEX idx_fgum_group_id ON federated_group_user_mapping(group_id); -CREATE INDEX idx_fgum_issuer_claim ON federated_group_user_mapping(issuer, claim_name, claim_value); +CREATE INDEX IF NOT EXISTS idx_fgum_user_uuid ON federated_group_user_mapping(user_uuid); +CREATE INDEX IF NOT EXISTS idx_fgum_group_id ON federated_group_user_mapping(group_id); +CREATE INDEX IF NOT EXISTS idx_fgum_issuer_claim ON federated_group_user_mapping(issuer, claim_name, claim_value); +DROP TRIGGER IF EXISTS update_federated_group_user_mapping_updated_at ON federated_group_user_mapping; CREATE TRIGGER update_federated_group_user_mapping_updated_at BEFORE UPDATE ON federated_group_user_mapping FOR EACH ROW EXECUTE FUNCTION update_updated_at_column();🤖 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 `@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql` around lines 5 - 56, Make the PostgreSQL migration idempotent like the Oracle sibling: guard both CREATE TABLE statements with IF NOT EXISTS, guard all CREATE INDEX statements with IF NOT EXISTS, and make each update trigger safely replaceable using CREATE OR REPLACE TRIGGER or DROP TRIGGER IF EXISTS before creation, while preserving the existing definitions and constraints.icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql (2)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused variable.
@seed_admin_idis set but the literal UUID is used at Lines 153 and 170; either use the variable or drop it.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql` at line 148, Remove the unused `@seed_admin_id` assignment, or replace the repeated literal UUIDs at the migration statements around lines 153 and 170 with `@seed_admin_id`; ensure the migration uses one consistent UUID source.
90-133: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
INSERT IGNOREdowngrades all errors to warnings.Beyond duplicate keys, it also suppresses foreign-key failures and truncation, so a partially migrated user set would be reported as success. Since Step 1a already clears conflicting non-admin rows, an explicit
NOT EXISTSguard (as used in the SQL Server script) would keep genuine errors visible.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql` around lines 90 - 133, The user migration INSERT in the dynamic SQL should not use INSERT IGNORE, which suppresses non-duplicate errors. Remove INSERT IGNORE and add an explicit NOT EXISTS guard against conflicting non-admin rows, matching the SQL Server migration’s behavior while allowing genuine foreign-key and truncation errors to surface.frontend/src/pages/EditGroup.tsx (1)
322-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMembership-source label logic is duplicated across pages.
The same
membershipSource→ label mapping is repeated inEditUser.tsxandProfile.tsx. Extracting a small helper (e.g. inpages/access-control/utils) keeps the three views consistent if new source values are added later.🤖 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 `@frontend/src/pages/EditGroup.tsx` around lines 322 - 332, Extract the repeated membershipSource-to-label mapping into a shared helper under pages/access-control/utils, then update EditGroup, EditUser, and Profile to use it for their membership-source labels. Preserve the existing labels for federated, manual_and_federated, and local sources, and ensure all three views use the same helper.icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql (1)
9-23: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueQuote dynamically composed identifiers.
Database names supplied through the configuration variables are concatenated directly into the generated statements. Using
QUOTENAME(@old_db)(and the same for the two target databases) keeps identifier handling robust for names that are not plain identifiers.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql` around lines 9 - 23, The dynamically composed database identifiers in the migration SQL should be safely quoted. Update the generated statements using `@old_db`, `@new_main_db`, and `@new_creds_db` to wrap each database name with QUOTENAME rather than concatenating raw variable values, while preserving the existing query behavior.
🤖 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 `@frontend/RUNTIME_CONFIG.md`:
- Around line 24-27: Update the configuration keys documented in
RUNTIME_CONFIG.md to use VITE_PASSWORD_LOGIN_DISABLED, matching
frontend/public/config.json.example and frontend usage, and add the two SSO
configuration keys documented by the example config. Keep the documented values
and surrounding runtime configuration entries consistent with the example.
In `@frontend/src/pages/AccessControl.tsx`:
- Around line 36-45: Replace the duplicated responsive SSO label JSX in
AccessControl and OrgAccessControl with the existing SSO_TAB_LABEL constant,
matching its use in ProjectAccessControl and ComponentAccessControl.
In `@icp_server/auth_service.bal`:
- Around line 1295-1363: Constrain target-group validation in the mapping
creation flow around targetGroup so a caller authorized at project or
integration scope cannot select groups with role assignments outside that scope.
Either restrict eligible groups to the mapping’s administrative scope or require
org-level permission when the target group has broader-scope assignments, while
preserving the existing organization-membership check.
- Around line 3618-3652: Update grantSuperAdminFromSSOClaims and the surrounding
SSO reconciliation flow so claim-based Super Admin membership is managed as a
federated grant that is re-evaluated on every login, including removing the
membership when the configured admin claim no longer matches. Reuse the existing
federated reconciliation mechanism and preserve the current validation and error
handling.
- Around line 1457-1468: Update the deletion flow around
storage:deleteSSOGroupMapping to also remove matching rows from
federated_group_user_mapping using the mapping’s organization, issuer, claim,
and group identifiers. Perform this cleanup as part of the same operation, while
preserving the existing not-found response and internal-error handling.
- Around line 368-373: Gate the federated group synchronization in the flow
around `syncFederatedGroupsFromSSOClaims` with `federatedAccessControlEnabled`,
so the call and its error handling execute only when the feature is enabled.
Preserve the existing synchronization behavior and internal-server-error
response when the flag is enabled.
- Line 1757: Update the user-groups update flow around
storage:getUserManualGroups so the response distinguishes manual group IDs from
effective membership IDs: rename the current/final group ID fields to
manualGroupIds/addedGroupIds, or derive and return effective IDs including
federated memberships. Ensure federated groups are not reported as removed
solely because they are absent from the manual-group query.
In `@icp_server/Config.toml`:
- Around line 155-157: Update the commented configuration key in the SSO
settings block to use the declared passwordLoginDisabled name instead of
disablePasswordLogin, while preserving its default value and explanatory
comment.
In `@icp_server/custom_auth/OIDC_SETUP_GUIDE.md`:
- Around line 86-88: Replace disablePasswordLogin with passwordLoginDisabled in
the configuration table at icp_server/custom_auth/OIDC_SETUP_GUIDE.md lines
86-88 and in the FAQ instruction at lines 308-308, ensuring both documentation
references match the server configuration key.
In `@icp_server/modules/storage/auth_repository.bal`:
- Around line 758-782: The insert branch in the federated membership
reconciliation loop should treat duplicate-key failures as benign concurrency
outcomes. Update the INSERT execution in the flow around findFederatedMembership
to classify its SQL error with classifySqlError, suppress only the duplicate-key
result, and continue synchronization; propagate all other database errors
unchanged.
- Around line 702-716: Update the federated group membership insertion flow to
handle Oracle identity retrieval, alongside the existing integer and MSSQL
string parsing in the code returning the mapping ID. When Oracle returns a ROWID
instead of the generated ID, read back federated_group_user_mapping.id using the
inserted row’s unique mapping tuple, then log and return that integer ID;
preserve the existing failure path when no valid ID is found.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/README.md`:
- Around line 72-81: The README migration documentation should cover the DDL
performed by both scripts: creation of org_secrets, ensuring
mi_composite_app_artifacts, and adding runtimes.key_id with its foreign key.
Update the prerequisites to require CREATE and ALTER privileges in addition to
DML, and explicitly warn that existing non-admin accounts are deleted and
replaced, advising a backup before migration.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql`:
- Around line 154-166: The Step 2a credential merge must warn when no row is
updated despite the legacy admin user existing. Update the dynamic SQL execution
flow around `@sql` and sp_executesql to capture @@ROWCOUNT, then check for zero
updates alongside the presence of the legacy admin user and print a clear
warning; retain the existing success message for rows that are merged.
- Around line 271-299: Update the org_secrets DDL block to be re-runnable by
guarding CREATE TABLE and runtimes.key_id creation with existence checks
consistent with the later artifact block. Split the ALTER TABLE adding key_id
and the ALTER TABLE adding fk_runtime_key_id into separate sp_executesql
batches, with the foreign-key batch executed only after the column exists.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql`:
- Around line 318-372: Make the Step 5 DDL re-runnable by adding an existence
guard to the org_secrets CREATE TABLE statement and replacing the unconditional
runtimes ALTER TABLE additions with guarded checks for both the key_id column
and fk_runtime_key_id constraint. Preserve the existing schema definitions and
ensure each change can safely be executed after a partial migration.
In `@icp_server/resources/db/init-scripts/mssql_init.sql`:
- Around line 377-388: Update the trigger statements in
icp_server/resources/db/init-scripts/mssql_init.sql: within
trg_sso_group_mappings_updated_at, target the aliased table with UPDATE sgm, and
within the corresponding federated group mapping trigger, use UPDATE fgum. If
the same triggers exist in
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql,
apply the identical alias-target changes there.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_h2.sql`:
- Around line 33-48: Change the user_uuid column definition in
federated_group_user_mapping from VARCHAR(36) to CHAR(36), keeping it NOT NULL
and preserving the existing foreign key to users(user_id) and all other
constraints.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql`:
- Around line 5-78: The SQL Server SSO migration is not idempotent because both
tables and triggers are created unconditionally. In
icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sql:5-78,
wrap sso_group_mappings, federated_group_user_mapping,
trg_sso_group_mappings_updated_at, and
trg_federated_group_user_mapping_updated_at creation in IF OBJECT_ID(...) IS
NULL guards, matching the H2 variant. No direct change is needed in
icp_server/resources/db/migration-scripts/README.md:203-204 because the guards
preserve its idempotency statement.
In `@icp_server/resources/db/migration-scripts/README.md`:
- Around line 203-204: Correct the idempotency statement in the migration README
to exclude engines where add_sso_group_mapping_tables_mssql.sql lacks existence
guards, or update that script’s table and trigger creation to guard against
existing objects. Ensure rerunning the migration after partial failure succeeds
for the supported engines.
In `@icp_server/tests/sso_federated_mapping_tests.bal`:
- Around line 221-229: Ensure teardown always runs after failures in
testSSOGroupMappingStorage, testFederatedMembershipReconciliation, and
testDeletedMappingRemovesMembershipOnNextLogin by moving their cleanup calls
into an on fail block or shared `@test`:AfterEach/@test:AfterGroups hook. In
icp_server/tests/sso_federated_mapping_tests.bal#L221-L229, include
role-mapping, role, project, user, and group deletions; at `#L297-L298` and
`#L361-L362`, include the corresponding user and group deletions.
---
Nitpick comments:
In `@frontend/src/components/LoginForm.tsx`:
- Around line 153-176: Update the `sx` props on the password and SSO buttons in
`LoginForm` to replace hardcoded colors `#1e1e1e`, `#333`, and `#ccc` with
appropriate MUI theme palette tokens, preserving the existing button appearance
and hover behavior.
In `@frontend/src/pages/access-control/SSOMappingsTab.tsx`:
- Around line 71-76: Confirm whether SSO mappings support multiple issuers; if
they do not, update the issuer input in SSOMappingsTab to be read-only while
retaining the window.API_CONFIG.ssoIssuer default and submitted value. If
multiple issuers are supported, preserve editability and add appropriate
validation against accepted issuer values before submission.
- Around line 178-273: Update the SSOMappingsTab mapping list flow around the
mappings rendering to add search and pagination, matching the established
behavior and controls used by UsersTab, GroupsTab, and RolesTab. Filter mappings
by the relevant claim, issuer, or group fields, paginate the filtered results,
and render the paginated collection in both mobile cards and the desktop
ListingTable while preserving empty-state and delete-action behavior.
In `@frontend/src/pages/AccessControl.tsx`:
- Around line 33-34: Replace the positional ORG_TABS.slice(0, 3) and
PROJECT_TABS.slice(0, 2) usage with name-based filtering that excludes
'sso-mappings'. Apply this consistently at all referenced usages while
preserving the existing tab ordering and behavior for the remaining tabs.
- Line 71: Make the tab configuration consistent across scopes by adding
variant="scrollable" and scrollButtons="auto" to the project- and
component-level Tabs instances near the existing org-level configurations.
Update the Tabs elements at all four referenced locations so the extra SSO tab
remains accessible on narrow viewports while preserving their current navigation
and value behavior.
In `@frontend/src/pages/EditGroup.tsx`:
- Around line 322-332: Extract the repeated membershipSource-to-label mapping
into a shared helper under pages/access-control/utils, then update EditGroup,
EditUser, and Profile to use it for their membership-source labels. Preserve the
existing labels for federated, manual_and_federated, and local sources, and
ensure all three views use the same helper.
In `@icp_server/modules/storage/user_repository.bal`:
- Around line 154-194: Update getGroupsForUser so each group’s manual and
federated membership status is computed once through LEFT JOIN-derived flags,
then reuse those flags for both filtering and membership_source classification
instead of repeating correlated EXISTS subqueries. Preserve the current group
results, ordering, and membership_source values.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql`:
- Around line 9-23: The dynamically composed database identifiers in the
migration SQL should be safely quoted. Update the generated statements using
`@old_db`, `@new_main_db`, and `@new_creds_db` to wrap each database name with
QUOTENAME rather than concatenating raw variable values, while preserving the
existing query behavior.
In `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql`:
- Line 148: Remove the unused `@seed_admin_id` assignment, or replace the repeated
literal UUIDs at the migration statements around lines 153 and 170 with
`@seed_admin_id`; ensure the migration uses one consistent UUID source.
- Around line 90-133: The user migration INSERT in the dynamic SQL should not
use INSERT IGNORE, which suppresses non-duplicate errors. Remove INSERT IGNORE
and add an explicit NOT EXISTS guard against conflicting non-admin rows,
matching the SQL Server migration’s behavior while allowing genuine foreign-key
and truncation errors to surface.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sql`:
- Around line 5-48: Make the MySQL migration idempotent by adding IF NOT EXISTS
to both CREATE TABLE statements, sso_group_mappings and
federated_group_user_mapping, matching the H2 migration behavior and allowing
safe reruns after partial failures.
In
`@icp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sql`:
- Around line 5-56: Make the PostgreSQL migration idempotent like the Oracle
sibling: guard both CREATE TABLE statements with IF NOT EXISTS, guard all CREATE
INDEX statements with IF NOT EXISTS, and make each update trigger safely
replaceable using CREATE OR REPLACE TRIGGER or DROP TRIGGER IF EXISTS before
creation, while preserving the existing definitions and constraints.
In `@icp_server/tests/auth_tests_v2.bal`:
- Around line 253-292: Add an authorization test alongside
testCreateSSOGroupMapping covering the SSO group-mapping endpoints, using a
caller that lacks the group-management permission and asserting the request is
rejected with the expected unauthorized status. Reuse the existing authV2Client,
endpoint path, and test authentication setup, and ensure the test validates
access control rather than mapping behavior.
In `@icp_server/tests/sso_config_validation_tests.bal`:
- Around line 75-83: Add a parallel validation test alongside
testSSOOnlyWithoutAdminClaimIsRejected that builds an SSO-only configuration
with a valid adminClaim and an empty adminValues list, then asserts
validateSSOConfig returns an error. Keep the test focused on ensuring SSO-only
deployments cannot be configured without any administrator values.
🪄 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 Plus
Run ID: ae7ed87c-57df-4a1c-9166-102f8f119eab
📒 Files selected for processing (47)
frontend/RUNTIME_CONFIG.mdfrontend/public/config.jsonfrontend/public/config.json.examplefrontend/src/api/auth.tsfrontend/src/api/authQueries.tsfrontend/src/components/LoginForm.tsxfrontend/src/config/api.tsfrontend/src/pages/AccessControl.tsxfrontend/src/pages/CreateGroup.tsxfrontend/src/pages/EditGroup.tsxfrontend/src/pages/EditUser.tsxfrontend/src/pages/Profile.tsxfrontend/src/pages/access-control/SSOMappingsTab.tsxfrontend/src/pages/access-control/UsersTab.tsxfrontend/src/paths.tsicp_server/Config.tomlicp_server/Dependencies.tomlicp_server/auth_service.balicp_server/config.balicp_server/custom_auth/OIDC_SETUP_GUIDE.mdicp_server/init.balicp_server/modules/auth/oidc.balicp_server/modules/storage/auth_repository.balicp_server/modules/storage/user_repository.balicp_server/modules/types/auth_types.balicp_server/modules/types/types.balicp_server/resources/db/icp-1.2.x-to-2.x.x/README.mdicp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sqlicp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sqlicp_server/resources/db/init-scripts/h2_init.sqlicp_server/resources/db/init-scripts/mssql_init.sqlicp_server/resources/db/init-scripts/mysql_init.sqlicp_server/resources/db/init-scripts/oracle_init.sqlicp_server/resources/db/init-scripts/postgresql_init.sqlicp_server/resources/db/migration-scripts/README.mdicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_h2.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mssql.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_mysql.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_oracle.sqlicp_server/resources/db/migration-scripts/add_sso_group_mapping_tables_postgresql.sqlicp_server/tests/Config.tomlicp_server/tests/auth_tests_v2.balicp_server/tests/mock_oidc_provider.balicp_server/tests/oidc_tests.balicp_server/tests/sso_config_validation_tests.balicp_server/tests/sso_federated_mapping_tests.balicp_server/webserver.bal
🛑 Comments failed to post (3)
icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql (2)
154-166: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Seed admin credential merge silently no-ops when the seed row is absent.
If the credentials row for
550e8400-...is missing or the seed admin was recreated with a differentuser_id, Step 2a updates nothing and Step 2b skips the row because the username already exists, leaving the v2 seed password in place. Consider reporting a warning when@@ROWCOUNT = 0while the legacyadminuser exists.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql` around lines 154 - 166, The Step 2a credential merge must warn when no row is updated despite the legacy admin user existing. Update the dynamic SQL execution flow around `@sql` and sp_executesql to capture @@ROWCOUNT, then check for zero updates alongside the presence of the legacy admin user and print a clear warning; retain the existing success message for rows that are merged.
271-299: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Compare guarded vs unguarded DDL across the migration scripts fd -t f 'v1_to_v2_.*\.sql' -x rg -n 'CREATE TABLE|OBJECT_ID|IF NOT EXISTS|ALTER TABLE' {}Repository: wso2/integration-control-plane
Length of output: 765
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the relevant migration sections around the mentioned objects and transaction settings. sed -n '1,430p' icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql # Show the transaction / XACT_ABORT behavior around the batch/exec blocks if present. rg -n 'XACT_ABORT|BEGIN TRANSACTION|COMMIT|ROLLBACK|BEGIN|GO|EXEC sp_executesql|`@sql`' icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sqlRepository: wso2/integration-control-plane
Length of output: 17845
🌐 Web query:
SQL Server T-SQL dynamic SQL add column then add foreign key same batch sp_executesql column not found💡 Result:
In SQL Server, T-SQL batches are compiled in their entirety before execution begins [1][2]. Because of this, any reference to a column in a table must be valid at the time of compilation [1][3]. If you add a column using an ALTER TABLE statement and then attempt to reference that column in the same batch, the compiler will identify that the column does not yet exist and throw an "Invalid column name" error, even if the code would technically be valid during execution [1][4]. Deferred name resolution, which allows SQL Server to delay checking for the existence of table objects until execution, does not apply to columns [2][5][6]. Once a table is referenced, its columns must exist at the time of compilation [2][7]. To resolve this issue, you must separate the DDL statements into distinct batches [1][4]. You have two primary approaches: 1. Use separate sp_executesql calls: This is the standard solution for dynamic SQL [8][9]. Execute the ALTER TABLE command in one call, and then perform the subsequent operations (such as adding the foreign key) in a separate sp_executesql call [8][4]. EXEC sp_executesql N'ALTER TABLE YourTable ADD NewColumn INT'; EXEC sp_executesql N'ALTER TABLE YourTable ADD CONSTRAINT FK_New FOREIGN KEY (NewColumn) REFERENCES OtherTable(ID)'; 2. Use the GO batch separator: If you are writing a static script, insert the GO command between the statements [1][10]. This forces SQL Server to treat the code before and after the GO as separate batches, ensuring the first batch (containing the column addition) is fully executed and committed before the second batch is compiled [1][11]. ALTER TABLE YourTable ADD NewColumn INT; GO ALTER TABLE YourTable ADD CONSTRAINT FK_New FOREIGN KEY (NewColumn) REFERENCES OtherTable(ID); By separating the commands, the second batch is compiled only after the column has been successfully added to the table, allowing the reference to be resolved correctly [1].
Citations:
- 1: https://stackoverflow.com/questions/66094621/cant-use-an-added-column-in-query
- 2: https://learn.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms190686(v=sql.105)
- 3: https://stackoverflow.com/questions/60136323/sql-server-stored-procedure-invalid-column-name
- 4: https://stackoverflow.com/questions/31739574/dynamic-sql-add-new-column-and-work-with-it
- 5: https://learn.microsoft.com/en-us/previous-versions/sql/sql-server-2005/ms190686(v=sql.90)
- 6: https://dba.stackexchange.com/questions/237935/does-sql-server-allow-make-visible-ddl-inside-a-transaction-to-the-transaction
- 7: https://stackoverflow.com/questions/4315861/why-does-microsoft-sql-server-check-columns-but-not-tables-in-stored-procs
- 8: https://stackoverflow.com/questions/45048898/need-help-updating-a-table-with-variable-columns-using-dynamic-sql
- 9: https://stackoverflow.com/questions/51819320/how-to-add-the-column-and-than-update-the-column-in-sql-server-using-procedure
- 10: https://database.guide/using-go-to-structure-t-sql-batches/
- 11: https://stackoverflow.com/questions/73533347/issue-generating-dynamically-a-batch-in-sql-statement
Make the org_secrets block re-runnable and split the DDL batches.
CREATE TABLE org_secrets,ALTER TABLE runtimes ADD key_id, and thefk_runtime_key_idconstraint are inside onesp_executesqlbatch. SQL Server parses the whole batch before executing it, so addingkey_idand then referencing it in the same batch can fail with an invalid column-name error even when the transaction is not yet committed. Guard the table DDL with an existence check orCREATE TABLE IF NOT EXISTSconsistent with the later artifact block, and split addingruntimes.key_idinto onesp_executesqlcall and creating its foreign key into a latersp_executesqlcall.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mssql.sql` around lines 271 - 299, Update the org_secrets DDL block to be re-runnable by guarding CREATE TABLE and runtimes.key_id creation with existence checks consistent with the later artifact block. Split the ALTER TABLE adding key_id and the ALTER TABLE adding fk_runtime_key_id into separate sp_executesql batches, with the foreign-key batch executed only after the column exists.icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql (1)
318-372: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Step 5 DDL is not re-runnable.
CREATE TABLE org_secretsand theruntimesALTERlack existence guards, unlikemi_composite_app_artifactsat Line 344. Because the DDL runs afterCOMMITand cannot be rolled back, a re-run after a partial failure fails on the first already-applied statement. RecommendCREATE TABLE IF NOT EXISTSfororg_secretsand a guarded column/constraint add forruntimes.🤖 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 `@icp_server/resources/db/icp-1.2.x-to-2.x.x/v1_to_v2_mysql.sql` around lines 318 - 372, Make the Step 5 DDL re-runnable by adding an existence guard to the org_secrets CREATE TABLE statement and replacing the unconditional runtimes ALTER TABLE additions with guarded checks for both the key_id column and fk_runtime_key_id constraint. Preserve the existing schema definitions and ensure each change can safely be executed after a partial migration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/pages/NotAuthorized.tsx (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
sxfor the stack spacing.Move
spacing={2}tosxso spacing consistently uses the required styling mechanism.Proposed fix
- <Stack direction="row" spacing={2} justifyContent="center"> + <Stack direction="row" justifyContent="center" sx={{ gap: 2 }}>🤖 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 `@frontend/src/pages/NotAuthorized.tsx` at line 72, Update the Stack element in NotAuthorized to remove the spacing prop and express the equivalent spacing through its sx styling configuration, preserving the existing spacing value and layout behavior.Source: Coding guidelines
🤖 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 `@icp_server/tests/sso_login_gate_tests.bal`:
- Around line 214-247: The test references hasGroup without a visible definition
in sso_login_gate_tests.bal. Define hasGroup within this test module or move it
to a shared helper module and import it, then keep the existing membership
assertion in testMembershipWithoutRolesIsNotAuthorization using that accessible
helper.
---
Nitpick comments:
In `@frontend/src/pages/NotAuthorized.tsx`:
- Line 72: Update the Stack element in NotAuthorized to remove the spacing prop
and express the equivalent spacing through its sx styling configuration,
preserving the existing spacing value and layout behavior.
🪄 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 Plus
Run ID: ee4c90b0-2cf9-4a51-8fe2-58f65b9ec2bb
📒 Files selected for processing (11)
frontend/src/auth/AuthContext.tsxfrontend/src/auth/tokenManager.tsfrontend/src/config/api.tsfrontend/src/config/routes.tsxfrontend/src/pages/NotAuthorized.tsxfrontend/src/pages/OIDCCallback.tsxfrontend/src/pages/access-control/SSOMappingsTab.tsxfrontend/src/paths.tsicp_server/auth_service.balicp_server/tests/mock_oidc_provider.balicp_server/tests/sso_login_gate_tests.bal
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/pages/access-control/SSOMappingsTab.tsx
- frontend/src/config/api.ts
- icp_server/auth_service.bal
| function testMembershipWithoutRolesIsNotAuthorization() returns error? { | ||
| string uniqueValue = uuid:createType1AsString(); | ||
| string userId = uuid:createType1AsString(); | ||
| string username = "sso-gate-roleless-" + uniqueValue; | ||
| string claimValue = "roleless-" + uniqueValue; | ||
|
|
||
| string groupId = check storage:createGroup({ | ||
| groupName: "SSO Gate Roleless Group " + uniqueValue, | ||
| description: "Temporary group with no role mappings" | ||
| }); | ||
| _ = check storage:createUserV2(userId, username, "SSO Gate Roleless", [], true); | ||
| string mappingId = check storage:createSSOGroupMapping({ | ||
| issuer: GATE_TEST_ISSUER, | ||
| claimName: "groups", | ||
| claimValue: claimValue, | ||
| groupId: groupId | ||
| }); | ||
|
|
||
| types:OIDCIdTokenClaims claims = buildGateClaims(userId, [claimValue]); | ||
| string[] permissions = check resolveLoginPermissions(userId, username, claims); | ||
|
|
||
| test:assertTrue(hasGroup(check storage:getUserGroups(userId), groupId), | ||
| "The mapping must still grant the federated membership"); | ||
| test:assertEquals(permissions.length(), 0, | ||
| "A group with no role mappings must resolve to no permissions"); | ||
| test:assertFalse(isLoginAuthorized(true, permissions), | ||
| "Membership of a group that grants nothing must not count as authorization"); | ||
|
|
||
| check storage:deleteSSOGroupMapping(mappingId, storage:DEFAULT_ORG_ID); | ||
| check storage:reconcileFederatedGroupUserMappings( | ||
| storage:DEFAULT_ORG_ID, GATE_TEST_ISSUER, userId, []); | ||
| check storage:deleteUserV2(userId, "test-cleanup-user"); | ||
| check storage:deleteGroup(groupId); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the definition of hasGroup used by sso_login_gate_tests.bal.
rg -n 'function hasGroup' icp_server/testsRepository: wso2/integration-control-plane
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file outline/usages =="
rg -n 'hasGroup|getUserGroups|module "icp_server/tests"|import "icp_server/tests"' icp_server/tests/sso_login_gate_tests.bal icp_server/tests/sso_federated_mapping_tests.bal
echo
echo "== import headers =="
sed -n '1,80p' icp_server/tests/sso_login_gate_tests.bal
echo "---"
sed -n '1,80p' icp_server/tests/sso_federated_mapping_tests.bal
echo
echo "== relevant helper definition =="
sed -n '370,390p' icp_server/tests/sso_federated_mapping_tests.balRepository: wso2/integration-control-plane
Length of output: 8000
Move hasGroup to a shared helper module or define it in this test.
icp_server/tests/sso_login_gate_tests.bal imports icp_server.auth, icp_server.storage, and icp_server.types, but the hasGroup helper is defined in a different test file (sso_federated_mapping_tests.bal), so this call is not in the visible module scope.
🤖 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 `@icp_server/tests/sso_login_gate_tests.bal` around lines 214 - 247, The test
references hasGroup without a visible definition in sso_login_gate_tests.bal.
Define hasGroup within this test module or move it to a shared helper module and
import it, then keep the existing membership assertion in
testMembershipWithoutRolesIsNotAuthorization using that accessible helper.
1 participant
Add SSO group mapping from IdP claims to ICP groups
Summary
ICP can already authenticate users through OIDC SSO, but authorization has always
been resolved purely from ICP database state — users are placed in ICP groups by
hand, groups receive scoped ICP roles, and roles carry permissions.
This PR lets an SSO deployment drive group membership from the identity
provider. An IdP group or role claim value is mapped to an existing ICP group;
on every SSO login the user's federated memberships are reconciled from their
token claims. ICP remains the source of role definitions and scoping.
Two things are deliberately not federated, and reviewers should read the rest
of this description with them in mind:
groups a user belongs to. The group's
group_role_mappingrows continue todecide what that membership actually grants.
group_role_mapping— a group-role mapping created at project level grantsproject-level access only. Mapping an IdP value to a group does not widen or
narrow that.
Deployment modes
Three configuration values produce three supported states (plus one rejected
combination):
ssoEnabledpasswordLoginDisabledfederatedAccessControlEnabledtruefalsefalsetruetruefalsetruetruetruetruefalsetrueIn mode 3, user↔group membership is owned by the IdP: manual membership
additions are blocked, while removals remain allowed (removals only reduce
access and are needed to revoke a stale super admin or clean up rows created
before the flag was enabled). Group, role and permission management stays fully
enabled in every mode.
New configuration
All values go in
icp_server/Config.tomlalongside the existing SSO block.Added by this PR
passwordLoginDisabledfalse/auth/loginwith403and stops advertising local user-store capabilities.ssoAdminClaim""groups. Supports dotted paths (realm_access.roles).ssoAdminValues[]["icp-platform-admins"].federatedAccessControlEnabledfalsepasswordLoginDisabled = true.Existing SSO keys, for context
ssoEnabled,ssoIssuer,ssoAuthorizationEndpoint,ssoTokenEndpoint,ssoLogoutEndpoint,ssoJwksUrl,ssoClientId,ssoClientSecret,ssoRedirectUri,ssoUsernameClaim(defaultemail),ssoScopes(default["openid","email","profile"]),ssoAllowInsecureTLS.To receive group claims, the IdP must issue them in the ID token and the scope
must be requested — e.g.
ssoScopes = ["openid","email","profile","groups"].Example: mode 3
Startup validation
Invalid combinations fail fast with a message naming the offending key:
passwordLoginDisabled = truerequiresssoEnabled = true, a non-emptyssoAdminClaim, and at least one non-emptyssoAdminValuesentry.federatedAccessControlEnabled = truerequires bothssoEnabled = trueand
passwordLoginDisabled = true.Derived frontend runtime config
Generated into the served
config.jsonbywebserver.bal— not set by hand:VITE_SSO_ENABLED,VITE_SSO_ISSUER,VITE_PASSWORD_LOGIN_DISABLED,VITE_FEDERATED_ACCESS_CONTROL_ENABLED.How it works
Login flow
JWKS and issuer.
ssoUsernameClaim.ssoAdminClaimmatches
ssoAdminValues, the user is idempotently added to the built-inSuper Adminsgroup as a manual membership. This grant is intentionallysticky: it is not removed when the claim later disappears, only when another
super admin removes it. It is exempt from the mode-3 guard, since it is the
bootstrap path.
matched against the token claims, and
federated_group_user_mappingisreconciled inside a transaction scoped to exactly one
(org, issuer, user):existing rows get
last_seen_atrefreshed, missing rows are inserted, stalerows are deleted. Manual memberships and rows owned by other issuers are
never touched. A reconciliation failure fails the login rather than issuing a
token from stale authorization state.
reflect the current state.
Claim extraction
The full validated ID token payload is retained, and claim values are read by
dotted path:
groups,roles,realm_access.roles,resource_access.icp.roles. String arrays are the primary format; a singlestring is normalized to one value; non-string entries in mixed arrays are
ignored; missing or unsupported shapes yield an empty list and log at debug
level.
Permission resolution
A new view
v_effective_group_user_mappingunions manual memberships fromgroup_user_mappingwith SSO-owned memberships fromfederated_group_user_mapping. The project, integration and environment accessviews are rebuilt on top of it, and
getUserEffectivePermissions()/getAllUserPermissions()read through it — sohasPermission(),hasAnyPermission(), JWT scope generation and the/runtime-statusWebSocketupgrade path all observe federated memberships without individual changes.
isUserInGroup()deliberately stays manual-only: the super admin bootstrap usesit to decide whether to write the sticky manual row.
Membership source
User and group membership responses expose
manual,federated, ormanual_and_federated, so the UI can badge SSO-managed rows and render themread-only in local removal controls. The manual user-group update endpoint
compares against manual memberships only, so an effective SSO membership can
never be silently converted into a local assignment.
Scoped mappings
Mappings can be created at organization, project or integration level.
sso_group_mappingscarries nullableproject_uuid/integration_uuidwith acheck constraint that an integration requires a project.
The mapping's scope records where the mapping is administered, letting
project or integration admins manage the mappings relevant to them. It does not
itself scope permissions — login sync applies all mappings for the issuer
regardless of the level they were created at. The access a user ends up with is
determined by the target group's
group_role_mappingrows, exactly as formanually assigned users.
So the worked example — an IdP
france_engrole granting theEngineersrolewithin the
Franceproject — is set up by scoping the Engineers group's role tothe France project, then mapping
france_engto that group.Listing shows mappings from every scope with their scope displayed; the UI offers
create/delete only for mappings matching the current level, while the API
authorizes at the mapping's own scope (so an org-level permission holder can
still manage lower-scoped mappings, since org scope subsumes them).
Mappings are immutable — created and deleted, never updated, mirroring how
group-role mappings behave. Changing a mapping means delete plus create, and
deleting a mapping removes the memberships it created on each affected user's
next login. There is no enable/disable toggle.
Groups and roles remain org-level entities, so the "create a group from the
mapping dialog" shortcut is offered at org level only; the group dropdown always
lists all org groups.
Database changes
Two tables and one view, applied to fresh-install schemas for H2, MySQL,
PostgreSQL, MSSQL and Oracle:
sso_group_mappings— maps a validated IdP claim value to an existing ICPgroup, with optional project/integration administration scope. Unique on
(org_uuid, issuer, claim_name, claim_value, group_id), deliberatelyexcluding the scope columns: the same claim-to-group pair has an identical
sync effect at any scope, so duplicates across scopes would be redundant. This
also avoids cross-database nullable-unique inconsistencies, since MSSQL treats
NULLs as equal in unique indexes while the others do not.
federated_group_user_mapping— SSO-owned memberships, unique on(org_uuid, issuer, user_uuid, group_id, claim_name, claim_value).v_effective_group_user_mapping— the manual ∪ federated union describedabove.
ICP groups are never auto-created from IdP values.
For existing deployments there is one consolidated migration per dialect,
add_sso_group_mapping_tables_<db>.sql, which creates the tables and thenrecreates the access views in the correct order.
API
user_mgt:manage_groupsoruser_mgt:update_group_roles.POSTand
DELETEauthorize at the mapping's own scope;GETuses an org-level readcheck, since every level shows the same list.
POSTaccepts optionalprojectUuid/integrationUuid, validating that theproject and integration exist and that the integration belongs to the project.
GETreturnsgroupName,projectUuid,integrationUuid,projectNameandintegrationNamefor display.PUT— mappings are immutable and aPUTreturns405.409on duplicates (naming the existing mapping's scope),404forunknown group/project/integration or mapping,
400for empty issuer, claimname or claim value and for an integration without a project,
403when thecaller lacks the permission at the requested scope.
Also added: a server-side
403onPOST /auth/orgs/{orgHandle}/userswheneverpasswordLoginDisabled = true(previously only hidden in the UI), and a403onmanual membership additions in mode 3.
UI
integration level, shown when SSO is enabled.
groups, takes the IdP group or role value as free text, and requiresselecting an existing ICP group. At project/integration level the scope is
pre-filled and locked.
offered only for mappings at the current level.
federated-only memberships are read-only in local removal controls.
controls remain.
disabled.
Verification
to MySQL, MSSQL and PostgreSQL, and both paths were confirmed to converge
on the same object set (tables, views, foreign keys, check constraint, unique
constraints, indexes).
PostgreSQL using the real authorization-code flow: JIT provisioning, super
admin grant plus idempotency and stickiness, mapping CRUD across all scopes,
cross-scope duplicate conflicts, immutability, membership sync, stale-row
removal on claim removal, manual memberships surviving sync, combined
membership badges, delete-mapping-removes-membership, mode-3 guards including
the bootstrap exemption, project-scoped roles granting at project scope but not
org scope,
membershipSourceresolution, and the/runtime-statusWebSocketupgrade succeeding for a user whose permissions come only from a federated
group.
Not covered: Oracle (scripts added, no instance available to exercise them),
and the UI was verified at the API and static-gating level rather than by
clicking through a browser.