diff --git a/src/openhound_github/kinds/edges.py b/src/openhound_github/kinds/edges.py index 99c3d8f..9f717eb 100644 --- a/src/openhound_github/kinds/edges.py +++ b/src/openhound_github/kinds/edges.py @@ -39,7 +39,6 @@ HAS_EXTERNAL_IDENTITY = "GH_HasExternalIdentity" HAS_SAML_IDENTITY_PROVIDER = "GH_HasSamlIdentityProvider" MAPS_TO_USER = "GH_MapsToUser" -SYNCED_TO_GH_USER = "GH_SyncedTo" # Normalized OpenGraph-SCIM edges SCIM_CONTAINS = "SCIM_Contains" diff --git a/src/openhound_github/kinds/nodes.py b/src/openhound_github/kinds/nodes.py index b8efdc1..022e8f8 100644 --- a/src/openhound_github/kinds/nodes.py +++ b/src/openhound_github/kinds/nodes.py @@ -81,6 +81,4 @@ SCIM_ROLE = "SCIM_Role" # External node kinds used only as correlation endpoints -AZ_USER = "AZUser" OKTA_USER = "Okta_User" -PINGONE_USER = "PingOne_User" diff --git a/src/openhound_github/models/external_identity.py b/src/openhound_github/models/external_identity.py index 1006102..4e945f7 100644 --- a/src/openhound_github/models/external_identity.py +++ b/src/openhound_github/models/external_identity.py @@ -2,14 +2,13 @@ from openhound.core.asset import BaseAsset, EdgeDef, NodeDef from openhound.core.models.entries_dataclass import ( - ConditionalEdgePath, Edge, EdgePath, EdgeProperties, ) from pydantic import BaseModel, ConfigDict, Field -from openhound_github.graph import GHEdgeProperties, GHNode, GHNodeProperties +from openhound_github.graph import GHNode, GHNodeProperties from openhound_github.kinds import edges as ek from openhound_github.kinds import nodes as nk from openhound_github.main import app @@ -18,9 +17,6 @@ DEFAULT_GITHUB_DEPLOYMENT_ID, ENTRA_OBJECT_ID_CLAIM, SAML_CONTRACT_VERSION, - detect_foreign_idp, - foreign_user_kind, - foreign_user_matchers, github_saml_service_provider_id, saml_account_match_values, saml_attribute_match_values, @@ -113,13 +109,6 @@ class User(BaseModel): description="External identity maps to a user", traversable=False, ), - EdgeDef( - start=nk.EXTERNAL_IDENTITY, - end=nk.USER, - kind=ek.SYNCED_TO_GH_USER, - description="Foreign IdP user is synced to a GitHub user", - traversable=True, - ), EdgeDef( start=nk.SAML_SERVICE_PROVIDER, end=nk.USER, @@ -150,12 +139,19 @@ def node_id(self) -> str: @property def as_node(self) -> GHNode: + display_name = ( + self.saml_identity.username + if self.saml_identity and self.saml_identity.username + else self.scim_identity.username + if self.scim_identity and self.scim_identity.username + else self.guid or self.node_id + ) return GHNode( kinds=[nk.EXTERNAL_IDENTITY], properties=GHExternalIdentityProperties( - name=self.guid or self.node_id, - displayname=self.guid or self.node_id, + name=display_name, + displayname=display_name, node_id=self.node_id, guid=self.guid, saml_identity_username=self.saml_identity.username @@ -209,66 +205,6 @@ def idp(self) -> dict: "environment_type": environment_type, } - @property - def _maps_to_user_edges(self): - foreign_idp_type, foreign_env_id = detect_foreign_idp( - issuer=self.idp["issuer"], - sso_url=self.idp["sso_url"], - ) - foreign_kind = foreign_user_kind(foreign_idp_type) - - foreign_username = None - if self.saml_identity and self.saml_identity.username: - foreign_username = self.saml_identity.username - elif self.scim_identity and self.scim_identity.username: - foreign_username = self.scim_identity.username - - matchers = foreign_user_matchers( - foreign_kind, - foreign_env_id, - foreign_username, - self.saml_identity.attributes if self.saml_identity else [], - ) - if foreign_kind and matchers: - yield Edge( - kind=ek.MAPS_TO_USER, - start=EdgePath(value=self.node_id, match_by="id"), - end=ConditionalEdgePath( - kind=foreign_kind, - property_matchers=matchers, - ), - properties=EdgeProperties(traversable=False), - ) - - # SyncedToGHUser: foreign IdP user → GitHub user (traversable, with composition) - if matchers and self.user and self.user.id: - composition_matchers = [ - matcher for matcher in matchers if matcher.key in {"name", "objectid"} - ] - composition_predicates = " OR ".join( - f"n.{matcher.key} = '{matcher.value}'" - for matcher in composition_matchers - ) - q = ( - f"MATCH p=()<-[:GH_SyncedToEnvironment]-(:GH_SamlIdentityProvider)" - f"-[:GH_HasExternalIdentity]->(:GH_ExternalIdentity)" - f"-[:GH_MapsToUser]->(n) " - f"WHERE {composition_predicates} RETURN p" - ) - yield Edge( - kind=ek.SYNCED_TO_GH_USER, - start=ConditionalEdgePath( - kind=foreign_kind, - property_matchers=matchers, - ), - end=EdgePath(value=self.user.id, match_by="id"), - properties=GHEdgeProperties( - traversable=True, - composed=True, - query_composition=q, - ), - ) - @property def service_provider_node_id(self) -> str | None: return github_saml_service_provider_id( @@ -363,5 +299,3 @@ def edges(self): end=EdgePath(value=self.user.id, match_by="id"), properties=EdgeProperties(traversable=False), ) - - yield from self._maps_to_user_edges diff --git a/src/openhound_github/models/saml_helpers.py b/src/openhound_github/models/saml_helpers.py index 7e8691a..caac256 100644 --- a/src/openhound_github/models/saml_helpers.py +++ b/src/openhound_github/models/saml_helpers.py @@ -3,27 +3,14 @@ from typing import Any from urllib.parse import quote, urlparse -from openhound.core.models.entries_dataclass import EdgeProperties, PropertyMatch - -from openhound_github.kinds import nodes as nk +from openhound.core.models.entries_dataclass import EdgeProperties SAML_CONTRACT_VERSION = "opengraph-saml-v0.3.0" ENTRA_OBJECT_ID_CLAIM = "http://schemas.microsoft.com/identity/claims/objectidentifier" -ENTRA_TENANT_ID_CLAIM = "http://schemas.microsoft.com/identity/claims/tenantid" DEFAULT_GITHUB_DEPLOYMENT_ID = "github.com" DEFAULT_GITHUB_WEB_ORIGIN = "https://github.com" -_FOREIGN_USER_KIND = { - "entra": nk.AZ_USER, - "okta": nk.OKTA_USER, - "pingone": nk.PINGONE_USER, -} -_FOREIGN_USER_ENVIRONMENT_PROPERTY = { - nk.OKTA_USER: "tenant_domain", - nk.PINGONE_USER: "environmentid", -} - @dataclass class SAMLRelationshipEdgeProperties(EdgeProperties): @@ -67,47 +54,6 @@ def detect_foreign_idp( return None, None -def foreign_user_kind(foreign_idp_type: str | None) -> str: - return _FOREIGN_USER_KIND.get(foreign_idp_type or "", "") - - -def foreign_user_matchers( - foreign_kind: str | None, - foreign_environment_id: str | None, - foreign_username: str | None, - saml_attributes: list[Any] | None = None, -) -> list[PropertyMatch]: - """Return tenant-scoped matchers for foreign IdP user correlation.""" - if foreign_kind == nk.AZ_USER: - tenant_ids = saml_attribute_match_values( - saml_attributes or [], - ENTRA_TENANT_ID_CLAIM, - ) - object_ids = saml_attribute_match_values( - saml_attributes or [], - ENTRA_OBJECT_ID_CLAIM, - ) - if ( - not foreign_environment_id - or len(tenant_ids) != 1 - or len(object_ids) != 1 - or tenant_ids[0].casefold() != foreign_environment_id.casefold() - ): - return [] - return [ - PropertyMatch(key="tenantid", value=tenant_ids[0].upper()), - PropertyMatch(key="objectid", value=object_ids[0].upper()), - ] - - environment_property = _FOREIGN_USER_ENVIRONMENT_PROPERTY.get(foreign_kind or "") - if not environment_property or not foreign_environment_id or not foreign_username: - return [] - return [ - PropertyMatch(key=environment_property, value=foreign_environment_id), - PropertyMatch(key="name", value=foreign_username.upper()), - ] - - def github_deployment_context(host: str) -> tuple[str, str]: """Return a stable deployment ID and browser origin for a GitHub API host.""" parsed = urlparse(host) diff --git a/tests/test_external_identity_tenant_matching.py b/tests/test_external_identity_tenant_matching.py deleted file mode 100644 index 8502ab7..0000000 --- a/tests/test_external_identity_tenant_matching.py +++ /dev/null @@ -1,242 +0,0 @@ -from dataclasses import asdict - -import pytest - -from openhound_github.kinds import edges as ek -from openhound_github.models.external_identity import ExternalIdentity - - -class _Lookup: - def __init__( - self, - issuer: str, - sso_url: str | None, - environment_type: str = "org", - ): - self.issuer = issuer - self.sso_url = sso_url - self.environment_type = environment_type - - def idp_for_environment(self, slug: str): - return [ - ( - f"idp:{slug}", - self.issuer, - self.sso_url, - f"environment:{slug}", - slug, - self.environment_type, - ) - ] - - -class _MissingIdpLookup: - def idp_for_environment(self, slug: str): - return [] - - -def _identity( - slug: str, - issuer: str, - sso_url: str | None, - saml_identity: dict, - environment_type: str = "org", -) -> ExternalIdentity: - identity = ExternalIdentity.model_validate( - { - "guid": f"guid:{slug}", - "id": f"external-identity:{slug}", - "samlIdentity": saml_identity, - "scimIdentity": None, - "user": {"id": f"github-user:{slug}", "login": "duplicate"}, - "environment_slug": slug, - } - ) - identity._lookup = _Lookup(issuer, sso_url, environment_type) - return identity - - -def _foreign_user_edges(identity: ExternalIdentity): - edges = list(identity.edges) - maps_to = next( - edge - for edge in edges - if edge.kind == ek.MAPS_TO_USER - and getattr(edge.end, "property_matchers", None) - ) - synced_to = next(edge for edge in edges if edge.kind == ek.SYNCED_TO_GH_USER) - return maps_to, synced_to - - -def test_external_identity_scopes_duplicate_okta_username_by_tenant() -> None: - first = _foreign_user_edges( - _identity( - "first-org", - "http://www.okta.com/example", - "https://first.example.okta.com/app/github/sso/saml", - {"username": "duplicate@example.com"}, - ) - ) - second = _foreign_user_edges( - _identity( - "second-org", - "http://www.okta.com/example", - "https://second.example.okta.com/app/github/sso/saml", - {"username": "duplicate@example.com"}, - environment_type="enterprise", - ) - ) - - for edges, tenant_domain in ( - (first, "first.example.okta.com"), - (second, "second.example.okta.com"), - ): - for endpoint in (edges[0].end, edges[1].start): - assert endpoint.kind == "Okta_User" - assert { - matcher.key: matcher.value for matcher in endpoint.property_matchers - } == { - "tenant_domain": tenant_domain, - "name": "DUPLICATE@EXAMPLE.COM", - } - - emitted_endpoint = asdict(first[0].end) - assert emitted_endpoint["kind"] == "Okta_User" - assert emitted_endpoint["match_by"] == "property" - assert [ - {"key": matcher["key"], "value": matcher["value"]} - for matcher in emitted_endpoint["property_matchers"] - ] == [ - {"key": "tenant_domain", "value": "first.example.okta.com"}, - {"key": "name", "value": "DUPLICATE@EXAMPLE.COM"}, - ] - - -def test_external_identity_uses_pingone_environment_scope() -> None: - maps_to, synced_to = _foreign_user_edges( - _identity( - "pingone-org", - "https://auth.pingone.com/ping-environment-id/saml20/idp/sso", - None, - {"username": "opaque-pingone-subject"}, - ) - ) - - for endpoint in (maps_to.end, synced_to.start): - assert endpoint.kind == "PingOne_User" - assert { - matcher.key: matcher.value for matcher in endpoint.property_matchers - } == { - "environmentid": "ping-environment-id", - "name": "OPAQUE-PINGONE-SUBJECT", - } - - -def _entra_identity( - tenant_claim: str | None = "11111111-2222-3333-4444-555555555555", - object_id_claim: str | None = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", -) -> ExternalIdentity: - attributes = [] - if tenant_claim is not None: - attributes.append( - { - "name": "http://schemas.microsoft.com/identity/claims/tenantid", - "value": tenant_claim, - } - ) - if object_id_claim is not None: - attributes.append( - { - "name": ( - "http://schemas.microsoft.com/identity/claims/objectidentifier" - ), - "value": object_id_claim, - } - ) - return _identity( - "entra-org", - "https://sts.windows.net/11111111-2222-3333-4444-555555555555/", - ( - "https://login.microsoftonline.com/" - "11111111-2222-3333-4444-555555555555/saml2" - ), - { - "username": "opaque-pairwise-name-id", - "attributes": attributes, - }, - ) - - -def test_entra_external_identity_uses_explicit_saml_claims() -> None: - maps_to, synced_to = _foreign_user_edges(_entra_identity()) - - for endpoint in (maps_to.end, synced_to.start): - assert endpoint.kind == "AZUser" - assert { - matcher.key: matcher.value for matcher in endpoint.property_matchers - } == { - "tenantid": "11111111-2222-3333-4444-555555555555".upper(), - "objectid": "AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE", - } - - -@pytest.mark.parametrize( - ("tenant_claim", "object_id_claim"), - [ - (None, "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), - ("11111111-2222-3333-4444-555555555555", None), - ( - "99999999-8888-7777-6666-555555555555", - "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", - ), - ], -) -def test_entra_external_identity_omits_unsafe_foreign_user_edges( - tenant_claim: str | None, - object_id_claim: str | None, -) -> None: - edges = list(_entra_identity(tenant_claim, object_id_claim).edges) - - assert ek.SYNCED_TO_GH_USER not in {edge.kind for edge in edges} - assert not any( - edge.kind == ek.MAPS_TO_USER - and getattr(edge.end, "property_matchers", None) - for edge in edges - ) - - -def test_external_identity_omits_okta_edges_without_tenant_scope() -> None: - edges = list( - _identity( - "unscoped-org", - "http://www.okta.com/example", - None, - {"username": "user@example.com"}, - ).edges - ) - - assert ek.SYNCED_TO_GH_USER not in {edge.kind for edge in edges} - assert not any( - edge.kind == ek.MAPS_TO_USER - and getattr(edge.end, "property_matchers", None) - for edge in edges - ) - - -def test_external_identity_missing_idp_fallback_includes_environment_type() -> None: - identity = _identity( - "missing-idp", - "http://www.okta.com/example", - "https://example.okta.com/app/github/sso/saml", - {"username": "user@example.com"}, - ) - identity._lookup = _MissingIdpLookup() - - assert identity.idp == { - "id": None, - "issuer": None, - "sso_url": None, - "environment_node_id": None, - "environment_name": None, - "environment_type": None, - } diff --git a/tests/test_saml_models.py b/tests/test_saml_models.py index 0f0ed67..1a79af3 100644 --- a/tests/test_saml_models.py +++ b/tests/test_saml_models.py @@ -55,6 +55,27 @@ def _saml_account_edge(identity: ExternalIdentity): return next(edge for edge in identity.edges if edge.kind == ek.SAML_HAS_ACCOUNT) +def test_external_identity_prefers_saml_username_for_display_name() -> None: + identity = _identity_with_lookup() + + assert identity.as_node.properties.name == "Alice@example.com" + assert identity.as_node.properties.displayname == "Alice@example.com" + + +def test_external_identity_uses_scim_username_without_saml_identity() -> None: + identity = _identity_with_lookup(samlIdentity=None) + + assert identity.as_node.properties.name == "scim-only@example.com" + assert identity.as_node.properties.displayname == "scim-only@example.com" + + +def test_external_identity_falls_back_to_guid_without_foreign_username() -> None: + identity = _identity_with_lookup(samlIdentity=None, scimIdentity=None) + + assert identity.as_node.properties.name == "guid-1" + assert identity.as_node.properties.displayname == "guid-1" + + def test_normalized_saml_nodes_expose_contract_metadata() -> None: service_provider = SamlServiceProvider( id="IDP_1", @@ -211,11 +232,12 @@ def test_saml_provider_replays_snake_case_fields_and_deployment_metadata() -> No assert provider.as_node.properties.github_web_origin == "https://github.example.com" -def test_saml_account_edge_emits_contract_evidence_without_changing_legacy_edges() -> None: +def test_saml_account_edge_emits_contract_evidence_and_direct_user_mapping() -> None: identity = _identity_with_lookup() edges = list(identity.edges) account_edge = next(edge for edge in edges if edge.kind == ek.SAML_HAS_ACCOUNT) + maps_to_edges = [edge for edge in edges if edge.kind == ek.MAPS_TO_USER] assert account_edge.properties.schema_contract_version == SAML_CONTRACT_VERSION assert account_edge.properties.match_values == [ @@ -236,7 +258,48 @@ def test_saml_account_edge_emits_contract_evidence_without_changing_legacy_edges assert account_edge.start.value == "github:saml:sp:org:acme" assert ek.HAS_EXTERNAL_IDENTITY in {edge.kind for edge in edges} - assert ek.MAPS_TO_USER in {edge.kind for edge in edges} + assert len(maps_to_edges) == 1 + assert maps_to_edges[0].start.value == "external-identity-1" + assert maps_to_edges[0].end.value == "USER_1" + assert maps_to_edges[0].end.match_by == "id" + + +def test_external_identity_does_not_emit_legacy_foreign_user_correlations() -> None: + identity = _identity_with_lookup() + identity._lookup = SimpleNamespace( + idp_for_environment=lambda _slug: [ + ( + "IDP_1", + "http://www.okta.com/example", + "https://example.okta.com/app/github/sso/saml", + "ORG_1", + "Acme", + "org", + ) + ] + ) + + edges = list(identity.edges) + maps_to_edges = [edge for edge in edges if edge.kind == ek.MAPS_TO_USER] + + assert "GH_SyncedTo" not in {edge.kind for edge in edges} + assert len(maps_to_edges) == 1 + assert maps_to_edges[0].end.value == "USER_1" + assert not getattr(maps_to_edges[0].end, "property_matchers", None) + + +def test_external_identity_missing_idp_fallback_includes_environment_type() -> None: + identity = _identity_with_lookup() + identity._lookup = SimpleNamespace(idp_for_environment=lambda _slug: []) + + assert identity.idp == { + "id": None, + "issuer": None, + "sso_url": None, + "environment_node_id": None, + "environment_name": None, + "environment_type": None, + } def test_org_scim_only_identity_does_not_emit_saml_account_edge() -> None: