diff --git a/src/src_auth_perms_sync/cli.py b/src/src_auth_perms_sync/cli.py index f4ff9db..8b5c16e 100644 --- a/src/src_auth_perms_sync/cli.py +++ b/src/src_auth_perms_sync/cli.py @@ -733,7 +733,7 @@ def require_set_input_file(maps_path: Path) -> None: raise SystemExit( "set input file does not exist: " f"{maps_path}\n" - "Run `uv run src-auth-perms-sync get` to create the default maps.yaml, " + "Run `src-auth-perms-sync get` to create the default maps.yaml, " "or pass a path to an existing maps file." ) diff --git a/src/src_auth_perms_sync/orgs/sync.py b/src/src_auth_perms_sync/orgs/sync.py index a37a876..7829891 100644 --- a/src/src_auth_perms_sync/orgs/sync.py +++ b/src/src_auth_perms_sync/orgs/sync.py @@ -618,7 +618,7 @@ def cmd_sync_saml_orgs( """Create/update Sourcegraph orgs from discovered SAML groups. Org names are deterministic and config-free: the Sourcegraph-safe form - of `synced--`. Invalid org-name + of `synced--`. Invalid org-name characters are converted to `-`; any resulting name collision fails before mutation so we never merge unrelated SAML groups accidentally. The `synced-` prefix marks tool ownership: only orgs carrying it are @@ -778,6 +778,7 @@ def _record_saml_group_user_memberships( targets, collisions, membership.provider_config_id, + membership.provider_display_name, membership.group_name, user, ) @@ -787,10 +788,11 @@ def _record_target_organization_membership( targets: dict[str, organization_types.TargetOrganization], collisions: set[str], provider_config_id: str, + provider_display_name: str, group_name: str, user: shared_types.SamlGroupUser | shared_types.ScopedSamlGroupUser, ) -> None: - organization_name = organization_name_for_saml_group(provider_config_id, group_name) + organization_name = organization_name_for_saml_group(provider_display_name, group_name) existing_target = targets.get(organization_name) if existing_target is not None and ( existing_target.provider_config_id != provider_config_id diff --git a/src/src_auth_perms_sync/permissions/full_set.py b/src/src_auth_perms_sync/permissions/full_set.py index 2faa4c3..07fb2b6 100644 --- a/src/src_auth_perms_sync/permissions/full_set.py +++ b/src/src_auth_perms_sync/permissions/full_set.py @@ -649,7 +649,7 @@ def _finish_full_set_apply_with_backup( log.info( "To roll back the explicit-permissions state captured in " "the before-snapshot, run:\n" - " uv run src-auth-perms-sync restore --restore-path %s --apply", + " src-auth-perms-sync restore --restore-path %s --apply", before_path, ) diff --git a/src/src_auth_perms_sync/shared/saml_groups.py b/src/src_auth_perms_sync/shared/saml_groups.py index 548eadb..bfdf0a5 100644 --- a/src/src_auth_perms_sync/shared/saml_groups.py +++ b/src/src_auth_perms_sync/shared/saml_groups.py @@ -41,17 +41,17 @@ _ORGANIZATION_NAME_DASH_RUN_RE = re.compile(r"-+") -def organization_name_for_saml_group(provider_config_id: str, group_name: str) -> str: +def organization_name_for_saml_group(provider_display_name: str, group_name: str) -> str: """Return the deterministic synced org name for one SAML group. - Shape: `synced--`. + Shape: `synced--`. """ - provider_part = _organization_name_part(provider_config_id, "auth provider configID") + provider_part = _organization_name_part(provider_display_name, "auth provider display name") group_part = _organization_name_part(group_name, "SAML group name") organization_name = f"{SYNCED_ORGANIZATION_NAME_PREFIX}{provider_part}-{group_part}" if len(organization_name) > ORGANIZATION_NAME_MAX_LENGTH: raise SystemExit( - f"FATAL: generated org name for configID={provider_config_id!r} " + f"FATAL: generated org name for auth provider displayName={provider_display_name!r} " f"group={group_name!r} is {len(organization_name)} characters; " f"Sourcegraph org names must be <= {ORGANIZATION_NAME_MAX_LENGTH}." ) @@ -198,9 +198,9 @@ def saml_group_memberships_for_user( providers_by_account_key: dict[tuple[str, str], shared_types.AuthProvider], attribute_names_by_provider: SamlGroupsAttributeNameByProvider, ) -> tuple[shared_types.SamlGroupMembership, ...]: - """Extract one user's distinct (configID, group) SAML memberships.""" + """Extract one user's distinct SAML provider/group memberships.""" memberships: list[shared_types.SamlGroupMembership] = [] - seen: set[tuple[str, str]] = set() + seen: set[tuple[str, str, str]] = set() for account in user["externalAccounts"]["nodes"]: if account["serviceType"] != SAML_SERVICE_TYPE: continue @@ -213,12 +213,13 @@ def saml_group_memberships_for_user( account["clientID"], ) for group_name in extract_saml_groups(account.get("accountData"), attribute_name): - membership_key = (provider["configID"], group_name) + membership_key = (provider["configID"], provider["displayName"], group_name) if membership_key in seen: continue memberships.append( shared_types.SamlGroupMembership( provider_config_id=provider["configID"], + provider_display_name=provider["displayName"], group_name=group_name, ) ) diff --git a/src/src_auth_perms_sync/shared/types.py b/src/src_auth_perms_sync/shared/types.py index 20c1df0..ff6184a 100644 --- a/src/src_auth_perms_sync/shared/types.py +++ b/src/src_auth_perms_sync/shared/types.py @@ -70,6 +70,7 @@ class MutationCounts: @dataclass(frozen=True, slots=True) class SamlGroupMembership: provider_config_id: str + provider_display_name: str group_name: str diff --git a/tests/e2e/fixtures/set-users-sync-saml-orgs-noop/after.json b/tests/e2e/fixtures/set-users-sync-saml-orgs-noop/after.json new file mode 100644 index 0000000..6315d99 --- /dev/null +++ b/tests/e2e/fixtures/set-users-sync-saml-orgs-noop/after.json @@ -0,0 +1,114 @@ +{ + "endpoint": "https://fixture.sourcegraph.test", + "authProviders": [ + { + "serviceType": "builtin", + "serviceID": "", + "clientID": "", + "displayName": "Builtin username/password", + "isBuiltin": true, + "configID": "" + }, + { + "serviceType": "saml", + "serviceID": "http://www.okta.com/test123", + "clientID": "https://sourcegraph.test/.auth/saml/metadata", + "displayName": "Okta", + "isBuiltin": false, + "configID": "okta" + } + ], + "externalServices": [ + { + "id": 1, + "kind": "GITHUB", + "displayName": "GitHub", + "url": "https://github.com/", + "config": "{}" + } + ], + "users": [ + { + "id": 1, + "username": "org_admin_09990", + "builtinAuth": true, + "createdAt": "2026-01-01T00:00:00Z", + "emails": [ + { + "email": "org_admin_09990@perms-sync.test", + "verified": true + } + ], + "externalAccounts": [] + }, + { + "id": 2, + "username": "test_user_09991", + "builtinAuth": false, + "createdAt": "2026-01-01T00:00:00Z", + "emails": [ + { + "email": "test_user_09991@perms-sync.test", + "verified": true + } + ], + "externalAccounts": [ + { + "serviceType": "saml", + "serviceID": "http://www.okta.com/test123", + "clientID": "https://sourcegraph.test/.auth/saml/metadata", + "accountData": { + "Values": { + "groups": { + "Values": [ + { + "Value": "engineering" + } + ] + } + } + } + } + ] + }, + { + "id": 3, + "username": "test_user_09992", + "builtinAuth": true, + "createdAt": "2026-01-01T00:00:00Z", + "emails": [ + { + "email": "test_user_09992@perms-sync.test", + "verified": true + } + ], + "externalAccounts": [] + } + ], + "repos": [ + { + "id": 101, + "name": "test-repo-49981", + "externalServiceID": 1, + "explicitPermissionsUsers": [ + "test_user_09991" + ], + "pendingBindIDs": [] + } + ], + "organizations": [ + { + "id": 2, + "name": "synced-Okta-engineering", + "members": [ + "test_user_09991" + ] + }, + { + "id": 1, + "name": "synced-okta-engineering", + "members": [] + } + ], + "currentUsername": "org_admin_09990" +} diff --git a/tests/e2e/fixtures/set-users-sync-saml-orgs-scoped/after.json b/tests/e2e/fixtures/set-users-sync-saml-orgs-scoped/after.json index 4ebbac7..001f52b 100644 --- a/tests/e2e/fixtures/set-users-sync-saml-orgs-scoped/after.json +++ b/tests/e2e/fixtures/set-users-sync-saml-orgs-scoped/after.json @@ -97,7 +97,7 @@ "organizations": [ { "id": 2, - "name": "synced-okta-engineering", + "name": "synced-Okta-engineering", "members": ["test_user_09991"] }, { diff --git a/tests/e2e/fixtures/sync-saml-orgs-full-cleanup/after.json b/tests/e2e/fixtures/sync-saml-orgs-full-cleanup/after.json index 7c5b5b2..2e927e0 100644 --- a/tests/e2e/fixtures/sync-saml-orgs-full-cleanup/after.json +++ b/tests/e2e/fixtures/sync-saml-orgs-full-cleanup/after.json @@ -97,7 +97,7 @@ "organizations": [ { "id": 2, - "name": "synced-okta-engineering", + "name": "synced-Okta-engineering", "members": [ "test_user_09991" ] diff --git a/tests/e2e/fixtures/sync-saml-orgs-users-scoped/after.json b/tests/e2e/fixtures/sync-saml-orgs-users-scoped/after.json index 62850bc..9c937f4 100644 --- a/tests/e2e/fixtures/sync-saml-orgs-users-scoped/after.json +++ b/tests/e2e/fixtures/sync-saml-orgs-users-scoped/after.json @@ -97,7 +97,7 @@ "organizations": [ { "id": 2, - "name": "synced-okta-engineering", + "name": "synced-Okta-engineering", "members": [ "test_user_09991" ] diff --git a/tests/tests.yaml b/tests/tests.yaml index ceec101..6d6cfc2 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -1374,11 +1374,13 @@ cases: # bigO: O(1) # reads: # users: 1 - # writes: none + # writes: 4 mutations description: >- - Scoped org sync is idempotent: with repo grants and synced org - membership already converged, a set --users --sync-saml-orgs apply - issues zero mutations and changes nothing. + Scoped org sync converges from the old configID-based org name to + the display-name-based org name: repo grants are already converged, + but org sync creates the display-name org, removes the creator's + auto-membership, adds the scoped user, and removes that user from + the old synced org. modes: - local args: @@ -1387,7 +1389,7 @@ cases: - test_user_09991 sync_saml_orgs: true apply: true - expectedMutations: 0 + expectedMutations: 4 sync-saml-orgs-full-cleanup: # scope: diff --git a/tests/unit/test_saml_groups.py b/tests/unit/test_saml_groups.py index 67932b9..a439478 100644 --- a/tests/unit/test_saml_groups.py +++ b/tests/unit/test_saml_groups.py @@ -230,10 +230,14 @@ def test_compact_saml_group_users_keeps_only_org_sync_fields(self) -> None: username="alice", saml_group_memberships=( shared_types.SamlGroupMembership( - provider_config_id="okta", group_name="engineering" + provider_config_id="okta", + provider_display_name="SAML", + group_name="engineering", ), shared_types.SamlGroupMembership( - provider_config_id="okta", group_name="platform" + provider_config_id="okta", + provider_display_name="SAML", + group_name="platform", ), ), ) @@ -243,11 +247,11 @@ def test_compact_saml_group_users_keeps_only_org_sync_fields(self) -> None: def test_organization_name_for_saml_group_uses_synced_prefix_and_sanitizes(self) -> None: self.assertEqual( - "synced-okta-eng-team", - saml_groups.organization_name_for_saml_group("okta", "eng team!"), + "synced-Okta-Prod-eng-team", + saml_groups.organization_name_for_saml_group("Okta Prod", "eng team!"), ) self.assertTrue( - saml_groups.is_synced_organization_name("synced-okta-eng-team"), + saml_groups.is_synced_organization_name("synced-Okta-Prod-eng-team"), ) self.assertFalse(saml_groups.is_synced_organization_name("okta-eng-team")) @@ -308,7 +312,9 @@ def test_compact_scoped_saml_group_users_keeps_users_without_groups(self) -> Non username="alice", saml_group_memberships=( shared_types.SamlGroupMembership( - provider_config_id="okta", group_name="engineering" + provider_config_id="okta", + provider_display_name="SAML", + group_name="engineering", ), ), # The manually created org is NOT tool-managed: it must