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 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 # ---------------------------------------------------------------------- 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) 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/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/plugins/app_group_lifecycle.py b/api/plugins/app_group_lifecycle.py index 51550fcc..f67732a5 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 @@ -107,7 +111,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 +119,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. @@ -232,17 +248,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.""" @@ -414,6 +450,43 @@ 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 +549,22 @@ 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 +583,54 @@ 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, + old_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). + 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. """ 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) + 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( diff --git a/api/routers/groups.py b/api/routers/groups.py index 12679a05..63320100 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, @@ -101,6 +101,34 @@ 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, + 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). 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, old_plugin_data=old_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 +219,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 +243,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 +254,16 @@ 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), + old_plugin_data=group.plugin_data or {}, + ) if not _perms.can_manage_group(db, current_user_id, group): raise HTTPException(403, "Current user is not allowed to perform this action") @@ -366,16 +399,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/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/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/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/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/plugin.py b/examples/plugins/app_group_lifecycle_google/plugin.py new file mode 100644 index 00000000..3553ee2a --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/plugin.py @@ -0,0 +1,728 @@ +""" +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 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 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 + +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__) + + +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 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.""" + + 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 + + # ---- 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 + + # ---- 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")} + + # ---- 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 + + # ---- 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/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/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..43e4fddf --- /dev/null +++ b/examples/plugins/app_group_lifecycle_google/test_plugin.py @@ -0,0 +1,1291 @@ +"""Tests for the Google Groups Lifecycle Plugin.""" + +import os +import sys +from pathlib import Path +from typing import Any +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 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, +) + +from api.models import App, AppGroup # noqa: E402 + + +@pytest.fixture +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) + groups_api = MagicMock() + discovery_client.groups.return_value = groups_api + return groups_api + + +@pytest.fixture +def plugin_instance(mocker: MockerFixture, mock_groups_api: MagicMock) -> GoogleGroupManagerPlugin: + 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: 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: 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: 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: GoogleGroupManagerPlugin) -> None: + 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: GoogleGroupManagerPlugin) -> None: + 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: 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: GoogleGroupManagerPlugin) -> None: + errors = plugin_instance.validate_plugin_app_config({}, PLUGIN_ID) + assert "enabled" in errors + + +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 + ) + 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: 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: 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: 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( + {"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: 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) + 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: GoogleGroupManagerPlugin) -> None: + assert plugin_instance._full_email("platform-security") == "platform-security@test-company.com" + + +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: GoogleGroupManagerPlugin) -> None: + assert plugin_instance._prefix_from_email("x@other.com") is None + + +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: 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: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: + # 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_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"}}, + } + 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: 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: 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" + assert kwargs["body"] == {"displayName": "New", "description": "d"} + assert kwargs["updateMask"] == "description,displayName" + + +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: 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: 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: GoogleGroupManagerPlugin, mock_groups_api: MagicMock) -> None: + 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_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 + assert props["display_name"].immutable is False + + +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"}) + 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: 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") + + 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: GoogleGroupManagerPlugin, mocker: MockerFixture) -> None: + 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: 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() -> MagicMock: + return MagicMock() + + +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" + ) + 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: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + 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: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + 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: 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) + ) + 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: 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. + 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: 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") + plugin_instance._reconcile(session_mock, group) + discover.assert_not_called() + + +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", + 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: 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", + 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: 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=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: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + 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: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + 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_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: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + 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: 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: 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( + "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: GoogleGroupManagerPlugin, mocker: MockerFixture, session_mock: MagicMock +) -> None: + 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: 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] + 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: 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"] + 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_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: + 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: GoogleGroupManagerPlugin, mocker: MockerFixture +) -> None: + 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"} + + +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"}) + 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() + + +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" + 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 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', 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/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'; diff --git a/tests/test_app_group_lifecycle_plugin.py b/tests/test_app_group_lifecycle_plugin.py index f8f4b2f8..9fa8b51d 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 @@ -32,7 +33,10 @@ get_config_value, get_status_value, hookimpl, + invoke_sync_all_groups, + 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, @@ -114,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 @@ -126,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 @@ -223,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.""" @@ -617,6 +641,48 @@ 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 + + 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.""" @@ -699,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: @@ -2144,3 +2264,256 @@ 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" + + +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 + + +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 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("") 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