Skip to content

Commit 8599bbf

Browse files
Use SAML provider display names for synced orgs (#45)
## Summary - use sanitized SAML auth provider display names in synced org names instead of configID - carry provider display names through parsed SAML memberships - update org sync fixtures for the renamed target orgs ## Test plan - uv run tests/run.py --local --------- Co-authored-by: Amp <amp@ampcode.com>
1 parent ce4b351 commit 8599bbf

4 files changed

Lines changed: 82 additions & 14 deletions

File tree

src/src_auth_perms_sync/orgs/sync.py

Lines changed: 65 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737

3838
_ALREADY_MEMBER_TEXT = "user is already a member of the organization"
3939
_ORGANIZATION_EXISTS_TEXT = "organization name is already taken"
40+
_ONLY_MEMBER_TEXT = "remove the only member of an organization"
4041

4142
# Re-exported here for callers that think in org-sync terms; the naming
4243
# rule lives in shared.saml_groups so user compaction can share it.
@@ -424,6 +425,8 @@ def _apply_organization_sync(
424425
"remove",
425426
parallelism,
426427
worker_pool,
428+
current_user=sync_state.current_user,
429+
targets=sync_state.targets,
427430
)
428431
return _OrganizationApplyResult(
429432
creates=create_counts,
@@ -1272,6 +1275,9 @@ def _apply_user_changes(
12721275
change_kind: organization_types.OrganizationChangeKind,
12731276
parallelism: int,
12741277
worker_pool: ThreadPoolExecutor | None = None,
1278+
*,
1279+
current_user: organization_types.OrgMember | None = None,
1280+
targets: dict[str, organization_types.TargetOrganization] | None = None,
12751281
) -> shared_types.MutationCounts:
12761282
if not changes:
12771283
return shared_types.MutationCounts()
@@ -1292,6 +1298,8 @@ def apply_change(change: organization_types.OrganizationUserChange) -> None:
12921298
change,
12931299
current_states[change.organization_name],
12941300
change_kind,
1301+
current_user=current_user,
1302+
target=targets.get(change.organization_name) if targets is not None else None,
12951303
)
12961304

12971305
def record_result(
@@ -1353,6 +1361,9 @@ def _apply_user_change(
13531361
change: organization_types.OrganizationUserChange,
13541362
state: organization_types.OrganizationState,
13551363
change_kind: organization_types.OrganizationChangeKind,
1364+
*,
1365+
current_user: organization_types.OrgMember | None = None,
1366+
target: organization_types.TargetOrganization | None = None,
13561367
) -> None:
13571368
if state.id is None:
13581369
raise RuntimeError(f"organization {change.organization_name!r} has no ID")
@@ -1382,10 +1393,60 @@ def _apply_user_change(
13821393
organization_name=change.organization_name,
13831394
username=change.username,
13841395
):
1385-
client.graphql(
1386-
queries.MUTATION_REMOVE_USER_FROM_ORGANIZATION,
1387-
{"organization": state.id, "user": change.user_id},
1388-
)
1396+
try:
1397+
client.graphql(
1398+
queries.MUTATION_REMOVE_USER_FROM_ORGANIZATION,
1399+
{"organization": state.id, "user": change.user_id},
1400+
)
1401+
except src.GraphQLError as exception:
1402+
if not _should_retry_last_member_removal(
1403+
exception, change, state, target, current_user
1404+
):
1405+
raise
1406+
_remove_last_other_member_via_current_user(client, change, state, current_user)
1407+
1408+
1409+
def _should_retry_last_member_removal(
1410+
exception: src.GraphQLError,
1411+
change: organization_types.OrganizationUserChange,
1412+
state: organization_types.OrganizationState,
1413+
target: organization_types.TargetOrganization | None,
1414+
current_user: organization_types.OrgMember | None,
1415+
) -> bool:
1416+
return (
1417+
_ONLY_MEMBER_TEXT in str(exception)
1418+
and current_user is not None
1419+
and target is not None
1420+
and not target.desired_members_by_id
1421+
and change.user_id != current_user["id"]
1422+
and len(state.members_by_id) == 1
1423+
and change.user_id in state.members_by_id
1424+
)
1425+
1426+
1427+
def _remove_last_other_member_via_current_user(
1428+
client: src.SourcegraphClient,
1429+
change: organization_types.OrganizationUserChange,
1430+
state: organization_types.OrganizationState,
1431+
current_user: organization_types.OrgMember | None,
1432+
) -> None:
1433+
if current_user is None:
1434+
raise RuntimeError("current user is required for last-member org cleanup")
1435+
log.info(
1436+
" Sourcegraph forbids removing %s as the only member of org %s; "
1437+
"temporarily adding current user %s so the stale synced org can be emptied.",
1438+
change.username,
1439+
change.organization_name,
1440+
current_user["username"],
1441+
)
1442+
temporary_membership = organization_types.OrganizationUserChange(
1443+
organization_name=change.organization_name,
1444+
user_id=current_user["id"],
1445+
username=current_user["username"],
1446+
)
1447+
_apply_user_change(client, temporary_membership, state, "add")
1448+
_apply_user_change(client, change, state, "remove")
1449+
_apply_user_change(client, temporary_membership, state, "remove")
13891450

13901451

13911452
def _snapshot_from_states(

tests/e2e/case_runner.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,10 @@ def _add_user_to_organization(self, variables: dict[str, object]) -> None:
721721
def _remove_user_from_organization(self, variables: dict[str, object]) -> None:
722722
organization = self._organization_by_graphql_id(variables["organization"])
723723
username = self._username_from_user_graphql_id(variables["user"])
724-
self._organization_members_by_id[organization["id"]].discard(username)
724+
members = self._organization_members_by_id[organization["id"]]
725+
if len(members) == 1 and username != self._current_username:
726+
raise src.GraphQLError("you can't remove the only member of an organization")
727+
members.discard(username)
725728
self._mutation_count += 1
726729

727730
def _organization_by_graphql_id(self, organization_id_value: object) -> FixtureOrganization:

tests/run.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@
138138
query TestSamlAuthProviders {
139139
site {
140140
authProviders {
141-
nodes { serviceType serviceID clientID configID }
141+
nodes { serviceType serviceID clientID displayName configID }
142142
}
143143
}
144144
}
@@ -1951,12 +1951,14 @@ def run_seeded_org_sync_check(self, environment: dict[str, str]) -> None:
19511951
for group in groups:
19521952
members_by_group.setdefault(group, set()).add(username)
19531953
expected_members_by_organization = {
1954-
organization_name_for_saml_group(provider["configID"], group): usernames
1954+
organization_name_for_saml_group(provider["displayName"], group): usernames
19551955
for group, usernames in members_by_group.items()
19561956
}
19571957

19581958
seeded_group = min(members_by_group)
1959-
seeded_organization = organization_name_for_saml_group(provider["configID"], seeded_group)
1959+
seeded_organization = organization_name_for_saml_group(
1960+
provider["displayName"], seeded_group
1961+
)
19601962
# The sync must REMOVE this member: no SAML group puts them in the org.
19611963
unjustified_member = next(
19621964
username

tests/tests.yaml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1379,8 +1379,8 @@ cases:
13791379
Scoped org sync converges from the old configID-based org name to
13801380
the display-name-based org name: repo grants are already converged,
13811381
but org sync creates the display-name org, removes the creator's
1382-
auto-membership, adds the scoped user, and removes that user from
1383-
the old synced org.
1382+
auto-membership, adds the scoped user, and empties the old synced
1383+
org via a temporary current-user membership.
13841384
modes:
13851385
- local
13861386
args:
@@ -1389,7 +1389,7 @@ cases:
13891389
- test_user_09991
13901390
sync_saml_orgs: true
13911391
apply: true
1392-
expectedMutations: 4
1392+
expectedMutations: 6
13931393

13941394
sync-saml-orgs-full-cleanup:
13951395
# scope:
@@ -1401,19 +1401,21 @@ cases:
14011401
# reads:
14021402
# users: all
14031403
# orgMembers: all
1404-
# writes: 4 mutations
1404+
# writes: 6 mutations
14051405
description: >-
14061406
Standalone full org sync converges synced orgs both ways: creates the
14071407
org a SAML group requires (cleaning up the creator's auto-membership)
14081408
and empties - but never deletes - a synced org whose SAML group
1409-
disappeared from every user's assertion.
1409+
disappeared from every user's assertion, using a temporary
1410+
current-user membership when Sourcegraph rejects removing another
1411+
user's final org membership.
14101412
modes:
14111413
- local
14121414
args:
14131415
command: sync_saml_orgs
14141416
full: true
14151417
apply: true
1416-
expectedMutations: 4
1418+
expectedMutations: 6
14171419

14181420
sync-saml-orgs-users-scoped:
14191421
# scope:

0 commit comments

Comments
 (0)