From d7b9ff41a87d77ec772c810c3dc1baef33885354 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 06:50:10 +0000 Subject: [PATCH 01/19] feat(okta): add group push-mapping service methods Add create/delete/list helpers for Okta group push mappings on OktaService, the link primitive the app group lifecycle plugins need to bind an Access-managed Okta group to an external group. delete PATCHes the mapping INACTIVE before DELETE (Okta rejects deleting active mappings) and defaults deleteTargetGroup to False so unlinking never removes the external group. Timeouts propagate like the other data-returning OktaService methods. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/services/okta_service.py | 97 ++++++++++++++++++++++++ tests/test_okta_service.py | 139 +++++++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+) diff --git a/api/services/okta_service.py b/api/services/okta_service.py index ac6bb59a..5529511a 100644 --- a/api/services/okta_service.py +++ b/api/services/okta_service.py @@ -463,6 +463,103 @@ async def _list_owners_for_group(groupId: str) -> list[User]: return asyncio.run(_list_owners_for_group(groupId)) + # https://developer.okta.com/docs/api/openapi/okta-management/management/tag/GroupPushMapping/#tag/GroupPushMapping/operation/createGroupPushMapping + def create_group_push_mapping(self, appId: str, sourceGroupId: str, targetGroupId: str) -> dict[str, Any]: + if not appId: + raise ValueError("appId is required") + if not sourceGroupId: + raise ValueError("sourceGroupId is required") + if not targetGroupId: + raise ValueError("targetGroupId is required") + + async def _create() -> dict[str, Any]: + async with self._okta_client() as client: + request_executor = client.get_request_executor() + request, error = await request_executor.create_request( + method="POST", + url="/api/v1/apps/{appId}/group-push/mappings".format(appId=appId), + body={"sourceGroupId": sourceGroupId, "targetGroupId": targetGroupId, "status": "ACTIVE"}, + headers={}, + oauth=False, + ) + + if error is not None: + raise Exception(error) + + response, error = await OktaService._retry(request_executor.execute, request) + if error is not None: + raise Exception(error) + assert response is not None + return response.get_body() + + return asyncio.run(_create()) + + # https://developer.okta.com/docs/api/openapi/okta-management/management/tag/GroupPushMapping/#tag/GroupPushMapping/operation/deleteGroupPushMapping + def delete_group_push_mapping(self, appId: str, mappingId: str, deleteTargetGroup: bool = False) -> None: + if not appId: + raise ValueError("appId is required") + if not mappingId: + raise ValueError("mappingId is required") + + async def _request(method: str, url: str, body: dict[str, Any]) -> None: + async with self._okta_client() as client: + request_executor = client.get_request_executor() + request, error = await request_executor.create_request( + method=method, url=url, body=body, headers={}, oauth=False + ) + if error is not None: + raise Exception(error) + _, error = await OktaService._retry(request_executor.execute, request) + if error is not None: + # A 404 means the mapping is already gone; deletion is idempotent, so treat it as + # success. This makes a retry or replay (e.g. after a partial earlier attempt) + # safe instead of failing on the now-absent mapping. + if getattr(error, "status", None) == 404: + return + raise Exception(error) + + async def _delete() -> None: + base = f"/api/v1/apps/{appId}/group-push/mappings/{mappingId}" + # Deactivate first; Okta rejects deleting an ACTIVE mapping. + await _request("PATCH", base, {"status": "INACTIVE"}) + await _request("DELETE", f"{base}?deleteTargetGroup={str(deleteTargetGroup).lower()}", {}) + + asyncio.run(_delete()) + + # https://developer.okta.com/docs/api/openapi/okta-management/management/tag/GroupPushMapping/#tag/GroupPushMapping/operation/listGroupPushMappings + def list_group_push_mappings(self, appId: str) -> list[dict[str, Any]]: + if not appId: + raise ValueError("appId is required") + + async def _list() -> list[dict[str, Any]]: + async with self._okta_client() as client: + request_executor = client.get_request_executor() + request, error = await request_executor.create_request( + method="GET", + url="/api/v1/apps/{appId}/group-push/mappings".format(appId=appId), + body={}, + headers={}, + oauth=False, + ) + if error is not None: + raise Exception(error) + response, error = await OktaService._retry(request_executor.execute, request) + if error is not None: + raise Exception(error) + assert response is not None + # The Group Push Mappings endpoint is paginated via Link headers; follow the + # `next` link so callers see every mapping, not just the first page. Otherwise + # link discovery silently misses any mapping past the first page. + mappings = list(response.get_body() or []) + while response.has_next(): + more, error = await OktaService._retry(response.next) + if error is not None: + raise Exception(error) + mappings.extend(more or []) + return mappings + + return asyncio.run(_list()) + # Wrapper class for the Okta API user model class User: diff --git a/tests/test_okta_service.py b/tests/test_okta_service.py index 27a458ee..e18136d7 100644 --- a/tests/test_okta_service.py +++ b/tests/test_okta_service.py @@ -1,9 +1,11 @@ import asyncio import threading from concurrent.futures import ThreadPoolExecutor +from contextlib import asynccontextmanager from typing import Any from unittest.mock import AsyncMock, MagicMock, patch +import pytest from okta.models.group_rule import GroupRule as OktaGroupRuleType from okta.request_executor import RequestExecutor @@ -165,3 +167,140 @@ def tracking_set_session(executor: Any, session: Any) -> None: # Each concurrent call got its own executor on its own event loop; nothing shared. assert len({id(executor) for executor in executors}) == call_count assert len({id(loop) for loop in loops}) == call_count + + +def _mock_okta_executor(mocker, svc, *, body=None, retry_side_effect=None): + """Wire svc._okta_client() to a mock request executor. Returns the executor. + + By default every _retry call resolves to a single successful response; pass + retry_side_effect to script per-call outcomes (e.g. to simulate an error status).""" + executor = MagicMock() + executor.create_request = AsyncMock(return_value=(MagicMock(), None)) + response = MagicMock() + response.get_body.return_value = body if body is not None else {} + response.has_next.return_value = False # single page by default; pagination tested separately + client = MagicMock() + client.get_request_executor.return_value = executor + + @asynccontextmanager + async def fake_client(): + yield client + + mocker.patch.object(svc, "_okta_client", fake_client) + if retry_side_effect is not None: + mocker.patch.object(OktaService, "_retry", AsyncMock(side_effect=retry_side_effect)) + else: + mocker.patch.object(OktaService, "_retry", AsyncMock(return_value=(response, None))) + return executor + + +def test_create_group_push_mapping_posts_active_mapping(mocker): + svc = OktaService() + executor = _mock_okta_executor(mocker, svc, body={"id": "map-123"}) + + result = svc.create_group_push_mapping("app-1", "src-1", "tgt-1") + + assert result == {"id": "map-123"} + _, kwargs = executor.create_request.call_args + assert kwargs["method"] == "POST" + assert kwargs["url"] == "/api/v1/apps/app-1/group-push/mappings" + assert kwargs["body"] == {"sourceGroupId": "src-1", "targetGroupId": "tgt-1", "status": "ACTIVE"} + + +@pytest.mark.parametrize("args", [("", "src", "tgt"), ("app", "", "tgt"), ("app", "src", "")]) +def test_create_group_push_mapping_requires_args(args): + svc = OktaService() + with pytest.raises(ValueError): + svc.create_group_push_mapping(*args) + + +def test_delete_group_push_mapping_deactivates_then_deletes(mocker): + svc = OktaService() + executor = _mock_okta_executor(mocker, svc) + + svc.delete_group_push_mapping("app-1", "map-1", deleteTargetGroup=True) + + calls = executor.create_request.call_args_list + assert len(calls) == 2 + assert calls[0].kwargs["method"] == "PATCH" + assert calls[0].kwargs["url"] == "/api/v1/apps/app-1/group-push/mappings/map-1" + assert calls[0].kwargs["body"] == {"status": "INACTIVE"} + assert calls[1].kwargs["method"] == "DELETE" + assert calls[1].kwargs["url"] == "/api/v1/apps/app-1/group-push/mappings/map-1?deleteTargetGroup=true" + + +@pytest.mark.parametrize("args", [("", "map-1"), ("app-1", "")]) +def test_delete_group_push_mapping_requires_args(args): + svc = OktaService() + with pytest.raises(ValueError): + svc.delete_group_push_mapping(*args) + + +def test_delete_group_push_mapping_idempotent_on_404(mocker): + # A 404 means the mapping is already gone; deletion is idempotent, so the operation must + # succeed rather than raise (covers retry/replay after a partial failure). + svc = OktaService() + not_found = MagicMock(status=404) + # PATCH (deactivate) succeeds; DELETE returns 404 (already gone). + _mock_okta_executor(mocker, svc, retry_side_effect=[(MagicMock(), None), (None, not_found)]) + + svc.delete_group_push_mapping("app-1", "map-1") # must not raise + + +def test_delete_group_push_mapping_raises_on_non_404(mocker): + # A genuine failure (e.g. 500) must still surface, not be swallowed by the 404 tolerance. + svc = OktaService() + server_error = MagicMock(status=500) + _mock_okta_executor(mocker, svc, retry_side_effect=[(MagicMock(), None), (None, server_error)]) + + with pytest.raises(Exception): + svc.delete_group_push_mapping("app-1", "map-1") + + +def test_list_group_push_mappings_returns_body(mocker): + svc = OktaService() + executor = _mock_okta_executor(mocker, svc, body=[{"id": "m1", "sourceGroupId": "g1"}]) + + result = svc.list_group_push_mappings("app-1") + + assert result == [{"id": "m1", "sourceGroupId": "g1"}] + _, kwargs = executor.create_request.call_args + assert kwargs["method"] == "GET" + assert kwargs["url"] == "/api/v1/apps/app-1/group-push/mappings" + + +def test_list_group_push_mappings_follows_pagination(mocker): + # The Group Push Mappings endpoint is paginated via Link headers; list_group_push_mappings + # must follow the `next` link and return every page, not just the first. + svc = OktaService() + executor = MagicMock() + executor.create_request = AsyncMock(return_value=(MagicMock(), None)) + client = MagicMock() + client.get_request_executor.return_value = executor + + @asynccontextmanager + async def fake_client(): + yield client + + mocker.patch.object(svc, "_okta_client", fake_client) + + page1 = MagicMock() + page1.get_body.return_value = [{"id": "m1"}] + page1.has_next.side_effect = [True, False] # one further page, then exhausted + # _retry is invoked first for the initial execute (-> page1 response), then for page1.next + # (-> the second page's body), matching the low-level OktaAPIResponse pagination contract. + mocker.patch.object( + OktaService, + "_retry", + AsyncMock(side_effect=[(page1, None), ([{"id": "m2"}], None)]), + ) + + result = svc.list_group_push_mappings("app-1") + + assert result == [{"id": "m1"}, {"id": "m2"}] + + +def test_list_group_push_mappings_requires_app_id(): + svc = OktaService() + with pytest.raises(ValueError): + svc.list_group_push_mappings("") From c5c851ccdd607c2a98e12bc44473025cf19d3310 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 06:57:40 +0000 Subject: [PATCH 02/19] feat(plugins): make group config hooks app-config aware; add config helpers The Google plugin needs an app's configuration when describing and validating a group's config (e.g. to surface an app-level email pattern), and an operation needs to tell whether a config edit actually changed. So: - thread the owning app's plugin config into the group-config-properties and group-config validation hooks (and their wrappers). The hookspec args are additive and pluggy passes only the args an impl declares, so existing implementations keep working unchanged. - add is_plugin_config_changed(old, new, plugin_id), comparing only a plugin's configuration (not status, which the plugin writes itself). - add set_config_value alongside set_status_value so plugins backfill inferred config without touching the raw "configuration" key directly. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/plugins/app_group_lifecycle.py | 79 +++++++++++++++++++++--- tests/test_app_group_lifecycle_plugin.py | 29 +++++++++ 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/api/plugins/app_group_lifecycle.py b/api/plugins/app_group_lifecycle.py index 51550fcc..d4802411 100644 --- a/api/plugins/app_group_lifecycle.py +++ b/api/plugins/app_group_lifecycle.py @@ -107,7 +107,7 @@ def validate_plugin_app_config(self, config: dict[str, Any], plugin_id: str | No @hookspec def get_plugin_group_config_properties( - self, plugin_id: str | None + self, plugin_id: str | None, app_config: dict[str, Any] ) -> dict[str, AppGroupLifecyclePluginConfigProperty] | None: """ Return the schema for app group configuration plugin data, a mapping of property IDs to descriptors. @@ -115,15 +115,27 @@ def get_plugin_group_config_properties( Args: plugin_id: If provided, only the plugin matching this ID should respond. If None, all plugins may respond. + app_config: The app-level configuration for this plugin, so the returned schema + can reflect app-specific constraints (e.g. validation patterns). + Empty when no app context is supplied. An implementation that + doesn't need it may omit this parameter from its signature; pluggy + only passes the arguments the implementation declares. """ @hookspec - def validate_plugin_group_config(self, config: dict[str, Any], plugin_id: str | None) -> dict[str, str] | None: + def validate_plugin_group_config( + self, config: dict[str, Any], app_config: dict[str, Any], plugin_id: str | None + ) -> dict[str, str] | None: """ Validate app group plugin config before saving. Args: - config: The configuration to validate. + config: The group configuration to validate. + app_config: The app-level configuration for this plugin, for validating the + group config against app-level settings (e.g. an allowed pattern). + An implementation that doesn't need it may omit this parameter from + its signature; pluggy only passes the arguments the implementation + declares. plugin_id: If provided, only the plugin matching this ID should respond. If None, all plugins may respond. @@ -414,6 +426,45 @@ def set_status_value(app_or_group: App | AppGroup, status_property_name: str, va app_or_group.plugin_data[plugin_id] = asdict(data) +def set_config_value(app_or_group: App | AppGroup, config_property_name: str, value: Any, plugin_id: str) -> None: + """ + Set a configuration value for a particular app group lifecycle plugin. + Should only be called by the plugin itself (e.g. to backfill config inferred + from an external system during reconciliation). + + Args: + app_or_group: The app or group to set the configuration value for. + config_property_name: The name of the configuration property to set. + value: The value to set. + plugin_id: The ID of the plugin. + """ + data = _get_data_for_plugin(app_or_group.plugin_data, plugin_id) + data.configuration[config_property_name] = value + app_or_group.plugin_data[plugin_id] = asdict(data) + + +def is_plugin_config_changed( + old_plugin_data: dict[str, Any], new_plugin_data: dict[str, Any], plugin_id: str +) -> bool: + """ + Determine whether a particular app group lifecycle plugin's configuration differs + between two plugin_data payloads. Only the plugin's configuration is compared; status + differences are ignored (plugins write their own status, and treating those writes as + a change would re-trigger configuration-driven reconciliation in a loop). + + Args: + old_plugin_data: The existing plugin_data. + new_plugin_data: The candidate plugin_data. + plugin_id: The ID of the plugin whose configuration to compare. + + Returns: + True if the plugin's configuration differs between the two payloads. + """ + old_config = _get_data_for_plugin(old_plugin_data, plugin_id).configuration + new_config = _get_data_for_plugin(new_plugin_data, plugin_id).configuration + return old_config != new_config + + def merge_app_lifecycle_plugin_data(app_or_group: App | AppGroup, old_plugin_data: dict[str, Any]) -> None: """ Update the app lifecycle plugin data on the new app or group object by merging with the plugin data from the existing object. @@ -476,19 +527,24 @@ def get_app_group_lifecycle_plugin_app_config_properties( def get_app_group_lifecycle_plugin_group_config_properties( - plugin_id: str, + plugin_id: str, app_plugin_data: dict[str, Any] | None = None ) -> dict[str, AppGroupLifecyclePluginConfigProperty]: """ Get the group-level configuration properties for a particular app group lifecycle plugin. Args: plugin_id: The ID of the plugin which should respond. + app_plugin_data: The owning app's plugin data, so the schema can reflect app-level + settings (e.g. validation patterns). Defaults to empty (no app context). Returns: A dictionary mapping configuration property names to schemas. """ + app_configuration = _get_data_for_plugin(app_plugin_data or {}, plugin_id).configuration hook = get_app_group_lifecycle_hook() - return _get_hook_call_response(hook.get_plugin_group_config_properties, plugin_id) + return _get_hook_call_response( + hook.get_plugin_group_config_properties, plugin_id, app_config=app_configuration + ) def validate_app_group_lifecycle_plugin_app_config(plugin_data: dict[str, Any], plugin_id: str) -> dict[str, str]: @@ -507,20 +563,29 @@ def validate_app_group_lifecycle_plugin_app_config(plugin_data: dict[str, Any], return _get_hook_call_response(hook.validate_plugin_app_config, plugin_id, config=configuration) -def validate_app_group_lifecycle_plugin_group_config(plugin_data: dict[str, Any], plugin_id: str) -> dict[str, str]: +def validate_app_group_lifecycle_plugin_group_config( + plugin_data: dict[str, Any], + plugin_id: str, + app_plugin_data: dict[str, Any] | None = None, +) -> dict[str, str]: """ Validate the group-level configuration data for a particular app group lifecycle plugin. Args: plugin_data: The group's plugin data property. plugin_id: The ID of the plugin which should respond. + app_plugin_data: The owning app's plugin data, so the plugin can validate the group + config against app-level settings. Defaults to empty (no app context). Returns: A dictionary mapping any invalid fields to error messages. """ configuration = _get_data_for_plugin(plugin_data, plugin_id).configuration + app_configuration = _get_data_for_plugin(app_plugin_data or {}, plugin_id).configuration hook = get_app_group_lifecycle_hook() - return _get_hook_call_response(hook.validate_plugin_group_config, plugin_id, config=configuration) + return _get_hook_call_response( + hook.validate_plugin_group_config, plugin_id, config=configuration, app_config=app_configuration + ) def get_app_group_lifecycle_plugin_app_status_properties( diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index f8f4b2f8..155dfd2f 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -32,7 +32,9 @@ get_config_value, get_status_value, hookimpl, + is_plugin_config_changed, merge_app_lifecycle_plugin_data, + set_config_value, set_status_value, validate_app_group_lifecycle_plugin_app_config, validate_app_group_lifecycle_plugin_group_config, @@ -617,6 +619,33 @@ def test_set_status_value(self, db: Db, test_plugin: DummyPlugin) -> None: last_sync = get_status_value(test_app, "last_sync", DummyPlugin.ID) assert last_sync == "2025-01-15T11:00:00Z" + def test_set_config_value(self, db: Db, test_plugin: DummyPlugin) -> None: + """Test setting configuration values in plugin data.""" + test_app = AppFactory.build(name="TestApp9b", plugin_data={}) + db.session.add(test_app) + db.session.commit() + + set_config_value(test_app, "category", "inferred_id", DummyPlugin.ID) + db.session.commit() + db.session.expire(test_app) + + assert get_config_value(test_app, "category", DummyPlugin.ID) == "inferred_id" + + def test_is_plugin_config_changed(self, db: Db, test_plugin: DummyPlugin) -> None: + """Only configuration differences count as a change; status differences do not.""" + base = {DummyPlugin.ID: {"configuration": {"group_id": "g1"}, "status": {"member_count": 1}}} + + # Identical configuration -> not changed, even when status differs. + status_only = {DummyPlugin.ID: {"configuration": {"group_id": "g1"}, "status": {"member_count": 9}}} + assert is_plugin_config_changed(base, status_only, DummyPlugin.ID) is False + + # Different configuration -> changed. + config_changed = {DummyPlugin.ID: {"configuration": {"group_id": "g2"}, "status": {"member_count": 1}}} + assert is_plugin_config_changed(base, config_changed, DummyPlugin.ID) is True + + # Missing plugin entries are treated as empty configuration. + assert is_plugin_config_changed({}, {}, DummyPlugin.ID) is False + class TestPluginValidation: """Tests for plugin configuration validation.""" From e03a36e947d8fcdbae57aa948fac6d92ca48f475 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:00:32 +0000 Subject: [PATCH 03/19] feat(plugins): rename sync_all_group_membership hook to sync_all_groups The hook reconciles all of an app's groups, not just memberships, so the name was misleading. Rename it to sync_all_groups and rename the CLI command to `sync-app-groups`. Both renames are backward compatible: the old sync_all_group_membership hookspec stays as a deprecated alias (so plugins on the old name still register and run), the old `sync-app-group-memberships` command remains as a hidden alias, and a new invoke_sync_all_groups wrapper dispatches to both hook names exactly once. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/manage.py | 27 ++++++++++++++++-------- api/plugins/app_group_lifecycle.py | 26 ++++++++++++++++++++--- tests/test_app_group_lifecycle_plugin.py | 16 ++++++++++++++ 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/api/manage.py b/api/manage.py index 2dc4dd3d..bda51f50 100644 --- a/api/manage.py +++ b/api/manage.py @@ -48,7 +48,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Any: db.init_app(engine=build_engine()) # Trigger plugin discovery once per CLI run. Notification, conditional # access, and app-group-lifecycle hooks are all consumed by the - # `sync` / `notify` / `sync-app-group-memberships` commands; the + # `sync` / `notify` / `sync-app-groups` commands; the # `init` family doesn't need them but the call is cheap (memoized). load_plugins() token = _session_scope.set(f"cli-{uuid.uuid4().hex}") @@ -308,13 +308,10 @@ def notify(owner: bool, role_owner: bool) -> None: expiring_access_notifications_user() -@cli.command("sync-app-group-memberships") -@_with_app_context -def sync_app_group_memberships() -> None: - """Invoke the periodic membership sync hook for all apps with app group lifecycle plugins configured.""" +def _sync_all_app_groups() -> None: from api.extensions import db from api.models import App - from api.plugins.app_group_lifecycle import get_app_group_lifecycle_hook + from api.plugins.app_group_lifecycle import invoke_sync_all_groups click.echo("Starting app group lifecycle plugin sync") @@ -330,12 +327,10 @@ def sync_app_group_memberships() -> None: click.echo(f"Found {len(apps)} app(s) with plugins configured") - hook = get_app_group_lifecycle_hook() - for app in apps: click.echo(f"Syncing app '{app.name}' (plugin: {app.app_group_lifecycle_plugin})") try: - hook.sync_all_group_membership(session=db.session, app=app, plugin_id=app.app_group_lifecycle_plugin) + invoke_sync_all_groups(session=db.session, app=app, plugin_id=app.app_group_lifecycle_plugin) db.session.commit() click.echo(f" ✓ Synced app '{app.name}'") except Exception as e: @@ -345,5 +340,19 @@ def sync_app_group_memberships() -> None: click.echo("Completed app group lifecycle plugin sync") +@cli.command("sync-app-groups") +@_with_app_context +def sync_app_groups() -> None: + """Invoke the periodic group-sync hook for all apps with app group lifecycle plugins configured.""" + _sync_all_app_groups() + + +@cli.command("sync-app-group-memberships", hidden=True) +@_with_app_context +def sync_app_group_memberships() -> None: + """Deprecated alias for `sync-app-groups`; kept so existing automation keeps working.""" + _sync_all_app_groups() + + if __name__ == "__main__": cli() diff --git a/api/plugins/app_group_lifecycle.py b/api/plugins/app_group_lifecycle.py index d4802411..c08d9553 100644 --- a/api/plugins/app_group_lifecycle.py +++ b/api/plugins/app_group_lifecycle.py @@ -244,17 +244,37 @@ def group_members_removed( """ @hookspec - def sync_all_group_membership(self, session: Session, app: App, plugin_id: str | None) -> None: + def sync_all_groups(self, session: Session, app: App, plugin_id: str | None) -> None: """ - Bulk sync all group memberships for an app. This is invoked periodically by a CLI command `flask sync-app-group-memberships`. + Bulk reconcile all of an app's groups (membership and any external group state). + Invoked periodically by the `access sync-app-groups` CLI command. Args: session: The Access database session. - app: The app for which to sync all group membership. + app: The app for which to sync all groups. plugin_id: If provided, only the plugin matching this ID should respond. If None, all plugins may respond. """ + @hookspec + def sync_all_group_membership(self, session: Session, app: App, plugin_id: str | None) -> None: + """ + Deprecated alias for ``sync_all_groups``, retained so plugins implementing the + older, narrower hook name keep being invoked. New plugins should implement + ``sync_all_groups`` instead; a plugin should implement only one of the two. + """ + + +def invoke_sync_all_groups(session: Session, app: App, plugin_id: str | None) -> None: + """ + Invoke the bulk-sync hook for an app, honoring both the current ``sync_all_groups`` + name and the deprecated ``sync_all_group_membership`` alias. A plugin should implement + only one of the two, so this dispatches to each exactly once. + """ + hook = get_app_group_lifecycle_hook() + hook.sync_all_groups(session=session, app=app, plugin_id=plugin_id) + hook.sync_all_group_membership(session=session, app=app, plugin_id=plugin_id) + def get_app_group_lifecycle_hook() -> pluggy.HookRelay: """Get the hook relay for app group lifecycle plugins.""" diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index 155dfd2f..18b9a2b1 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -32,6 +32,7 @@ get_config_value, get_status_value, hookimpl, + invoke_sync_all_groups, is_plugin_config_changed, merge_app_lifecycle_plugin_data, set_config_value, @@ -646,6 +647,21 @@ def test_is_plugin_config_changed(self, db: Db, test_plugin: DummyPlugin) -> Non # Missing plugin entries are treated as empty configuration. assert is_plugin_config_changed({}, {}, DummyPlugin.ID) is False + def test_invoke_sync_all_groups_dispatches_current_and_deprecated_names(self, mocker: MockerFixture) -> None: + """The wrapper invokes both the current `sync_all_groups` hook and the deprecated + `sync_all_group_membership` alias once each, so plugins on either name still run.""" + hook = mocker.Mock() + mocker.patch("api.plugins.app_group_lifecycle.get_app_group_lifecycle_hook", return_value=hook) + + invoke_sync_all_groups(session=mocker.sentinel.session, app=mocker.sentinel.app, plugin_id="p1") + + hook.sync_all_groups.assert_called_once_with( + session=mocker.sentinel.session, app=mocker.sentinel.app, plugin_id="p1" + ) + hook.sync_all_group_membership.assert_called_once_with( + session=mocker.sentinel.session, app=mocker.sentinel.app, plugin_id="p1" + ) + class TestPluginValidation: """Tests for plugin configuration validation.""" From 8b96e01fcbce133251eaa61d1695cc4bbbc8dc80 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:03:01 +0000 Subject: [PATCH 04/19] feat(operations): add ModifyGroupPluginData and route put_group through it Group plugin_data edits previously merged the patch inline in put_group, and nothing fired the app group lifecycle group_updated hook when a group's plugin configuration changed outside of a rename/description edit. Introduce ModifyGroupPluginData to own that business logic: it merges the partial patch (preserving other plugins' entries and plugin-managed status the patch omits) and fires group_updated only when the configured plugin's configuration actually changes -- never on status-only writes, which the plugin makes itself and which would otherwise re-trigger reconciliation in a loop. put_group now delegates to it. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/operations/__init__.py | 2 + api/operations/modify_group_plugin_data.py | 81 +++++++++++ api/routers/groups.py | 14 +- tests/test_app_group_lifecycle_plugin.py | 152 +++++++++++++++++++++ 4 files changed, 239 insertions(+), 10 deletions(-) create mode 100644 api/operations/modify_group_plugin_data.py diff --git a/api/operations/__init__.py b/api/operations/__init__.py index 260e4ddc..60b1f45a 100644 --- a/api/operations/__init__.py +++ b/api/operations/__init__.py @@ -18,6 +18,7 @@ from api.operations.modify_app_tags import ModifyAppTags from api.operations.modify_group_tags import ModifyGroupTags from api.operations.modify_group_details import ModifyGroupDetails +from api.operations.modify_group_plugin_data import ModifyGroupPluginData from api.operations.modify_group_type import ModifyGroupType from api.operations.modify_group_users import ModifyGroupUsers from api.operations.modify_role_groups import ModifyRoleGroups @@ -39,6 +40,7 @@ "CreateGroup", "ModifyAppTags", "ModifyGroupDetails", + "ModifyGroupPluginData", "ModifyGroupTags", "ModifyGroupType", "ModifyGroupUsers", diff --git a/api/operations/modify_group_plugin_data.py b/api/operations/modify_group_plugin_data.py new file mode 100644 index 00000000..d8a3c531 --- /dev/null +++ b/api/operations/modify_group_plugin_data.py @@ -0,0 +1,81 @@ +import logging +from typing import Any + +from api.extensions import db +from api.models import AppGroup, OktaGroup +from api.plugins.app_group_lifecycle import ( + get_app_group_lifecycle_hook, + get_app_group_lifecycle_plugin_to_invoke, + is_plugin_config_changed, + merge_app_lifecycle_plugin_data, +) + + +class ModifyGroupPluginData: + """Apply a (partial) plugin_data patch to a group and fire group_updated when the + configured app group lifecycle plugin's *configuration* changes. + + Owns the plugin_data merge semantics so callers only supply the patch: + - top-level plugin entries the patch omits are preserved; + - for app group lifecycle plugins, per-plugin configuration/status keys the patch + omits are preserved too, so a partial configuration edit does not wipe + plugin-managed status (e.g. a linked external group id). + + group_updated fires only on configuration changes -- never on status-only changes, + which the plugin itself writes and which would otherwise re-trigger reconciliation in + a loop -- and only after the merge, so the hook observes the fully-merged plugin_data. + """ + + def __init__(self, *, group: OktaGroup, plugin_data: dict[str, Any]): + self.group = group + self.plugin_data = plugin_data + + def execute(self) -> OktaGroup: + old_plugin_data = self.group.plugin_data or {} + old_name = self.group.name + old_description = self.group.description or "" + plugin_id = get_app_group_lifecycle_plugin_to_invoke(self.group) + + # Apply the patch: start from the incoming data, preserving top-level plugin + # entries it didn't mention. + merged_plugin_data = dict(self.plugin_data) + changed = bool(old_plugin_data) and merged_plugin_data != old_plugin_data + if changed: + for key in old_plugin_data: + if key not in merged_plugin_data: + merged_plugin_data[key] = old_plugin_data[key] + + # Decide whether to fire the hook before the per-plugin merge below, which + # mutates old_plugin_data in place (it shares the configuration/status dicts) + # and would otherwise erase the difference we're testing for. + config_changed = plugin_id is not None and is_plugin_config_changed( + old_plugin_data, merged_plugin_data, plugin_id + ) + + self.group.plugin_data = merged_plugin_data + + # Preserve per-plugin configuration/status keys the patch omitted for app group + # lifecycle plugins (partial-patch semantics). + if changed and type(self.group) is AppGroup: + merge_app_lifecycle_plugin_data(self.group, old_plugin_data) + + db.session.commit() + + if config_changed: + try: + hook = get_app_group_lifecycle_hook() + hook.group_updated( + session=db.session, + group=self.group, + old_name=old_name, + old_description=old_description, + plugin_id=plugin_id, + ) + db.session.commit() + except Exception: + logging.getLogger("api").exception( + f"Failed to invoke group_updated hook for group {self.group.id} with plugin '{plugin_id}'" + ) + db.session.rollback() + + return self.group diff --git a/api/routers/groups.py b/api/routers/groups.py index 12679a05..0db101ce 100644 --- a/api/routers/groups.py +++ b/api/routers/groups.py @@ -34,6 +34,7 @@ CreateGroup, DeleteGroup, ModifyGroupDetails, + ModifyGroupPluginData, ModifyGroupTags, ModifyGroupType, ModifyGroupUsers, @@ -42,7 +43,6 @@ from fastapi_pagination.ext.sqlalchemy import paginate from api.pagination import Page, validated -from api.plugins.app_group_lifecycle import merge_app_lifecycle_plugin_data from api.routers._eager import ( group_tag_map_options, role_group_map_options, @@ -366,16 +366,10 @@ def put_group( raise HTTPException(400, str(e)) from e old_plugin_data_for_audit = copy.deepcopy(group.plugin_data) if group.plugin_data else {} - old_plugin_data = group.plugin_data if new_plugin_data is not None: - group.plugin_data = new_plugin_data - if old_plugin_data and group.plugin_data != old_plugin_data: - for key in old_plugin_data: - if key not in group.plugin_data: - group.plugin_data[key] = old_plugin_data[key] - if type(group) is AppGroup: - merge_app_lifecycle_plugin_data(group, old_plugin_data) - db.commit() + ModifyGroupPluginData(group=group, plugin_data=new_plugin_data).execute() + else: + db.commit() ModifyGroupTags( group=group, diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index 18b9a2b1..c4249893 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -2189,3 +2189,155 @@ def test_no_audit_log_when_plugin_unchanged_at_group_level( audit_logs = [record for record in caplog.records if record.levelname == "INFO"] plugin_logs = [log for log in audit_logs if EventType.group_modify_plugin.value in log.message] assert len(plugin_logs) == 0 + + +class TestModifyGroupPluginData: + """ModifyGroupPluginData fires group_updated only on configuration changes.""" + + def _make_app_group(self, db, mocker): + from api.models import AppGroup + mocker.patch.object(okta, "update_group") + mocker.patch.object(okta, "create_group") + app = AppFactory.create() + app.app_group_lifecycle_plugin = DummyPlugin.ID + app.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + db.session.add(app) + group = AppGroupFactory.create( + app_id=app.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{app.name}-Eng", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {}}}, + ) + db.session.add(group) + db.session.commit() + return app, group + + def test_fires_group_updated_on_config_change(self, db, app, test_plugin, mocker): + from api.operations.modify_group_plugin_data import ModifyGroupPluginData + _, group = self._make_app_group(db, mocker) + + new_plugin_data = {DummyPlugin.ID: {"configuration": {"group_id": "g-new"}, "status": {}}} + ModifyGroupPluginData(group=group, plugin_data=new_plugin_data).execute() + + assert len(test_plugin.group_updated_calls) == 1 + group_id, _old_name, _old_desc = test_plugin.group_updated_calls[0] + assert group_id == group.id + assert group.plugin_data[DummyPlugin.ID]["configuration"]["group_id"] == "g-new" + + def test_does_not_fire_on_status_only_change(self, db, app, test_plugin, mocker): + from api.operations.modify_group_plugin_data import ModifyGroupPluginData + _, group = self._make_app_group(db, mocker) + + new_plugin_data = { + DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {"member_count": 5}} + } + ModifyGroupPluginData(group=group, plugin_data=new_plugin_data).execute() + + assert test_plugin.group_updated_calls == [] + assert group.plugin_data[DummyPlugin.ID]["status"]["member_count"] == 5 + + def test_does_not_fire_without_lifecycle_plugin(self, db, app, test_plugin, mocker): + from api.models import AppGroup + from api.operations.modify_group_plugin_data import ModifyGroupPluginData + mocker.patch.object(okta, "update_group") + mocker.patch.object(okta, "create_group") + a = AppFactory.create() + # no app_group_lifecycle_plugin set on the app + group = AppGroupFactory.create( + app_id=a.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {}}}, + ) + db.session.add(a) + db.session.add(group) + db.session.commit() + + new_plugin_data = {DummyPlugin.ID: {"configuration": {"group_id": "g-new"}, "status": {}}} + ModifyGroupPluginData(group=group, plugin_data=new_plugin_data).execute() + + assert test_plugin.group_updated_calls == [] + + def test_preserves_other_plugins_top_level_entry(self, db, app, test_plugin, mocker): + """A patch mentioning only one plugin must not drop other plugins' entries.""" + from api.models import AppGroup + from api.operations.modify_group_plugin_data import ModifyGroupPluginData + + mocker.patch.object(okta, "update_group") + a = AppFactory.create() + a.app_group_lifecycle_plugin = DummyPlugin.ID + a.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + group = AppGroupFactory.create( + app_id=a.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + plugin_data={ + DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {}}, + "other_plugin": {"configuration": {"keep": "me"}, "status": {}}, + }, + ) + db.session.add(a) + db.session.add(group) + db.session.commit() + + ModifyGroupPluginData( + group=group, + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-new"}, "status": {}}}, + ).execute() + + assert group.plugin_data["other_plugin"] == {"configuration": {"keep": "me"}, "status": {}} + assert group.plugin_data[DummyPlugin.ID]["configuration"]["group_id"] == "g-new" + + def test_preserves_status_omitted_from_config_patch(self, db, app, test_plugin, mocker): + """A config-only patch must preserve plugin-managed status it didn't mention.""" + from api.models import AppGroup + from api.operations.modify_group_plugin_data import ModifyGroupPluginData + + mocker.patch.object(okta, "update_group") + a = AppFactory.create() + a.app_group_lifecycle_plugin = DummyPlugin.ID + a.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + group = AppGroupFactory.create( + app_id=a.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {"member_count": 7}}}, + ) + db.session.add(a) + db.session.add(group) + db.session.commit() + + ModifyGroupPluginData( + group=group, + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-new"}}}, + ).execute() + + assert group.plugin_data[DummyPlugin.ID]["configuration"]["group_id"] == "g-new" + assert group.plugin_data[DummyPlugin.ID]["status"] == {"member_count": 7} + + def test_put_group_config_change_fires_group_updated( + self, client, db, app, test_plugin, mocker, url_for + ): + from api.models import AppGroup + mocker.patch.object(okta, "update_group") + mocker.patch.object(okta, "create_group") + a = AppFactory.create() + a.app_group_lifecycle_plugin = DummyPlugin.ID + a.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + group = AppGroupFactory.create( + app_id=a.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {}}}, + ) + db.session.add(a) + db.session.add(group) + db.session.commit() + + url = url_for("api-groups.group_by_id_put", group_id=group.id) + response = client.put( + url, + json={ + "type": "app_group", + "name": group.name, + "plugin_data": {DummyPlugin.ID: {"configuration": {"group_id": "g-new"}, "status": {}}}, + }, + ) + assert response.status_code == 200 + assert any(call[0] == group.id for call in test_plugin.group_updated_calls) + assert response.json()["plugin_data"][DummyPlugin.ID]["configuration"]["group_id"] == "g-new" From ab7741893dd173c874b6d2521479b868a066cd0d Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:04:27 +0000 Subject: [PATCH 05/19] feat(groups): validate plugin group config when creating a group post_group accepted any plugin_data without checking it against the configured plugin's schema, so an invalid group config was only caught later during reconciliation. Validate it up front and return 400, mirroring put_group. Extract the shared check into _validate_group_plugin_config so create and update validate identically. The helper takes the resolved plugin id and the owning app's plugin_data from the caller, because a freshly-built group can't lazy-load its app (lazy="raise_on_sql"); post_group loads the App directly to get them. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/routers/groups.py | 54 ++++++++++++++++++------ tests/test_app_group_lifecycle_plugin.py | 46 ++++++++++++++++++++ 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/api/routers/groups.py b/api/routers/groups.py index 0db101ce..31547569 100644 --- a/api/routers/groups.py +++ b/api/routers/groups.py @@ -101,6 +101,30 @@ def _load_group_with_options(db: DbSession, group_id: str) -> OktaGroup | None: ).first() +def _validate_group_plugin_config( + plugin_data: dict[str, Any], + app_plugin_data: dict[str, Any], + plugin_id: str | None, +) -> None: + """Validate group-level plugin_data against the configured plugin's schema, raising + HTTP 400 on invalid config. No-op when the app has no app group lifecycle plugin. + + Callers resolve the plugin id and the owning app's plugin_data themselves, since + group-create and group-update obtain them differently (a freshly-built group can't + lazy-load its app).""" + if plugin_id is None: + return + + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + try: + errors = validate_app_group_lifecycle_plugin_group_config(plugin_data, plugin_id, app_plugin_data) + except ValueError as e: + raise HTTPException(400, f"plugin_data: {e}") from e + if errors: + raise HTTPException(400, f"plugin_data: {errors}") + + @router.get("", name="groups") def list_groups( request: Request, @@ -191,6 +215,14 @@ def post_group( 400, "App owner groups cannot be created directly. They are created automatically when an app is created." ) + if isinstance(group, AppGroup) and group.plugin_data: + # group.app uses lazy="raise_on_sql" and the group isn't in the session yet, + # so load the App directly to resolve the plugin id (mirrors put_group's pattern). + _app = db.scalars(select(App).where(App.id == group.app_id).where(App.deleted_at.is_(None))).first() + plugin_id = _app.app_group_lifecycle_plugin if _app is not None else None + app_plugin_data = _app.plugin_data if _app is not None else {} + _validate_group_plugin_config(group.plugin_data, app_plugin_data, plugin_id) + try: created = CreateGroup(group=group, tags=body.tags_to_add or [], current_user_id=current_user_id).execute() except ValueError as e: @@ -207,10 +239,7 @@ def put_group( db: DbSession, current_user_id: CurrentUserId, ) -> GroupDetail: - from api.plugins.app_group_lifecycle import ( - get_app_group_lifecycle_plugin_to_invoke, - validate_app_group_lifecycle_plugin_group_config, - ) + from api.plugins.app_group_lifecycle import get_app_group_lifecycle_plugin_to_invoke fields_set = body.model_fields_set group = _load_group_with_options(db, group_id) @@ -221,16 +250,15 @@ def put_group( # `null`) to an empty string for ModifyGroupDetails. description = (body.description or "") if "description" in fields_set else None - # Validate plugin_data for app groups against the configured plugin's schema. + # Validate plugin_data for app groups against the configured plugin's schema. The + # group is loaded with its app (joinedload), so the app's plugin config is available. if isinstance(body, _AppGroupUpdateBody) and "plugin_data" in fields_set and isinstance(group, AppGroup): - plugin_id = get_app_group_lifecycle_plugin_to_invoke(group) - if plugin_id is not None: - try: - errors = validate_app_group_lifecycle_plugin_group_config(body.plugin_data, plugin_id) - except ValueError as e: - raise HTTPException(400, f"plugin_data: {e}") from e - if errors: - raise HTTPException(400, f"plugin_data: {errors}") + app_plugin_data = group.app.plugin_data if group.app is not None else {} + _validate_group_plugin_config( + body.plugin_data, + app_plugin_data, + get_app_group_lifecycle_plugin_to_invoke(group), + ) if not _perms.can_manage_group(db, current_user_id, group): raise HTTPException(403, "Current user is not allowed to perform this action") diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index c4249893..c8ed6f96 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -2341,3 +2341,49 @@ def test_put_group_config_change_fires_group_updated( assert response.status_code == 200 assert any(call[0] == group.id for call in test_plugin.group_updated_calls) assert response.json()["plugin_data"][DummyPlugin.ID]["configuration"]["group_id"] == "g-new" + + +class TestPostGroupPluginValidation: + def test_post_group_rejects_invalid_group_config(self, client, db, app, test_plugin, mocker, url_for): + from okta.models.group import Group as OktaGroup + mocker.patch.object(okta, "create_group", return_value=OktaGroup({"id": "test-okta-id-123"})) + a = AppFactory.create() + a.app_group_lifecycle_plugin = DummyPlugin.ID + a.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + db.session.add(a) + db.session.commit() + + url = url_for("api-groups.groups_create") + response = client.post( + url, + json={ + "type": "app_group", + "name": f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + "app_id": a.id, + # DummyPlugin.validate_plugin_group_config requires "group_id" + "plugin_data": {DummyPlugin.ID: {"configuration": {}, "status": {}}}, + }, + ) + assert response.status_code == 400 + assert "group_id" in response.text + + def test_post_group_accepts_valid_group_config(self, client, db, app, test_plugin, mocker, url_for): + from okta.models.group import Group as OktaGroup + mocker.patch.object(okta, "create_group", return_value=OktaGroup({"id": "test-okta-id-456"})) + a = AppFactory.create() + a.app_group_lifecycle_plugin = DummyPlugin.ID + a.plugin_data = {DummyPlugin.ID: {"configuration": {"enabled": True}, "status": {}}} + db.session.add(a) + db.session.commit() + + url = url_for("api-groups.groups_create") + response = client.post( + url, + json={ + "type": "app_group", + "name": f"{AppGroup.APP_GROUP_NAME_PREFIX}{a.name}-Eng", + "app_id": a.id, + "plugin_data": {DummyPlugin.ID: {"configuration": {"group_id": "ext-123"}, "status": {}}}, + }, + ) + assert response.status_code == 201 From a11281369b5ac9f00684a819f37d5b25cc289010 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:07:55 +0000 Subject: [PATCH 06/19] feat(plugins): support immutable group config fields Some plugin group config can't change after creation -- e.g. a Google group's email, which the external API treats as an immutable key. Add an `immutable` flag to the group config property schema (and its wire mirror plus generated client type) and enforce it host-side: on update, validation rejects any change to a field the plugin declares immutable, while create still sets it freely. put_group passes the existing group's plugin_data so the validator can compare. Enforcement is generic, so a plugin only declares immutable=True. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/plugins/app_group_lifecycle.py | 33 +++++- api/routers/groups.py | 9 +- api/schemas/plugin_schemas.py | 1 + src/api/apiSchemas.ts | 4 + tests/test_app_group_lifecycle_plugin.py | 127 +++++++++++++++++++++++ 5 files changed, 170 insertions(+), 4 deletions(-) diff --git a/api/plugins/app_group_lifecycle.py b/api/plugins/app_group_lifecycle.py index c08d9553..c4cd81f8 100644 --- a/api/plugins/app_group_lifecycle.py +++ b/api/plugins/app_group_lifecycle.py @@ -51,6 +51,10 @@ class AppGroupLifecyclePluginConfigProperty: default_value: Any = None required: bool = False validation: dict[str, Any] | None = None + # When True, the host rejects edits to this field on update (group config only); + # the value may be set freely at create time. Enforced in + # validate_app_group_lifecycle_plugin_group_config. + immutable: bool = False @dataclass @@ -587,6 +591,7 @@ def validate_app_group_lifecycle_plugin_group_config( plugin_data: dict[str, Any], plugin_id: str, app_plugin_data: dict[str, Any] | None = None, + old_plugin_data: dict[str, Any] | None = None, ) -> dict[str, str]: """ Validate the group-level configuration data for a particular app group lifecycle plugin. @@ -596,6 +601,8 @@ def validate_app_group_lifecycle_plugin_group_config( plugin_id: The ID of the plugin which should respond. app_plugin_data: The owning app's plugin data, so the plugin can validate the group config against app-level settings. Defaults to empty (no app context). + old_plugin_data: The group's existing plugin data, supplied on update so the host can + reject edits to immutable config properties. Omitted on create. Returns: A dictionary mapping any invalid fields to error messages. @@ -603,10 +610,32 @@ def validate_app_group_lifecycle_plugin_group_config( configuration = _get_data_for_plugin(plugin_data, plugin_id).configuration app_configuration = _get_data_for_plugin(app_plugin_data or {}, plugin_id).configuration hook = get_app_group_lifecycle_hook() - return _get_hook_call_response( - hook.validate_plugin_group_config, plugin_id, config=configuration, app_config=app_configuration + errors = dict( + _get_hook_call_response( + hook.validate_plugin_group_config, plugin_id, config=configuration, app_config=app_configuration + ) ) + # On update, apply immutable-field semantics generically (plugins only declare + # `immutable=True`): a changed immutable field is rejected, while a plugin error for an + # *unchanged* immutable field is suppressed -- the user can't action it on this update + # (the field is locked), and it was acceptable at creation or was adopted/grandfathered + # from external state, so it must not block the rest of the update. + if old_plugin_data is not None: + old_configuration = _get_data_for_plugin(old_plugin_data, plugin_id).configuration + properties = _get_hook_call_response( + hook.get_plugin_group_config_properties, plugin_id, app_config=app_configuration + ) + for name, prop in properties.items(): + if not getattr(prop, "immutable", False): + continue + if old_configuration.get(name) != configuration.get(name): + errors.setdefault(name, f"The '{name}' field cannot be changed after creation") + else: + errors.pop(name, None) + + return errors + def get_app_group_lifecycle_plugin_app_status_properties( plugin_id: str, diff --git a/api/routers/groups.py b/api/routers/groups.py index 31547569..63320100 100644 --- a/api/routers/groups.py +++ b/api/routers/groups.py @@ -105,20 +105,24 @@ def _validate_group_plugin_config( plugin_data: dict[str, Any], app_plugin_data: dict[str, Any], plugin_id: str | None, + old_plugin_data: dict[str, Any] | None = None, ) -> None: """Validate group-level plugin_data against the configured plugin's schema, raising HTTP 400 on invalid config. No-op when the app has no app group lifecycle plugin. Callers resolve the plugin id and the owning app's plugin_data themselves, since group-create and group-update obtain them differently (a freshly-built group can't - lazy-load its app).""" + lazy-load its app). On update, callers also pass the existing group's plugin_data as + `old_plugin_data` so the host can reject changes to immutable config fields.""" if plugin_id is None: return from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config try: - errors = validate_app_group_lifecycle_plugin_group_config(plugin_data, plugin_id, app_plugin_data) + errors = validate_app_group_lifecycle_plugin_group_config( + plugin_data, plugin_id, app_plugin_data, old_plugin_data=old_plugin_data + ) except ValueError as e: raise HTTPException(400, f"plugin_data: {e}") from e if errors: @@ -258,6 +262,7 @@ def put_group( body.plugin_data, app_plugin_data, get_app_group_lifecycle_plugin_to_invoke(group), + old_plugin_data=group.plugin_data or {}, ) if not _perms.can_manage_group(db, current_user_id, group): diff --git a/api/schemas/plugin_schemas.py b/api/schemas/plugin_schemas.py index 02e7b993..2d0b5cb0 100644 --- a/api/schemas/plugin_schemas.py +++ b/api/schemas/plugin_schemas.py @@ -32,6 +32,7 @@ class PluginConfigProp(BaseModel): default_value: Any = None required: bool = False validation: Optional[dict[str, Any]] = None + immutable: bool = False class PluginStatusProp(BaseModel): diff --git a/src/api/apiSchemas.ts b/src/api/apiSchemas.ts index d22ad5f5..a8cbe48a 100644 --- a/src/api/apiSchemas.ts +++ b/src/api/apiSchemas.ts @@ -934,6 +934,10 @@ export type PluginConfigProp = { validation?: { [key: string]: any; } | null; + /** + * @default false + */ + immutable?: boolean; }; /** diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index c8ed6f96..93b00949 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -9,6 +9,7 @@ - Plugin lifecycle hooks """ +from dataclasses import asdict from typing import Any, Generator import pytest @@ -117,6 +118,13 @@ def get_plugin_group_config_properties( type="text", required=True, ), + "region": AppGroupLifecyclePluginConfigProperty( + display_name="Region", + help_text="Immutable region; set once at creation", + type="text", + required=False, + immutable=True, + ), } @hookimpl @@ -129,6 +137,10 @@ def validate_plugin_group_config(self, config: dict[str, Any], plugin_id: str | errors["group_id"] = "The 'group_id' field is required" elif not isinstance(config["group_id"], str): errors["group_id"] = "The 'group_id' field must be a string" + # `region` is immutable; a value outside the allowed set models a constraint added + # after some groups were created (i.e. a grandfathered/adopted value). + if config.get("region") not in (None, "us", "eu"): + errors["region"] = "The 'region' field must be 'us' or 'eu'" return errors @@ -226,6 +238,15 @@ def test_plugin(app: FastAPI, mocker: MockerFixture) -> Generator[DummyPlugin, N plugin_module._cached_plugin_registry = None +def test_config_property_immutable_defaults_false_and_serializes() -> None: + prop = AppGroupLifecyclePluginConfigProperty(display_name="X") + assert prop.immutable is False + assert asdict(prop)["immutable"] is False + + prop2 = AppGroupLifecyclePluginConfigProperty(display_name="Y", immutable=True) + assert asdict(prop2)["immutable"] is True + + class TestPluginRegistration: """Tests for plugin registration and discovery.""" @@ -744,6 +765,60 @@ def test_valid_group_config( response = client.put(url, json=data) assert response.status_code == 200 + def test_put_group_rejects_immutable_field_change( + self, client: TestClient, db: Db, app: FastAPI, test_plugin: DummyPlugin, mocker: MockerFixture, url_for: Any + ) -> None: + """Test that changing an immutable group configuration field is rejected.""" + test_app = AppFactory.build(name="TestAppImm1", app_group_lifecycle_plugin=DummyPlugin.ID) + test_group = AppGroupFactory.build( + app_id=test_app.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{test_app.name}{AppGroup.APP_NAME_GROUP_NAME_SEPARATOR}Immg", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}, "status": {}}}, + ) + db.session.add(test_app) + db.session.add(test_group) + db.session.commit() + mocker.patch.object(okta, "update_group") + + url = url_for("api-groups.group_by_id", group_id=test_group.id) + data = { + "type": "app_group", + "name": test_group.name, + "description": "", + "app_id": test_group.app_id, + "plugin_data": {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "eu"}}}, + } + response = client.put(url, json=data) + assert response.status_code == 400 + assert "region" in response.json()["detail"] + + def test_put_group_allows_mutable_field_change( + self, client: TestClient, db: Db, app: FastAPI, test_plugin: DummyPlugin, mocker: MockerFixture, url_for: Any + ) -> None: + """Test that changing a mutable group configuration field is accepted and persisted.""" + test_app = AppFactory.build(name="TestAppImm2", app_group_lifecycle_plugin=DummyPlugin.ID) + test_group = AppGroupFactory.build( + app_id=test_app.id, + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{test_app.name}{AppGroup.APP_NAME_GROUP_NAME_SEPARATOR}Mutg", + plugin_data={DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}, "status": {}}}, + ) + db.session.add(test_app) + db.session.add(test_group) + db.session.commit() + mocker.patch.object(okta, "update_group") + + url = url_for("api-groups.group_by_id", group_id=test_group.id) + data = { + "type": "app_group", + "name": test_group.name, + "description": "", + "app_id": test_group.app_id, + "plugin_data": {DummyPlugin.ID: {"configuration": {"group_id": "g2", "region": "us"}}}, + } + response = client.put(url, json=data) + assert response.status_code == 200 + assert response.json()["plugin_data"][DummyPlugin.ID]["configuration"]["group_id"] == "g2" + def test_invalid_group_config( self, client: TestClient, db: Db, app: FastAPI, test_plugin: DummyPlugin, url_for: Any ) -> None: @@ -2387,3 +2462,55 @@ def test_post_group_accepts_valid_group_config(self, client, db, app, test_plugi }, ) assert response.status_code == 201 + + +def test_validate_group_config_rejects_immutable_change_on_update(test_plugin: DummyPlugin) -> None: + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + old = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}}} + new = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "eu"}}} + errors = validate_app_group_lifecycle_plugin_group_config(new, DummyPlugin.ID, old_plugin_data=old) + assert "region" in errors + + +def test_validate_group_config_allows_immutable_on_create(test_plugin: DummyPlugin) -> None: + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + new = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}}} + # No old_plugin_data -> create path -> immutable field freely set. + errors = validate_app_group_lifecycle_plugin_group_config(new, DummyPlugin.ID) + assert "region" not in errors + + +def test_validate_group_config_allows_mutable_change_on_update(test_plugin: DummyPlugin) -> None: + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + old = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}}} + new = {DummyPlugin.ID: {"configuration": {"group_id": "g2", "region": "us"}}} + errors = validate_app_group_lifecycle_plugin_group_config(new, DummyPlugin.ID, old_plugin_data=old) + assert errors == {} + + +def test_validate_group_config_enforces_immutable_field_on_create(test_plugin: DummyPlugin) -> None: + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + # On create (no old_plugin_data) an immutable field is validated like any other. + new = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "legacy"}}} + errors = validate_app_group_lifecycle_plugin_group_config(new, DummyPlugin.ID) + assert "region" in errors + + +def test_validate_group_config_suppresses_unchanged_immutable_field_error_on_update(test_plugin: DummyPlugin) -> None: + from api.plugins.app_group_lifecycle import validate_app_group_lifecycle_plugin_group_config + + # A grandfathered/adopted immutable value that now fails plugin validation must not block + # an update that leaves it unchanged (it's locked and can't be fixed via this update). + old = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "legacy"}}} + new = {DummyPlugin.ID: {"configuration": {"group_id": "g2", "region": "legacy"}}} + errors = validate_app_group_lifecycle_plugin_group_config(new, DummyPlugin.ID, old_plugin_data=old) + assert "region" not in errors + + # But changing the immutable field is still rejected. + changed = {DummyPlugin.ID: {"configuration": {"group_id": "g1", "region": "us"}}} + errors = validate_app_group_lifecycle_plugin_group_config(changed, DummyPlugin.ID, old_plugin_data=old) + assert "region" in errors From d4c8c0333626e2fee2f344c417dc457c03359ede Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:10:11 +0000 Subject: [PATCH 07/19] feat(plugins): let group-config-props reflect a given app's config The group config schema can depend on the owning app's plugin config (for example, an app-level pattern surfaced as a client-side validation rule), but the group-config-props endpoint had no app context. Accept an optional app_id query param, load that app's plugin_data, and thread it into the schema lookup; regenerate the client so the param is available to the UI. Without app_id the behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/routers/plugins.py | 17 +++++++++++++++-- src/api/apiComponents.ts | 7 ++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/api/routers/plugins.py b/api/routers/plugins.py index be18454a..4a387758 100644 --- a/api/routers/plugins.py +++ b/api/routers/plugins.py @@ -6,8 +6,11 @@ from fastapi import APIRouter +from sqlalchemy import select from api.auth.dependencies import CurrentUserId +from api.database import DbSession +from api.models import App from api.plugins.app_group_lifecycle import ( PluginNotFoundError, get_app_group_lifecycle_plugin_app_config_properties, @@ -60,12 +63,22 @@ def app_config_props(plugin_id: str, current_user_id: CurrentUserId) -> AppGroup "/app-group-lifecycle/{plugin_id}/group-config-props", name="app_group_lifecycle_plugin_group_config_props", ) -def group_config_props(plugin_id: str, current_user_id: CurrentUserId) -> AppGroupLifecyclePluginGroupConfig: +def group_config_props( + plugin_id: str, current_user_id: CurrentUserId, db: DbSession, app_id: str | None = None +) -> AppGroupLifecyclePluginGroupConfig: _ensure_plugin(plugin_id) + # When an app_id is supplied, pass its plugin config so the schema can reflect + # app-level constraints. + app_plugin_data = None + if app_id is not None: + app = db.scalars(select(App).where(App.id == app_id).where(App.deleted_at.is_(None))).first() + app_plugin_data = app.plugin_data if app is not None else None return AppGroupLifecyclePluginGroupConfig.model_validate( { name: asdict(schema) - for name, schema in get_app_group_lifecycle_plugin_group_config_properties(plugin_id).items() + for name, schema in get_app_group_lifecycle_plugin_group_config_properties( + plugin_id, app_plugin_data + ).items() } ) diff --git a/src/api/apiComponents.ts b/src/api/apiComponents.ts index 7a3082c6..8dfca66e 100644 --- a/src/api/apiComponents.ts +++ b/src/api/apiComponents.ts @@ -2102,6 +2102,10 @@ export type AppGroupLifecyclePluginGroupConfigPropsPathParams = { pluginId: string; }; +export type AppGroupLifecyclePluginGroupConfigPropsQueryParams = { + app_id?: string | null; +}; + export type AppGroupLifecyclePluginGroupConfigPropsError = Fetcher.ErrorWrapper<{ status: Exclude; payload: Schemas.ProblemDetail; @@ -2109,6 +2113,7 @@ export type AppGroupLifecyclePluginGroupConfigPropsError = Fetcher.ErrorWrapper< export type AppGroupLifecyclePluginGroupConfigPropsVariables = { pathParams: AppGroupLifecyclePluginGroupConfigPropsPathParams; + queryParams?: AppGroupLifecyclePluginGroupConfigPropsQueryParams; } & ApiContext['fetcherOptions']; export const fetchAppGroupLifecyclePluginGroupConfigProps = ( @@ -2120,7 +2125,7 @@ export const fetchAppGroupLifecyclePluginGroupConfigProps = ( AppGroupLifecyclePluginGroupConfigPropsError, undefined, {}, - {}, + AppGroupLifecyclePluginGroupConfigPropsQueryParams, AppGroupLifecyclePluginGroupConfigPropsPathParams >({ url: '/api/plugins/app-group-lifecycle/{pluginId}/group-config-props', From 23e83c22d140eac6aed13b84b252540be6db645d Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:11:10 +0000 Subject: [PATCH 08/19] feat(ui): validate plugin config client-side and lock immutable fields Surface plugin group config validation in the configuration form so the UI catches bad input before submitting (the backend remains authoritative): each text field applies the schema's validation.patterns as react-hook-form rules. Immutable fields stay editable at create time and become read-only (not disabled, so their value is still submitted) when editing an existing app or group, with a helper-text note explaining why. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...pLifecyclePluginConfigurationForm.test.tsx | 42 ++++++ ...pGroupLifecyclePluginConfigurationForm.tsx | 139 +++++++++++++++--- src/pages/apps/CreateUpdate.tsx | 1 + src/pages/groups/CreateUpdate.tsx | 2 + src/setupTests.ts | 2 +- 5 files changed, 164 insertions(+), 22 deletions(-) create mode 100644 src/components/AppGroupLifecyclePluginConfigurationForm.test.tsx diff --git a/src/components/AppGroupLifecyclePluginConfigurationForm.test.tsx b/src/components/AppGroupLifecyclePluginConfigurationForm.test.tsx new file mode 100644 index 00000000..e934c956 --- /dev/null +++ b/src/components/AppGroupLifecyclePluginConfigurationForm.test.tsx @@ -0,0 +1,42 @@ +import {describe, it, expect, vi} from 'vitest'; + +// Mock MUI and React Hook Form modules so the test environment (jsdom / Node) +// does not need @emotion/styled or styled-components. We only exercise the +// pure helper isFieldLocked, which has no UI dependencies. +vi.mock('@mui/material/Box', () => ({default: () => null})); +vi.mock('@mui/material/CircularProgress', () => ({default: () => null})); +vi.mock('@mui/material/FormControl', () => ({default: () => null})); +vi.mock('@mui/material/FormHelperText', () => ({default: () => null})); +vi.mock('@mui/material/MenuItem', () => ({default: () => null})); +vi.mock('@mui/material/Select', () => ({default: () => null})); +vi.mock('@mui/material/TextField', () => ({default: () => null})); +vi.mock('@mui/material/Typography', () => ({default: () => null})); +vi.mock('@mui/material/Checkbox', () => ({default: () => null})); +vi.mock('@mui/material/FormControlLabel', () => ({default: () => null})); +vi.mock('react-hook-form', () => ({useFormContext: () => ({}), Controller: () => null})); +vi.mock('../api/apiComponents', () => ({ + useAppGroupLifecyclePlugins: () => ({data: [], isLoading: false}), + useAppGroupLifecyclePluginAppConfigProps: () => ({data: {}, isLoading: false}), + useAppGroupLifecyclePluginGroupConfigProps: () => ({data: {}, isLoading: false}), +})); + +import {isFieldLocked} from './AppGroupLifecyclePluginConfigurationForm'; +import {PluginConfigProp} from '../api/apiSchemas'; + +const prop = (over: Partial): PluginConfigProp => + ({display_name: 'X', type: 'text', required: false, ...over}) as PluginConfigProp; + +describe('isFieldLocked', () => { + it('locks an immutable field when editing an existing entity', () => { + expect(isFieldLocked(prop({immutable: true}), true)).toBe(true); + }); + + it('does not lock an immutable field at create time', () => { + expect(isFieldLocked(prop({immutable: true}), false)).toBe(false); + }); + + it('never locks a mutable field', () => { + expect(isFieldLocked(prop({immutable: false}), true)).toBe(false); + expect(isFieldLocked(prop({}), true)).toBe(false); + }); +}); diff --git a/src/components/AppGroupLifecyclePluginConfigurationForm.tsx b/src/components/AppGroupLifecyclePluginConfigurationForm.tsx index 6d51aa28..bf826085 100644 --- a/src/components/AppGroupLifecyclePluginConfigurationForm.tsx +++ b/src/components/AppGroupLifecyclePluginConfigurationForm.tsx @@ -23,6 +23,9 @@ import { } from '../api/apiComponents'; import {PluginConfigProp, PluginInfo} from '../api/apiSchemas'; +// Helper-text note appended to a locked (immutable, edit-mode) config field. +const LOCKED_NOTE = 'Cannot be changed after creation.'; + type PluginConfiguration = { [propertyId: string]: any; }; @@ -47,16 +50,72 @@ interface AppGroupLifecyclePluginConfigurationFormProps { * Callback when plugin selection changes (app level only) */ onPluginChange?: (pluginId: string | null) => void; + + /** + * Whether the entity being configured already exists (edit mode); immutable fields lock when true. + */ + isExistingEntity?: boolean; + + /** + * For group-level config, the owning app's id. Passed to the group-config-props + * lookup so the schema reflects app-level constraints (e.g. an email pattern surfaced + * as a client-side validation rule). + */ + appId?: string; } /** * Renders a single configuration field based on its schema */ -function ConfigField({property, value, fieldName}: {property: PluginConfigProp; value: any; fieldName: string}) { - const {register, control} = useFormContext(); +// Build react-hook-form `validate` rules from a config property's optional +// `validation.patterns` (a list of {regex, message}). Each non-empty value must +// match every pattern; emptiness is left to the `required` rule, and a malformed +// regex is ignored client-side since the backend validation is authoritative. + +// An immutable config field may be set freely at create time and must lock on edit. +// Rendered read-only (not disabled) so its value is still submitted — a disabled input +// is omitted from the form payload, which the backend would read as a change and reject. +export function isFieldLocked(property: PluginConfigProp, isExistingEntity: boolean): boolean { + return !!property.immutable && isExistingEntity; +} + +function patternValidators(property: PluginConfigProp): Record true | string> { + const patterns = ((property.validation?.patterns ?? []) as Array<{regex: string; message?: string}>) || []; + const validators: Record true | string> = {}; + patterns.forEach((p, i) => { + validators[`pattern_${i}`] = (value: any) => { + if (value === undefined || value === null || value === '') return true; + try { + return new RegExp(p.regex).test(String(value)) || (p.message ?? `Must match ${p.regex}`); + } catch { + return true; + } + }; + }); + return validators; +} + +function ConfigField({ + property, + value, + fieldName, + locked, +}: { + property: PluginConfigProp; + value: any; + fieldName: string; + locked: boolean; +}) { + const {register, control, getFieldState, formState} = useFormContext(); + // Subscribe to this field's validation error so client-side failures surface inline + // (reading getFieldState off formState subscribes to formState.errors). + const fieldError = getFieldState(fieldName, formState).error; switch (property.type) { - case 'boolean': + case 'boolean': { + const boolHelp = locked + ? `${property.help_text ? property.help_text + ' ' : ''}${LOCKED_NOTE}` + : property.help_text; return ( ( - } label={property.display_name} /> + } + label={property.display_name} + /> )} /> - {property.help_text && {property.help_text}} + {boolHelp && {boolHelp}} ); + } - case 'number': + case 'number': { + const numHelp = locked + ? `${property.help_text ? property.help_text + ' ' : ''}${LOCKED_NOTE}` + : property.help_text; return ( ); + } case 'text': - default: + default: { + const textHelp = locked + ? `${property.help_text ? property.help_text + ' ' : ''}${LOCKED_NOTE}` + : property.help_text; return ( ); + } } } @@ -111,16 +197,26 @@ export default function AppGroupLifecyclePluginConfigurationForm({ selectedPluginId, currentConfig = {}, onPluginChange, + isExistingEntity = false, + appId, }: AppGroupLifecyclePluginConfigurationFormProps) { const {data: plugins, isLoading: pluginsLoading} = useAppGroupLifecyclePlugins({}); - const useConfigPropertiesHook = - entityType === 'app' ? useAppGroupLifecyclePluginAppConfigProps : useAppGroupLifecyclePluginGroupConfigProps; - - const {data: configProperties, isLoading: configLoading} = useConfigPropertiesHook( + // Call both hooks unconditionally (rules of hooks) and select by entity type; only the + // group lookup takes an app_id, so the app's config (e.g. its email pattern) is reflected + // in the group config schema and validated client-side. + const appConfigProps = useAppGroupLifecyclePluginAppConfigProps( {pathParams: {pluginId: selectedPluginId || ''}}, - {enabled: !!selectedPluginId}, + {enabled: !!selectedPluginId && entityType === 'app'}, + ); + const groupConfigProps = useAppGroupLifecyclePluginGroupConfigProps( + { + pathParams: {pluginId: selectedPluginId || ''}, + queryParams: appId ? {app_id: appId} : undefined, + }, + {enabled: !!selectedPluginId && entityType === 'group'}, ); + const {data: configProperties, isLoading: configLoading} = entityType === 'app' ? appConfigProps : groupConfigProps; const selectedPlugin = React.useMemo(() => { if (!plugins || !selectedPluginId) return null; @@ -202,6 +298,7 @@ export default function AppGroupLifecyclePluginConfigurationForm({ property={property as PluginConfigProp} value={currentConfig[propertyId]} fieldName={fieldName} + locked={isFieldLocked(property as PluginConfigProp, isExistingEntity)} /> ); })} diff --git a/src/pages/apps/CreateUpdate.tsx b/src/pages/apps/CreateUpdate.tsx index aa137585..f70d43f5 100644 --- a/src/pages/apps/CreateUpdate.tsx +++ b/src/pages/apps/CreateUpdate.tsx @@ -234,6 +234,7 @@ function AppDialog(props: AppDialogProps) { : {} } onPluginChange={setSelectedAppGroupLifecyclePluginId} + isExistingEntity={props.app != null} /> )} diff --git a/src/pages/groups/CreateUpdate.tsx b/src/pages/groups/CreateUpdate.tsx index 2a868129..0afca911 100644 --- a/src/pages/groups/CreateUpdate.tsx +++ b/src/pages/groups/CreateUpdate.tsx @@ -361,11 +361,13 @@ function GroupDialog(props: GroupDialogProps) { )} diff --git a/src/setupTests.ts b/src/setupTests.ts index 74b1a275..8f2609b7 100644 --- a/src/setupTests.ts +++ b/src/setupTests.ts @@ -2,4 +2,4 @@ // allows you to do things like: // expect(element).toHaveTextContent(/react/i) // learn more: https://github.com/testing-library/jest-dom -import '@testing-library/jest-dom/extend-expect'; +import '@testing-library/jest-dom'; From 36d2fa80c86bd19aaec6b69f8461ca08cf1f1cb4 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:11:30 +0000 Subject: [PATCH 09/19] feat(api): make the root log level configurable via LOG_LEVEL Logging was hard-coded to INFO, so diagnosing issues in a deployed environment (e.g. tracing app group lifecycle plugin reconciliation) meant editing code. Read the level from a LOG_LEVEL environment variable, defaulting to INFO. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/app.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/app.py b/api/app.py index 3d8da937..037770c2 100644 --- a/api/app.py +++ b/api/app.py @@ -96,8 +96,9 @@ def _flatten_query_param_models(openapi_schema: dict[str, Any]) -> None: def _configure_logging() -> None: - logging.basicConfig(level=logging.INFO, stream=sys.stdout, format="%(message)s") - logging.root.setLevel(logging.INFO) + level = environ.get("LOG_LEVEL", "INFO").upper() + logging.basicConfig(level=level, stream=sys.stdout, format="%(message)s") + logging.root.setLevel(level) token_filter = TokenSanitizingFilter() logging.getLogger("authlib").addFilter(token_filter) logging.getLogger("uvicorn.access").addFilter(token_filter) From c4771e663b877f044a0ca6dc442ec8f14e555e57 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:12:13 +0000 Subject: [PATCH 10/19] build: add a make target to run sync-app-groups with .env loaded Running the app group lifecycle sync locally needs the plugin credentials (Google/Okta) from .env, but `access` has no --env-file flag and the existing `sync`/`notify` targets don't load it. Add a sync-app-groups target that sources .env (set -a auto-exports it) while still forcing DATABASE_URI to the local instance, like the other targets, so it hits the same migrated database the dev server uses. Co-Authored-By: Claude Opus 4.8 (1M context) --- Makefile | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index dab47216..5d888057 100644 --- a/Makefile +++ b/Makefile @@ -28,6 +28,7 @@ help: @echo "Management commands:" @echo " make sync access sync" @echo " make notify access notify" + @echo " make sync-app-groups access sync-app-groups (loads .env)" @echo "" @echo "Docker:" @echo " make build docker build" @@ -80,7 +81,7 @@ run: .env dev db-migrate @mkdir -p .claude @printf '%s\n' "$(BACKEND_PORT)" > .claude/.api-port $(ACTIVATE) DATABASE_URI=$(LOCAL_DB_URI) \ - uvicorn --reload --host 0.0.0.0 --port $(BACKEND_PORT) api.asgi:app & \ + uvicorn --env-file .env --reload --host 0.0.0.0 --port $(BACKEND_PORT) api.asgi:app & \ npm install && npx vite --host 0.0.0.0 --port $(FRONTEND_PORT) .PHONY: run-frontend @@ -92,7 +93,7 @@ run-backend: .env dev db-migrate @mkdir -p .claude @printf '%s\n' "$(BACKEND_PORT)" > .claude/.api-port $(ACTIVATE) DATABASE_URI=$(LOCAL_DB_URI) \ - uvicorn --reload --host 0.0.0.0 --port $(BACKEND_PORT) api.asgi:app + uvicorn --env-file .env --reload --host 0.0.0.0 --port $(BACKEND_PORT) api.asgi:app # ---------------------------------------------------------------------- # Database / migrations @@ -136,6 +137,18 @@ sync: dev notify: dev $(ACTIVATE) DATABASE_URI=$(LOCAL_DB_URI) access notify +# Unlike `sync`/`notify`, this loads the full .env so the app-group lifecycle +# plugins get the credentials they need (e.g. Google/Okta). `access` has no +# --env-file flag, so we source .env into the environment ourselves: `set -a` +# auto-exports every var defined while sourcing. We then force +# DATABASE_URI=$(LOCAL_DB_URI) -- exactly like `sync`/`notify`/`run-backend` -- +# so this points at the same migrated instance/access.db the dev server uses, +# rather than whatever DATABASE_URI .env happens to set. +.PHONY: sync-app-groups +sync-app-groups: .env dev + $(ACTIVATE) set -a && . ./.env && set +a && \ + DATABASE_URI=$(LOCAL_DB_URI) access sync-app-groups + # ---------------------------------------------------------------------- # Docker # ---------------------------------------------------------------------- From 7eb989bee9dfe65c7ba58b6fc7abd2d9680190fc Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:31:49 +0000 Subject: [PATCH 11/19] chore(plugin): scaffold the google_group_manager plugin package Add the package skeleton for an example app group lifecycle plugin that manages Google groups for Access groups: __init__, a setup.py declaring the access_app_group_lifecycle entry point and Google client dependencies, and a README documenting configuration, the Cloud Identity Groups API auth model (a Group Administrator service account, no domain-wide delegation), the GOOGLE_WORKSPACE_CUSTOMER_ID group parent, and that the group email is immutable after creation. Also broaden tox's pytest invocation to collect the whole repo so this plugin's standalone suite runs in CI alongside core tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/README.md | 131 ++++++++++++++++++ .../app_group_lifecycle_google/__init__.py | 7 + .../app_group_lifecycle_google/setup.py | 26 ++++ tox.ini | 8 +- 4 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 examples/plugins/app_group_lifecycle_google/README.md create mode 100644 examples/plugins/app_group_lifecycle_google/__init__.py create mode 100644 examples/plugins/app_group_lifecycle_google/setup.py diff --git a/examples/plugins/app_group_lifecycle_google/README.md b/examples/plugins/app_group_lifecycle_google/README.md new file mode 100644 index 00000000..911f0e71 --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/README.md @@ -0,0 +1,131 @@ +# App Group Lifecycle Google Group Management Plugin + +This plugin automatically creates, modifies, and deletes Google Groups corresponding to the Access groups that are configured to use it. It links those Google Groups to the corresponding Access-managed Okta group via Okta group push. Group membership is handled entirely by Okta group push; the plugin does not implement membership hooks. + +## Overview + +When an Access group is created or deleted or its plugin configuration is modified, the plugin: + +1. Creates, deletes, or modifies the properties (name, email, description) the corresponding Google Group in the configured Google Workspace domain. +2. Creates or removes an Okta group push mapping between the Access-managed Okta group and the Google Group, so Okta keeps group membership in sync automatically. + +Additionally, Google groups are periodically reconciled (created or updated) to ensure eventual alignment to the source of truth in Access. + +## Files + +- **[`__init__.py`](./__init__.py)**: Plugin package initialization +- **[`plugin.py`](./plugin.py)**: Plugin implementation +- **[`setup.py`](./setup.py)**: Setup script defining plugin metadata and entry points + +## Configuration + +### App-Level Configuration + +| Key | Type | Required | Description | +|-----|------|----------|-------------| +| `enabled` | boolean | yes | Enable or disable this plugin for the app. | +| `email_pattern` | text | no | Optional regex applied to the group email prefix to validate it before creating the Google Group. | + +### Group-Level Configuration + +| Key | Type | Required | Description | +|-----|------|----------|-------------| +| `email` | text | yes | The local-part (prefix) of the Google Group email address. The full address is `{email}@{GOOGLE_WORKSPACE_DOMAIN}`. Immutable after the group is created (the Cloud Identity `groupKey` cannot be changed). | +| `display_name` | text | yes | The display name for the Google Group. | + +### Group-Level Status + +| Key | Description | +|-----|-------------| +| `push_mapping_id` | The Okta group push mapping ID linking the Okta group to the Google Group. | +| `google_group_id` | The Google Group resource ID. | +| `sync_status` | One of `synced`, `pending`, or `error`. | +| `sync_error` | Error message if `sync_status` is `error`; otherwise empty. | +| `last_synced_at` | Timestamp of the last successful sync. | + +There are no app-level status properties. + +## Environment Variables + +| Variable | Description | +|----------|-------------| +| `GOOGLE_WORKSPACE_OKTA_APP_ID` | The Okta application ID for the Google Workspace application. See https://support.okta.com/help/s/article/How-to-obtain-an-application-ID. | +| `GOOGLE_WORKSPACE_DOMAIN` | The Google Workspace domain (e.g. `acme.com`). Used to construct the full Google Group email address. | +| `GOOGLE_WORKSPACE_CUSTOMER_ID` | The Google Workspace customer ID (e.g. `C0xxxxxxx`), used as the Cloud Identity group parent (`customers/{id}`). See https://knowledge.workspace.google.com/admin/getting-started/find-your-customer-id. | + +## Authentication and Authorization + +### Calling the Okta API + +For this plugin to work, Access must have permission to create and delete group push mappings (e.g. via the App Admin role) for the configured Google Workspace Okta application. + +### Calling the Google API + +This plugin uses the **Cloud Identity Groups API** (`cloudidentity.googleapis.com`, scope `https://www.googleapis.com/auth/cloud-identity.groups`) for all group CRUD. Because that API authorizes the calling principal directly, assign the service account reachable via [Application Default Credentials](https://cloud.google.com/docs/authentication/application-default-credentials) the Google Workspace **Group Administrator** admin role: https://knowledge.workspace.google.com/admin/users/assign-specific-admin-roles#service-account. Group membership is still synchronized by Okta group push. + +## Installation + +The plugin code is included in the published Access Docker image, but not installed. +Add these lines to the overlay Dockerfile based on the published image: + +```dockerfile +# Install the google group management plugin +WORKDIR /app/plugins +RUN pip install ./app_group_lifecycle_google + +# Reset working directory +WORKDIR /app +``` + +For local development, see [Development](#development) below. + +## Development + +### Install the Plugin + +Add the plugin and all its requirements to your virtual environment: + +```bash +pip install -e examples/plugins/app_group_lifecycle_google +``` + +### Set the Google Application Default Credentials + +Use your own identity, if you have the needed permissions: + +```bash +gcloud auth login --update-adc +``` + +Otherwise, impersonate Access via: + +```bash +gcloud auth application-default login --impersonate-service-account --scopes=https://www.googleapis.com/auth/cloud-identity.groups,https://www.googleapis.com/auth/cloud-platform +``` + +See https://docs.cloud.google.com/docs/authentication/use-service-account-impersonation#adc. + +### Testing + +The plugin's tests live alongside it in [`test_plugin.py`](./test_plugin.py). They stub the +Cloud Identity Groups API client and the Okta service, so they require neither Google/Okta +credentials nor the `google-*` libraries to be installed. + +Run the test suite with `tox` from the repository root (its pytest invocation collects the +whole repository, including this plugin, so the plugin suite runs alongside the core Access +tests both locally and in CI): + +```bash +tox +``` + +To run just this plugin's tests, or a single test, pass the path through to pytest via the +`test` environment: + +```bash +# Just this plugin's suite +tox -e test -- examples/plugins/app_group_lifecycle_google/test_plugin.py + +# A single test +tox -e test -- examples/plugins/app_group_lifecycle_google/test_plugin.py::test_metadata +``` \ No newline at end of file diff --git a/examples/plugins/app_group_lifecycle_google/__init__.py b/examples/plugins/app_group_lifecycle_google/__init__.py new file mode 100644 index 00000000..cb13a856 --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/__init__.py @@ -0,0 +1,7 @@ +""" +App Group Lifecycle Google Group Management Plugin + +Plugin that automatically creates and manages Google Groups for Access groups. +""" + +__version__ = "0.1.0" diff --git a/examples/plugins/app_group_lifecycle_google/setup.py b/examples/plugins/app_group_lifecycle_google/setup.py new file mode 100644 index 00000000..6f670c0c --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/setup.py @@ -0,0 +1,26 @@ +""" +Setup script for the App Group Lifecycle Google Group Management Plugin. + +This registers the plugin with Access via the setuptools entry_points mechanism. +""" + +from setuptools import setup + +setup( + name="app_group_lifecycle_google", + version="0.1.0", + description="Plugin for managing Google groups linked to Access groups", + author="Discord", + packages=["app_group_lifecycle_google"], + package_dir={"app_group_lifecycle_google": "."}, + install_requires=[ + "SQLAlchemy", + "google-api-python-client>=2.0.0", + "google-auth>=2.0.0", + ], + entry_points={ + "access_app_group_lifecycle": [ + "google_group_manager=app_group_lifecycle_google.plugin:google_group_manager_plugin", + ], + }, +) diff --git a/tox.ini b/tox.ini index a9c9ed0b..4ff15be0 100644 --- a/tox.ini +++ b/tox.ini @@ -11,22 +11,22 @@ setenv = PIP_CONSTRAINT = {toxinidir}/constraints.txt commands= - pytest -s tests + pytest -s ruff check . mypy . [testenv:test] commands= - pytest -s tests {posargs} + pytest -s {posargs} [testenv:test-verbose] commands= - pytest -rP tests {posargs} + pytest -rP {posargs} [testenv:test-with-postgresql] commands= - pytest -s tests {posargs} + pytest -s {posargs} setenv = TEST_DATABASE_URI = postgresql+pg8000://postgres:postgres@localhost:5432 From 88566884679d415177564ac33af78c7a9206d82c Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:32:02 +0000 Subject: [PATCH 12/19] feat(plugin): add config/status schema, validation, and email helpers Introduce the GoogleGroupManagerPlugin class with its app/group config and status schemas, config validation, and the email helpers that compose and parse the group's address. Group config validation enforces the Google-safe email charset and, when the app sets one, its email_pattern; both are also surfaced as client-side validation patterns. The email prefix is declared immutable, since the Cloud Identity groupKey can't change after creation. Google API and Okta wiring follow in later changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/plugin.py | 291 ++++++++++++++++++ .../app_group_lifecycle_google/test_plugin.py | 218 +++++++++++++ 2 files changed, 509 insertions(+) create mode 100644 examples/plugins/app_group_lifecycle_google/plugin.py create mode 100644 examples/plugins/app_group_lifecycle_google/test_plugin.py diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py new file mode 100644 index 00000000..37a6fc9b --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -0,0 +1,291 @@ +""" +App Group Lifecycle Google Group Management Plugin + +Creates, updates, and deletes Google groups for Access groups and links them via +Okta group push. All create/update/sync paths run one idempotent reconcile. +""" + +import logging +import os +import re +from typing import Any + +from google.auth import default +from googleapiclient.discovery import build + +from api.models import AppGroup +from api.plugins.app_group_lifecycle import ( + AppGroupLifecyclePluginConfigProperty, + AppGroupLifecyclePluginMetadata, + AppGroupLifecyclePluginStatusProperty, + get_config_value, + hookimpl, +) + +PLUGIN_ID = "google_group_manager" + +GOOGLE_GROUP_API_SCOPES = ["https://www.googleapis.com/auth/cloud-identity.groups"] + +ENV_OKTA_APP_ID = "GOOGLE_WORKSPACE_OKTA_APP_ID" +ENV_DOMAIN = "GOOGLE_WORKSPACE_DOMAIN" +ENV_CUSTOMER_ID = "GOOGLE_WORKSPACE_CUSTOMER_ID" + +# Required label marking a Cloud Identity group as a Google Workspace (discussion) group. +GROUP_DISCUSSION_FORUM_LABEL = "cloudidentity.googleapis.com/groups.discussion_forum" + +# App config keys +CONFIG_ENABLED = "enabled" +CONFIG_EMAIL_PATTERN = "email_pattern" +# Group config keys +CONFIG_EMAIL = "email" +CONFIG_DISPLAY_NAME = "display_name" +# Group status keys +STATUS_PUSH_MAPPING_ID = "push_mapping_id" +STATUS_GOOGLE_GROUP_ID = "google_group_id" +STATUS_SYNC_STATUS = "sync_status" +STATUS_SYNC_ERROR = "sync_error" +STATUS_LAST_SYNCED_AT = "last_synced_at" +# sync_status values +SYNC_SYNCED = "synced" +SYNC_PENDING = "pending" +SYNC_ERROR = "error" + +OKTA_GOOGLE_GROUP_PROFILE_FIELD_EMAIL = "googleGroupEmail" + +# Conservative subset of Google group local-part rules: lowercase alphanumerics +# plus . _ - internally; must start and end alphanumeric. +GOOGLE_LOCAL_PART_RE = re.compile(r"^[a-z0-9]([a-z0-9._-]*[a-z0-9])?$") + +logger = logging.getLogger(__name__) + + +class GoogleGroupManagerPlugin: + """Manages the Google-group lifecycle for Access groups.""" + + def __init__(self) -> None: + okta_app_id = os.environ.get(ENV_OKTA_APP_ID) + if not okta_app_id: + raise ValueError(f"{ENV_OKTA_APP_ID} environment variable is required") + self._okta_app_id = okta_app_id + + domain = os.environ.get(ENV_DOMAIN) + if not domain: + raise ValueError(f"{ENV_DOMAIN} environment variable is required") + self._domain = domain + + customer_id = os.environ.get(ENV_CUSTOMER_ID) + if not customer_id: + raise ValueError(f"{ENV_CUSTOMER_ID} environment variable is required") + self._customer_id = customer_id + + credentials, _ = default(scopes=GOOGLE_GROUP_API_SCOPES) + self._groups_api = build("cloudidentity", "v1", credentials=credentials).groups() + + # ---- Helpers ---- + + def _is_enabled(self, group: AppGroup) -> bool: + return bool(get_config_value(group.app, CONFIG_ENABLED, PLUGIN_ID, False)) + + def _full_email(self, prefix: str) -> str: + return f"{prefix}@{self._domain}" + + def _prefix_from_email(self, email: str) -> str | None: + suffix = f"@{self._domain}" + if not email.endswith(suffix): + return None + return email[: -len(suffix)] + + def _validate_email_against_pattern(self, prefix: str, pattern: str | None) -> str | None: + """Return an error message if the prefix violates the pattern, else None.""" + if not pattern: + return None + try: + if re.search(pattern, prefix) is None: + return f"The email prefix '{prefix}' does not match the required pattern '{pattern}'" + except re.error: + # A malformed pattern is reported at app-config validation; ignore here. + return None + return None + + def _group_config(self, group: AppGroup) -> tuple[str, str] | None: + """(email_prefix, display_name) if both present, else None.""" + email = get_config_value(group, CONFIG_EMAIL, PLUGIN_ID) + display_name = get_config_value(group, CONFIG_DISPLAY_NAME, PLUGIN_ID) + if email and display_name: + return email, display_name + return None + + # ---- Metadata ---- + + @hookimpl + def get_plugin_metadata(self) -> AppGroupLifecyclePluginMetadata | None: + return AppGroupLifecyclePluginMetadata( + id=PLUGIN_ID, + display_name="Google Group Management", + description=f"Creates and manages Google groups in the domain {self._domain}.", + ) + + # ---- Config schema ---- + + @hookimpl + def get_plugin_app_config_properties( + self, plugin_id: str | None + ) -> dict[str, AppGroupLifecyclePluginConfigProperty] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + return { + CONFIG_ENABLED: AppGroupLifecyclePluginConfigProperty( + display_name="Enabled?", + help_text="Enable automatic Google group management for this app", + type="boolean", + default_value=True, + required=True, + ), + CONFIG_EMAIL_PATTERN: AppGroupLifecyclePluginConfigProperty( + display_name="Email Prefix Pattern", + help_text=( + "Optional regex that each group's email prefix must match, " + f"e.g. ^gcp- to require addresses like gcp-security@{self._domain}" + ), + type="text", + required=False, + ), + } + + @hookimpl + def get_plugin_group_config_properties( + self, plugin_id: str | None, app_config: dict[str, Any] + ) -> dict[str, AppGroupLifecyclePluginConfigProperty] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + + # The email prefix must satisfy the Google-safe charset and, if the app configures + # one, the app's email_pattern. Surface both as client-side validation rules so the + # UI can reject an out-of-pattern prefix before submitting; the backend remains + # authoritative (it enforces the same in validate_plugin_group_config). + email_patterns = [ + { + "regex": GOOGLE_LOCAL_PART_RE.pattern, + "message": "Only lowercase letters, digits, and . _ - ; must start and end with a letter or digit", + } + ] + app_email_pattern = app_config.get(CONFIG_EMAIL_PATTERN) if isinstance(app_config, dict) else None + if app_email_pattern: + email_patterns.append( + {"regex": app_email_pattern, "message": f"Must match this app's email pattern: {app_email_pattern}"} + ) + + return { + CONFIG_EMAIL: AppGroupLifecyclePluginConfigProperty( + display_name="Google Group Email Prefix", + help_text=( + f"The local part of the address; the group will be prefix@{self._domain}. " + "Cannot be changed after the group is created." + ), + type="text", + required=True, + immutable=True, + validation={"patterns": email_patterns}, + ), + CONFIG_DISPLAY_NAME: AppGroupLifecyclePluginConfigProperty( + display_name="Google Group Display Name", + help_text="The display name of the linked Google group", + type="text", + required=True, + ), + } + + # ---- Status schema ---- + + @hookimpl + def get_plugin_app_status_properties( + self, plugin_id: str | None + ) -> dict[str, AppGroupLifecyclePluginStatusProperty] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + # This plugin has no app-level status; sync state is tracked per group. + return {} + + @hookimpl + def get_plugin_group_status_properties( + self, plugin_id: str | None + ) -> dict[str, AppGroupLifecyclePluginStatusProperty] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + return { + STATUS_PUSH_MAPPING_ID: AppGroupLifecyclePluginStatusProperty( + display_name="Okta Push Mapping ID", type="text" + ), + STATUS_GOOGLE_GROUP_ID: AppGroupLifecyclePluginStatusProperty( + display_name="Google Group ID", type="text" + ), + STATUS_SYNC_STATUS: AppGroupLifecyclePluginStatusProperty( + display_name="Sync Status", help_text="synced, pending, or error", type="text" + ), + STATUS_SYNC_ERROR: AppGroupLifecyclePluginStatusProperty( + display_name="Sync Error", type="text" + ), + STATUS_LAST_SYNCED_AT: AppGroupLifecyclePluginStatusProperty( + display_name="Last Synced", type="date" + ), + } + + # ---- Validation ---- + + @hookimpl + def validate_plugin_app_config(self, config: dict[str, Any], plugin_id: str | None) -> dict[str, str] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + errors: dict[str, str] = {} + if CONFIG_ENABLED not in config: + errors[CONFIG_ENABLED] = "The 'enabled' field is required" + elif not isinstance(config[CONFIG_ENABLED], bool): + errors[CONFIG_ENABLED] = "The 'enabled' field must be a boolean" + + pattern = config.get(CONFIG_EMAIL_PATTERN) + if pattern: + if not isinstance(pattern, str): + errors[CONFIG_EMAIL_PATTERN] = "The 'email_pattern' field must be a string" + else: + try: + re.compile(pattern) + except re.error as e: + errors[CONFIG_EMAIL_PATTERN] = f"Invalid regex: {e}" + return errors + + @hookimpl + def validate_plugin_group_config( + self, config: dict[str, Any], app_config: dict[str, Any], plugin_id: str | None + ) -> dict[str, str] | None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return None + errors: dict[str, str] = {} + + display_name = config.get(CONFIG_DISPLAY_NAME) + if CONFIG_DISPLAY_NAME not in config: + errors[CONFIG_DISPLAY_NAME] = "The 'display_name' field is required" + elif not isinstance(display_name, str) or not display_name: + errors[CONFIG_DISPLAY_NAME] = "The 'display_name' field must be a non-empty string" + + email = config.get(CONFIG_EMAIL) + if CONFIG_EMAIL not in config: + errors[CONFIG_EMAIL] = "The 'email' field is required" + elif not isinstance(email, str): + errors[CONFIG_EMAIL] = "The 'email' field must be a string" + elif not GOOGLE_LOCAL_PART_RE.match(email): + errors[CONFIG_EMAIL] = ( + "The 'email' prefix may contain only lowercase letters, digits, and . _ - " + "and must start and end with a letter or digit" + ) + else: + # The email is a valid prefix; also enforce the app's optional email_pattern + # here so a violation is reported synchronously at create/update (a 400), + # not only later during reconciliation. app_config is the app-level + # configuration for this plugin (empty if the app has none). + pattern_error = self._validate_email_against_pattern( + email, app_config.get(CONFIG_EMAIL_PATTERN) if isinstance(app_config, dict) else None + ) + if pattern_error: + errors[CONFIG_EMAIL] = pattern_error + + return errors diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py new file mode 100644 index 00000000..e0c9ab8b --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -0,0 +1,218 @@ +"""Tests for the Google Groups Lifecycle Plugin.""" + +import os +import sys +from pathlib import Path +from unittest.mock import MagicMock, Mock + +import pytest +from pytest_mock import MockerFixture + +# The plugin instantiates at import time and needs these env vars + Google libs. +os.environ["GOOGLE_WORKSPACE_OKTA_APP_ID"] = "test-okta-app-123" +os.environ["GOOGLE_WORKSPACE_DOMAIN"] = "test-company.com" +os.environ["GOOGLE_WORKSPACE_CUSTOMER_ID"] = "C0test" + +mock_google_auth = MagicMock() +mock_google_auth.default = MagicMock(return_value=(MagicMock(), None)) +mock_googleapiclient_discovery = MagicMock() +mock_googleapiclient_discovery.build = MagicMock(return_value=MagicMock()) + +sys.modules["google"] = MagicMock() +sys.modules["google.auth"] = mock_google_auth +sys.modules["google.cloud"] = MagicMock() +sys.modules["google.cloud.sql"] = MagicMock() +sys.modules["google.cloud.sql.connector"] = MagicMock() +sys.modules["googleapiclient"] = MagicMock() +sys.modules["googleapiclient.discovery"] = mock_googleapiclient_discovery + + +class _FakeHttpError(Exception): + """Stand-in for googleapiclient.errors.HttpError carrying an HTTP status.""" + + def __init__(self, status: int) -> None: + super().__init__(f"HTTP {status}") + self.resp = Mock(status=status) + + +_errors_module = MagicMock() +_errors_module.HttpError = _FakeHttpError +sys.modules["googleapiclient.errors"] = _errors_module + +plugin_dir = Path(__file__).parent +if str(plugin_dir) not in sys.path: + sys.path.insert(0, str(plugin_dir)) + +from api.models import App, AppGroup # noqa: E402 +from plugin import ( # noqa: E402 + PLUGIN_ID, + GoogleGroupManagerPlugin, +) + + +@pytest.fixture +def mock_groups_api(mocker: MockerFixture): + mocker.patch("plugin.default", return_value=(Mock(), None)) + discovery_client = MagicMock() + mocker.patch("plugin.build", return_value=discovery_client) + groups_api = MagicMock() + discovery_client.groups.return_value = groups_api + return groups_api + + +@pytest.fixture +def plugin_instance(mocker: MockerFixture, mock_groups_api): + mocker.patch.dict(os.environ, { + "GOOGLE_WORKSPACE_OKTA_APP_ID": "test-okta-app-123", + "GOOGLE_WORKSPACE_DOMAIN": "test-company.com", + "GOOGLE_WORKSPACE_CUSTOMER_ID": "C0test", + }) + return GoogleGroupManagerPlugin() + + +def test_metadata(plugin_instance): + meta = plugin_instance.get_plugin_metadata() + assert meta.id == PLUGIN_ID + assert meta.display_name + + +def test_app_config_properties_shape(plugin_instance): + props = plugin_instance.get_plugin_app_config_properties(PLUGIN_ID) + assert set(props) == {"enabled", "email_pattern"} + assert props["enabled"].required is True + + +def test_group_config_properties_shape(plugin_instance): + props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) + assert set(props) == {"email", "display_name"} + assert props["email"].required is True + assert props["display_name"].required is True + + +def test_group_config_properties_surface_validation_patterns(plugin_instance): + from plugin import GOOGLE_LOCAL_PART_RE + + # With no app pattern, the email property carries just the Google-safe charset rule. + patterns = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {})["email"].validation["patterns"] + assert [p["regex"] for p in patterns] == [GOOGLE_LOCAL_PART_RE.pattern] + + # With an app email_pattern, it is appended as a second rule. + patterns = plugin_instance.get_plugin_group_config_properties( + PLUGIN_ID, {"email_pattern": r"^sec-"} + )["email"].validation["patterns"] + assert [p["regex"] for p in patterns] == [GOOGLE_LOCAL_PART_RE.pattern, r"^sec-"] + + +def test_group_status_properties_shape(plugin_instance): + props = plugin_instance.get_plugin_group_status_properties(PLUGIN_ID) + assert set(props) == { + "push_mapping_id", "google_group_id", "sync_status", "sync_error", "last_synced_at", + } + + +@pytest.mark.parametrize("pattern,ok", [(None, True), (r"^[a-z-]+$", True), (r"([", False)]) +def test_validate_app_config_email_pattern(plugin_instance, pattern, ok): + config = {"enabled": True} + if pattern is not None: + config["email_pattern"] = pattern + errors = plugin_instance.validate_plugin_app_config(config, PLUGIN_ID) + assert (errors == {}) is ok + + +def test_validate_app_config_requires_enabled(plugin_instance): + errors = plugin_instance.validate_plugin_app_config({}, PLUGIN_ID) + assert "enabled" in errors + + +def test_validate_group_config_valid(plugin_instance): + errors = plugin_instance.validate_plugin_group_config( + {"email": "platform-security", "display_name": "Platform Security"}, {}, PLUGIN_ID + ) + assert errors == {} + + +@pytest.mark.parametrize("config,bad_key", [ + ({"display_name": "X"}, "email"), # missing email + ({"email": "ok"}, "display_name"), # missing display_name + ({"email": "Bad-Upper", "display_name": "X"}, "email"), # uppercase fails charset + ({"email": "-bad", "display_name": "X"}, "email"), # leading hyphen fails charset +]) + + +def test_validate_group_config_errors(plugin_instance, config, bad_key): + errors = plugin_instance.validate_plugin_group_config(config, {}, PLUGIN_ID) + assert bad_key in errors + + +def test_validate_group_config_ignores_other_plugin(plugin_instance): + assert plugin_instance.validate_plugin_group_config({}, {}, "some_other_plugin") is None + + +def test_validate_group_config_enforces_app_email_pattern(plugin_instance): + # A prefix that is charset-valid but violates the app's email_pattern is rejected. + app_config = {"email_pattern": r"^sec-"} + errors = plugin_instance.validate_plugin_group_config( + {"email": "platform", "display_name": "X"}, app_config, PLUGIN_ID + ) + assert "email" in errors + + # A prefix that satisfies the app pattern passes. + errors = plugin_instance.validate_plugin_group_config( + {"email": "sec-platform", "display_name": "X"}, app_config, PLUGIN_ID + ) + assert errors == {} + + +def _group(mocker, *, app_config=None, group_config=None, status=None, description=""): + app = Mock(spec=App) + app.plugin_data = {PLUGIN_ID: {"configuration": app_config or {"enabled": True}, "status": {}}} + group = Mock(spec=AppGroup) + group.id = "grp-1" + group.name = "App-Google-Platform-Security" + group.description = description + group.app = app + group.plugin_data = {PLUGIN_ID: {"configuration": group_config or {}, "status": status or {}}} + return group + + +def test_full_email_appends_domain(plugin_instance): + assert plugin_instance._full_email("platform-security") == "platform-security@test-company.com" + + +def test_prefix_from_email_strips_domain(plugin_instance): + assert plugin_instance._prefix_from_email("platform-security@test-company.com") == "platform-security" + + +def test_prefix_from_email_returns_none_on_domain_mismatch(plugin_instance): + assert plugin_instance._prefix_from_email("x@other.com") is None + + +def test_is_enabled_reads_app_config(plugin_instance, mocker): + group = _group(mocker) + mocker.patch("plugin.get_config_value", return_value=True) + assert plugin_instance._is_enabled(group) is True + + +def test_validate_email_against_pattern(plugin_instance): + assert plugin_instance._validate_email_against_pattern("platform", r"^sec-") is not None + assert plugin_instance._validate_email_against_pattern("sec-platform", r"^sec-") is None + assert plugin_instance._validate_email_against_pattern("anything", None) is None + + +def test_group_config_returns_pair_or_none(plugin_instance, mocker): + # both present -> tuple; missing one -> None + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "email": "sec", "display_name": "Security", + }.get(key, default)) + assert plugin_instance._group_config(_group(mocker)) == ("sec", "Security") + + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "email": "sec", + }.get(key, default)) + assert plugin_instance._group_config(_group(mocker)) is None + + +def test_email_config_property_is_immutable(plugin_instance): + props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) + assert props["email"].immutable is True + assert props["display_name"].immutable is False From 679a0c1fce2755625c09f21f2bef075abc36c006 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:32:13 +0000 Subject: [PATCH 13/19] feat(plugin): add Cloud Identity Groups API wrappers for group CRUD Add the create/get/patch/delete and lookup-by-email helpers over the Cloud Identity Groups API (cloudidentity.googleapis.com). Groups are addressed by resource name; create sets the customer parent and the discussion-forum label; patch only ever touches the mutable displayName/description (groupKey is immutable); lookup resolves an email to the bare group id. These back the reconcile logic added later. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/plugin.py | 67 ++++++++++++++++++ .../app_group_lifecycle_google/test_plugin.py | 70 ++++++++++++++++++- 2 files changed, 135 insertions(+), 2 deletions(-) diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py index 37a6fc9b..563f145e 100644 --- a/examples/plugins/app_group_lifecycle_google/plugin.py +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -12,6 +12,7 @@ from google.auth import default from googleapiclient.discovery import build +from googleapiclient.errors import HttpError from api.models import AppGroup from api.plugins.app_group_lifecycle import ( @@ -59,6 +60,16 @@ logger = logging.getLogger(__name__) +def _is_group_absent_error(error: HttpError) -> bool: + """Whether a Cloud Identity error means the group is not visible to us. + + The Groups API returns 403 (PERMISSION_DENIED, "...or it may not exist") rather than + 404 for a group the caller can't see -- including one that simply doesn't exist yet. + Treating both as "absent" lets reconcile create the group instead of erroring; a + genuine permission problem then surfaces on the subsequent create call.""" + return getattr(getattr(error, "resp", None), "status", None) in (403, 404) + + class GoogleGroupManagerPlugin: """Manages the Google-group lifecycle for Access groups.""" @@ -289,3 +300,59 @@ def validate_plugin_group_config( errors[CONFIG_EMAIL] = pattern_error return errors + + # ---- Google API wrappers (Cloud Identity Groups API) ---- + + def _resource_name(self, google_group_id: str) -> str: + return f"groups/{google_group_id}" + + def _create_google_group(self, prefix: str, display_name: str, description: str) -> str: + body = { + "parent": f"customers/{self._customer_id}", + "groupKey": {"id": self._full_email(prefix)}, + "displayName": display_name, + "description": description or "", + "labels": {GROUP_DISCUSSION_FORUM_LABEL: ""}, + } + # initialGroupConfig=EMPTY: the admin-role service account creates the group without + # being added as an owner; Okta group push owns membership and ownership. + operation = self._groups_api.create(body=body, initialGroupConfig="EMPTY").execute() + created = operation.get("response") or {} + name = created.get("name") + if not name: + raise ValueError(f"Google group creation returned no resource name: {operation}") + return name.split("/", 1)[1] + + def _get_google_group(self, google_group_id: str) -> dict[str, Any]: + return self._groups_api.get(name=self._resource_name(google_group_id)).execute() + + def _patch_google_group( + self, google_group_id: str, *, display_name: str | None = None, description: str | None = None + ) -> None: + """Patch a Google group's mutable properties. groupKey (email) is immutable, so only + displayName/description are patchable; pass only the fields to change. No-op if none.""" + body: dict[str, Any] = {} + if display_name is not None: + body["displayName"] = display_name + if description is not None: + body["description"] = description + if not body: + return + update_mask = ",".join(sorted(body)) + self._groups_api.patch( + name=self._resource_name(google_group_id), body=body, updateMask=update_mask + ).execute() + + def _delete_google_group(self, google_group_id: str) -> None: + self._groups_api.delete(name=self._resource_name(google_group_id)).execute() + + def _lookup_google_group_id(self, email: str) -> str | None: + """Resolve an email to its bare Cloud Identity group id, or None if no such group.""" + try: + result = self._groups_api.lookup(groupKey_id=email).execute() + except HttpError as e: + if _is_group_absent_error(e): + return None + raise + name = result.get("name") + return name.split("/", 1)[1] if name else None diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index e0c9ab8b..299a643b 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -43,12 +43,14 @@ def __init__(self, status: int) -> None: if str(plugin_dir) not in sys.path: sys.path.insert(0, str(plugin_dir)) -from api.models import App, AppGroup # noqa: E402 from plugin import ( # noqa: E402 + GROUP_DISCUSSION_FORUM_LABEL, PLUGIN_ID, GoogleGroupManagerPlugin, ) +from api.models import App, AppGroup # noqa: E402 + @pytest.fixture def mock_groups_api(mocker: MockerFixture): @@ -212,7 +214,71 @@ def test_group_config_returns_pair_or_none(plugin_instance, mocker): assert plugin_instance._group_config(_group(mocker)) is None -def test_email_config_property_is_immutable(plugin_instance): +def test_create_google_group_calls_create(plugin_instance, mock_groups_api): + mock_groups_api.create().execute.return_value = { + "done": True, + "response": {"name": "groups/ggid-1", "groupKey": {"id": "platform-security@test-company.com"}}, + } + group_id = plugin_instance._create_google_group("platform-security", "Platform Security", "desc") + assert group_id == "ggid-1" + kwargs = mock_groups_api.create.call_args.kwargs + assert kwargs["initialGroupConfig"] == "EMPTY" + assert kwargs["body"] == { + "parent": "customers/C0test", + "groupKey": {"id": "platform-security@test-company.com"}, + "displayName": "Platform Security", + "description": "desc", + "labels": {GROUP_DISCUSSION_FORUM_LABEL: ""}, + } + + +def test_get_google_group_calls_get_by_resource_name(plugin_instance, mock_groups_api): + mock_groups_api.get().execute.return_value = {"name": "groups/ggid-1"} + assert plugin_instance._get_google_group("ggid-1")["name"] == "groups/ggid-1" + assert mock_groups_api.get.call_args.kwargs == {"name": "groups/ggid-1"} + + +def test_patch_google_group_sets_update_mask(plugin_instance, mock_groups_api): + plugin_instance._patch_google_group("ggid-1", display_name="New", description="d") + kwargs = mock_groups_api.patch.call_args.kwargs + assert kwargs["name"] == "groups/ggid-1" + assert kwargs["body"] == {"displayName": "New", "description": "d"} + assert kwargs["updateMask"] == "description,displayName" + + +def test_patch_google_group_noop_when_no_fields(plugin_instance, mock_groups_api): + plugin_instance._patch_google_group("ggid-1") + mock_groups_api.patch.assert_not_called() + + +def test_delete_google_group_calls_delete_by_resource_name(plugin_instance, mock_groups_api): + plugin_instance._delete_google_group("ggid-1") + assert mock_groups_api.delete.call_args.kwargs == {"name": "groups/ggid-1"} + + +def test_lookup_returns_bare_id(plugin_instance, mock_groups_api): + mock_groups_api.lookup().execute.return_value = {"name": "groups/ggid-9"} + assert plugin_instance._lookup_google_group_id("x@test-company.com") == "ggid-9" + assert mock_groups_api.lookup.call_args.kwargs == {"groupKey_id": "x@test-company.com"} + + +def test_lookup_returns_none_on_404(plugin_instance, mock_groups_api): + from googleapiclient.errors import HttpError + + mock_groups_api.lookup().execute.side_effect = HttpError(404) + assert plugin_instance._lookup_google_group_id("missing@test-company.com") is None + + +def test_lookup_returns_none_on_403(plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock) -> None: + # Cloud Identity returns 403 (permission-denied "or it may not exist") for a group + # that doesn't exist, not 404; the lookup must treat it as absent, not raise. + from googleapiclient.errors import HttpError + + mock_groups_api.lookup().execute.side_effect = HttpError(403) + assert plugin_instance._lookup_google_group_id("missing@test-company.com") is None + + +def test_email_config_property_is_immutable(plugin_instance: GoogleGroupManagerPlugin) -> None: props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) assert props["email"].immutable is True assert props["display_name"].immutable is False From 6ae1b3196350bd0e54c13916fe882b83fb03058e Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:32:25 +0000 Subject: [PATCH 14/19] feat(plugin): add Okta push-mapping creation and link discovery Add the Okta side of linking: create a group push mapping between an Access-managed Okta group and the Google group, find the Okta-imported target group by its googleGroupEmail profile attribute, and discover an existing link (mapping id + Google email) for a group so out-of-band links can be adopted during reconciliation. Cloud Identity group ids differ from the Directory ids Okta stores, so the join is by email, which the caller resolves to an id. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/plugin.py | 67 +++++++++++ .../app_group_lifecycle_google/test_plugin.py | 104 ++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py index 563f145e..c0d0f27a 100644 --- a/examples/plugins/app_group_lifecycle_google/plugin.py +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -21,7 +21,9 @@ AppGroupLifecyclePluginStatusProperty, get_config_value, hookimpl, + set_status_value, ) +from api.services import okta # used by the push-mapping/discovery helpers PLUGIN_ID = "google_group_manager" @@ -70,6 +72,13 @@ def _is_group_absent_error(error: HttpError) -> bool: return getattr(getattr(error, "resp", None), "status", None) in (403, 404) +class AmbiguousOktaTargetError(Exception): + """More than one Okta target group matches a Google group email, so a push mapping cannot be + created unambiguously. This is a misconfiguration (e.g. a stale + re-imported target sharing + the same googleGroupEmail) that will not self-heal, so it is surfaced as a sync error rather + than conflated with the not-yet-imported case (which simply defers).""" + + class GoogleGroupManagerPlugin: """Manages the Google-group lifecycle for Access groups.""" @@ -356,3 +365,61 @@ def _lookup_google_group_id(self, email: str) -> str | None: raise name = result.get("name") return name.split("/", 1)[1] if name else None + + # ---- Okta push-mapping + discovery ---- + + def _okta_target_group_id(self, email: str) -> str | None: + """Find the Okta-imported target group for a Google group email, if present. + The email is the stable join key (the Directory numeric id Cloud Identity does + not reproduce, but Okta's googleGroupEmail attribute is immutable).""" + query = f'type eq "APP_GROUP" and profile.{OKTA_GOOGLE_GROUP_PROFILE_FIELD_EMAIL} eq "{email}"' + matches = okta.list_groups(query_params={"search": query}) + if len(matches) > 1: + raise AmbiguousOktaTargetError( + f"{len(matches)} Okta target groups carry googleGroupEmail '{email}'; " + "cannot create a push mapping unambiguously" + ) + if not matches: + return None + return str(matches[0].group.id) + + def _create_push_mapping(self, group: AppGroup, email: str) -> bool: + """Create the Okta push mapping. Returns False (defer) if Okta has not + imported the target group yet.""" + target_group_id = self._okta_target_group_id(email) + if target_group_id is None: + return False + result = okta.create_group_push_mapping( + appId=self._okta_app_id, sourceGroupId=group.id, targetGroupId=target_group_id + ) + mapping_id = result.get("id") + if not mapping_id: + raise ValueError(f"Okta push mapping creation returned no id: {result}") + set_status_value(group, STATUS_PUSH_MAPPING_ID, mapping_id, PLUGIN_ID) + return True + + def _discover_existing_link(self, group: AppGroup) -> dict[str, Any] | None: + """Find an existing push mapping for this Access group and recover the linked + Google group email from the Okta target group profile. Returns None if no link + exists. The caller resolves the email to a Cloud Identity id via lookup.""" + mappings = okta.list_group_push_mappings(self._okta_app_id) + mapping = next((m for m in mappings if m.get("sourceGroupId") == group.id), None) + if mapping is None: + logger.debug(f"No mapping found for group {group.name}.") + return None + + target_group_id = mapping.get("targetGroupId") + if not target_group_id: + logger.debug(f"Failed to get target group ID mapped to {group.name}. Mapping:\n{mapping}") + return None + + profile = okta.get_group(target_group_id).group.profile + email = getattr(profile, OKTA_GOOGLE_GROUP_PROFILE_FIELD_EMAIL, None) + if not email: + logger.debug( + f"Google group email could not be resolved for target group mapped to {group.name}.\n" + f"Target group {target_group_id} has profile:\n{profile}" + ) + return None + + return {"email": str(email), "push_mapping_id": mapping.get("id")} diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index 299a643b..099963f8 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -46,6 +46,7 @@ def __init__(self, status: int) -> None: from plugin import ( # noqa: E402 GROUP_DISCUSSION_FORUM_LABEL, PLUGIN_ID, + STATUS_PUSH_MAPPING_ID, GoogleGroupManagerPlugin, ) @@ -282,3 +283,106 @@ def test_email_config_property_is_immutable(plugin_instance: GoogleGroupManagerP props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) assert props["email"].immutable is True assert props["display_name"].immutable is False + + +def test_create_push_mapping_sets_status(plugin_instance, mocker): + group = _group(mocker) + mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-1"))]) + mocker.patch("plugin.okta.create_group_push_mapping", return_value={"id": "map-1"}) + set_status = mocker.patch("plugin.set_status_value") + + created = plugin_instance._create_push_mapping(group, "sec@test-company.com") + + assert created is True + set_status.assert_any_call(group, STATUS_PUSH_MAPPING_ID, "map-1", PLUGIN_ID) + + +def test_create_push_mapping_defers_when_target_not_imported(plugin_instance, mocker): + group = _group(mocker) + mocker.patch("plugin.okta.list_groups", return_value=[]) # Okta hasn't imported it yet + create = mocker.patch("plugin.okta.create_group_push_mapping") + + created = plugin_instance._create_push_mapping(group, "sec@test-company.com") + + assert created is False + create.assert_not_called() + + +def test_discover_existing_link_finds_mapping(plugin_instance, mocker): + group = _group(mocker) + mocker.patch( + "plugin.okta.list_group_push_mappings", + return_value=[{"id": "map-9", "sourceGroupId": "grp-1", "targetGroupId": "okta-tgt-9"}], + ) + tgt = Mock() + tgt.group = Mock(id="okta-tgt-9") + tgt.group.profile = Mock(googleGroupEmail="found@test-company.com") + mocker.patch("plugin.okta.get_group", return_value=tgt) + + link = plugin_instance._discover_existing_link(group) + + assert link == { + "email": "found@test-company.com", + "push_mapping_id": "map-9", + } + + +def test_discover_existing_link_returns_none_when_no_mapping(plugin_instance, mocker): + group = _group(mocker) + mocker.patch("plugin.okta.list_group_push_mappings", return_value=[]) + assert plugin_instance._discover_existing_link(group) is None + + +def test_okta_target_group_id_searches_by_email(plugin_instance, mocker): + list_groups = mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-7"))]) + assert plugin_instance._okta_target_group_id("sec@test-company.com") == "okta-tgt-7" + search = list_groups.call_args.kwargs["query_params"]["search"] + assert "googleGroupEmail" in search + assert "sec@test-company.com" in search + + +def test_okta_target_group_id_none_when_not_imported( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # Zero matches means Okta hasn't imported the group yet -> defer (None), not an error. + mocker.patch("plugin.okta.list_groups", return_value=[]) + assert plugin_instance._okta_target_group_id("sec@test-company.com") is None + + +def test_okta_target_group_id_raises_on_ambiguous_match( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # More than one Okta target group carrying the same googleGroupEmail is a misconfiguration + # that will never self-heal; it must surface as an error, not be conflated with "not imported". + from plugin import AmbiguousOktaTargetError + + mocker.patch( + "plugin.okta.list_groups", + return_value=[Mock(group=Mock(id="okta-tgt-1")), Mock(group=Mock(id="okta-tgt-2"))], + ) + with pytest.raises(AmbiguousOktaTargetError): + plugin_instance._okta_target_group_id("sec@test-company.com") + + +def test_create_push_mapping_resolves_target_by_email( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + group = _group(mocker) + mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-1"))]) + mocker.patch("plugin.okta.create_group_push_mapping", return_value={"id": "map-1"}) + set_status = mocker.patch("plugin.set_status_value") + + assert plugin_instance._create_push_mapping(group, "sec@test-company.com") is True + set_status.assert_any_call(group, STATUS_PUSH_MAPPING_ID, "map-1", PLUGIN_ID) + + +def test_discover_existing_link_recovers_email(plugin_instance, mocker): + group = _group(mocker) + profile = Mock(googleGroupEmail="sec@test-company.com") + mocker.patch("plugin.okta.list_group_push_mappings", return_value=[ + {"id": "map-1", "sourceGroupId": "grp-1", "targetGroupId": "okta-tgt-1"}, + ]) + mocker.patch("plugin.okta.get_group", return_value=Mock(group=Mock(profile=profile))) + + link = plugin_instance._discover_existing_link(group) + assert link == {"email": "sec@test-company.com", "push_mapping_id": "map-1"} From 9bd08385b4d1fe3f5ad5f7accbab4a9a1fe60763 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:32:36 +0000 Subject: [PATCH 15/19] feat(plugin): add the idempotent reconcile core Reconcile resolves a group's Cloud Identity id (cached id, re-looked-up by email when missing or 404, or adopted from an existing Okta link), creates the Google group when none exists and config is present, and otherwise adopts or enforces its properties: missing Access config is backfilled from the live group, present config is pushed to Google. The email/groupKey is immutable so it's never patched, which also grandfathers groups whose prefix predates a later email_pattern. Blocking Google/Okta errors are recorded as an error sync status (committed inside the hook so it survives the host's post-error rollback), and the push mapping is (re)created, deferring to pending until Okta has imported the group. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/plugin.py | 250 +++++++- .../app_group_lifecycle_google/test_plugin.py | 573 ++++++++++++++++++ 2 files changed, 822 insertions(+), 1 deletion(-) diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py index c0d0f27a..4d0b8ca1 100644 --- a/examples/plugins/app_group_lifecycle_google/plugin.py +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -8,19 +8,24 @@ import logging import os import re +from datetime import datetime from typing import Any from google.auth import default from googleapiclient.discovery import build from googleapiclient.errors import HttpError +from sqlalchemy import select, text +from sqlalchemy.orm import Session -from api.models import AppGroup +from api.models import App, AppGroup from api.plugins.app_group_lifecycle import ( AppGroupLifecyclePluginConfigProperty, AppGroupLifecyclePluginMetadata, AppGroupLifecyclePluginStatusProperty, get_config_value, + get_status_value, hookimpl, + set_config_value, set_status_value, ) from api.services import okta # used by the push-mapping/discovery helpers @@ -423,3 +428,246 @@ def _discover_existing_link(self, group: AppGroup) -> dict[str, Any] | None: return None return {"email": str(email), "push_mapping_id": mapping.get("id")} + + # ---- Status setters ---- + + def _mark(self, session: Session, group: AppGroup, status: str, error: str | None = None) -> None: + set_status_value(group, STATUS_SYNC_STATUS, status, PLUGIN_ID) + set_status_value(group, STATUS_SYNC_ERROR, error, PLUGIN_ID) + if status == SYNC_SYNCED: + set_status_value(group, STATUS_LAST_SYNCED_AT, datetime.utcnow().isoformat(), PLUGIN_ID) + session.add(group) + session.commit() + + # ---- Reconcile ---- + + def _owned_group_id(self, group: AppGroup) -> str | None: + """The Google group id this Access group already owns (claimed on a prior reconcile), + if it still exists. The recorded id is an ownership token -- it is written only after + the ownership check passes (see _claim_group_id) -- so a live cached id needs no + re-check. Clears the cached id and returns None if the group was deleted out of band, + so the caller re-resolves/recreates. Returns None when nothing is cached.""" + cached = get_status_value(group, STATUS_GOOGLE_GROUP_ID, PLUGIN_ID) + if not cached: + return None + try: + self._get_google_group(cached) + return cached + except HttpError as e: + if not _is_group_absent_error(e): + raise + logger.info(f"Cached Google group id {cached} for {group.name} is gone; clearing and re-resolving.") + set_status_value(group, STATUS_GOOGLE_GROUP_ID, None, PLUGIN_ID) + return None + + def _lock_claim(self, session: Session, candidate_id: str) -> None: + """Serialize concurrent claims of the same Google group, closing the check-then-claim race + in _claim_group_id. Takes a Postgres transaction-level advisory lock keyed on the candidate + id (auto-released at commit/rollback), so a second reconcile claiming the same id blocks + until the first commits and can then observe it as owned. A no-op on non-Postgres backends + (e.g. the SQLite test DB), where the relevant sync paths are single-writer.""" + bind = session.get_bind() + if bind is None or bind.dialect.name != "postgresql": + return + # hashtextextended maps the id to the bigint the advisory-lock functions take; key + # collisions only cause extra (harmless) serialization. + session.execute(text("SELECT pg_advisory_xact_lock(hashtextextended(:key, 0))"), {"key": candidate_id}) + + def _claim_group_id( + self, session: Session, group: AppGroup, candidate_id: str, display_email: str | None = None + ) -> str | None: + """Record candidate_id as this group's owned Google group, but ONLY after confirming no + other Access group already owns it -- refusing (and marking SYNC_ERROR) rather than + clobbering / double-linking a group owned elsewhere. Returns the id on success, or None + when refused. A no-op confirmation when we already hold this id. + + Persisting the id is gated on the ownership check (not the reverse) so a refused group + never carries another group's id into its status, where group_deleted would later act on + it. The check and the claim are serialized by an advisory lock on the candidate id (see + _lock_claim) so two concurrent reconciles can't both pass the check for the same group.""" + if get_status_value(group, STATUS_GOOGLE_GROUP_ID, PLUGIN_ID) == candidate_id: + return candidate_id + self._lock_claim(session, candidate_id) + owner = self._google_group_owner(session, group, candidate_id) + if owner is not None: + self._mark( + session, + group, + SYNC_ERROR, + f"Google group {display_email or candidate_id} is already managed by Access group " + f"'{owner.name}'; refusing to adopt it.", + ) + return None + set_status_value(group, STATUS_GOOGLE_GROUP_ID, candidate_id, PLUGIN_ID) + return candidate_id + + def _email_from_status(self, group: AppGroup) -> str | None: + """Recover the group email from a cached id when the Access-side config is absent + (adoption path). Returns the full email, or None when there is no cached id or the cached + group was deleted out of band (mirrors _owned_group_id's absent-error handling, so a + vanished group defers rather than hard-erroring reconcile).""" + google_group_id = get_status_value(group, STATUS_GOOGLE_GROUP_ID, PLUGIN_ID) + if not google_group_id: + return None + try: + live = self._get_google_group(google_group_id) + except HttpError as e: + if not _is_group_absent_error(e): + raise + logger.info(f"Cached Google group id {google_group_id} for {group.name} is gone; cannot recover email.") + return None + return (live.get("groupKey") or {}).get("id") + + def _google_group_owner(self, session: Session, group: AppGroup, google_group_id: str) -> AppGroup | None: + """Another active Access group that already owns this Google group -- i.e. records its id + in status. Used to refuse adopting (and clobbering / double-linking) a Google group already + owned elsewhere. + + Ownership keys on the recorded google_group_id ALONE, not on whether a push mapping exists + yet: the id is recorded only after this same ownership check passes (see _claim_group_id), + while the push mapping is created later and may defer until Okta imports the group. Were we + to also require a push mapping, a second group reconciling during that window would not see + the real owner and would double-claim the group. + + Access state is the source of truth here (not Okta's imported target groups, which lag + behind and may not be queryable yet). The search spans every app configured with this + plugin, not just this group's app: one Google Workspace can back several Access apps, and + those apps all set this same plugin id, so a Google group can be owned by a group in any + of them. + + The id predicate is pushed into SQL (a JSON path lookup on the stored status), so this + stays a point lookup -- not a scan of every plugin-managed group on each reconcile. It also + only runs for a group that isn't yet linked.""" + google_group_id_path = (PLUGIN_ID, "status", STATUS_GOOGLE_GROUP_ID) + return session.scalars( + select(AppGroup) + .join(App, AppGroup.app_id == App.id) + .where(App.app_group_lifecycle_plugin == PLUGIN_ID) + .where(App.deleted_at.is_(None)) + .where(AppGroup.id != group.id) + .where(AppGroup.deleted_at.is_(None)) + .where(AppGroup.plugin_data[google_group_id_path].as_string() == google_group_id) + .limit(1) + ).first() + + def _reconcile(self, session: Session, group: AppGroup) -> None: + """Idempotent: resolve/adopt/create the Google group, enforce its properties, + link via Okta push, and record sync status. Commits sync_status inside the hook + so it survives the host's post-hook rollback on error.""" + if not self._is_enabled(group): + return + + try: + config = self._group_config(group) + email = self._full_email(config[0]) if config is not None else None + + # A Google group id we already own (claimed on a prior reconcile), if still live. + google_group_id = self._owned_group_id(group) + + if google_group_id is None: + # Not yet owned. Find a candidate -- an existing Google group at our email, or one + # behind an out-of-band Okta link -- then CLAIM it: record it only after confirming + # no other Access group owns it. Refuse rather than adopt a group owned elsewhere. + candidate = self._lookup_google_group_id(email) if email is not None else None + link = None + if candidate is None: + link = self._discover_existing_link(group) + if link is not None and link.get("email"): + logger.info(f"Backfilling group link for {group.name} that was added out-of-band...") + candidate = self._lookup_google_group_id(link["email"]) + if candidate is not None: + display_email = email or (link.get("email") if link else None) + google_group_id = self._claim_group_id(session, group, candidate, display_email) + if google_group_id is None: + return # owned by another Access group; _claim_group_id marked the error + if link is not None and link.get("push_mapping_id"): + set_status_value(group, STATUS_PUSH_MAPPING_ID, link["push_mapping_id"], PLUGIN_ID) + + if google_group_id is None: + # Nothing to adopt -> create from config (or skip when config is absent). Create is + # self-guarding against duplicate prefixes: Cloud Identity rejects a second group at + # the same email, so no ownership check is needed before recording the new id. + if config is None: + logger.info(f"Skipping {group.name} due to missing required config.") + return + logger.info(f"Adding and linking a new Google group for {group.name}...") + prefix, display_name = config + pattern = get_config_value(group.app, CONFIG_EMAIL_PATTERN, PLUGIN_ID) + pattern_error = self._validate_email_against_pattern(prefix, pattern) + if pattern_error: + self._mark(session, group, SYNC_ERROR, pattern_error) + return + google_group_id = self._create_google_group(prefix, display_name, group.description or "") + set_status_value(group, STATUS_GOOGLE_GROUP_ID, google_group_id, PLUGIN_ID) + else: + # We own this live Google group (cached or just claimed) -> enforce/adopt its props. + logger.debug(f"Reconciling group properties for {group.name}...") + live = self._get_google_group(google_group_id) + reconcile_error = self._adopt_or_enforce(session, group, google_group_id, live) + if reconcile_error is not None: + self._mark(session, group, SYNC_ERROR, reconcile_error) + return + + # Ensure the push mapping exists; may defer if Okta hasn't imported yet. An ambiguous + # target (duplicate imports sharing the email) won't self-heal, so it errors rather + # than deferring forever. + if not get_status_value(group, STATUS_PUSH_MAPPING_ID, PLUGIN_ID): + resolved_email = email or self._email_from_status(group) + try: + linked = resolved_email is not None and self._create_push_mapping(group, resolved_email) + except AmbiguousOktaTargetError as e: + self._mark(session, group, SYNC_ERROR, str(e)) + return + if not linked: + self._mark(session, group, SYNC_PENDING, "Awaiting Okta import of the Google group") + return + + self._mark(session, group, SYNC_SYNCED) + except Exception as e: + logger.exception(f"Reconcile failed for group {group.name}") + try: + self._mark(session, group, SYNC_ERROR, str(e)) + except Exception: + logger.exception("Failed to persist error status") + raise + + def _adopt_or_enforce( + self, session: Session, group: AppGroup, google_group_id: str, live: dict[str, Any] + ) -> str | None: + """For an existing live Google group: adopt missing Access-side values from it, + or enforce present values onto it. The email (groupKey) is immutable in the Cloud + Identity API and host-blocked from changing, so it is never patched here. Returns + an error string or None.""" + config = self._group_config(group) + live_email = (live.get("groupKey") or {}).get("id", "") or "" + + if config is None: + logger.info(f"Backfilling group properties from Google to Access for {group.name}...") + inferred_prefix = self._prefix_from_email(live_email) + if inferred_prefix is None: + return f"Live Google group email '{live_email}' is not in domain {self._domain}" + set_config_value(group, CONFIG_EMAIL, inferred_prefix, PLUGIN_ID) + set_config_value(group, CONFIG_DISPLAY_NAME, live.get("displayName", "") or "", PLUGIN_ID) + else: + logger.debug(f"Pushing Access group config to Google for {group.name}...") + _, display_name = config + patch_display_name = display_name if (live.get("displayName") or "") != display_name else None + # Description is handled below for both directions; only push it here when Access + # has a (differing) non-empty description. + access_desc = group.description or "" + patch_description = ( + access_desc if access_desc and (live.get("description") or "") != access_desc else None + ) + self._patch_google_group( + google_group_id, display_name=patch_display_name, description=patch_description + ) + + # Description sync (both directions): clobber if Access has one, else backfill. + access_desc = group.description or "" + google_desc = live.get("description", "") or "" + if not access_desc and google_desc: + logger.info(f"Backfilling group description from Google to Access for {group.name}...") + group.description = google_desc + session.add(group) + okta.update_group(group.id, group.name, google_desc) + return None diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index 099963f8..1d0d7426 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -44,9 +44,17 @@ def __init__(self, status: int) -> None: sys.path.insert(0, str(plugin_dir)) from plugin import ( # noqa: E402 + CONFIG_DISPLAY_NAME, + CONFIG_EMAIL, GROUP_DISCUSSION_FORUM_LABEL, PLUGIN_ID, + STATUS_GOOGLE_GROUP_ID, STATUS_PUSH_MAPPING_ID, + STATUS_SYNC_ERROR, + STATUS_SYNC_STATUS, + SYNC_ERROR, + SYNC_PENDING, + SYNC_SYNCED, GoogleGroupManagerPlugin, ) @@ -279,6 +287,40 @@ def test_lookup_returns_none_on_403(plugin_instance: GoogleGroupManagerPlugin, m assert plugin_instance._lookup_google_group_id("missing@test-company.com") is None +def test_email_from_status_returns_email_when_present( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + group = _group(mocker, status={STATUS_GOOGLE_GROUP_ID: "ggid-1"}) + mocker.patch.object(plugin_instance, "_get_google_group", return_value={"groupKey": {"id": "sec@test-company.com"}}) + assert plugin_instance._email_from_status(group) == "sec@test-company.com" + + +@pytest.mark.parametrize("status", [403, 404]) +def test_email_from_status_returns_none_when_group_absent( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, status: int +) -> None: + # The cached group was deleted out of band: recovering the email must treat it as absent + # (like _owned_group_id) and return None, not raise -- otherwise reconcile turns a transient + # race into a hard SYNC_ERROR instead of a clean deferral. + from googleapiclient.errors import HttpError + + group = _group(mocker, status={STATUS_GOOGLE_GROUP_ID: "ggid-gone"}) + mocker.patch.object(plugin_instance, "_get_google_group", side_effect=HttpError(status)) + assert plugin_instance._email_from_status(group) is None + + +def test_email_from_status_reraises_non_absent_error( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # A non-absent error (e.g. 500) is a real failure and must surface, not be swallowed. + from googleapiclient.errors import HttpError + + group = _group(mocker, status={STATUS_GOOGLE_GROUP_ID: "ggid-1"}) + mocker.patch.object(plugin_instance, "_get_google_group", side_effect=HttpError(500)) + with pytest.raises(HttpError): + plugin_instance._email_from_status(group) + + def test_email_config_property_is_immutable(plugin_instance: GoogleGroupManagerPlugin) -> None: props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) assert props["email"].immutable is True @@ -333,6 +375,482 @@ def test_discover_existing_link_returns_none_when_no_mapping(plugin_instance, mo assert plugin_instance._discover_existing_link(group) is None +@pytest.fixture +def session_mock(): + return MagicMock() + + +def test_reconcile_creates_when_no_link_and_config_present(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={"email": "platform-security", "display_name": "Platform Security"}, + description="Sec team") + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "platform-security", "display_name": "Platform Security", + "email_pattern": None, + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-1") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + create.assert_called_once_with("platform-security", "Platform Security", "Sec team") + set_status.assert_any_call(group, STATUS_GOOGLE_GROUP_ID, "ggid-1", PLUGIN_ID) + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID) + + +def test_reconcile_enforces_config_onto_existing_group(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={"email": "new-prefix", "display_name": "New Name"}, + status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, + description="New desc") + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "new-prefix", "display_name": "New Name", "email_pattern": None, + }.get(key, default)) + mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { + "google_group_id": "ggid-1", "push_mapping_id": "map-1", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_get_google_group", return_value={ + "name": "groups/ggid-1", "groupKey": {"id": "old-prefix@test-company.com"}, + "displayName": "Old Name", "description": "Old desc", + }) + patch = mocker.patch.object(plugin_instance, "_patch_google_group") + mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + patch.assert_called_once() + # The email (groupKey) is immutable and never patched; only displayName/description. + assert patch.call_args.kwargs == {"display_name": "New Name", "description": "New desc"} + + +def test_reconcile_adopts_missing_config_from_live_group(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={}, description="") # no config, no description + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ + "google_group_id": "ggid-1", "push_mapping_id": "map-1", "email": "adopted@test-company.com", + }) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-1") + mocker.patch.object(plugin_instance, "_get_google_group", return_value={ + "name": "groups/ggid-1", "groupKey": {"id": "adopted@test-company.com"}, + "displayName": "Adopted Name", "description": "Adopted desc", + }) + mocker.patch("plugin.set_status_value") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) # not owned elsewhere + seed = mocker.patch("plugin.set_config_value") + update_group = mocker.patch("plugin.okta.update_group") + patch = mocker.patch.object(plugin_instance, "_patch_google_group") + + plugin_instance._reconcile(session_mock, group) + + seed.assert_any_call(group, CONFIG_EMAIL, "adopted", PLUGIN_ID) + seed.assert_any_call(group, CONFIG_DISPLAY_NAME, "Adopted Name", PLUGIN_ID) + # Empty Access description backfilled from Google + pushed to Okta; Google not mutated. + assert group.description == "Adopted desc" + update_group.assert_called_once() + patch.assert_not_called() + + +def test_reconcile_flags_error_on_domain_mismatch_adoption(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={}) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: {"enabled": True}.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ + "google_group_id": "ggid-1", "push_mapping_id": "map-1", "email": "x@other-domain.com", + }) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-1") + mocker.patch.object( + plugin_instance, + "_get_google_group", + return_value={ + "name": "groups/ggid-1", + "groupKey": {"id": "x@other-domain.com"}, + "displayName": "X", + "description": "", + }, + ) + mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) # not owned elsewhere + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) + + +def test_reconcile_grandfathers_unchanged_legacy_email(plugin_instance, mocker, session_mock): + # An existing group whose prefix violates a later-added pattern is left alone: + # the email (groupKey) is immutable and never patched, so the pattern is never + # re-enforced on an existing group and reconcile marks it synced, not error. + group = _group(mocker, group_config={"email": "legacy", "display_name": "Legacy"}, + status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, + description="d") + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "legacy", "display_name": "Legacy", "email_pattern": r"^sec-", + }.get(key, default)) + mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { + "google_group_id": "ggid-1", "push_mapping_id": "map-1", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_get_google_group", return_value={ + "name": "groups/ggid-1", "groupKey": {"id": "legacy@test-company.com"}, + "displayName": "Legacy", "description": "d", + }) + mocker.patch.object(plugin_instance, "_patch_google_group") + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + # Marked synced, not error, despite the prefix not matching ^sec-. + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID) + + +def test_reconcile_skips_when_disabled(plugin_instance, mocker, session_mock): + group = _group(mocker) + mocker.patch("plugin.get_config_value", return_value=False) # enabled = False + discover = mocker.patch.object(plugin_instance, "_discover_existing_link") + plugin_instance._reconcile(session_mock, group) + discover.assert_not_called() + + +def test_reconcile_marks_pending_when_push_mapping_defers(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={"email": "sec", "display_name": "Sec"}, description="d") + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Sec", "email_pattern": None, + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-1") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=False) # Okta import not ready -> defer + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_PENDING, PLUGIN_ID) + synced = [c for c in set_status.call_args_list if c.args[1:] == (STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID)] + assert synced == [] # never reached SYNC_SYNCED + + +def test_reconcile_create_path_rejects_pattern_violation(plugin_instance, mocker, session_mock): + group = _group(mocker, group_config={"email": "platform", "display_name": "P"}, description="d") + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "platform", "display_name": "P", "email_pattern": r"^sec-", + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + create = mocker.patch.object(plugin_instance, "_create_google_group") + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + create.assert_not_called() + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) + + +def test_reconcile_creates_when_no_group_exists(plugin_instance, mocker): + group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Security", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-new") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + session = MagicMock() + + plugin_instance._reconcile(session, group) + + create.assert_called_once() + assert group.plugin_data[PLUGIN_ID]["status"][STATUS_GOOGLE_GROUP_ID] == "ggid-new" + + +def test_reconcile_creates_when_lookup_403s_for_absent_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, mock_groups_api: MagicMock, session_mock: Any +) -> None: + # Repro: Cloud Identity's groups:lookup returns 403 ("permission denied ... or it may + # not exist") for a group that does not exist yet, not 404. Reconcile must treat that + # as absent and create the group (deferring the link to pending), never marking error. + from googleapiclient.errors import HttpError + + group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) + mock_groups_api.lookup().execute.side_effect = HttpError(403) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-new") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=False) # Okta hasn't imported yet + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + create.assert_called_once() + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_PENDING, PLUGIN_ID) + assert not [c for c in set_status.call_args_list if c.args == (group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID)] + + +def test_reconcile_adopts_existing_group_by_email_lookup( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Security", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-existing") + mocker.patch.object(plugin_instance, "_get_google_group", return_value={ + "name": "groups/ggid-existing", + "groupKey": {"id": "sec@test-company.com"}, + "displayName": "Security", + "description": "", + }) + create = mocker.patch.object(plugin_instance, "_create_google_group") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) + session = MagicMock() + + plugin_instance._reconcile(session, group) + + create.assert_not_called() + assert group.plugin_data[PLUGIN_ID]["status"][STATUS_GOOGLE_GROUP_ID] == "ggid-existing" + + +def test_reconcile_refuses_google_group_owned_by_another_access_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # A group whose email resolves to a Google group already managed by another Access group -- + # in a *different* app sharing this plugin -- must be flagged error, not adopted/clobbered. + group = _group(mocker, group_config={"email": "shared", "display_name": "Shared"}) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "shared", + "display_name": "Shared", + }.get(key, default), + ) + # No push mapping yet -> resolve adopts the existing group by email and the owner check runs. + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-shared") + # The established owner lives in a different app but already records the id + a push mapping. + owner = Mock(spec=AppGroup) + owner.id = "owner-grp" + owner.name = "App-Other-Owner" + owner.plugin_data = { + PLUGIN_ID: { + "configuration": {}, + "status": {STATUS_GOOGLE_GROUP_ID: "ggid-shared", STATUS_PUSH_MAPPING_ID: "map-owner"}, + } + } + session = MagicMock() + session.scalars.return_value.first.return_value = owner + + enforce = mocker.patch.object(plugin_instance, "_adopt_or_enforce") + get_live = mocker.patch.object(plugin_instance, "_get_google_group") + create_mapping = mocker.patch.object(plugin_instance, "_create_push_mapping") + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session, group) + + # Bailed before fetching/enforcing the live group or creating a second mapping. + get_live.assert_not_called() + enforce.assert_not_called() + create_mapping.assert_not_called() + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) + # The owning group's name is plumbed into the error. + error_msg = next(c.args[2] for c in set_status.call_args_list if c.args[1] == STATUS_SYNC_ERROR) + assert "App-Other-Owner" in error_msg + + +def test_reconcile_does_not_persist_id_when_owned_by_another_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # Refusing adoption must not leave the other group's id in this group's status. If it did, + # group_deleted (which keys off that id) would later delete a Google group we never owned. + # Uses the real status helpers so we can observe what is (not) persisted. + group = _group(mocker, group_config={"email": "shared", "display_name": "Shared"}, status={}) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "shared", + "display_name": "Shared", + }.get(key, default), + ) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-shared") + owner = Mock(spec=AppGroup) + owner.name = "App-Other-Owner" + session = MagicMock() + session.scalars.return_value.first.return_value = owner + + plugin_instance._reconcile(session, group) + + status = group.plugin_data[PLUGIN_ID]["status"] + assert status.get(STATUS_GOOGLE_GROUP_ID) is None # id of the group we don't own was NOT recorded + assert status.get(STATUS_SYNC_STATUS) == SYNC_ERROR + + +def test_reconcile_runs_owner_check_in_config_absent_adoption( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # Adoption path: no Access-side config (so the resolved email is None), but an out-of-band + # Okta link resolves to a Google group already owned by another Access group. The ownership + # check must run here too -- previously it was skipped whenever the config email was absent, + # letting two groups co-manage one Google group. + group = _group(mocker, group_config={}, status={}) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: {"enabled": True}.get(key, default), + ) + mocker.patch.object( + plugin_instance, + "_discover_existing_link", + return_value={"email": "shared@test-company.com", "push_mapping_id": "map-x"}, + ) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-shared") + owner = Mock(spec=AppGroup) + owner.name = "App-Other-Owner" + session = MagicMock() + session.scalars.return_value.first.return_value = owner + enforce = mocker.patch.object(plugin_instance, "_adopt_or_enforce") + + plugin_instance._reconcile(session, group) + + enforce.assert_not_called() # refused before adopting/clobbering the shared group + status = group.plugin_data[PLUGIN_ID]["status"] + assert status.get(STATUS_GOOGLE_GROUP_ID) is None # neither the id... + assert status.get(STATUS_PUSH_MAPPING_ID) is None # ...nor the link's mapping was adopted + assert status.get(STATUS_SYNC_STATUS) == SYNC_ERROR + + +def test_google_group_owner_matches_on_google_group_id_alone( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # Ownership keys on the google_group_id status path ALONE -- not push_mapping_id. A group + # that has claimed the id but not yet created its push mapping (the mapping defers until + # Okta imports the group) still counts as the owner, so a racing group won't double-claim + # during that window. The predicate is enforced in SQL, across this plugin's apps; the + # helper returns its single match, or None when there is none. + from sqlalchemy.dialects import postgresql + + group = _group(mocker) + session = MagicMock() + + session.scalars.return_value.first.return_value = None + assert plugin_instance._google_group_owner(session, group, "ggid-x") is None + + stmt = session.scalars.call_args.args[0] + # The JSON path elements are bound parameters, so render against the Postgres dialect (which + # can compile JSONPathType) and inspect the SQL plus its params for the status path used. + compiled = stmt.compile(dialect=postgresql.dialect()) # type: ignore[no-untyped-call] + rendered = str(compiled) + str(compiled.params) + assert STATUS_GOOGLE_GROUP_ID in rendered # filters on the ownership id + assert STATUS_PUSH_MAPPING_ID not in rendered # but NOT on whether a mapping exists yet + + owner = Mock(spec=AppGroup) + session.scalars.return_value.first.return_value = owner + assert plugin_instance._google_group_owner(session, group, "ggid-x") is owner + + +def test_claim_takes_advisory_lock_before_owner_check_on_postgres( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # The check-then-claim is serialized by a Postgres transaction-level advisory lock keyed on + # the candidate id, so two concurrent reconciles can't both pass the ownership check and adopt + # the same pre-existing Google group. The lock must be taken BEFORE the ownership query. + group = _group(mocker, status={}) + session = MagicMock() + session.get_bind.return_value.dialect.name = "postgresql" + session.scalars.return_value.first.return_value = None # not owned elsewhere + mocker.patch("plugin.set_status_value") + + plugin_instance._claim_group_id(session, group, "ggid-x", "x@test-company.com") + + lock_call = session.execute.call_args_list[0] + assert "pg_advisory_xact_lock" in str(lock_call.args[0]) + assert lock_call.args[1] == {"key": "ggid-x"} # keyed on the candidate id + order = [c[0] for c in session.mock_calls if c[0] in ("execute", "scalars")] + assert order[0] == "execute" # lock precedes the ownership lookup + + +def test_claim_skips_advisory_lock_off_postgres( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + # Advisory locks are Postgres-only; on other backends (e.g. the SQLite test DB) the claim + # must not emit the lock statement, which would error. + group = _group(mocker, status={}) + session = MagicMock() + session.get_bind.return_value.dialect.name = "sqlite" + session.scalars.return_value.first.return_value = None + mocker.patch("plugin.set_status_value") + + plugin_instance._claim_group_id(session, group, "ggid-x", None) + + session.execute.assert_not_called() + + +def test_reconcile_relooks_up_when_cached_id_404s(plugin_instance, mocker): + from googleapiclient.errors import HttpError + + group = _group( + mocker, + group_config={"email": "sec", "display_name": "Security"}, + status={STATUS_GOOGLE_GROUP_ID: "stale-id"}, + ) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Security", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_get_google_group", side_effect=HttpError(404)) + relookup = mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-fresh") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + session = MagicMock() + + plugin_instance._reconcile(session, group) + + relookup.assert_called_once_with("sec@test-company.com") + create.assert_called_once() + assert group.plugin_data[PLUGIN_ID]["status"][STATUS_GOOGLE_GROUP_ID] == "ggid-fresh" + + +def test_enforce_patches_display_name_not_email(plugin_instance, mocker): + group = _group( + mocker, + group_config={"email": "sec", "display_name": "New Name"}, + status={STATUS_GOOGLE_GROUP_ID: "ggid-1"}, + description="d", + ) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "New Name", + }.get(key, default)) + live = { + "name": "groups/ggid-1", + "groupKey": {"id": "sec@test-company.com"}, + "displayName": "Old Name", + "description": "d", + } + mocker.patch.object(plugin_instance, "_get_google_group", return_value=live) + patch = mocker.patch.object(plugin_instance, "_patch_google_group") + mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) + session = MagicMock() + + plugin_instance._reconcile(session, group) + + patch.assert_called_once() + # Only displayName changes; description unchanged (None) and groupKey is immutable. + assert patch.call_args.kwargs == {"display_name": "New Name", "description": None} + + def test_okta_target_group_id_searches_by_email(plugin_instance, mocker): list_groups = mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-7"))]) assert plugin_instance._okta_target_group_id("sec@test-company.com") == "okta-tgt-7" @@ -364,6 +882,38 @@ def test_okta_target_group_id_raises_on_ambiguous_match( plugin_instance._okta_target_group_id("sec@test-company.com") +def test_reconcile_errors_on_ambiguous_okta_target( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + # A duplicate Okta target must mark the group SYNC_ERROR, not park it as SYNC_PENDING forever. + group = _group(mocker, group_config={"email": "sec", "display_name": "Sec"}, description="d") + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Sec", + "email_pattern": None, + }.get(key, default), + ) + mocker.patch("plugin.get_status_value", return_value=None) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) + mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-1") + # Two Okta target groups match the same email -> _create_push_mapping hits ambiguity. + mocker.patch( + "plugin.okta.list_groups", + return_value=[Mock(group=Mock(id="okta-tgt-1")), Mock(group=Mock(id="okta-tgt-2"))], + ) + set_status = mocker.patch("plugin.set_status_value") + + plugin_instance._reconcile(session_mock, group) + + set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) + assert not [c for c in set_status.call_args_list if c.args[1:] == (STATUS_SYNC_STATUS, SYNC_PENDING, PLUGIN_ID)] + assert not [c for c in set_status.call_args_list if c.args[1:] == (STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID)] + + def test_create_push_mapping_resolves_target_by_email( plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture ) -> None: @@ -386,3 +936,26 @@ def test_discover_existing_link_recovers_email(plugin_instance, mocker): link = plugin_instance._discover_existing_link(group) assert link == {"email": "sec@test-company.com", "push_mapping_id": "map-1"} + + +def test_reconcile_ignores_stale_push_mapping_when_group_gone(plugin_instance, mocker): + # Out-of-band Okta link exists, but its Google group was deleted (lookup -> None). + # The stale push_mapping_id must NOT be adopted; reconcile re-creates and re-links. + group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Security", + }.get(key, default)) + mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) + mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ + "email": "sec@test-company.com", "push_mapping_id": "stale-map", + }) + create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-new") + create_mapping = mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) + session = MagicMock() + + plugin_instance._reconcile(session, group) + + # The stale mapping id was not adopted, so a fresh mapping is created. + assert group.plugin_data[PLUGIN_ID]["status"].get(STATUS_PUSH_MAPPING_ID) != "stale-map" + create.assert_called_once() + create_mapping.assert_called_once() From 9c081fd4de1bf37c3f49b063b3b4d52fb3cf66f9 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:32:47 +0000 Subject: [PATCH 16/19] feat(plugin): wire up lifecycle hooks and bulk sync Implement the app group lifecycle hooks on top of reconcile: group_created and group_updated reconcile the group; group_deleted unlinks the Okta push mapping (without deleting the target) and deletes the Google group, resolving the id by email when no status id is stored; sync_all_groups reconciles every group for an app, isolating per-group failures so one bad group doesn't abort the sweep. Register the plugin singleton for entry-point discovery. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/plugin.py | 67 ++++++++++ .../app_group_lifecycle_google/test_plugin.py | 121 +++++++++++++++++- 2 files changed, 186 insertions(+), 2 deletions(-) diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py index 4d0b8ca1..bffc4693 100644 --- a/examples/plugins/app_group_lifecycle_google/plugin.py +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -671,3 +671,70 @@ def _adopt_or_enforce( session.add(group) okta.update_group(group.id, group.name, google_desc) return None + + # ---- Lifecycle hooks ---- + + @hookimpl + def group_created(self, session: Session, group: AppGroup, plugin_id: str | None) -> None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return + self._reconcile(session, group) + + @hookimpl + def group_updated( + self, session: Session, group: AppGroup, old_name: str, old_description: str, plugin_id: str | None + ) -> None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return + self._reconcile(session, group) + + @hookimpl + def group_deleted(self, session: Session, group: AppGroup, plugin_id: str | None) -> None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return + if not self._is_enabled(group): + return + + # Delete only a Google group this Access group provably owns: the recorded + # google_group_id, written only after the reconcile ownership check passes. We + # deliberately do NOT fall back to resolving the id by the (shared) email -- that could + # resolve to, and destroy, a Google group owned by a different Access group that merely + # collided on the prefix (e.g. one refused adoption, which therefore carries no id here). + # The cost of being conservative is that a group we created but crashed before recording + # is orphaned rather than cleaned up; the next reconcile re-resolves and records it. + google_group_id = get_status_value(group, STATUS_GOOGLE_GROUP_ID, PLUGIN_ID) + if not google_group_id: + logger.info(f"Group {group.name} owns no linked Google group; nothing to delete") + return + + mapping_id = get_status_value(group, STATUS_PUSH_MAPPING_ID, PLUGIN_ID) + if mapping_id: + # Best-effort unlink: a failure here must not block deleting the Google group, which is + # the authoritative cleanup when the Access group is deleted. A leftover mapping points + # at a now-deleted group, which is harmless and separately cleanable. + try: + okta.delete_group_push_mapping(appId=self._okta_app_id, mappingId=mapping_id, deleteTargetGroup=False) + logger.info(f"Unlinked Okta push mapping {mapping_id} for Access group {group.name}") + except Exception: + logger.exception( + f"Failed to unlink Okta push mapping {mapping_id} for {group.name}; " + "deleting the Google group anyway" + ) + self._delete_google_group(google_group_id) + logger.info(f"Deleted Google group {google_group_id} for Access group {group.name}") + + @hookimpl + def sync_all_groups(self, session: Session, app: App, plugin_id: str | None) -> None: + if plugin_id is not None and plugin_id != PLUGIN_ID: + return + groups = session.scalars( + select(AppGroup).where(AppGroup.app_id == app.id).where(AppGroup.deleted_at.is_(None)) + ).all() + for group in groups: + try: + self._reconcile(session, group) + except Exception: + logger.exception(f"Sync reconcile failed for group {group.name}") + + +google_group_manager_plugin = GoogleGroupManagerPlugin() diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index 1d0d7426..5e83a885 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -148,8 +148,6 @@ def test_validate_group_config_valid(plugin_instance): ({"email": "Bad-Upper", "display_name": "X"}, "email"), # uppercase fails charset ({"email": "-bad", "display_name": "X"}, "email"), # leading hyphen fails charset ]) - - def test_validate_group_config_errors(plugin_instance, config, bad_key): errors = plugin_instance.validate_plugin_group_config(config, {}, PLUGIN_ID) assert bad_key in errors @@ -851,6 +849,115 @@ def test_enforce_patches_display_name_not_email(plugin_instance, mocker): assert patch.call_args.kwargs == {"display_name": "New Name", "description": None} +def test_group_created_calls_reconcile(plugin_instance, mocker, session_mock): + group = _group(mocker) + reconcile = mocker.patch.object(plugin_instance, "_reconcile") + plugin_instance.group_created(session=session_mock, group=group, plugin_id=PLUGIN_ID) + reconcile.assert_called_once_with(session_mock, group) + + +def test_group_updated_calls_reconcile(plugin_instance, mocker, session_mock): + group = _group(mocker) + reconcile = mocker.patch.object(plugin_instance, "_reconcile") + plugin_instance.group_updated( + session=session_mock, group=group, old_name="old", old_description="d", plugin_id=PLUGIN_ID + ) + reconcile.assert_called_once_with(session_mock, group) + + +def test_hooks_ignore_other_plugin(plugin_instance, mocker, session_mock): + group = _group(mocker) + reconcile = mocker.patch.object(plugin_instance, "_reconcile") + plugin_instance.group_created(session=session_mock, group=group, plugin_id="some_other_plugin") + reconcile.assert_not_called() + + +def test_group_deleted_unlinks_then_deletes(plugin_instance, mocker, session_mock): + group = _group(mocker, status={"push_mapping_id": "map-1", "google_group_id": "ggid-1"}) + mocker.patch("plugin.get_config_value", return_value=True) # enabled + mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { + "push_mapping_id": "map-1", "google_group_id": "ggid-1", + }.get(key, default)) + delete_mapping = mocker.patch("plugin.okta.delete_group_push_mapping") + delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") + mgr = mocker.MagicMock() + mgr.attach_mock(delete_mapping, "delete_mapping") + mgr.attach_mock(delete_group, "delete_group") + + plugin_instance.group_deleted(session=session_mock, group=group, plugin_id=PLUGIN_ID) + + delete_mapping.assert_called_once_with( + appId=plugin_instance._okta_app_id, mappingId="map-1", deleteTargetGroup=False + ) + delete_group.assert_called_once_with("ggid-1") + # unlink must precede the Google group delete + assert [c[0] for c in mgr.mock_calls] == ["delete_mapping", "delete_group"] + + +def test_group_deleted_skips_when_unmanaged(plugin_instance, mocker, session_mock): + group = _group(mocker, status={}) + # enabled=True, but no email/display_name config -> genuinely unmanaged. + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) # no google_group_id + delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") + plugin_instance.group_deleted(session=session_mock, group=group, plugin_id=PLUGIN_ID) + delete_group.assert_not_called() + + +def test_group_deleted_deletes_google_group_even_if_unlink_fails( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + # The Google group is the authoritative resource; a failure to unlink the Okta push mapping + # must not prevent deleting it when the Access group is deleted. + group = _group(mocker, status={"push_mapping_id": "map-1", "google_group_id": "ggid-1"}) + mocker.patch("plugin.get_config_value", return_value=True) # enabled + mocker.patch( + "plugin.get_status_value", + side_effect=lambda obj, key, pid, default=None: { + "push_mapping_id": "map-1", + "google_group_id": "ggid-1", + }.get(key, default), + ) + mocker.patch("plugin.okta.delete_group_push_mapping", side_effect=Exception("okta boom")) + delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") + + plugin_instance.group_deleted(session=session_mock, group=group, plugin_id=PLUGIN_ID) + + delete_group.assert_called_once_with("ggid-1") + + +def test_group_deleted_does_not_fall_back_to_email_lookup( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + # With no recorded google_group_id, group_deleted must NOT resolve a Google group by the + # (shared) email and delete it: that email could resolve to a group owned by a different + # Access group -- e.g. one that collided on prefix and was refused adoption. Deletion is + # gated on the ownership-recording status id, which a refused group never carries. + group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}, status={}) + mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { + "enabled": True, "email": "sec", "display_name": "Security", + }.get(key, default)) + mocker.patch("plugin.get_status_value", return_value=None) # no google_group_id, no push_mapping_id + lookup = mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-del") + delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") + + plugin_instance.group_deleted(session=session_mock, group=group, plugin_id=PLUGIN_ID) + + lookup.assert_not_called() # never resolves by the shared email + delete_group.assert_not_called() # nothing we provably own -> nothing to delete + + +def test_sync_all_reconciles_each_group(plugin_instance, mocker, session_mock): + app = Mock(spec=App) + g1, g2 = Mock(spec=AppGroup), Mock(spec=AppGroup) + session_mock.scalars.return_value.all.return_value = [g1, g2] + reconcile = mocker.patch.object(plugin_instance, "_reconcile") + plugin_instance.sync_all_groups(session=session_mock, app=app, plugin_id=PLUGIN_ID) + assert reconcile.call_count == 2 + + def test_okta_target_group_id_searches_by_email(plugin_instance, mocker): list_groups = mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-7"))]) assert plugin_instance._okta_target_group_id("sec@test-company.com") == "okta-tgt-7" @@ -959,3 +1066,13 @@ def test_reconcile_ignores_stale_push_mapping_when_group_gone(plugin_instance, m assert group.plugin_data[PLUGIN_ID]["status"].get(STATUS_PUSH_MAPPING_ID) != "stale-map" create.assert_called_once() create_mapping.assert_called_once() + + +def test_sync_all_continues_after_group_failure(plugin_instance, mocker, session_mock): + app = Mock(spec=App) + g1, g2 = Mock(spec=AppGroup), Mock(spec=AppGroup) + g1.name = "Group1" + session_mock.scalars.return_value.all.return_value = [g1, g2] + reconcile = mocker.patch.object(plugin_instance, "_reconcile", side_effect=[RuntimeError("boom"), None]) + plugin_instance.sync_all_groups(session=session_mock, app=app, plugin_id=PLUGIN_ID) + assert reconcile.call_count == 2 # g2 still reconciled despite g1 raising From c11f13b4564630e361f8414ba240fa5c8d660b20 Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 07:38:45 +0000 Subject: [PATCH 17/19] style: apply ruff format to app group lifecycle plugin and edits Run ruff format over the new google_group_manager plugin (which was hand-wrapped and never formatted) and the few app_group_lifecycle framework/test lines these changes rewrapped, so `tox -e ruff` (ruff format --check) passes. Formatting only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/plugins/app_group_lifecycle.py | 8 +- .../app_group_lifecycle_google/plugin.py | 24 +- .../app_group_lifecycle_google/test_plugin.py | 363 ++++++++++++------ tests/test_app_group_lifecycle_plugin.py | 15 +- 4 files changed, 271 insertions(+), 139 deletions(-) diff --git a/api/plugins/app_group_lifecycle.py b/api/plugins/app_group_lifecycle.py index c4cd81f8..f67732a5 100644 --- a/api/plugins/app_group_lifecycle.py +++ b/api/plugins/app_group_lifecycle.py @@ -467,9 +467,7 @@ def set_config_value(app_or_group: App | AppGroup, config_property_name: str, va app_or_group.plugin_data[plugin_id] = asdict(data) -def is_plugin_config_changed( - old_plugin_data: dict[str, Any], new_plugin_data: dict[str, Any], plugin_id: str -) -> bool: +def is_plugin_config_changed(old_plugin_data: dict[str, Any], new_plugin_data: dict[str, Any], plugin_id: str) -> bool: """ Determine whether a particular app group lifecycle plugin's configuration differs between two plugin_data payloads. Only the plugin's configuration is compared; status @@ -566,9 +564,7 @@ def get_app_group_lifecycle_plugin_group_config_properties( """ app_configuration = _get_data_for_plugin(app_plugin_data or {}, plugin_id).configuration hook = get_app_group_lifecycle_hook() - return _get_hook_call_response( - hook.get_plugin_group_config_properties, plugin_id, app_config=app_configuration - ) + return _get_hook_call_response(hook.get_plugin_group_config_properties, plugin_id, app_config=app_configuration) def validate_app_group_lifecycle_plugin_app_config(plugin_data: dict[str, Any], plugin_id: str) -> dict[str, str]: diff --git a/examples/plugins/app_group_lifecycle_google/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py index bffc4693..3553ee2a 100644 --- a/examples/plugins/app_group_lifecycle_google/plugin.py +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -241,18 +241,12 @@ def get_plugin_group_status_properties( STATUS_PUSH_MAPPING_ID: AppGroupLifecyclePluginStatusProperty( display_name="Okta Push Mapping ID", type="text" ), - STATUS_GOOGLE_GROUP_ID: AppGroupLifecyclePluginStatusProperty( - display_name="Google Group ID", type="text" - ), + STATUS_GOOGLE_GROUP_ID: AppGroupLifecyclePluginStatusProperty(display_name="Google Group ID", type="text"), STATUS_SYNC_STATUS: AppGroupLifecyclePluginStatusProperty( display_name="Sync Status", help_text="synced, pending, or error", type="text" ), - STATUS_SYNC_ERROR: AppGroupLifecyclePluginStatusProperty( - display_name="Sync Error", type="text" - ), - STATUS_LAST_SYNCED_AT: AppGroupLifecyclePluginStatusProperty( - display_name="Last Synced", type="date" - ), + STATUS_SYNC_ERROR: AppGroupLifecyclePluginStatusProperty(display_name="Sync Error", type="text"), + STATUS_LAST_SYNCED_AT: AppGroupLifecyclePluginStatusProperty(display_name="Last Synced", type="date"), } # ---- Validation ---- @@ -353,9 +347,7 @@ def _patch_google_group( if not body: return update_mask = ",".join(sorted(body)) - self._groups_api.patch( - name=self._resource_name(google_group_id), body=body, updateMask=update_mask - ).execute() + self._groups_api.patch(name=self._resource_name(google_group_id), body=body, updateMask=update_mask).execute() def _delete_google_group(self, google_group_id: str) -> None: self._groups_api.delete(name=self._resource_name(google_group_id)).execute() @@ -655,12 +647,8 @@ def _adopt_or_enforce( # Description is handled below for both directions; only push it here when Access # has a (differing) non-empty description. access_desc = group.description or "" - patch_description = ( - access_desc if access_desc and (live.get("description") or "") != access_desc else None - ) - self._patch_google_group( - google_group_id, display_name=patch_display_name, description=patch_description - ) + patch_description = access_desc if access_desc and (live.get("description") or "") != access_desc else None + self._patch_google_group(google_group_id, display_name=patch_display_name, description=patch_description) # Description sync (both directions): clobber if Access has one, else backfill. access_desc = group.description or "" diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index 5e83a885..da2e41af 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -73,11 +73,14 @@ def mock_groups_api(mocker: MockerFixture): @pytest.fixture def plugin_instance(mocker: MockerFixture, mock_groups_api): - mocker.patch.dict(os.environ, { - "GOOGLE_WORKSPACE_OKTA_APP_ID": "test-okta-app-123", - "GOOGLE_WORKSPACE_DOMAIN": "test-company.com", - "GOOGLE_WORKSPACE_CUSTOMER_ID": "C0test", - }) + mocker.patch.dict( + os.environ, + { + "GOOGLE_WORKSPACE_OKTA_APP_ID": "test-okta-app-123", + "GOOGLE_WORKSPACE_DOMAIN": "test-company.com", + "GOOGLE_WORKSPACE_CUSTOMER_ID": "C0test", + }, + ) return GoogleGroupManagerPlugin() @@ -108,16 +111,20 @@ def test_group_config_properties_surface_validation_patterns(plugin_instance): assert [p["regex"] for p in patterns] == [GOOGLE_LOCAL_PART_RE.pattern] # With an app email_pattern, it is appended as a second rule. - patterns = plugin_instance.get_plugin_group_config_properties( - PLUGIN_ID, {"email_pattern": r"^sec-"} - )["email"].validation["patterns"] + patterns = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {"email_pattern": r"^sec-"})[ + "email" + ].validation["patterns"] assert [p["regex"] for p in patterns] == [GOOGLE_LOCAL_PART_RE.pattern, r"^sec-"] def test_group_status_properties_shape(plugin_instance): props = plugin_instance.get_plugin_group_status_properties(PLUGIN_ID) assert set(props) == { - "push_mapping_id", "google_group_id", "sync_status", "sync_error", "last_synced_at", + "push_mapping_id", + "google_group_id", + "sync_status", + "sync_error", + "last_synced_at", } @@ -142,12 +149,15 @@ def test_validate_group_config_valid(plugin_instance): assert errors == {} -@pytest.mark.parametrize("config,bad_key", [ - ({"display_name": "X"}, "email"), # missing email - ({"email": "ok"}, "display_name"), # missing display_name - ({"email": "Bad-Upper", "display_name": "X"}, "email"), # uppercase fails charset - ({"email": "-bad", "display_name": "X"}, "email"), # leading hyphen fails charset -]) +@pytest.mark.parametrize( + "config,bad_key", + [ + ({"display_name": "X"}, "email"), # missing email + ({"email": "ok"}, "display_name"), # missing display_name + ({"email": "Bad-Upper", "display_name": "X"}, "email"), # uppercase fails charset + ({"email": "-bad", "display_name": "X"}, "email"), # leading hyphen fails charset + ], +) def test_validate_group_config_errors(plugin_instance, config, bad_key): errors = plugin_instance.validate_plugin_group_config(config, {}, PLUGIN_ID) assert bad_key in errors @@ -210,14 +220,21 @@ def test_validate_email_against_pattern(plugin_instance): def test_group_config_returns_pair_or_none(plugin_instance, mocker): # both present -> tuple; missing one -> None - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) assert plugin_instance._group_config(_group(mocker)) == ("sec", "Security") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "email": "sec", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "email": "sec", + }.get(key, default), + ) assert plugin_instance._group_config(_group(mocker)) is None @@ -379,12 +396,18 @@ def session_mock(): def test_reconcile_creates_when_no_link_and_config_present(plugin_instance, mocker, session_mock): - group = _group(mocker, group_config={"email": "platform-security", "display_name": "Platform Security"}, - description="Sec team") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "platform-security", "display_name": "Platform Security", - "email_pattern": None, - }.get(key, default)) + group = _group( + mocker, group_config={"email": "platform-security", "display_name": "Platform Security"}, description="Sec team" + ) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "platform-security", + "display_name": "Platform Security", + "email_pattern": None, + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) @@ -400,19 +423,38 @@ def test_reconcile_creates_when_no_link_and_config_present(plugin_instance, mock def test_reconcile_enforces_config_onto_existing_group(plugin_instance, mocker, session_mock): - group = _group(mocker, group_config={"email": "new-prefix", "display_name": "New Name"}, - status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, - description="New desc") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "new-prefix", "display_name": "New Name", "email_pattern": None, - }.get(key, default)) - mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { - "google_group_id": "ggid-1", "push_mapping_id": "map-1", - }.get(key, default)) - mocker.patch.object(plugin_instance, "_get_google_group", return_value={ - "name": "groups/ggid-1", "groupKey": {"id": "old-prefix@test-company.com"}, - "displayName": "Old Name", "description": "Old desc", - }) + group = _group( + mocker, + group_config={"email": "new-prefix", "display_name": "New Name"}, + status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, + description="New desc", + ) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "new-prefix", + "display_name": "New Name", + "email_pattern": None, + }.get(key, default), + ) + mocker.patch( + "plugin.get_status_value", + side_effect=lambda obj, key, pid, default=None: { + "google_group_id": "ggid-1", + "push_mapping_id": "map-1", + }.get(key, default), + ) + mocker.patch.object( + plugin_instance, + "_get_google_group", + return_value={ + "name": "groups/ggid-1", + "groupKey": {"id": "old-prefix@test-company.com"}, + "displayName": "Old Name", + "description": "Old desc", + }, + ) patch = mocker.patch.object(plugin_instance, "_patch_google_group") mocker.patch("plugin.set_status_value") @@ -425,18 +467,33 @@ def test_reconcile_enforces_config_onto_existing_group(plugin_instance, mocker, def test_reconcile_adopts_missing_config_from_live_group(plugin_instance, mocker, session_mock): group = _group(mocker, group_config={}, description="") # no config, no description - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) - mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ - "google_group_id": "ggid-1", "push_mapping_id": "map-1", "email": "adopted@test-company.com", - }) + mocker.patch.object( + plugin_instance, + "_discover_existing_link", + return_value={ + "google_group_id": "ggid-1", + "push_mapping_id": "map-1", + "email": "adopted@test-company.com", + }, + ) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-1") - mocker.patch.object(plugin_instance, "_get_google_group", return_value={ - "name": "groups/ggid-1", "groupKey": {"id": "adopted@test-company.com"}, - "displayName": "Adopted Name", "description": "Adopted desc", - }) + mocker.patch.object( + plugin_instance, + "_get_google_group", + return_value={ + "name": "groups/ggid-1", + "groupKey": {"id": "adopted@test-company.com"}, + "displayName": "Adopted Name", + "description": "Adopted desc", + }, + ) mocker.patch("plugin.set_status_value") mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) # not owned elsewhere @@ -456,11 +513,19 @@ def test_reconcile_adopts_missing_config_from_live_group(plugin_instance, mocker def test_reconcile_flags_error_on_domain_mismatch_adoption(plugin_instance, mocker, session_mock): group = _group(mocker, group_config={}) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: {"enabled": True}.get(key, default)) + mocker.patch( + "plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: {"enabled": True}.get(key, default) + ) mocker.patch("plugin.get_status_value", return_value=None) - mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ - "google_group_id": "ggid-1", "push_mapping_id": "map-1", "email": "x@other-domain.com", - }) + mocker.patch.object( + plugin_instance, + "_discover_existing_link", + return_value={ + "google_group_id": "ggid-1", + "push_mapping_id": "map-1", + "email": "x@other-domain.com", + }, + ) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-1") mocker.patch.object( plugin_instance, @@ -484,19 +549,38 @@ def test_reconcile_grandfathers_unchanged_legacy_email(plugin_instance, mocker, # An existing group whose prefix violates a later-added pattern is left alone: # the email (groupKey) is immutable and never patched, so the pattern is never # re-enforced on an existing group and reconcile marks it synced, not error. - group = _group(mocker, group_config={"email": "legacy", "display_name": "Legacy"}, - status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, - description="d") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "legacy", "display_name": "Legacy", "email_pattern": r"^sec-", - }.get(key, default)) - mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { - "google_group_id": "ggid-1", "push_mapping_id": "map-1", - }.get(key, default)) - mocker.patch.object(plugin_instance, "_get_google_group", return_value={ - "name": "groups/ggid-1", "groupKey": {"id": "legacy@test-company.com"}, - "displayName": "Legacy", "description": "d", - }) + group = _group( + mocker, + group_config={"email": "legacy", "display_name": "Legacy"}, + status={"google_group_id": "ggid-1", "push_mapping_id": "map-1"}, + description="d", + ) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "legacy", + "display_name": "Legacy", + "email_pattern": r"^sec-", + }.get(key, default), + ) + mocker.patch( + "plugin.get_status_value", + side_effect=lambda obj, key, pid, default=None: { + "google_group_id": "ggid-1", + "push_mapping_id": "map-1", + }.get(key, default), + ) + mocker.patch.object( + plugin_instance, + "_get_google_group", + return_value={ + "name": "groups/ggid-1", + "groupKey": {"id": "legacy@test-company.com"}, + "displayName": "Legacy", + "description": "d", + }, + ) mocker.patch.object(plugin_instance, "_patch_google_group") set_status = mocker.patch("plugin.set_status_value") @@ -516,9 +600,15 @@ def test_reconcile_skips_when_disabled(plugin_instance, mocker, session_mock): def test_reconcile_marks_pending_when_push_mapping_defers(plugin_instance, mocker, session_mock): group = _group(mocker, group_config={"email": "sec", "display_name": "Sec"}, description="d") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Sec", "email_pattern": None, - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Sec", + "email_pattern": None, + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) @@ -535,9 +625,15 @@ def test_reconcile_marks_pending_when_push_mapping_defers(plugin_instance, mocke def test_reconcile_create_path_rejects_pattern_violation(plugin_instance, mocker, session_mock): group = _group(mocker, group_config={"email": "platform", "display_name": "P"}, description="d") - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "platform", "display_name": "P", "email_pattern": r"^sec-", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "platform", + "display_name": "P", + "email_pattern": r"^sec-", + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) @@ -552,9 +648,14 @@ def test_reconcile_create_path_rejects_pattern_violation(plugin_instance, mocker def test_reconcile_creates_when_no_group_exists(plugin_instance, mocker): group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-new") @@ -601,16 +702,25 @@ def test_reconcile_adopts_existing_group_by_email_lookup( plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture ) -> None: group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-existing") - mocker.patch.object(plugin_instance, "_get_google_group", return_value={ - "name": "groups/ggid-existing", - "groupKey": {"id": "sec@test-company.com"}, - "displayName": "Security", - "description": "", - }) + mocker.patch.object( + plugin_instance, + "_get_google_group", + return_value={ + "name": "groups/ggid-existing", + "groupKey": {"id": "sec@test-company.com"}, + "displayName": "Security", + "description": "", + }, + ) create = mocker.patch.object(plugin_instance, "_create_google_group") mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) mocker.patch.object(plugin_instance, "_google_group_owner", return_value=None) @@ -803,9 +913,14 @@ def test_reconcile_relooks_up_when_cached_id_404s(plugin_instance, mocker): group_config={"email": "sec", "display_name": "Security"}, status={STATUS_GOOGLE_GROUP_ID: "stale-id"}, ) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) mocker.patch.object(plugin_instance, "_get_google_group", side_effect=HttpError(404)) relookup = mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) mocker.patch.object(plugin_instance, "_discover_existing_link", return_value=None) @@ -827,9 +942,14 @@ def test_enforce_patches_display_name_not_email(plugin_instance, mocker): status={STATUS_GOOGLE_GROUP_ID: "ggid-1"}, description="d", ) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "New Name", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "New Name", + }.get(key, default), + ) live = { "name": "groups/ggid-1", "groupKey": {"id": "sec@test-company.com"}, @@ -875,9 +995,13 @@ def test_hooks_ignore_other_plugin(plugin_instance, mocker, session_mock): def test_group_deleted_unlinks_then_deletes(plugin_instance, mocker, session_mock): group = _group(mocker, status={"push_mapping_id": "map-1", "google_group_id": "ggid-1"}) mocker.patch("plugin.get_config_value", return_value=True) # enabled - mocker.patch("plugin.get_status_value", side_effect=lambda obj, key, pid, default=None: { - "push_mapping_id": "map-1", "google_group_id": "ggid-1", - }.get(key, default)) + mocker.patch( + "plugin.get_status_value", + side_effect=lambda obj, key, pid, default=None: { + "push_mapping_id": "map-1", + "google_group_id": "ggid-1", + }.get(key, default), + ) delete_mapping = mocker.patch("plugin.okta.delete_group_push_mapping") delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") mgr = mocker.MagicMock() @@ -897,9 +1021,12 @@ def test_group_deleted_unlinks_then_deletes(plugin_instance, mocker, session_moc def test_group_deleted_skips_when_unmanaged(plugin_instance, mocker, session_mock): group = _group(mocker, status={}) # enabled=True, but no email/display_name config -> genuinely unmanaged. - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) # no google_group_id delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") plugin_instance.group_deleted(session=session_mock, group=group, plugin_id=PLUGIN_ID) @@ -936,9 +1063,14 @@ def test_group_deleted_does_not_fall_back_to_email_lookup( # Access group -- e.g. one that collided on prefix and was refused adoption. Deletion is # gated on the ownership-recording status id, which a refused group never carries. group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}, status={}) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) mocker.patch("plugin.get_status_value", return_value=None) # no google_group_id, no push_mapping_id lookup = mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value="ggid-del") delete_group = mocker.patch.object(plugin_instance, "_delete_google_group") @@ -1036,9 +1168,12 @@ def test_create_push_mapping_resolves_target_by_email( def test_discover_existing_link_recovers_email(plugin_instance, mocker): group = _group(mocker) profile = Mock(googleGroupEmail="sec@test-company.com") - mocker.patch("plugin.okta.list_group_push_mappings", return_value=[ - {"id": "map-1", "sourceGroupId": "grp-1", "targetGroupId": "okta-tgt-1"}, - ]) + mocker.patch( + "plugin.okta.list_group_push_mappings", + return_value=[ + {"id": "map-1", "sourceGroupId": "grp-1", "targetGroupId": "okta-tgt-1"}, + ], + ) mocker.patch("plugin.okta.get_group", return_value=Mock(group=Mock(profile=profile))) link = plugin_instance._discover_existing_link(group) @@ -1049,13 +1184,23 @@ def test_reconcile_ignores_stale_push_mapping_when_group_gone(plugin_instance, m # Out-of-band Okta link exists, but its Google group was deleted (lookup -> None). # The stale push_mapping_id must NOT be adopted; reconcile re-creates and re-links. group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) - mocker.patch("plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: { - "enabled": True, "email": "sec", "display_name": "Security", - }.get(key, default)) + mocker.patch( + "plugin.get_config_value", + side_effect=lambda obj, key, pid, default=None: { + "enabled": True, + "email": "sec", + "display_name": "Security", + }.get(key, default), + ) mocker.patch.object(plugin_instance, "_lookup_google_group_id", return_value=None) - mocker.patch.object(plugin_instance, "_discover_existing_link", return_value={ - "email": "sec@test-company.com", "push_mapping_id": "stale-map", - }) + mocker.patch.object( + plugin_instance, + "_discover_existing_link", + return_value={ + "email": "sec@test-company.com", + "push_mapping_id": "stale-map", + }, + ) create = mocker.patch.object(plugin_instance, "_create_google_group", return_value="ggid-new") create_mapping = mocker.patch.object(plugin_instance, "_create_push_mapping", return_value=True) session = MagicMock() diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index 93b00949..9fa8b51d 100644 --- a/tests/test_app_group_lifecycle_plugin.py +++ b/tests/test_app_group_lifecycle_plugin.py @@ -2271,6 +2271,7 @@ class TestModifyGroupPluginData: def _make_app_group(self, db, mocker): from api.models import AppGroup + mocker.patch.object(okta, "update_group") mocker.patch.object(okta, "create_group") app = AppFactory.create() @@ -2288,6 +2289,7 @@ def _make_app_group(self, db, mocker): def test_fires_group_updated_on_config_change(self, db, app, test_plugin, mocker): from api.operations.modify_group_plugin_data import ModifyGroupPluginData + _, group = self._make_app_group(db, mocker) new_plugin_data = {DummyPlugin.ID: {"configuration": {"group_id": "g-new"}, "status": {}}} @@ -2300,11 +2302,10 @@ def test_fires_group_updated_on_config_change(self, db, app, test_plugin, mocker def test_does_not_fire_on_status_only_change(self, db, app, test_plugin, mocker): from api.operations.modify_group_plugin_data import ModifyGroupPluginData + _, group = self._make_app_group(db, mocker) - new_plugin_data = { - DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {"member_count": 5}} - } + new_plugin_data = {DummyPlugin.ID: {"configuration": {"group_id": "g-old"}, "status": {"member_count": 5}}} ModifyGroupPluginData(group=group, plugin_data=new_plugin_data).execute() assert test_plugin.group_updated_calls == [] @@ -2313,6 +2314,7 @@ def test_does_not_fire_on_status_only_change(self, db, app, test_plugin, mocker) def test_does_not_fire_without_lifecycle_plugin(self, db, app, test_plugin, mocker): from api.models import AppGroup from api.operations.modify_group_plugin_data import ModifyGroupPluginData + mocker.patch.object(okta, "update_group") mocker.patch.object(okta, "create_group") a = AppFactory.create() @@ -2386,10 +2388,9 @@ def test_preserves_status_omitted_from_config_patch(self, db, app, test_plugin, assert group.plugin_data[DummyPlugin.ID]["configuration"]["group_id"] == "g-new" assert group.plugin_data[DummyPlugin.ID]["status"] == {"member_count": 7} - def test_put_group_config_change_fires_group_updated( - self, client, db, app, test_plugin, mocker, url_for - ): + def test_put_group_config_change_fires_group_updated(self, client, db, app, test_plugin, mocker, url_for): from api.models import AppGroup + mocker.patch.object(okta, "update_group") mocker.patch.object(okta, "create_group") a = AppFactory.create() @@ -2421,6 +2422,7 @@ def test_put_group_config_change_fires_group_updated( class TestPostGroupPluginValidation: def test_post_group_rejects_invalid_group_config(self, client, db, app, test_plugin, mocker, url_for): from okta.models.group import Group as OktaGroup + mocker.patch.object(okta, "create_group", return_value=OktaGroup({"id": "test-okta-id-123"})) a = AppFactory.create() a.app_group_lifecycle_plugin = DummyPlugin.ID @@ -2444,6 +2446,7 @@ def test_post_group_rejects_invalid_group_config(self, client, db, app, test_plu def test_post_group_accepts_valid_group_config(self, client, db, app, test_plugin, mocker, url_for): from okta.models.group import Group as OktaGroup + mocker.patch.object(okta, "create_group", return_value=OktaGroup({"id": "test-okta-id-456"})) a = AppFactory.create() a.app_group_lifecycle_plugin = DummyPlugin.ID From 533f3c11b898247a6c7cf2cf7e7911dd411da58e Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Fri, 26 Jun 2026 19:34:47 +0000 Subject: [PATCH 18/19] style: add type annotations to google_group_manager test_plugin Annotate the fixtures, the _group helper, and every test function in the google_group_manager plugin's test module so `mypy` passes. The helper's return annotation also clears the cascading no-untyped-call errors at its ~40 call sites. Annotations only; no behavior change (58 tests still pass). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app_group_lifecycle_google/test_plugin.py | 176 ++++++++++++------ 1 file changed, 122 insertions(+), 54 deletions(-) diff --git a/examples/plugins/app_group_lifecycle_google/test_plugin.py b/examples/plugins/app_group_lifecycle_google/test_plugin.py index da2e41af..43e4fddf 100644 --- a/examples/plugins/app_group_lifecycle_google/test_plugin.py +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -3,6 +3,7 @@ import os import sys from pathlib import Path +from typing import Any from unittest.mock import MagicMock, Mock import pytest @@ -62,7 +63,7 @@ def __init__(self, status: int) -> None: @pytest.fixture -def mock_groups_api(mocker: MockerFixture): +def mock_groups_api(mocker: MockerFixture) -> MagicMock: mocker.patch("plugin.default", return_value=(Mock(), None)) discovery_client = MagicMock() mocker.patch("plugin.build", return_value=discovery_client) @@ -72,7 +73,7 @@ def mock_groups_api(mocker: MockerFixture): @pytest.fixture -def plugin_instance(mocker: MockerFixture, mock_groups_api): +def plugin_instance(mocker: MockerFixture, mock_groups_api: MagicMock) -> GoogleGroupManagerPlugin: mocker.patch.dict( os.environ, { @@ -84,26 +85,26 @@ def plugin_instance(mocker: MockerFixture, mock_groups_api): return GoogleGroupManagerPlugin() -def test_metadata(plugin_instance): +def test_metadata(plugin_instance: GoogleGroupManagerPlugin) -> None: meta = plugin_instance.get_plugin_metadata() assert meta.id == PLUGIN_ID assert meta.display_name -def test_app_config_properties_shape(plugin_instance): +def test_app_config_properties_shape(plugin_instance: GoogleGroupManagerPlugin) -> None: props = plugin_instance.get_plugin_app_config_properties(PLUGIN_ID) assert set(props) == {"enabled", "email_pattern"} assert props["enabled"].required is True -def test_group_config_properties_shape(plugin_instance): +def test_group_config_properties_shape(plugin_instance: GoogleGroupManagerPlugin) -> None: props = plugin_instance.get_plugin_group_config_properties(PLUGIN_ID, {}) assert set(props) == {"email", "display_name"} assert props["email"].required is True assert props["display_name"].required is True -def test_group_config_properties_surface_validation_patterns(plugin_instance): +def test_group_config_properties_surface_validation_patterns(plugin_instance: GoogleGroupManagerPlugin) -> None: from plugin import GOOGLE_LOCAL_PART_RE # With no app pattern, the email property carries just the Google-safe charset rule. @@ -117,7 +118,7 @@ def test_group_config_properties_surface_validation_patterns(plugin_instance): assert [p["regex"] for p in patterns] == [GOOGLE_LOCAL_PART_RE.pattern, r"^sec-"] -def test_group_status_properties_shape(plugin_instance): +def test_group_status_properties_shape(plugin_instance: GoogleGroupManagerPlugin) -> None: props = plugin_instance.get_plugin_group_status_properties(PLUGIN_ID) assert set(props) == { "push_mapping_id", @@ -129,20 +130,22 @@ def test_group_status_properties_shape(plugin_instance): @pytest.mark.parametrize("pattern,ok", [(None, True), (r"^[a-z-]+$", True), (r"([", False)]) -def test_validate_app_config_email_pattern(plugin_instance, pattern, ok): - config = {"enabled": True} +def test_validate_app_config_email_pattern( + plugin_instance: GoogleGroupManagerPlugin, pattern: str | None, ok: bool +) -> None: + config: dict[str, Any] = {"enabled": True} if pattern is not None: config["email_pattern"] = pattern errors = plugin_instance.validate_plugin_app_config(config, PLUGIN_ID) assert (errors == {}) is ok -def test_validate_app_config_requires_enabled(plugin_instance): +def test_validate_app_config_requires_enabled(plugin_instance: GoogleGroupManagerPlugin) -> None: errors = plugin_instance.validate_plugin_app_config({}, PLUGIN_ID) assert "enabled" in errors -def test_validate_group_config_valid(plugin_instance): +def test_validate_group_config_valid(plugin_instance: GoogleGroupManagerPlugin) -> None: errors = plugin_instance.validate_plugin_group_config( {"email": "platform-security", "display_name": "Platform Security"}, {}, PLUGIN_ID ) @@ -158,16 +161,18 @@ def test_validate_group_config_valid(plugin_instance): ({"email": "-bad", "display_name": "X"}, "email"), # leading hyphen fails charset ], ) -def test_validate_group_config_errors(plugin_instance, config, bad_key): +def test_validate_group_config_errors( + plugin_instance: GoogleGroupManagerPlugin, config: dict[str, Any], bad_key: str +) -> None: errors = plugin_instance.validate_plugin_group_config(config, {}, PLUGIN_ID) assert bad_key in errors -def test_validate_group_config_ignores_other_plugin(plugin_instance): +def test_validate_group_config_ignores_other_plugin(plugin_instance: GoogleGroupManagerPlugin) -> None: assert plugin_instance.validate_plugin_group_config({}, {}, "some_other_plugin") is None -def test_validate_group_config_enforces_app_email_pattern(plugin_instance): +def test_validate_group_config_enforces_app_email_pattern(plugin_instance: GoogleGroupManagerPlugin) -> None: # A prefix that is charset-valid but violates the app's email_pattern is rejected. app_config = {"email_pattern": r"^sec-"} errors = plugin_instance.validate_plugin_group_config( @@ -182,7 +187,14 @@ def test_validate_group_config_enforces_app_email_pattern(plugin_instance): assert errors == {} -def _group(mocker, *, app_config=None, group_config=None, status=None, description=""): +def _group( + mocker: MockerFixture, + *, + app_config: dict[str, Any] | None = None, + group_config: dict[str, Any] | None = None, + status: dict[str, Any] | None = None, + description: str = "", +) -> Mock: app = Mock(spec=App) app.plugin_data = {PLUGIN_ID: {"configuration": app_config or {"enabled": True}, "status": {}}} group = Mock(spec=AppGroup) @@ -194,31 +206,31 @@ def _group(mocker, *, app_config=None, group_config=None, status=None, descripti return group -def test_full_email_appends_domain(plugin_instance): +def test_full_email_appends_domain(plugin_instance: GoogleGroupManagerPlugin) -> None: assert plugin_instance._full_email("platform-security") == "platform-security@test-company.com" -def test_prefix_from_email_strips_domain(plugin_instance): +def test_prefix_from_email_strips_domain(plugin_instance: GoogleGroupManagerPlugin) -> None: assert plugin_instance._prefix_from_email("platform-security@test-company.com") == "platform-security" -def test_prefix_from_email_returns_none_on_domain_mismatch(plugin_instance): +def test_prefix_from_email_returns_none_on_domain_mismatch(plugin_instance: GoogleGroupManagerPlugin) -> None: assert plugin_instance._prefix_from_email("x@other.com") is None -def test_is_enabled_reads_app_config(plugin_instance, mocker): +def test_is_enabled_reads_app_config(plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: group = _group(mocker) mocker.patch("plugin.get_config_value", return_value=True) assert plugin_instance._is_enabled(group) is True -def test_validate_email_against_pattern(plugin_instance): +def test_validate_email_against_pattern(plugin_instance: GoogleGroupManagerPlugin) -> None: assert plugin_instance._validate_email_against_pattern("platform", r"^sec-") is not None assert plugin_instance._validate_email_against_pattern("sec-platform", r"^sec-") is None assert plugin_instance._validate_email_against_pattern("anything", None) is None -def test_group_config_returns_pair_or_none(plugin_instance, mocker): +def test_group_config_returns_pair_or_none(plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: # both present -> tuple; missing one -> None mocker.patch( "plugin.get_config_value", @@ -238,7 +250,9 @@ def test_group_config_returns_pair_or_none(plugin_instance, mocker): assert plugin_instance._group_config(_group(mocker)) is None -def test_create_google_group_calls_create(plugin_instance, mock_groups_api): +def test_create_google_group_calls_create( + plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock +) -> None: mock_groups_api.create().execute.return_value = { "done": True, "response": {"name": "groups/ggid-1", "groupKey": {"id": "platform-security@test-company.com"}}, @@ -256,13 +270,17 @@ def test_create_google_group_calls_create(plugin_instance, mock_groups_api): } -def test_get_google_group_calls_get_by_resource_name(plugin_instance, mock_groups_api): +def test_get_google_group_calls_get_by_resource_name( + plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock +) -> None: mock_groups_api.get().execute.return_value = {"name": "groups/ggid-1"} assert plugin_instance._get_google_group("ggid-1")["name"] == "groups/ggid-1" assert mock_groups_api.get.call_args.kwargs == {"name": "groups/ggid-1"} -def test_patch_google_group_sets_update_mask(plugin_instance, mock_groups_api): +def test_patch_google_group_sets_update_mask( + plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock +) -> None: plugin_instance._patch_google_group("ggid-1", display_name="New", description="d") kwargs = mock_groups_api.patch.call_args.kwargs assert kwargs["name"] == "groups/ggid-1" @@ -270,23 +288,27 @@ def test_patch_google_group_sets_update_mask(plugin_instance, mock_groups_api): assert kwargs["updateMask"] == "description,displayName" -def test_patch_google_group_noop_when_no_fields(plugin_instance, mock_groups_api): +def test_patch_google_group_noop_when_no_fields( + plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock +) -> None: plugin_instance._patch_google_group("ggid-1") mock_groups_api.patch.assert_not_called() -def test_delete_google_group_calls_delete_by_resource_name(plugin_instance, mock_groups_api): +def test_delete_google_group_calls_delete_by_resource_name( + plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock +) -> None: plugin_instance._delete_google_group("ggid-1") assert mock_groups_api.delete.call_args.kwargs == {"name": "groups/ggid-1"} -def test_lookup_returns_bare_id(plugin_instance, mock_groups_api): +def test_lookup_returns_bare_id(plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock) -> None: mock_groups_api.lookup().execute.return_value = {"name": "groups/ggid-9"} assert plugin_instance._lookup_google_group_id("x@test-company.com") == "ggid-9" assert mock_groups_api.lookup.call_args.kwargs == {"groupKey_id": "x@test-company.com"} -def test_lookup_returns_none_on_404(plugin_instance, mock_groups_api): +def test_lookup_returns_none_on_404(plugin_instance: GoogleGroupManagerPlugin, mock_groups_api: MagicMock) -> None: from googleapiclient.errors import HttpError mock_groups_api.lookup().execute.side_effect = HttpError(404) @@ -342,7 +364,7 @@ def test_email_config_property_is_immutable(plugin_instance: GoogleGroupManagerP assert props["display_name"].immutable is False -def test_create_push_mapping_sets_status(plugin_instance, mocker): +def test_create_push_mapping_sets_status(plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: group = _group(mocker) mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-1"))]) mocker.patch("plugin.okta.create_group_push_mapping", return_value={"id": "map-1"}) @@ -354,7 +376,9 @@ def test_create_push_mapping_sets_status(plugin_instance, mocker): set_status.assert_any_call(group, STATUS_PUSH_MAPPING_ID, "map-1", PLUGIN_ID) -def test_create_push_mapping_defers_when_target_not_imported(plugin_instance, mocker): +def test_create_push_mapping_defers_when_target_not_imported( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: group = _group(mocker) mocker.patch("plugin.okta.list_groups", return_value=[]) # Okta hasn't imported it yet create = mocker.patch("plugin.okta.create_group_push_mapping") @@ -365,7 +389,7 @@ def test_create_push_mapping_defers_when_target_not_imported(plugin_instance, mo create.assert_not_called() -def test_discover_existing_link_finds_mapping(plugin_instance, mocker): +def test_discover_existing_link_finds_mapping(plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: group = _group(mocker) mocker.patch( "plugin.okta.list_group_push_mappings", @@ -384,18 +408,22 @@ def test_discover_existing_link_finds_mapping(plugin_instance, mocker): } -def test_discover_existing_link_returns_none_when_no_mapping(plugin_instance, mocker): +def test_discover_existing_link_returns_none_when_no_mapping( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: group = _group(mocker) mocker.patch("plugin.okta.list_group_push_mappings", return_value=[]) assert plugin_instance._discover_existing_link(group) is None @pytest.fixture -def session_mock(): +def session_mock() -> MagicMock: return MagicMock() -def test_reconcile_creates_when_no_link_and_config_present(plugin_instance, mocker, session_mock): +def test_reconcile_creates_when_no_link_and_config_present( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group( mocker, group_config={"email": "platform-security", "display_name": "Platform Security"}, description="Sec team" ) @@ -422,7 +450,9 @@ def test_reconcile_creates_when_no_link_and_config_present(plugin_instance, mock set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID) -def test_reconcile_enforces_config_onto_existing_group(plugin_instance, mocker, session_mock): +def test_reconcile_enforces_config_onto_existing_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group( mocker, group_config={"email": "new-prefix", "display_name": "New Name"}, @@ -465,7 +495,9 @@ def test_reconcile_enforces_config_onto_existing_group(plugin_instance, mocker, assert patch.call_args.kwargs == {"display_name": "New Name", "description": "New desc"} -def test_reconcile_adopts_missing_config_from_live_group(plugin_instance, mocker, session_mock): +def test_reconcile_adopts_missing_config_from_live_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, group_config={}, description="") # no config, no description mocker.patch( "plugin.get_config_value", @@ -511,7 +543,9 @@ def test_reconcile_adopts_missing_config_from_live_group(plugin_instance, mocker patch.assert_not_called() -def test_reconcile_flags_error_on_domain_mismatch_adoption(plugin_instance, mocker, session_mock): +def test_reconcile_flags_error_on_domain_mismatch_adoption( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, group_config={}) mocker.patch( "plugin.get_config_value", side_effect=lambda obj, key, pid, default=None: {"enabled": True}.get(key, default) @@ -545,7 +579,9 @@ def test_reconcile_flags_error_on_domain_mismatch_adoption(plugin_instance, mock set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) -def test_reconcile_grandfathers_unchanged_legacy_email(plugin_instance, mocker, session_mock): +def test_reconcile_grandfathers_unchanged_legacy_email( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: # An existing group whose prefix violates a later-added pattern is left alone: # the email (groupKey) is immutable and never patched, so the pattern is never # re-enforced on an existing group and reconcile marks it synced, not error. @@ -590,7 +626,9 @@ def test_reconcile_grandfathers_unchanged_legacy_email(plugin_instance, mocker, set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_SYNCED, PLUGIN_ID) -def test_reconcile_skips_when_disabled(plugin_instance, mocker, session_mock): +def test_reconcile_skips_when_disabled( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker) mocker.patch("plugin.get_config_value", return_value=False) # enabled = False discover = mocker.patch.object(plugin_instance, "_discover_existing_link") @@ -598,7 +636,9 @@ def test_reconcile_skips_when_disabled(plugin_instance, mocker, session_mock): discover.assert_not_called() -def test_reconcile_marks_pending_when_push_mapping_defers(plugin_instance, mocker, session_mock): +def test_reconcile_marks_pending_when_push_mapping_defers( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, group_config={"email": "sec", "display_name": "Sec"}, description="d") mocker.patch( "plugin.get_config_value", @@ -623,7 +663,9 @@ def test_reconcile_marks_pending_when_push_mapping_defers(plugin_instance, mocke assert synced == [] # never reached SYNC_SYNCED -def test_reconcile_create_path_rejects_pattern_violation(plugin_instance, mocker, session_mock): +def test_reconcile_create_path_rejects_pattern_violation( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, group_config={"email": "platform", "display_name": "P"}, description="d") mocker.patch( "plugin.get_config_value", @@ -646,7 +688,9 @@ def test_reconcile_create_path_rejects_pattern_violation(plugin_instance, mocker set_status.assert_any_call(group, STATUS_SYNC_STATUS, SYNC_ERROR, PLUGIN_ID) -def test_reconcile_creates_when_no_group_exists(plugin_instance, mocker): +def test_reconcile_creates_when_no_group_exists( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) mocker.patch( "plugin.get_config_value", @@ -905,7 +949,9 @@ def test_claim_skips_advisory_lock_off_postgres( session.execute.assert_not_called() -def test_reconcile_relooks_up_when_cached_id_404s(plugin_instance, mocker): +def test_reconcile_relooks_up_when_cached_id_404s( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: from googleapiclient.errors import HttpError group = _group( @@ -935,7 +981,9 @@ def test_reconcile_relooks_up_when_cached_id_404s(plugin_instance, mocker): assert group.plugin_data[PLUGIN_ID]["status"][STATUS_GOOGLE_GROUP_ID] == "ggid-fresh" -def test_enforce_patches_display_name_not_email(plugin_instance, mocker): +def test_enforce_patches_display_name_not_email( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: group = _group( mocker, group_config={"email": "sec", "display_name": "New Name"}, @@ -969,14 +1017,18 @@ def test_enforce_patches_display_name_not_email(plugin_instance, mocker): assert patch.call_args.kwargs == {"display_name": "New Name", "description": None} -def test_group_created_calls_reconcile(plugin_instance, mocker, session_mock): +def test_group_created_calls_reconcile( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker) reconcile = mocker.patch.object(plugin_instance, "_reconcile") plugin_instance.group_created(session=session_mock, group=group, plugin_id=PLUGIN_ID) reconcile.assert_called_once_with(session_mock, group) -def test_group_updated_calls_reconcile(plugin_instance, mocker, session_mock): +def test_group_updated_calls_reconcile( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker) reconcile = mocker.patch.object(plugin_instance, "_reconcile") plugin_instance.group_updated( @@ -985,14 +1037,18 @@ def test_group_updated_calls_reconcile(plugin_instance, mocker, session_mock): reconcile.assert_called_once_with(session_mock, group) -def test_hooks_ignore_other_plugin(plugin_instance, mocker, session_mock): +def test_hooks_ignore_other_plugin( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker) reconcile = mocker.patch.object(plugin_instance, "_reconcile") plugin_instance.group_created(session=session_mock, group=group, plugin_id="some_other_plugin") reconcile.assert_not_called() -def test_group_deleted_unlinks_then_deletes(plugin_instance, mocker, session_mock): +def test_group_deleted_unlinks_then_deletes( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, status={"push_mapping_id": "map-1", "google_group_id": "ggid-1"}) mocker.patch("plugin.get_config_value", return_value=True) # enabled mocker.patch( @@ -1018,7 +1074,9 @@ def test_group_deleted_unlinks_then_deletes(plugin_instance, mocker, session_moc assert [c[0] for c in mgr.mock_calls] == ["delete_mapping", "delete_group"] -def test_group_deleted_skips_when_unmanaged(plugin_instance, mocker, session_mock): +def test_group_deleted_skips_when_unmanaged( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: group = _group(mocker, status={}) # enabled=True, but no email/display_name config -> genuinely unmanaged. mocker.patch( @@ -1081,7 +1139,9 @@ def test_group_deleted_does_not_fall_back_to_email_lookup( delete_group.assert_not_called() # nothing we provably own -> nothing to delete -def test_sync_all_reconciles_each_group(plugin_instance, mocker, session_mock): +def test_sync_all_reconciles_each_group( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: app = Mock(spec=App) g1, g2 = Mock(spec=AppGroup), Mock(spec=AppGroup) session_mock.scalars.return_value.all.return_value = [g1, g2] @@ -1090,7 +1150,9 @@ def test_sync_all_reconciles_each_group(plugin_instance, mocker, session_mock): assert reconcile.call_count == 2 -def test_okta_target_group_id_searches_by_email(plugin_instance, mocker): +def test_okta_target_group_id_searches_by_email( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: list_groups = mocker.patch("plugin.okta.list_groups", return_value=[Mock(group=Mock(id="okta-tgt-7"))]) assert plugin_instance._okta_target_group_id("sec@test-company.com") == "okta-tgt-7" search = list_groups.call_args.kwargs["query_params"]["search"] @@ -1165,7 +1227,9 @@ def test_create_push_mapping_resolves_target_by_email( set_status.assert_any_call(group, STATUS_PUSH_MAPPING_ID, "map-1", PLUGIN_ID) -def test_discover_existing_link_recovers_email(plugin_instance, mocker): +def test_discover_existing_link_recovers_email( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: group = _group(mocker) profile = Mock(googleGroupEmail="sec@test-company.com") mocker.patch( @@ -1180,7 +1244,9 @@ def test_discover_existing_link_recovers_email(plugin_instance, mocker): assert link == {"email": "sec@test-company.com", "push_mapping_id": "map-1"} -def test_reconcile_ignores_stale_push_mapping_when_group_gone(plugin_instance, mocker): +def test_reconcile_ignores_stale_push_mapping_when_group_gone( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: # Out-of-band Okta link exists, but its Google group was deleted (lookup -> None). # The stale push_mapping_id must NOT be adopted; reconcile re-creates and re-links. group = _group(mocker, group_config={"email": "sec", "display_name": "Security"}) @@ -1213,7 +1279,9 @@ def test_reconcile_ignores_stale_push_mapping_when_group_gone(plugin_instance, m create_mapping.assert_called_once() -def test_sync_all_continues_after_group_failure(plugin_instance, mocker, session_mock): +def test_sync_all_continues_after_group_failure( + plugin_instance: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: app = Mock(spec=App) g1, g2 = Mock(spec=AppGroup), Mock(spec=AppGroup) g1.name = "Group1" From ea6855bb6c9cf2480833fb29b6638bf3d74f549a Mon Sep 17 00:00:00 2001 From: Brynn Arborico Date: Sat, 27 Jun 2026 01:08:16 +0000 Subject: [PATCH 19/19] Ship the Google group lifecycle plugin source in the image Copy examples/plugins/app_group_lifecycle_google into the image at /app/plugins without installing it here, so downstream images (e.g. Discord's overlay) can opt in by pip installing it. This keeps the plugin source single-homed in this repo while leaving activation to the consumer. Co-Authored-By: Claude Opus 4.8 --- Dockerfile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Dockerfile b/Dockerfile index dbbf5cbd..83e6fb0d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -49,6 +49,12 @@ COPY requirements.txt api/ ./api/ COPY migrations/ ./migrations/ COPY alembic.ini setup.py ./ COPY ./config ./config + +# Ship prod-ready plugin source into the image WITHOUT installing it here. +# Downstream images opt in by pip installing the plugins, +# which keeps the source single-homed in this repo while leaving activation to the consumer. +COPY ./examples/plugins/app_group_lifecycle_google ./plugins/app_group_lifecycle_google + RUN pip install -r ./api/requirements.txt # Install the project itself so the `access` console script declared in # setup.py (entry_points -> api.manage:cli) is available on PATH for