From e0681f6f0aad4ca2d445ff12d5aae25e91548dc5 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 00:48:01 -0400 Subject: [PATCH 1/6] refactor(handlers): extract admin user listing & search into admin_user_operations Slice 1 of the admin_operations split (#773): move the user listing/search concern (adminListUsers, adminSearchUser and their private helpers) from the 1,838-line admin_operations module into src/handlers/admin_user_operations.py, with zero behavior change. - Handler/helper bodies are moved verbatim. The only adjustments: the two Cognito throttling branches now reference TRANSIENT_ERROR_CODES directly (the file-local alias is unchanged in admin_operations, and the same frozenset comes from utils.dynamodb), and _raise_batch_lookup_error / _actor_sub are imported at function level from admin_operations (a top-level import would be circular: the dispatcher map imports these handlers), following the established campaign_operations precedent. - The Lambda entry point (handlers.admin_operations.lambda_handler) and the AppSync handler mapping are unchanged: admin_operations imports the two handlers for its operation dispatcher map. No IaC change: the Lambda payload already ships the whole src/ tree. - Tests: patch targets for the moved symbols are repointed to src.handlers.admin_user_operations; the test_cognito_filter_call_sites and test_admin_group_name_contract table rows follow the moved call sites. - Verified: uv run pytest tests/unit -q (1841 passed, 100% coverage), ruff check, ruff format --check, mypy src/ (strict), xenon complexity gate, cspell, and tofu validate on the dev environment. --- src/handlers/admin_operations.py | 592 +--------------- src/handlers/admin_user_operations.py | 646 ++++++++++++++++++ tests/unit/test_admin_group_name_contract.py | 2 +- tests/unit/test_admin_operations.py | 238 +++---- tests/unit/test_admin_search_accounts_scan.py | 10 +- tests/unit/test_admin_search_email_index.py | 4 +- tests/unit/test_cognito_filter_call_sites.py | 6 +- 7 files changed, 787 insertions(+), 711 deletions(-) create mode 100644 src/handlers/admin_user_operations.py diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index f0435c279..0d0088ebe 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -7,6 +7,9 @@ accounts record and the Cognito user - createManagedCatalog: Create an ADMIN_MANAGED global catalog +The adminListUsers / adminSearchUser handlers live in ``admin_user_operations`` +(#773) and are imported below so the dispatcher map stays the single registry. + User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs with ``adminGetUserProfiles`` before deleting the profile rows, and calls @@ -26,12 +29,10 @@ principals. """ -import re import uuid -from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timezone from decimal import Decimal, InvalidOperation -from typing import TYPE_CHECKING, Any, Callable, Dict, NoReturn, Optional, cast +from typing import TYPE_CHECKING, Any, Dict, NoReturn, Optional, cast from botocore.exceptions import BotoCoreError, ClientError @@ -58,7 +59,6 @@ from utils.cognito import is_transient_cognito_error from utils.cognito_filters import cognito_user_filter from utils.dynamodb import ( - EMAIL_SEARCH_KEY, TRANSIENT_ERROR_CODES, batch_get_chunked, get_required_env, @@ -74,7 +74,6 @@ from ..utils.cognito import is_transient_cognito_error from ..utils.cognito_filters import cognito_user_filter from ..utils.dynamodb import ( - EMAIL_SEARCH_KEY, TRANSIENT_ERROR_CODES, batch_get_chunked, get_required_env, @@ -95,6 +94,12 @@ except ModuleNotFoundError: from ..utils.handlers import lambda_handler as with_error_handling + +# The user listing & search handlers were extracted to their own module (#773) but +# stay registered on this Lambda. The import is safe at module load because +# `admin_user_operations` only imports from this module inside functions. +from .admin_user_operations import admin_list_users, admin_search_user + # Handle both Lambda (absolute) and unit test (relative) imports. mypy sees the # relative path it can resolve; the runtime fallback tries absolute first for Lambda. if TYPE_CHECKING: # pragma: no cover @@ -184,257 +189,6 @@ def _raise_gather_failures( ) -def _get_user_groups(cognito: Any, user_pool_id: str, username: str, logger: Any) -> list[str]: - """Get the groups a user belongs to; failures raise a typed AppError (#291).""" - try: - response = cognito.admin_list_groups_for_user( - UserPoolId=user_pool_id, - Username=username, - ) - return [group["GroupName"] for group in response.get("Groups", [])] - except ClientError as e: - _raise_batch_lookup_error("load user groups", logger, e, username=mask_email(username)) - - -def _batch_get_user_groups(cognito: Any, user_pool_id: str, usernames: list[str], logger: Any) -> dict[str, list[str]]: - """Fetch Cognito groups for multiple users in parallel. - - Cognito does not expose a batch group-membership API, so we parallelize - the per-user calls to avoid the N+1 fan-out. Lookup failures raise an - AppError (retryable RESOURCE_BUSY on throttling) so the admin UI shows an - error instead of silently incomplete group data (#291). - """ - unique_usernames = list(dict.fromkeys(u for u in usernames if u)) - if not unique_usernames: - return {} - - def _fetch(username: str) -> tuple[str, list[str]]: - return username, _get_user_groups(cognito, user_pool_id, username, logger) - - return _collect_parallel_keyed_results(_fetch, unique_usernames, "load user groups", logger) - - -def _collect_parallel_keyed_results( - fetch: Callable[[Any], tuple[Any, Any]], items: list[Any], operation: str, logger: Any -) -> dict[Any, Any]: - """Run per-item fetch calls in parallel and collect (key, value) pairs into a dict. - - AppErrors (including retryable RESOURCE_BUSY) propagate unchanged; any other - exception is translated via _raise_batch_lookup_error. - """ - results: dict[Any, Any] = {} - try: - with ThreadPoolExecutor(max_workers=10) as executor: - for key, value in executor.map(fetch, items): - results[key] = value - except AppError: - raise - except Exception as e: - _raise_batch_lookup_error(operation, logger, e) - return results - - -def _dedup_account_keys(account_ids: list[str]) -> list[Dict[str, str]]: - """Build the de-duplicated BatchGetItem keys for the Accounts table.""" - seen: set[str] = set() - keys: list[Dict[str, str]] = [] - for account_id in account_ids: - db_account_id = account_id if account_id.startswith("ACCOUNT#") else f"ACCOUNT#{account_id}" - if db_account_id in seen: - continue - seen.add(db_account_id) - keys.append({"accountId": db_account_id}) - return keys - - -def _apply_display_name_item(item: Dict[str, Any], display_names: dict[str, str]) -> None: - """Store an account item's display name (when it has one) in the map.""" - raw_id = item.get("accountId", "") - key = raw_id[8:] if raw_id.startswith("ACCOUNT#") else raw_id - given_name = str(item.get("givenName", "")) - family_name = str(item.get("familyName", "")) - if given_name or family_name: - display_names[key] = f"{given_name} {family_name}".strip() - - -def _batch_get_display_names(account_ids: list[str], logger: Any) -> dict[str, str]: - """Batch fetch display names from the Accounts table using BatchGetItem. - - Lookup failures raise an AppError (retryable RESOURCE_BUSY on throttling) - so the admin UI shows an error instead of silently incomplete names (#291). - """ - display_names: dict[str, str] = {} - keys = _dedup_account_keys(account_ids) - if not keys: - return display_names - - accounts_table_name = get_required_env("ACCOUNTS_TABLE_NAME") - - def _store_display_name(item: Dict[str, Any]) -> None: - _apply_display_name_item(item, display_names) - - batch_get_chunked( - accounts_table_name, - keys, - _store_display_name, - consistent_read=False, - logger=logger, - ) - return display_names - - -def _list_cognito_users( - cognito: Any, user_pool_id: str, limit: int, next_token: str | None, logger: Any -) -> tuple[list[Dict[str, Any]], str | None]: - """Call Cognito list_users API and return (users, pagination_token).""" - list_params: Dict[str, Any] = {"UserPoolId": user_pool_id, "Limit": limit} - if next_token: - list_params["PaginationToken"] = next_token - - try: - response = cognito.list_users(**list_params) - except ClientError as e: - logger.error("Cognito list_users failed", error=str(e)) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to list users") - - return response.get("Users", []), response.get("PaginationToken") - - -@with_error_handling(error_message="Failed to list users") -def admin_list_users(event: Dict[str, Any], context: Any) -> Dict[str, Any]: - """ - List all Cognito users with their DynamoDB Account data (admin only). - - AppSync Lambda resolver for adminListUsers query. - Retrieves users from Cognito and enriches with DynamoDB Account data. - - Args: - event: AppSync event with identity and arguments - context: Lambda context - - Returns: - AdminUserConnection with users array and optional nextToken - - Raises: - AppError: If not admin or Cognito error - """ - logger = get_logger(__name__) - - require_admin_mfa(event) - - arguments = event.get("arguments", {}) - raw_limit = arguments.get("limit") - limit = max(1, min(raw_limit or 20, 60)) - next_token = arguments.get("nextToken") - - user_pool_id = get_required_env("USER_POOL_ID") - cognito = get_cognito_client() - - cognito_users, pagination_token = _list_cognito_users(cognito, user_pool_id, limit, next_token, logger) - - # Batch DynamoDB display-name lookups and parallelize Cognito group lookups - # to avoid the per-user N+1 fan-out. - account_ids = [_extract_cognito_attributes(user)[1].get("sub", user.get("Username", "")) for user in cognito_users] - usernames = [user.get("Username", "") for user in cognito_users] - display_names = _batch_get_display_names(account_ids, logger) - groups_map = _batch_get_user_groups(cognito, user_pool_id, usernames, logger) - - actor_sub = _actor_sub(event) - admin_users = _build_admin_users_from_cognito(cognito_users, display_names, groups_map) - - logger.info( - "Listed users", - count=len(admin_users), - has_more=bool(pagination_token), - actor_sub=actor_sub, - ) - return {"users": admin_users, "nextToken": pagination_token} - - -def _execute_search_strategy(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: - """Execute search strategy based on query format.""" - if query.startswith("ACCOUNT#"): - return _search_by_account_prefix(query, cognito, user_pool_id, logger) - if _looks_like_uuid(query): - return _search_by_uuid(query, cognito, user_pool_id, logger) - return _search_by_general_query(query, cognito, user_pool_id, logger) - - -@with_error_handling(error_message="Failed to search user") -def admin_search_user(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Search for users by email, name, or accountId (admin only). - - AppSync Lambda resolver for adminSearchUser query. - Search strategy: - 1. If query looks like UUID or ACCOUNT#UUID, search Cognito by sub directly (single result) - 2. Otherwise (minimum `_ACCOUNT_SEARCH_MIN_QUERY_LENGTH` characters): - a. Search DynamoDB Accounts table: an email fragment uses the - emailSearchIndex GSI (prefix query); every other query (a name - fragment) scans the table, bounded by the - `_ACCOUNTS_SCAN_SAFETY_LIMIT` safety limit. Both cover logged-in - users. - b. Search Cognito with prefix matching (all users, including those who haven't logged in) - c. Merge results, deduplicate by accountId - - Args: - event: AppSync event with identity and arguments - context: Lambda context - - Returns: - List of AdminUser objects matching the query (empty list if none found) - - Raises: - AppError: If not admin, missing query, or Cognito error - """ - logger = get_logger(__name__) - - require_admin_mfa(event) - - query = str(event.get("arguments", {}).get("query", "")).strip() - if not query: - raise AppError(ErrorCode.INVALID_INPUT, "Search query is required") - - _validate_search_query(query) - - user_pool_id = get_required_env("USER_POOL_ID") - cognito = get_cognito_client() - - # Determine search strategy based on query format. - # The map values are Cognito user dicts so we can batch enrich them. - results_map = _execute_search_strategy(query, cognito, user_pool_id, logger) - cognito_users = list(results_map.values()) - - # Batch DynamoDB display-name lookups and parallelize Cognito group lookups. - account_ids = [_extract_cognito_attributes(user)[1].get("sub", user.get("Username", "")) for user in cognito_users] - usernames = [user.get("Username", "") for user in cognito_users] - display_names = _batch_get_display_names(account_ids, logger) - groups_map = _batch_get_user_groups(cognito, user_pool_id, usernames, logger) - - return _build_admin_users_from_cognito(cognito_users, display_names, groups_map) - - -def _search_by_account_prefix(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: - """Search by ACCOUNT# prefix and return Cognito-user results map.""" - sub_value = query[8:] # Remove "ACCOUNT#" - cognito_user = _search_user_by_sub(cognito, user_pool_id, sub_value, logger) - if cognito_user: - attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} - account_id = attributes.get("sub", cognito_user.get("Username", "")) - return {account_id: cognito_user} - return {} - - -def _search_by_uuid(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: - """Search by UUID (sub) and return Cognito-user results map.""" - cognito_user = _search_user_by_sub(cognito, user_pool_id, query, logger) - if cognito_user: - attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} - account_id = attributes.get("sub", cognito_user.get("Username", "")) - return {account_id: cognito_user} - return {} - - def _actor_sub(event: Dict[str, Any]) -> str: """Return the acting administrator's Cognito ``sub`` for audit logging (#507). @@ -474,332 +228,6 @@ def _require_admin_and_get_target_account_id(event: Dict[str, Any]) -> tuple[str return account_id, actor_sub -def _add_dynamodb_users_to_map( - results_map: dict[str, Dict[str, Any]], query: str, cognito: Any, user_pool_id: str, logger: Any -) -> None: - """Search DynamoDB and add matching Cognito users to results map.""" - accounts = _search_accounts_in_dynamodb(query, logger) - for account in accounts: - account_id = account.get("accountId", "") - if account_id.startswith("ACCOUNT#"): - sub_value = account_id[8:] - cognito_user = _search_user_by_sub(cognito, user_pool_id, sub_value, logger) - if cognito_user: - attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} - key = attributes.get("sub", cognito_user.get("Username", "")) - results_map[key] = cognito_user - - -def _add_cognito_users_to_map( - results_map: dict[str, Dict[str, Any]], query: str, cognito: Any, user_pool_id: str, logger: Any -) -> None: - """Search Cognito by email and add users to results map.""" - cognito_users = _search_users_in_cognito_by_email_prefix(cognito, user_pool_id, query, logger) - for cognito_user in cognito_users: - attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} - account_id = attributes.get("sub", cognito_user.get("Username", "")) - if account_id not in results_map: - results_map[account_id] = cognito_user - - -def _search_by_general_query(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: - """Search DynamoDB and Cognito by general query and return merged results map.""" - results_map: dict[str, Dict[str, Any]] = {} - _add_dynamodb_users_to_map(results_map, query, cognito, user_pool_id, logger) - _add_cognito_users_to_map(results_map, query, cognito, user_pool_id, logger) - return results_map - - -def _matches_query(item: Dict[str, Any], query_lower: str) -> bool: - """Check if account item matches query (case-insensitive).""" - email = str(item.get("email", "")).lower() - given_name = str(item.get("givenName", "")).lower() - family_name = str(item.get("familyName", "")).lower() - return query_lower in email or query_lower in given_name or query_lower in family_name - - -def _filter_matching_accounts( - items: list[Dict[str, Any]], query_lower: str, matches: list[Dict[str, Any]], max_results: int -) -> None: - """Filter accounts that match the query and add to matches list.""" - for item in items: - if len(matches) >= max_results: - break - if _matches_query(item, query_lower): - matches.append(item) - - -def _scan_accounts_page(paginator_params: Dict[str, Any]) -> tuple[list[Dict[str, Any]], Dict[str, Any] | None]: - """Scan one page of accounts. Returns (items, last_evaluated_key).""" - response = tables.accounts.scan(**paginator_params) - items = response.get("Items", []) - last_key = response.get("LastEvaluatedKey") - return items, last_key - - -# Limit search results to prevent overwhelming responses. -_ACCOUNT_SEARCH_MAX_RESULTS = 50 -# Safety limit on how many account items a search scan may read. -_ACCOUNTS_SCAN_SAFETY_LIMIT = 1000 -# Shortest query an admin may search for. The only way to match a partial query -# is a full accounts-table scan, and a one or two character query can match -# most of the table while telling the admin nothing, so it is rejected outright -# instead of returning a truncated arbitrary subset (#576). -_ACCOUNT_SEARCH_MIN_QUERY_LENGTH = 3 - -# Accounts GSI for email prefix search: constant HASH key + email RANGE key, so -# begins_with(email, :prefix) is a legal key condition (#586). Declared in -# tofu/application/modules/dynamodb/main.tf; the index is sparse, so an account -# is only searchable once it carries the constant emailSearchKey attribute. -_EMAIL_SEARCH_INDEX = "emailSearchIndex" -_EMAIL_PREFIX_KEY_CONDITION = "emailSearchKey = :searchKey AND begins_with(email, :prefix)" - - -def _looks_like_email_query(query: str) -> bool: - """Check whether the query is an email fragment (it contains the local/domain separator). - - Only email-shaped queries are answered by the prefix index; a bare name - fragment (which ``_is_valid_email_prefix`` also accepts) is a substring - search, and no prefix index can answer that. - """ - return "@" in query - - -def _query_email_search_index( - query: str, query_lower: str, max_results: int, logger: Any -) -> list[Dict[str, Any]] | None: - """Answer an email prefix query with one Query against the emailSearchIndex GSI. - - That index is a constant HASH key (``emailSearchKey``) plus ``email`` as the - RANGE key, which is what makes ``begins_with(email, :prefix)`` a legal key - condition; the exact-lookup ``email-index`` is HASH-only and so can only be - matched with equality and cannot answer a partial email (#576). - - Returns the matching items (capped at ``max_results``), or None when the - index cannot answer the query -- index unavailable, or nothing indexed -- - in which case the caller falls back to the scan. The scan fallback is what - keeps accounts that predate the index (and so carry no ``emailSearchKey``) - findable until the backfill has covered every row. - """ - try: - response = tables.accounts.query( - IndexName=_EMAIL_SEARCH_INDEX, - KeyConditionExpression=_EMAIL_PREFIX_KEY_CONDITION, - ExpressionAttributeValues={":searchKey": EMAIL_SEARCH_KEY, ":prefix": query_lower}, - Limit=max_results, - ) - except ClientError as e: - logger.warning( - "DynamoDB email prefix query failed, falling back to scan", - error=str(e), - query=mask_email(query), - ) - return None - index_items = cast(list[Dict[str, Any]], response.get("Items", [])) - if not index_items: - return None - if response.get("LastEvaluatedKey"): - logger.warning( - "DynamoDB email prefix search hit the result limit; results may be truncated", - query=mask_email(query), - max_results=max_results, - ) - return index_items[:max_results] - - -def _scan_accounts_page_into( - paginator_params: Dict[str, Any], query_lower: str, matches: list[Dict[str, Any]], max_results: int -) -> tuple[int, Dict[str, Any] | None]: - """Scan one page and append its matching accounts to matches. Returns (page_size, last_key).""" - items, last_key = _scan_accounts_page(paginator_params) - _filter_matching_accounts(items, query_lower, matches, max_results) - return len(items), last_key - - -def _warn_if_accounts_scan_truncated( - scanned_count: int, last_key: Dict[str, Any] | None, query: str, logger: Any -) -> None: - """Warn when the account scan stopped at the safety limit with more pages left.""" - if scanned_count >= _ACCOUNTS_SCAN_SAFETY_LIMIT and last_key: - logger.warning( - "DynamoDB accounts search reached max scan limit; results may be truncated", - query=mask_email(query), - scanned_count=scanned_count, - ) - - -def _scan_accounts_matches(query: str, query_lower: str, max_results: int, logger: Any) -> list[Dict[str, Any]]: - """Scan the Accounts table, collecting matches until the result/scan limits are hit.""" - matches: list[Dict[str, Any]] = [] - scanned_count = 0 - paginator_params: Dict[str, Any] = {} - last_key: Dict[str, Any] | None = None - while scanned_count < _ACCOUNTS_SCAN_SAFETY_LIMIT and len(matches) < max_results: - count, last_key = _scan_accounts_page_into(paginator_params, query_lower, matches, max_results) - scanned_count += count - if not last_key: - break - paginator_params["ExclusiveStartKey"] = last_key - _warn_if_accounts_scan_truncated(scanned_count, last_key, query, logger) - return matches - - -def _search_accounts_in_dynamodb(query: str, logger: Any) -> list[Dict[str, Any]]: - """Search the accounts table for accounts matching a partial query. - - Searches email, givenName, and familyName fields. An email fragment is - answered by the emailSearchIndex GSI, whose constant HASH key plus ``email`` - RANGE key make a ``begins_with`` key condition legal: one index query, no - table scan (#586). A name fragment is a substring search that no prefix - index can answer, so it is still served by a full scan of the accounts - table, bounded by the `_ACCOUNTS_SCAN_SAFETY_LIMIT` safety limit and - truncated at `_ACCOUNT_SEARCH_MAX_RESULTS` matches. Either path falls back - to the scan when the index has no answer. - - Returns all matching accounts (up to max_results limit). - """ - query_lower = query.lower() - - try: - if _looks_like_email_query(query): - index_items = _query_email_search_index(query, query_lower, _ACCOUNT_SEARCH_MAX_RESULTS, logger) - if index_items is not None: - return index_items - return _scan_accounts_matches(query, query_lower, _ACCOUNT_SEARCH_MAX_RESULTS, logger) - except ClientError as e: - logger.warning("DynamoDB search failed", error=str(e), query=mask_email(query)) - return [] - - -def _looks_like_uuid(value: str) -> bool: - """Check if a string looks like a UUID.""" - # UUID format: 8-4-4-4-12 hex characters - uuid_pattern = r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" - return bool(re.match(uuid_pattern, value.lower())) - - -def _is_valid_email_prefix(value: str) -> bool: - """Check if a string is a valid email prefix (local part or local@partial-domain).""" - try: - cognito_user_filter("email_prefix", value) - except AppError: - return False - return True - - -def _validate_search_query(query: str) -> None: - """Validate that the admin search query is a safe UUID, email prefix, or ACCOUNT#UUID. - - A non-UUID query is served by a full accounts-table scan, so a query below - `_ACCOUNT_SEARCH_MIN_QUERY_LENGTH` is rejected rather than scanning the - table for a result that cannot narrow anything down (#576). - """ - if query.startswith("ACCOUNT#"): - if _looks_like_uuid(query[8:]): - return - elif _looks_like_uuid(query): - return - elif _is_valid_email_prefix(query): - if len(query) < _ACCOUNT_SEARCH_MIN_QUERY_LENGTH: - raise AppError( - ErrorCode.INVALID_INPUT, - f"Search query must be at least {_ACCOUNT_SEARCH_MIN_QUERY_LENGTH} characters", - ) - return - - raise AppError( - ErrorCode.INVALID_INPUT, - "Search query must be a valid UUID, email prefix, or ACCOUNT#UUID", - ) - - -def _search_user_by_sub(cognito: Any, user_pool_id: str, sub: str, logger: Any) -> Dict[str, Any] | None: - """Search for a Cognito user by sub (account ID).""" - try: - response = cognito.list_users( - UserPoolId=user_pool_id, - Filter=cognito_user_filter("sub", sub), - Limit=1, - ) - users = response.get("Users", []) - return users[0] if users else None - except ClientError as e: - error_code = e.response.get("Error", {}).get("Code", "") - if error_code in _THROTTLING_ERROR_CODES: - logger.warning("Cognito search by sub throttled", error=str(e), error_code=error_code, sub=sub) - raise AppError(ErrorCode.RESOURCE_BUSY, "Temporarily unable to load data. Please retry.") from e - logger.warning("Cognito search by sub failed", error=str(e), sub=sub) - return None - - -def _search_users_in_cognito_by_email_prefix( - cognito: Any, user_pool_id: str, query: str, logger: Any -) -> list[Dict[str, Any]]: - """ - Search Cognito users by email prefix. - - Uses Cognito's email ^= filter for prefix matching. - This finds users who haven't logged in yet (no DynamoDB record). - - Returns up to 50 matching users. - """ - try: - response = cognito.list_users( - UserPoolId=user_pool_id, - Filter=cognito_user_filter("email_prefix", query), # Prefix match on email - Limit=50, # Reasonable limit for search results - ) - return list(response.get("Users", [])) - except ClientError as e: - error_code = e.response.get("Error", {}).get("Code", "") - if error_code in _THROTTLING_ERROR_CODES: - logger.warning( - "Cognito email prefix search throttled", - error=str(e), - error_code=error_code, - query=mask_email(query), - ) - raise AppError(ErrorCode.RESOURCE_BUSY, "Temporarily unable to load data. Please retry.") from e - logger.warning("Cognito email prefix search failed", error=str(e), query=mask_email(query)) - return [] - - -def _extract_cognito_attributes(cognito_user: Dict[str, Any]) -> tuple[str, Dict[str, str]]: - """Extract username and attributes from Cognito user.""" - username = cognito_user.get("Username", "") - attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} - return username, attributes - - -def _build_admin_users_from_cognito( - cognito_users: list[Dict[str, Any]], - display_names: dict[str, str], - groups_map: dict[str, list[str]], -) -> list[Dict[str, Any]]: - """Build AdminUser objects from Cognito users plus cached display names and groups.""" - admin_users = [] - for cognito_user in cognito_users: - username, attributes = _extract_cognito_attributes(cognito_user) - account_id = attributes.get("sub", username) - groups = groups_map.get(username, []) - created_at = cognito_user.get("UserCreateDate") - last_modified_at = cognito_user.get("UserLastModifiedDate") - admin_users.append( - { - "accountId": account_id, - "email": attributes.get("email", ""), - "displayName": display_names.get(account_id), - "status": cognito_user.get("UserStatus", "UNKNOWN"), - "enabled": cognito_user.get("Enabled", True), - "emailVerified": attributes.get("email_verified", "false").lower() == "true", - "isAdmin": "ADMIN" in groups, - "createdAt": created_at.isoformat() if created_at else None, - "lastModifiedAt": last_modified_at.isoformat() if last_modified_at else None, - } - ) - return admin_users - - def _find_cognito_user_by_sub( cognito: Any, user_pool_id: str, account_id: str, logger: Any ) -> tuple[str | None, str | None]: diff --git a/src/handlers/admin_user_operations.py b/src/handlers/admin_user_operations.py new file mode 100644 index 000000000..b808d4e02 --- /dev/null +++ b/src/handlers/admin_user_operations.py @@ -0,0 +1,646 @@ +""" +Admin user listing & search handlers (extracted from admin_operations, #773). + +Provides: +- adminListUsers: List Cognito users enriched with DynamoDB account data +- adminSearchUser: Search users by email, name, or accountId + +Both handlers stay registered on the shared admin-operations Lambda: +``admin_operations`` imports them for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +import re +from concurrent.futures import ThreadPoolExecutor +from typing import TYPE_CHECKING, Any, Callable, Dict, cast + +from botocore.exceptions import ClientError + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.auth import require_admin_mfa + from utils.boto import get_cognito_client + from utils.cognito_filters import cognito_user_filter + from utils.dynamodb import ( + EMAIL_SEARCH_KEY, + TRANSIENT_ERROR_CODES, + batch_get_chunked, + get_required_env, + tables, + ) + from utils.errors import AppError, ErrorCode + from utils.logging import get_logger, mask_email +except ModuleNotFoundError: # pragma: no cover + from ..utils.auth import require_admin_mfa + from ..utils.boto import get_cognito_client + from ..utils.cognito_filters import cognito_user_filter + from ..utils.dynamodb import ( + EMAIL_SEARCH_KEY, + TRANSIENT_ERROR_CODES, + batch_get_chunked, + get_required_env, + tables, + ) + from ..utils.errors import AppError, ErrorCode + from ..utils.logging import get_logger, mask_email + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _get_user_groups(cognito: Any, user_pool_id: str, username: str, logger: Any) -> list[str]: + """Get the groups a user belongs to; failures raise a typed AppError (#291). + + The import is function-level: a top-level import would be circular + (`admin_operations` imports this module for its operation dispatcher map). + """ + from .admin_operations import _raise_batch_lookup_error + + try: + response = cognito.admin_list_groups_for_user( + UserPoolId=user_pool_id, + Username=username, + ) + return [group["GroupName"] for group in response.get("Groups", [])] + except ClientError as e: + _raise_batch_lookup_error("load user groups", logger, e, username=mask_email(username)) + + +def _batch_get_user_groups(cognito: Any, user_pool_id: str, usernames: list[str], logger: Any) -> dict[str, list[str]]: + """Fetch Cognito groups for multiple users in parallel. + + Cognito does not expose a batch group-membership API, so we parallelize + the per-user calls to avoid the N+1 fan-out. Lookup failures raise an + AppError (retryable RESOURCE_BUSY on throttling) so the admin UI shows an + error instead of silently incomplete group data (#291). + """ + unique_usernames = list(dict.fromkeys(u for u in usernames if u)) + if not unique_usernames: + return {} + + def _fetch(username: str) -> tuple[str, list[str]]: + return username, _get_user_groups(cognito, user_pool_id, username, logger) + + return _collect_parallel_keyed_results(_fetch, unique_usernames, "load user groups", logger) + + +def _collect_parallel_keyed_results( + fetch: Callable[[Any], tuple[Any, Any]], items: list[Any], operation: str, logger: Any +) -> dict[Any, Any]: + """Run per-item fetch calls in parallel and collect (key, value) pairs into a dict. + + AppErrors (including retryable RESOURCE_BUSY) propagate unchanged; any other + exception is translated via _raise_batch_lookup_error. The import is + function-level: a top-level import would be circular (`admin_operations` + imports this module for its operation dispatcher map). + """ + from .admin_operations import _raise_batch_lookup_error + + results: dict[Any, Any] = {} + try: + with ThreadPoolExecutor(max_workers=10) as executor: + for key, value in executor.map(fetch, items): + results[key] = value + except AppError: + raise + except Exception as e: + _raise_batch_lookup_error(operation, logger, e) + return results + + +def _dedup_account_keys(account_ids: list[str]) -> list[Dict[str, str]]: + """Build the de-duplicated BatchGetItem keys for the Accounts table.""" + seen: set[str] = set() + keys: list[Dict[str, str]] = [] + for account_id in account_ids: + db_account_id = account_id if account_id.startswith("ACCOUNT#") else f"ACCOUNT#{account_id}" + if db_account_id in seen: + continue + seen.add(db_account_id) + keys.append({"accountId": db_account_id}) + return keys + + +def _apply_display_name_item(item: Dict[str, Any], display_names: dict[str, str]) -> None: + """Store an account item's display name (when it has one) in the map.""" + raw_id = item.get("accountId", "") + key = raw_id[8:] if raw_id.startswith("ACCOUNT#") else raw_id + given_name = str(item.get("givenName", "")) + family_name = str(item.get("familyName", "")) + if given_name or family_name: + display_names[key] = f"{given_name} {family_name}".strip() + + +def _batch_get_display_names(account_ids: list[str], logger: Any) -> dict[str, str]: + """Batch fetch display names from the Accounts table using BatchGetItem. + + Lookup failures raise an AppError (retryable RESOURCE_BUSY on throttling) + so the admin UI shows an error instead of silently incomplete names (#291). + """ + display_names: dict[str, str] = {} + keys = _dedup_account_keys(account_ids) + if not keys: + return display_names + + accounts_table_name = get_required_env("ACCOUNTS_TABLE_NAME") + + def _store_display_name(item: Dict[str, Any]) -> None: + _apply_display_name_item(item, display_names) + + batch_get_chunked( + accounts_table_name, + keys, + _store_display_name, + consistent_read=False, + logger=logger, + ) + return display_names + + +def _list_cognito_users( + cognito: Any, user_pool_id: str, limit: int, next_token: str | None, logger: Any +) -> tuple[list[Dict[str, Any]], str | None]: + """Call Cognito list_users API and return (users, pagination_token).""" + list_params: Dict[str, Any] = {"UserPoolId": user_pool_id, "Limit": limit} + if next_token: + list_params["PaginationToken"] = next_token + + try: + response = cognito.list_users(**list_params) + except ClientError as e: + logger.error("Cognito list_users failed", error=str(e)) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to list users") + + return response.get("Users", []), response.get("PaginationToken") + + +@with_error_handling(error_message="Failed to list users") +def admin_list_users(event: Dict[str, Any], context: Any) -> Dict[str, Any]: + """ + List all Cognito users with their DynamoDB Account data (admin only). + + AppSync Lambda resolver for adminListUsers query. + Retrieves users from Cognito and enriches with DynamoDB Account data. + + Args: + event: AppSync event with identity and arguments + context: Lambda context + + Returns: + AdminUserConnection with users array and optional nextToken + + Raises: + AppError: If not admin or Cognito error + """ + from .admin_operations import _actor_sub + + logger = get_logger(__name__) + + require_admin_mfa(event) + + arguments = event.get("arguments", {}) + raw_limit = arguments.get("limit") + limit = max(1, min(raw_limit or 20, 60)) + next_token = arguments.get("nextToken") + + user_pool_id = get_required_env("USER_POOL_ID") + cognito = get_cognito_client() + + cognito_users, pagination_token = _list_cognito_users(cognito, user_pool_id, limit, next_token, logger) + + # Batch DynamoDB display-name lookups and parallelize Cognito group lookups + # to avoid the per-user N+1 fan-out. + account_ids = [_extract_cognito_attributes(user)[1].get("sub", user.get("Username", "")) for user in cognito_users] + usernames = [user.get("Username", "") for user in cognito_users] + display_names = _batch_get_display_names(account_ids, logger) + groups_map = _batch_get_user_groups(cognito, user_pool_id, usernames, logger) + + actor_sub = _actor_sub(event) + admin_users = _build_admin_users_from_cognito(cognito_users, display_names, groups_map) + + logger.info( + "Listed users", + count=len(admin_users), + has_more=bool(pagination_token), + actor_sub=actor_sub, + ) + return {"users": admin_users, "nextToken": pagination_token} + + +def _execute_search_strategy(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: + """Execute search strategy based on query format.""" + if query.startswith("ACCOUNT#"): + return _search_by_account_prefix(query, cognito, user_pool_id, logger) + if _looks_like_uuid(query): + return _search_by_uuid(query, cognito, user_pool_id, logger) + return _search_by_general_query(query, cognito, user_pool_id, logger) + + +@with_error_handling(error_message="Failed to search user") +def admin_search_user(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Search for users by email, name, or accountId (admin only). + + AppSync Lambda resolver for adminSearchUser query. + Search strategy: + 1. If query looks like UUID or ACCOUNT#UUID, search Cognito by sub directly (single result) + 2. Otherwise (minimum `_ACCOUNT_SEARCH_MIN_QUERY_LENGTH` characters): + a. Search DynamoDB Accounts table: an email fragment uses the + emailSearchIndex GSI (prefix query); every other query (a name + fragment) scans the table, bounded by the + `_ACCOUNTS_SCAN_SAFETY_LIMIT` safety limit. Both cover logged-in + users. + b. Search Cognito with prefix matching (all users, including those who haven't logged in) + c. Merge results, deduplicate by accountId + + Args: + event: AppSync event with identity and arguments + context: Lambda context + + Returns: + List of AdminUser objects matching the query (empty list if none found) + + Raises: + AppError: If not admin, missing query, or Cognito error + """ + logger = get_logger(__name__) + + require_admin_mfa(event) + + query = str(event.get("arguments", {}).get("query", "")).strip() + if not query: + raise AppError(ErrorCode.INVALID_INPUT, "Search query is required") + + _validate_search_query(query) + + user_pool_id = get_required_env("USER_POOL_ID") + cognito = get_cognito_client() + + # Determine search strategy based on query format. + # The map values are Cognito user dicts so we can batch enrich them. + results_map = _execute_search_strategy(query, cognito, user_pool_id, logger) + cognito_users = list(results_map.values()) + + # Batch DynamoDB display-name lookups and parallelize Cognito group lookups. + account_ids = [_extract_cognito_attributes(user)[1].get("sub", user.get("Username", "")) for user in cognito_users] + usernames = [user.get("Username", "") for user in cognito_users] + display_names = _batch_get_display_names(account_ids, logger) + groups_map = _batch_get_user_groups(cognito, user_pool_id, usernames, logger) + + return _build_admin_users_from_cognito(cognito_users, display_names, groups_map) + + +def _search_by_account_prefix(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: + """Search by ACCOUNT# prefix and return Cognito-user results map.""" + sub_value = query[8:] # Remove "ACCOUNT#" + cognito_user = _search_user_by_sub(cognito, user_pool_id, sub_value, logger) + if cognito_user: + attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} + account_id = attributes.get("sub", cognito_user.get("Username", "")) + return {account_id: cognito_user} + return {} + + +def _search_by_uuid(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: + """Search by UUID (sub) and return Cognito-user results map.""" + cognito_user = _search_user_by_sub(cognito, user_pool_id, query, logger) + if cognito_user: + attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} + account_id = attributes.get("sub", cognito_user.get("Username", "")) + return {account_id: cognito_user} + return {} + + +def _add_dynamodb_users_to_map( + results_map: dict[str, Dict[str, Any]], query: str, cognito: Any, user_pool_id: str, logger: Any +) -> None: + """Search DynamoDB and add matching Cognito users to results map.""" + accounts = _search_accounts_in_dynamodb(query, logger) + for account in accounts: + account_id = account.get("accountId", "") + if account_id.startswith("ACCOUNT#"): + sub_value = account_id[8:] + cognito_user = _search_user_by_sub(cognito, user_pool_id, sub_value, logger) + if cognito_user: + attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} + key = attributes.get("sub", cognito_user.get("Username", "")) + results_map[key] = cognito_user + + +def _add_cognito_users_to_map( + results_map: dict[str, Dict[str, Any]], query: str, cognito: Any, user_pool_id: str, logger: Any +) -> None: + """Search Cognito by email and add users to results map.""" + cognito_users = _search_users_in_cognito_by_email_prefix(cognito, user_pool_id, query, logger) + for cognito_user in cognito_users: + attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} + account_id = attributes.get("sub", cognito_user.get("Username", "")) + if account_id not in results_map: + results_map[account_id] = cognito_user + + +def _search_by_general_query(query: str, cognito: Any, user_pool_id: str, logger: Any) -> dict[str, Dict[str, Any]]: + """Search DynamoDB and Cognito by general query and return merged results map.""" + results_map: dict[str, Dict[str, Any]] = {} + _add_dynamodb_users_to_map(results_map, query, cognito, user_pool_id, logger) + _add_cognito_users_to_map(results_map, query, cognito, user_pool_id, logger) + return results_map + + +def _matches_query(item: Dict[str, Any], query_lower: str) -> bool: + """Check if account item matches query (case-insensitive).""" + email = str(item.get("email", "")).lower() + given_name = str(item.get("givenName", "")).lower() + family_name = str(item.get("familyName", "")).lower() + return query_lower in email or query_lower in given_name or query_lower in family_name + + +def _filter_matching_accounts( + items: list[Dict[str, Any]], query_lower: str, matches: list[Dict[str, Any]], max_results: int +) -> None: + """Filter accounts that match the query and add to matches list.""" + for item in items: + if len(matches) >= max_results: + break + if _matches_query(item, query_lower): + matches.append(item) + + +def _scan_accounts_page(paginator_params: Dict[str, Any]) -> tuple[list[Dict[str, Any]], Dict[str, Any] | None]: + """Scan one page of accounts. Returns (items, last_evaluated_key).""" + response = tables.accounts.scan(**paginator_params) + items = response.get("Items", []) + last_key = response.get("LastEvaluatedKey") + return items, last_key + + +# Limit search results to prevent overwhelming responses. +_ACCOUNT_SEARCH_MAX_RESULTS = 50 +# Safety limit on how many account items a search scan may read. +_ACCOUNTS_SCAN_SAFETY_LIMIT = 1000 +# Shortest query an admin may search for. The only way to match a partial query +# is a full accounts-table scan, and a one or two character query can match +# most of the table while telling the admin nothing, so it is rejected outright +# instead of returning a truncated arbitrary subset (#576). +_ACCOUNT_SEARCH_MIN_QUERY_LENGTH = 3 + +# Accounts GSI for email prefix search: constant HASH key + email RANGE key, so +# begins_with(email, :prefix) is a legal key condition (#586). Declared in +# tofu/application/modules/dynamodb/main.tf; the index is sparse, so an account +# is only searchable once it carries the constant emailSearchKey attribute. +_EMAIL_SEARCH_INDEX = "emailSearchIndex" +_EMAIL_PREFIX_KEY_CONDITION = "emailSearchKey = :searchKey AND begins_with(email, :prefix)" + + +def _looks_like_email_query(query: str) -> bool: + """Check whether the query is an email fragment (it contains the local/domain separator). + + Only email-shaped queries are answered by the prefix index; a bare name + fragment (which ``_is_valid_email_prefix`` also accepts) is a substring + search, and no prefix index can answer that. + """ + return "@" in query + + +def _query_email_search_index( + query: str, query_lower: str, max_results: int, logger: Any +) -> list[Dict[str, Any]] | None: + """Answer an email prefix query with one Query against the emailSearchIndex GSI. + + That index is a constant HASH key (``emailSearchKey``) plus ``email`` as the + RANGE key, which is what makes ``begins_with(email, :prefix)`` a legal key + condition; the exact-lookup ``email-index`` is HASH-only and so can only be + matched with equality and cannot answer a partial email (#576). + + Returns the matching items (capped at ``max_results``), or None when the + index cannot answer the query -- index unavailable, or nothing indexed -- + in which case the caller falls back to the scan. The scan fallback is what + keeps accounts that predate the index (and so carry no ``emailSearchKey``) + findable until the backfill has covered every row. + """ + try: + response = tables.accounts.query( + IndexName=_EMAIL_SEARCH_INDEX, + KeyConditionExpression=_EMAIL_PREFIX_KEY_CONDITION, + ExpressionAttributeValues={":searchKey": EMAIL_SEARCH_KEY, ":prefix": query_lower}, + Limit=max_results, + ) + except ClientError as e: + logger.warning( + "DynamoDB email prefix query failed, falling back to scan", + error=str(e), + query=mask_email(query), + ) + return None + index_items = cast(list[Dict[str, Any]], response.get("Items", [])) + if not index_items: + return None + if response.get("LastEvaluatedKey"): + logger.warning( + "DynamoDB email prefix search hit the result limit; results may be truncated", + query=mask_email(query), + max_results=max_results, + ) + return index_items[:max_results] + + +def _scan_accounts_page_into( + paginator_params: Dict[str, Any], query_lower: str, matches: list[Dict[str, Any]], max_results: int +) -> tuple[int, Dict[str, Any] | None]: + """Scan one page and append its matching accounts to matches. Returns (page_size, last_key).""" + items, last_key = _scan_accounts_page(paginator_params) + _filter_matching_accounts(items, query_lower, matches, max_results) + return len(items), last_key + + +def _warn_if_accounts_scan_truncated( + scanned_count: int, last_key: Dict[str, Any] | None, query: str, logger: Any +) -> None: + """Warn when the account scan stopped at the safety limit with more pages left.""" + if scanned_count >= _ACCOUNTS_SCAN_SAFETY_LIMIT and last_key: + logger.warning( + "DynamoDB accounts search reached max scan limit; results may be truncated", + query=mask_email(query), + scanned_count=scanned_count, + ) + + +def _scan_accounts_matches(query: str, query_lower: str, max_results: int, logger: Any) -> list[Dict[str, Any]]: + """Scan the Accounts table, collecting matches until the result/scan limits are hit.""" + matches: list[Dict[str, Any]] = [] + scanned_count = 0 + paginator_params: Dict[str, Any] = {} + last_key: Dict[str, Any] | None = None + while scanned_count < _ACCOUNTS_SCAN_SAFETY_LIMIT and len(matches) < max_results: + count, last_key = _scan_accounts_page_into(paginator_params, query_lower, matches, max_results) + scanned_count += count + if not last_key: + break + paginator_params["ExclusiveStartKey"] = last_key + _warn_if_accounts_scan_truncated(scanned_count, last_key, query, logger) + return matches + + +def _search_accounts_in_dynamodb(query: str, logger: Any) -> list[Dict[str, Any]]: + """Search the accounts table for accounts matching a partial query. + + Searches email, givenName, and familyName fields. An email fragment is + answered by the emailSearchIndex GSI, whose constant HASH key plus ``email`` + RANGE key make a ``begins_with`` key condition legal: one index query, no + table scan (#586). A name fragment is a substring search that no prefix + index can answer, so it is still served by a full scan of the accounts + table, bounded by the `_ACCOUNTS_SCAN_SAFETY_LIMIT` safety limit and + truncated at `_ACCOUNT_SEARCH_MAX_RESULTS` matches. Either path falls back + to the scan when the index has no answer. + + Returns all matching accounts (up to max_results limit). + """ + query_lower = query.lower() + + try: + if _looks_like_email_query(query): + index_items = _query_email_search_index(query, query_lower, _ACCOUNT_SEARCH_MAX_RESULTS, logger) + if index_items is not None: + return index_items + return _scan_accounts_matches(query, query_lower, _ACCOUNT_SEARCH_MAX_RESULTS, logger) + except ClientError as e: + logger.warning("DynamoDB search failed", error=str(e), query=mask_email(query)) + return [] + + +def _looks_like_uuid(value: str) -> bool: + """Check if a string looks like a UUID.""" + # UUID format: 8-4-4-4-12 hex characters + uuid_pattern = r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" + return bool(re.match(uuid_pattern, value.lower())) + + +def _is_valid_email_prefix(value: str) -> bool: + """Check if a string is a valid email prefix (local part or local@partial-domain).""" + try: + cognito_user_filter("email_prefix", value) + except AppError: + return False + return True + + +def _validate_search_query(query: str) -> None: + """Validate that the admin search query is a safe UUID, email prefix, or ACCOUNT#UUID. + + A non-UUID query is served by a full accounts-table scan, so a query below + `_ACCOUNT_SEARCH_MIN_QUERY_LENGTH` is rejected rather than scanning the + table for a result that cannot narrow anything down (#576). + """ + if query.startswith("ACCOUNT#"): + if _looks_like_uuid(query[8:]): + return + elif _looks_like_uuid(query): + return + elif _is_valid_email_prefix(query): + if len(query) < _ACCOUNT_SEARCH_MIN_QUERY_LENGTH: + raise AppError( + ErrorCode.INVALID_INPUT, + f"Search query must be at least {_ACCOUNT_SEARCH_MIN_QUERY_LENGTH} characters", + ) + return + + raise AppError( + ErrorCode.INVALID_INPUT, + "Search query must be a valid UUID, email prefix, or ACCOUNT#UUID", + ) + + +def _search_user_by_sub(cognito: Any, user_pool_id: str, sub: str, logger: Any) -> Dict[str, Any] | None: + """Search for a Cognito user by sub (account ID).""" + try: + response = cognito.list_users( + UserPoolId=user_pool_id, + Filter=cognito_user_filter("sub", sub), + Limit=1, + ) + users = response.get("Users", []) + return users[0] if users else None + except ClientError as e: + error_code = e.response.get("Error", {}).get("Code", "") + if error_code in TRANSIENT_ERROR_CODES: + logger.warning("Cognito search by sub throttled", error=str(e), error_code=error_code, sub=sub) + raise AppError(ErrorCode.RESOURCE_BUSY, "Temporarily unable to load data. Please retry.") from e + logger.warning("Cognito search by sub failed", error=str(e), sub=sub) + return None + + +def _search_users_in_cognito_by_email_prefix( + cognito: Any, user_pool_id: str, query: str, logger: Any +) -> list[Dict[str, Any]]: + """ + Search Cognito users by email prefix. + + Uses Cognito's email ^= filter for prefix matching. + This finds users who haven't logged in yet (no DynamoDB record). + + Returns up to 50 matching users. + """ + try: + response = cognito.list_users( + UserPoolId=user_pool_id, + Filter=cognito_user_filter("email_prefix", query), # Prefix match on email + Limit=50, # Reasonable limit for search results + ) + return list(response.get("Users", [])) + except ClientError as e: + error_code = e.response.get("Error", {}).get("Code", "") + if error_code in TRANSIENT_ERROR_CODES: + logger.warning( + "Cognito email prefix search throttled", + error=str(e), + error_code=error_code, + query=mask_email(query), + ) + raise AppError(ErrorCode.RESOURCE_BUSY, "Temporarily unable to load data. Please retry.") from e + logger.warning("Cognito email prefix search failed", error=str(e), query=mask_email(query)) + return [] + + +def _extract_cognito_attributes(cognito_user: Dict[str, Any]) -> tuple[str, Dict[str, str]]: + """Extract username and attributes from Cognito user.""" + username = cognito_user.get("Username", "") + attributes = {attr["Name"]: attr["Value"] for attr in cognito_user.get("Attributes", [])} + return username, attributes + + +def _build_admin_users_from_cognito( + cognito_users: list[Dict[str, Any]], + display_names: dict[str, str], + groups_map: dict[str, list[str]], +) -> list[Dict[str, Any]]: + """Build AdminUser objects from Cognito users plus cached display names and groups.""" + admin_users = [] + for cognito_user in cognito_users: + username, attributes = _extract_cognito_attributes(cognito_user) + account_id = attributes.get("sub", username) + groups = groups_map.get(username, []) + created_at = cognito_user.get("UserCreateDate") + last_modified_at = cognito_user.get("UserLastModifiedDate") + admin_users.append( + { + "accountId": account_id, + "email": attributes.get("email", ""), + "displayName": display_names.get(account_id), + "status": cognito_user.get("UserStatus", "UNKNOWN"), + "enabled": cognito_user.get("Enabled", True), + "emailVerified": attributes.get("email_verified", "false").lower() == "true", + "isAdmin": "ADMIN" in groups, + "createdAt": created_at.isoformat() if created_at else None, + "lastModifiedAt": last_modified_at.isoformat() if last_modified_at else None, + } + ) + return admin_users diff --git a/tests/unit/test_admin_group_name_contract.py b/tests/unit/test_admin_group_name_contract.py index f335a2922..34d9870ee 100644 --- a/tests/unit/test_admin_group_name_contract.py +++ b/tests/unit/test_admin_group_name_contract.py @@ -41,7 +41,7 @@ "is_admin() (Python)", ), ( - "src/handlers/admin_operations.py", + "src/handlers/admin_user_operations.py", r'"isAdmin": "([^"]+)" in groups', "admin list-users (Python)", ), diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index 69d713391..d83f15c45 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -25,17 +25,19 @@ from src.handlers.admin_operations import ( _batch_get_campaign_catalogs, + _extract_unique_catalog_ids, + admin_purge_user_account, + admin_reset_user_password, + create_managed_catalog, + lambda_handler, +) +from src.handlers.admin_user_operations import ( _batch_get_display_names, _batch_get_user_groups, - _extract_unique_catalog_ids, _search_user_by_sub, _search_users_in_cognito_by_email_prefix, admin_list_users, - admin_purge_user_account, - admin_reset_user_password, admin_search_user, - create_managed_catalog, - lambda_handler, ) from src.utils.errors import AppError, ErrorCode from tests.unit.fixtures import accept_batch_deletes @@ -235,7 +237,7 @@ def test_dispatch_admin_list_users( } # Mock Cognito - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_cognito.admin_list_groups_for_user.return_value = {"Groups": []} @@ -383,9 +385,9 @@ def test_success_returns_users( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -446,9 +448,9 @@ def test_success_with_pagination( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -495,9 +497,9 @@ def test_success_no_dynamodb_account( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -544,9 +546,9 @@ def test_success_dynamodb_account_without_name( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -590,7 +592,7 @@ def test_success_empty_user_list( "arguments": {}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -615,7 +617,7 @@ def test_limit_clamped_to_max( "arguments": {"limit": 100}, # Over max } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -642,7 +644,7 @@ def test_limit_clamped_to_min( "arguments": {"limit": -5}, # Below min } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -669,7 +671,7 @@ def test_limit_explicit_none_defaults( "arguments": {"limit": None}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -742,7 +744,7 @@ def test_cognito_list_users_error( "arguments": {}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.side_effect = ClientError( {"Error": {"Code": "InternalErrorException", "Message": "Internal error"}}, @@ -772,9 +774,9 @@ def test_group_lookup_error_handled_gracefully( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -818,9 +820,9 @@ def test_multiple_users_with_mixed_admin_status( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -884,9 +886,9 @@ def test_dynamodb_lookup_error_handled_gracefully( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -930,7 +932,7 @@ def test_unexpected_exception_handled( "arguments": {}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() # Raise a generic exception mock_cognito.list_users.side_effect = RuntimeError("Unexpected error") @@ -3919,10 +3921,10 @@ def test_search_user_by_email_partial_match( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4018,10 +4020,10 @@ def test_search_user_multiple_matches( ] with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4096,10 +4098,10 @@ def test_search_user_by_name_case_insensitive( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4154,10 +4156,10 @@ def test_search_user_by_uuid_success( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables"), - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables"), + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4206,10 +4208,10 @@ def test_search_user_by_account_prefixed_uuid( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables"), - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables"), + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4247,8 +4249,8 @@ def test_search_user_not_found( "arguments": {"query": "nonexistent"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4367,10 +4369,10 @@ def test_search_user_admin_found( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4426,10 +4428,10 @@ def test_search_user_via_lambda_handler( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4473,8 +4475,8 @@ def test_search_user_by_account_prefix_not_found( "arguments": {"query": "ACCOUNT#123e4567-e89b-12d3-a456-426614174000"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables"): + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables"): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4501,8 +4503,8 @@ def test_search_user_by_uuid_not_found( "arguments": {"query": "a1b2c3d4-e5f6-7890-abcd-ef1234567890"}, # Valid UUID } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables"): + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables"): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4529,8 +4531,8 @@ def test_search_user_dynamodb_account_no_cognito_user( "arguments": {"query": "orphan"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4579,10 +4581,10 @@ def test_search_user_duplicate_deduplicated( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4637,10 +4639,10 @@ def test_search_user_cognito_only_user_added( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4676,8 +4678,8 @@ def test_search_user_malformed_account_id_skipped( "arguments": {"query": "malformed"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4726,10 +4728,10 @@ def test_search_user_dynamodb_pagination( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4769,8 +4771,8 @@ def test_search_user_unexpected_exception( "arguments": {"query": "test"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables"): + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables"): mock_get_client.side_effect = RuntimeError("Unexpected error") result = admin_search_user(event, lambda_context) @@ -4795,7 +4797,7 @@ def test_search_user_by_uuid_throttled_returns_resource_busy( "arguments": {"query": uuid_str}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_client = MagicMock() mock_get_client.return_value = mock_client mock_client.list_users.side_effect = ClientError( @@ -4824,7 +4826,7 @@ def test_search_user_by_account_id_throttled_returns_resource_busy( "arguments": {"query": "ACCOUNT#12345678-1234-1234-1234-123456789abc"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: mock_client = MagicMock() mock_get_client.return_value = mock_client mock_client.list_users.side_effect = ClientError( @@ -4854,8 +4856,8 @@ def test_search_user_general_query_sub_lookup_throttled_returns_resource_busy( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -4891,8 +4893,8 @@ def test_search_user_general_query_email_prefix_throttled_returns_resource_busy( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -5414,7 +5416,7 @@ def test_batch_get_user_groups_executor_error_handled( """A failure inside the parallel executor raises INTERNAL_ERROR.""" mock_cognito = MagicMock() - with patch("src.handlers.admin_operations.ThreadPoolExecutor") as mock_executor_cls: + with patch("src.handlers.admin_user_operations.ThreadPoolExecutor") as mock_executor_cls: mock_executor = MagicMock() mock_executor_cls.return_value = mock_executor mock_executor.__enter__ = MagicMock(return_value=mock_executor) @@ -6713,10 +6715,10 @@ def test_search_user_account_without_name( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6767,10 +6769,10 @@ def test_search_user_build_admin_user_get_item_fails( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6811,8 +6813,8 @@ def test_search_user_cognito_sub_search_fails( "arguments": {"query": "test"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6848,8 +6850,8 @@ def test_search_user_cognito_email_prefix_search_fails( "arguments": {"query": "test"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6882,10 +6884,10 @@ def test_search_user_dynamodb_items_not_matching( } with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6939,8 +6941,8 @@ def test_search_user_dynamodb_scan_client_error( "arguments": {"query": "test"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_user_operations.tables") as mock_tables: mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -6976,10 +6978,10 @@ def test_search_user_max_scan_limit_reached( large_items_batch = [{"accountId": f"ACCOUNT#sub-{i}", "email": f"test{i}@example.com"} for i in range(1001)] with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client @@ -7033,10 +7035,10 @@ def test_search_user_max_results_reached_early( ] with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.tables") as mock_tables, - patch("src.handlers.admin_operations._batch_get_display_names") as mock_batch_names, - patch("src.handlers.admin_operations._batch_get_user_groups") as mock_batch_groups, + patch("src.handlers.admin_user_operations.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_user_operations.tables") as mock_tables, + patch("src.handlers.admin_user_operations._batch_get_display_names") as mock_batch_names, + patch("src.handlers.admin_user_operations._batch_get_user_groups") as mock_batch_groups, ): mock_client = MagicMock() mock_get_client.return_value = mock_client diff --git a/tests/unit/test_admin_search_accounts_scan.py b/tests/unit/test_admin_search_accounts_scan.py index eb39f39f7..9290d90d2 100644 --- a/tests/unit/test_admin_search_accounts_scan.py +++ b/tests/unit/test_admin_search_accounts_scan.py @@ -24,7 +24,7 @@ import pytest from moto import mock_aws -from src.handlers.admin_operations import admin_search_user +from src.handlers.admin_user_operations import admin_search_user from src.utils.errors import ErrorCode from tests.unit.table_schemas import create_accounts_table_schema @@ -118,13 +118,13 @@ def search(accounts_table: Any, monkeypatch: Any) -> Any: cognito_users = [_cognito_user(item["accountId"].removeprefix("ACCOUNT#"), item["email"]) for item in ACCOUNT_ITEMS] cognito = _FakeCognito(cognito_users) - monkeypatch.setattr("src.handlers.admin_operations.tables", SimpleNamespace(accounts=accounts_table)) - monkeypatch.setattr("src.handlers.admin_operations.get_cognito_client", lambda: cognito) + monkeypatch.setattr("src.handlers.admin_user_operations.tables", SimpleNamespace(accounts=accounts_table)) + monkeypatch.setattr("src.handlers.admin_user_operations.get_cognito_client", lambda: cognito) monkeypatch.setattr( - "src.handlers.admin_operations._batch_get_display_names", + "src.handlers.admin_user_operations._batch_get_display_names", lambda account_ids, logger: {i: "Alice Anderson" for i in account_ids}, ) - monkeypatch.setattr("src.handlers.admin_operations._batch_get_user_groups", lambda *args, **kwargs: {}) + monkeypatch.setattr("src.handlers.admin_user_operations._batch_get_user_groups", lambda *args, **kwargs: {}) def call(query: str) -> Any: event = { diff --git a/tests/unit/test_admin_search_email_index.py b/tests/unit/test_admin_search_email_index.py index 680e4ecf9..ea5ed0c83 100644 --- a/tests/unit/test_admin_search_email_index.py +++ b/tests/unit/test_admin_search_email_index.py @@ -19,7 +19,7 @@ import pytest from botocore.exceptions import ClientError -from src.handlers.admin_operations import _search_accounts_in_dynamodb, admin_search_user +from src.handlers.admin_user_operations import _search_accounts_in_dynamodb, admin_search_user from src.handlers.post_authentication import lambda_handler as post_auth_lambda_handler from src.utils.dynamodb import EMAIL_SEARCH_KEY, tables from src.utils.logging import get_logger @@ -300,7 +300,7 @@ def test_admin_search_user_uses_the_index_for_a_partial_email( "Enabled": True, "UserStatus": "CONFIRMED", } - monkeypatch.setattr("src.handlers.admin_operations.get_cognito_client", lambda: _FakeCognito([cognito_user])) + monkeypatch.setattr("src.handlers.admin_user_operations.get_cognito_client", lambda: _FakeCognito([cognito_user])) dynamodb_calls.clear() results = admin_search_user(event, lambda_context) diff --git a/tests/unit/test_cognito_filter_call_sites.py b/tests/unit/test_cognito_filter_call_sites.py index 48d854033..e7929f0a6 100644 --- a/tests/unit/test_cognito_filter_call_sites.py +++ b/tests/unit/test_cognito_filter_call_sites.py @@ -14,7 +14,7 @@ import pytest -from src.handlers import account_operations, admin_operations, pre_signup +from src.handlers import account_operations, admin_operations, admin_user_operations, pre_signup from src.utils.errors import AppError, ErrorCode POOL = "us-east-1_TEST123" @@ -34,14 +34,14 @@ FILTER_SITES: List[Tuple[str, Callable[..., Any], str, str, str]] = [ ( "admin _search_user_by_sub", - admin_operations._search_user_by_sub, + admin_user_operations._search_user_by_sub, "sub", UUID, f'sub = "{UUID}"', ), ( "admin _search_users_in_cognito_by_email_prefix", - admin_operations._search_users_in_cognito_by_email_prefix, + admin_user_operations._search_users_in_cognito_by_email_prefix, "query", "user@exa", 'email ^= "user@exa"', From a4896f33736ef859778dac45ddf21b9455f05106 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 01:30:34 -0400 Subject: [PATCH 2/6] refactor(handlers): extract adminPurgeUserAccount handler into admin_account_purge Moves the whole account-purge concern (the adminPurgeUserAccount handler plus its private helpers) out of admin_operations into its own module, continuing the #773 extraction. Zero behavior change: the dispatcher map in admin_operations keeps registering the handler (the dispatcher stays the single registry), the module is imported back for that map only at module load, and every shared helper is imported inside functions to keep the load-time acyclic edge. Test patch targets and inline imports follow the code to the new module namespace; the code itself is byte-identical (verified), including the function-level import of the shared helpers from admin_operations. --- src/handlers/admin_account_purge.py | 338 +++++++++++++++++++ src/handlers/admin_operations.py | 295 +--------------- tests/unit/test_admin_operations.py | 96 +++--- tests/unit/test_cognito_filter_call_sites.py | 6 +- 4 files changed, 397 insertions(+), 338 deletions(-) create mode 100644 src/handlers/admin_account_purge.py diff --git a/src/handlers/admin_account_purge.py b/src/handlers/admin_account_purge.py new file mode 100644 index 000000000..703d02e56 --- /dev/null +++ b/src/handlers/admin_account_purge.py @@ -0,0 +1,338 @@ +""" +Admin account purge handler (extracted from admin_operations, #773). + +Provides: +- adminPurgeUserAccount: Sweep the client-unreachable residue, then delete the + accounts record and the Cognito user + +The handler stays registered on the shared admin-operations Lambda: +``admin_operations`` imports it for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +from typing import TYPE_CHECKING, Any, Dict + +from botocore.exceptions import BotoCoreError, ClientError + +# Sibling handler modules use a same-package relative import, which resolves both +# in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). +from .deletion_cascade import ( + delete_inbound_shares, + delete_invites_for_owned_profiles, + delete_user_s3_reports, + normalize_account_id, + run_deletion_steps, +) + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.appsync_types import get_caller_id + from utils.boto import get_cognito_client + from utils.cognito import is_transient_cognito_error + from utils.cognito_filters import cognito_user_filter + from utils.dynamodb import get_required_env, tables + from utils.errors import AppError, ErrorCode + from utils.logging import get_logger, mask_email + from utils.payment_methods import delete_all_user_qr_codes +except ModuleNotFoundError: # pragma: no cover + from ..utils.appsync_types import get_caller_id + from ..utils.boto import get_cognito_client + from ..utils.cognito import is_transient_cognito_error + from ..utils.cognito_filters import cognito_user_filter + from ..utils.dynamodb import get_required_env, tables + from ..utils.errors import AppError, ErrorCode + from ..utils.logging import get_logger, mask_email + from ..utils.payment_methods import delete_all_user_qr_codes + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _find_cognito_user_by_sub( + cognito: Any, user_pool_id: str, account_id: str, logger: Any +) -> tuple[str | None, str | None]: + """Find Cognito user by sub and return (username, email). + + Only an empty Cognito response indicates the user is already absent. + Lookup failures (throttling, InternalErrorException, etc.) raise an + AppError so the caller does not proceed to delete DynamoDB data while + the Cognito user still exists. + """ + raw_sub = account_id[8:] if account_id.startswith("ACCOUNT#") else account_id + try: + users_response = cognito.list_users( + UserPoolId=user_pool_id, + # Built in one step so the value that was checked is the value sent (#124, #560). + Filter=cognito_user_filter("sub", raw_sub), + Limit=1, + ) + users = users_response.get("Users", []) + if users: + username = users[0]["Username"] + attributes = {attr["Name"]: attr["Value"] for attr in users[0].get("Attributes", [])} + email = attributes.get("email", "") + return username, email + except (ClientError, BotoCoreError) as e: + # BotoCoreError is a transport fault, not a service verdict, so it + # classifies as retryable below; the lookup runs before any delete, so + # nothing was mutated and the retry converges. + logger.error("Cognito lookup by sub failed", error=str(e), account_id=account_id) + if is_transient_cognito_error(e): + raise AppError( + ErrorCode.RESOURCE_BUSY, + "Failed to look up Cognito user. Retry the purge to complete the deletion.", + ) from e + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to look up Cognito user") from e + return None, None + + +def _delete_user_from_cognito( + cognito: Any, user_pool_id: str, username: str, email: str, logger: Any, actor_sub: str = "" +) -> None: + """Delete user from Cognito, treating UserNotFoundException as idempotent success. + + Failures propagate unchanged — the caller owns failure + logging and typed-error mapping, so the real AWS error code survives to + where the context (account, actor, partial state) is added. + """ + masked_email = mask_email(email) + try: + cognito.admin_delete_user(UserPoolId=user_pool_id, Username=username) + logger.info( + "Deleted user from Cognito", + username=mask_email(username), + email=masked_email, + actor_sub=actor_sub, + ) + except ClientError as e: + error_code = e.response.get("Error", {}).get("Code", "") + if error_code == "UserNotFoundException": + logger.info( + "Cognito user already deleted", + username=mask_email(username), + email=masked_email, + actor_sub=actor_sub, + ) + return + raise + + +def _delete_cognito_user_after_sweep( + cognito: Any, + user_pool_id: str, + username: str | None, + email: str, + account_id: str, + actor_sub: str, + logger: Any, +) -> None: + """Delete the Cognito user after the data sweep, or note it is already absent. + + Failure handling is not here: ``run_deletion_steps`` owns the + partial-state report for both deletion paths, logging once with the + caller's ``log_context`` (actor attribution included) before raising the + typed error whose code matches the retry guidance in its message. + """ + if username: + _delete_user_from_cognito(cognito, user_pool_id, username, email, logger, actor_sub) + else: + logger.info("Cognito user already absent", account_id=account_id, actor_sub=actor_sub) + + +def _sweep_purge_residue(profiles_for_sweep: list[Dict[str, Any]], account_id: str, logger: Any) -> None: + """Sweep the four residue classes the browser cannot reach itself (#521).""" + delete_invites_for_owned_profiles(profiles_for_sweep, logger) + delete_inbound_shares(account_id, logger) + delete_user_s3_reports(profiles_for_sweep, logger) + delete_all_user_qr_codes(account_id, logger) + + +def _account_exists_in_dynamodb(account_id: str, logger: Any, actor_sub: str = "") -> bool: + """Check whether an account record exists in DynamoDB.""" + db_account_id = normalize_account_id(account_id) + try: + response = tables.accounts.get_item(Key={"accountId": db_account_id}, ProjectionExpression="accountId") + exists = "Item" in response + if not exists: + logger.info("Account not found in DynamoDB", account_id=db_account_id, actor_sub=actor_sub) + return exists + except ClientError as e: + logger.error("Failed to check account existence in DynamoDB", error=str(e), account_id=db_account_id) + raise + + +def _check_not_self_deletion(caller_id: str, account_id: str) -> None: + """Prevent admin from deleting their own account. + + Normalize both IDs to their canonical ACCOUNT# form before comparing so a + caller passing ``ACCOUNT#`` cannot bypass the guard (#125). + """ + if normalize_account_id(account_id) == normalize_account_id(caller_id): + raise AppError(ErrorCode.INVALID_INPUT, "Cannot delete your own account") + + +@with_error_handling(error_message="Failed to purge user account") +def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: + """ + Delete the account record and the Cognito user (admin only). + + AppSync Lambda resolver for the adminPurgeUserAccount mutation. This is the + deliberately minimal server-side operation that user deletion cannot be + completed without (#521): the browser holds no AWS credentials, so it can + neither delete the ``accounts`` table row nor call Cognito + ``AdminDeleteUser``. Everything else a browser CAN delete (orders, + campaigns, shares, profiles) is deleted by the client through the + per-entity ``adminDeleteUser*`` mutations, and catalogs are never deleted. + + The caller supplies the profile IDs it read via ``adminGetUserProfiles`` + BEFORE deleting the profile rows, as the ``profileIds`` argument. The purge + first verifies each of them is actually gone with a strongly consistent + read — a surviving profile means the client cascade has not completed, so + it refuses with CONFLICT and deletes nothing — then sweeps the four + classes the browser cannot reach, reusing the shared deletion_cascade + helpers: invites for the account's profiles, inbound shares on other + owners' profiles, S3 report objects, and payment-QR S3 objects. Invites + and reports are keyed by profileId, which survives the deleted profile + rows. + + Call this after the client's per-entity deletes, so a failure there leaves + the account intact rather than half-deleted. + + Args: + event: AppSync event with identity and arguments + context: Lambda context + + Returns: + True if the account was purged successfully + + Raises: + AppError: If not admin, self-purge is attempted, the user does not + exist, a supplied profile still exists (CONFLICT), or a deletion + error occurs. An absent Cognito user is treated as idempotent + success. When the sweep succeeded but the Cognito delete failed, + the error names the partial state so the operator knows the + account needs a retry to complete. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + profile_ids = _validate_profile_ids_argument(event.get("arguments", {}).get("profileIds")) + + # get_caller_id reads the raw `identity.sub` (no normalization), so this + # guard's decision is unchanged from the inline read it replaced; it only + # hardens a malformed event to None instead of raising (#660). + caller_id = get_caller_id(event) + _check_not_self_deletion(str(caller_id), account_id) + + user_pool_id = get_required_env("USER_POOL_ID") + cognito = get_cognito_client() + + username, email = _find_cognito_user_by_sub(cognito, user_pool_id, account_id, logger) + account_exists = _account_exists_in_dynamodb(account_id, logger, actor_sub) + + if not username and not account_exists: + raise AppError(ErrorCode.NOT_FOUND, f"User not found: {account_id}") + + # The purge runs after the client's per-entity deletes (#521). It verifies + # rather than trusts: while any supplied profile still exists the cascade + # has not completed, so nothing is deleted and the account stays intact. + _assert_profiles_deleted(account_id, profile_ids, logger, actor_sub) + + profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] + # The sweep runs before the Cognito delete and before the accounts row, so + # a retry after any failure below converges. Both phases and their failure + # classification live in the shared helper so the two deletion paths report + # them identically; ``log_context`` carries the actor attribution every + # admin audit line must have (#507). + run_deletion_steps( + sweep=lambda: _sweep_purge_residue(profiles_for_sweep, account_id, logger), + delete_cognito=lambda: _delete_cognito_user_after_sweep( + cognito, user_pool_id, username, email or "", account_id, actor_sub, logger + ), + logger=logger, + log_context={"account_id": account_id, "actor_sub": actor_sub}, + sweep_log_message=( + "Data sweep failed before the Cognito delete; the Cognito user and the accounts record are untouched" + ), + sweep_retry_message=( + "Data sweep failed before the Cognito delete; the Cognito user and the " + "accounts record are untouched. Retry the purge to complete the deletion." + ), + sweep_fatal_message=( + "Data sweep failed before the Cognito delete; the Cognito user and the accounts record are untouched." + ), + partial_retry_message=( + "Account data was swept but the Cognito user could not be confirmed deleted; " + "the accounts record still exists. Retry the purge to complete the deletion." + ), + partial_fatal_message=( + "Account data was swept but the Cognito user could not be confirmed deleted; " + "the accounts record still exists and the Cognito user may also survive — " + "complete the deletion manually." + ), + ) + + if account_exists: + db_account_id = normalize_account_id(account_id) + try: + tables.accounts.delete_item(Key={"accountId": db_account_id}) + logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) + except ClientError as e: + logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e + else: + logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) + + logger.info("User account purged", account_id=account_id, email=mask_email(email), actor_sub=actor_sub) + return True + + +def _validate_profile_ids_argument(profile_ids: Any) -> list[str]: + """Validate the client-supplied profileIds list for the purge (#521). + + The schema requires ``[ID!]!``; this guards the Lambda entry point itself, + which a caller can reach without going through GraphQL validation. + """ + if not isinstance(profile_ids, list) or not all( + isinstance(profile_id, str) and profile_id.strip() for profile_id in profile_ids + ): + raise AppError(ErrorCode.INVALID_INPUT, "profileIds must be a list of profile IDs") + return profile_ids + + +def _assert_profiles_deleted(account_id: str, profile_ids: list[str], logger: Any, actor_sub: str = "") -> None: + """Refuse the purge while any client-reported-deleted profile still exists (#521). + + The strongly consistent read is required because the profile rows were + just deleted by a different writer (the client's cascade moments earlier). + The import is function-level: a top-level import would be circular + (`admin_operations` imports this module for its operation dispatcher map). + """ + from .admin_operations import _raise_batch_lookup_error + + db_account_id = normalize_account_id(account_id) + for profile_id in profile_ids: + try: + response = tables.profiles.get_item( + Key={"ownerAccountId": db_account_id, "profileId": profile_id}, + ConsistentRead=True, + ) + except ClientError as e: + _raise_batch_lookup_error("verify profile deletion", logger, e, profile_id=profile_id) + if "Item" in response: + logger.warning("Purge refused: profile still present", profile_id=profile_id, actor_sub=actor_sub) + raise AppError( + ErrorCode.CONFLICT, + "The account's profiles must be deleted before the account can be purged", + ) diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 0d0088ebe..e263f87f0 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -3,12 +3,11 @@ Provides: - adminResetUserPassword: Send password reset email to user -- adminPurgeUserAccount: Sweep the client-unreachable residue, then delete the - accounts record and the Cognito user - createManagedCatalog: Create an ADMIN_MANAGED global catalog The adminListUsers / adminSearchUser handlers live in ``admin_user_operations`` -(#773) and are imported below so the dispatcher map stays the single registry. +and the adminPurgeUserAccount handler lives in ``admin_account_purge`` (#773); +both are imported below so the dispatcher map stays the single registry. User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs @@ -34,21 +33,17 @@ from decimal import Decimal, InvalidOperation from typing import TYPE_CHECKING, Any, Dict, NoReturn, Optional, cast -from botocore.exceptions import BotoCoreError, ClientError +from botocore.exceptions import ClientError # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). from .deletion_cascade import ( - delete_inbound_shares, - delete_invites_for_owned_profiles, delete_user_campaigns, delete_user_orders, delete_user_profiles, - delete_user_s3_reports, delete_user_shares, get_user_profiles, normalize_account_id, - run_deletion_steps, ) # Handle both Lambda (absolute) and unit test (relative) imports @@ -56,7 +51,6 @@ from utils.appsync_types import get_caller_id from utils.auth import require_admin_mfa from utils.boto import get_cognito_client - from utils.cognito import is_transient_cognito_error from utils.cognito_filters import cognito_user_filter from utils.dynamodb import ( TRANSIENT_ERROR_CODES, @@ -66,12 +60,10 @@ ) from utils.errors import AppError, ErrorCode from utils.logging import get_logger, mask_email - from utils.payment_methods import delete_all_user_qr_codes except ModuleNotFoundError: # pragma: no cover from ..utils.appsync_types import get_caller_id from ..utils.auth import require_admin_mfa from ..utils.boto import get_cognito_client - from ..utils.cognito import is_transient_cognito_error from ..utils.cognito_filters import cognito_user_filter from ..utils.dynamodb import ( TRANSIENT_ERROR_CODES, @@ -81,7 +73,6 @@ ) from ..utils.errors import AppError, ErrorCode from ..utils.logging import get_logger, mask_email - from ..utils.payment_methods import delete_all_user_qr_codes # The decorator stays typed for mypy via the relative import below; at runtime # the absolute import resolves in the Lambda zip (package `utils`) and the @@ -95,6 +86,11 @@ from ..utils.handlers import lambda_handler as with_error_handling +# The account purge handler was extracted to its own module (#773) but stays +# registered on this Lambda. The import is safe at module load because +# `admin_account_purge` only imports from this module inside functions. +from .admin_account_purge import admin_purge_user_account + # The user listing & search handlers were extracted to their own module (#773) but # stay registered on this Lambda. The import is safe at module load because # `admin_user_operations` only imports from this module inside functions. @@ -228,119 +224,6 @@ def _require_admin_and_get_target_account_id(event: Dict[str, Any]) -> tuple[str return account_id, actor_sub -def _find_cognito_user_by_sub( - cognito: Any, user_pool_id: str, account_id: str, logger: Any -) -> tuple[str | None, str | None]: - """Find Cognito user by sub and return (username, email). - - Only an empty Cognito response indicates the user is already absent. - Lookup failures (throttling, InternalErrorException, etc.) raise an - AppError so the caller does not proceed to delete DynamoDB data while - the Cognito user still exists. - """ - raw_sub = account_id[8:] if account_id.startswith("ACCOUNT#") else account_id - try: - users_response = cognito.list_users( - UserPoolId=user_pool_id, - # Built in one step so the value that was checked is the value sent (#124, #560). - Filter=cognito_user_filter("sub", raw_sub), - Limit=1, - ) - users = users_response.get("Users", []) - if users: - username = users[0]["Username"] - attributes = {attr["Name"]: attr["Value"] for attr in users[0].get("Attributes", [])} - email = attributes.get("email", "") - return username, email - except (ClientError, BotoCoreError) as e: - # BotoCoreError is a transport fault, not a service verdict, so it - # classifies as retryable below; the lookup runs before any delete, so - # nothing was mutated and the retry converges. - logger.error("Cognito lookup by sub failed", error=str(e), account_id=account_id) - if is_transient_cognito_error(e): - raise AppError( - ErrorCode.RESOURCE_BUSY, - "Failed to look up Cognito user. Retry the purge to complete the deletion.", - ) from e - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to look up Cognito user") from e - return None, None - - -def _delete_user_from_cognito( - cognito: Any, user_pool_id: str, username: str, email: str, logger: Any, actor_sub: str = "" -) -> None: - """Delete user from Cognito, treating UserNotFoundException as idempotent success. - - Failures propagate unchanged — the caller owns failure - logging and typed-error mapping, so the real AWS error code survives to - where the context (account, actor, partial state) is added. - """ - masked_email = mask_email(email) - try: - cognito.admin_delete_user(UserPoolId=user_pool_id, Username=username) - logger.info( - "Deleted user from Cognito", - username=mask_email(username), - email=masked_email, - actor_sub=actor_sub, - ) - except ClientError as e: - error_code = e.response.get("Error", {}).get("Code", "") - if error_code == "UserNotFoundException": - logger.info( - "Cognito user already deleted", - username=mask_email(username), - email=masked_email, - actor_sub=actor_sub, - ) - return - raise - - -def _delete_cognito_user_after_sweep( - cognito: Any, - user_pool_id: str, - username: str | None, - email: str, - account_id: str, - actor_sub: str, - logger: Any, -) -> None: - """Delete the Cognito user after the data sweep, or note it is already absent. - - Failure handling is not here: ``run_deletion_steps`` owns the - partial-state report for both deletion paths, logging once with the - caller's ``log_context`` (actor attribution included) before raising the - typed error whose code matches the retry guidance in its message. - """ - if username: - _delete_user_from_cognito(cognito, user_pool_id, username, email, logger, actor_sub) - else: - logger.info("Cognito user already absent", account_id=account_id, actor_sub=actor_sub) - - -def _sweep_purge_residue(profiles_for_sweep: list[Dict[str, Any]], account_id: str, logger: Any) -> None: - """Sweep the four residue classes the browser cannot reach itself (#521).""" - delete_invites_for_owned_profiles(profiles_for_sweep, logger) - delete_inbound_shares(account_id, logger) - delete_user_s3_reports(profiles_for_sweep, logger) - delete_all_user_qr_codes(account_id, logger) - - -def _account_exists_in_dynamodb(account_id: str, logger: Any, actor_sub: str = "") -> bool: - """Check whether an account record exists in DynamoDB.""" - db_account_id = normalize_account_id(account_id) - try: - response = tables.accounts.get_item(Key={"accountId": db_account_id}, ProjectionExpression="accountId") - exists = "Item" in response - if not exists: - logger.info("Account not found in DynamoDB", account_id=db_account_id, actor_sub=actor_sub) - return exists - except ClientError as e: - logger.error("Failed to check account existence in DynamoDB", error=str(e), account_id=db_account_id) - raise - - def _find_user_by_email(cognito: Any, user_pool_id: str, email: str, logger: Any) -> str: """Find username by email. Returns username.""" try: @@ -423,168 +306,6 @@ def admin_reset_user_password(event: Dict[str, Any], context: Any) -> bool: return True -def _check_not_self_deletion(caller_id: str, account_id: str) -> None: - """Prevent admin from deleting their own account. - - Normalize both IDs to their canonical ACCOUNT# form before comparing so a - caller passing ``ACCOUNT#`` cannot bypass the guard (#125). - """ - if normalize_account_id(account_id) == normalize_account_id(caller_id): - raise AppError(ErrorCode.INVALID_INPUT, "Cannot delete your own account") - - -@with_error_handling(error_message="Failed to purge user account") -def admin_purge_user_account(event: Dict[str, Any], context: Any) -> bool: - """ - Delete the account record and the Cognito user (admin only). - - AppSync Lambda resolver for the adminPurgeUserAccount mutation. This is the - deliberately minimal server-side operation that user deletion cannot be - completed without (#521): the browser holds no AWS credentials, so it can - neither delete the ``accounts`` table row nor call Cognito - ``AdminDeleteUser``. Everything else a browser CAN delete (orders, - campaigns, shares, profiles) is deleted by the client through the - per-entity ``adminDeleteUser*`` mutations, and catalogs are never deleted. - - The caller supplies the profile IDs it read via ``adminGetUserProfiles`` - BEFORE deleting the profile rows, as the ``profileIds`` argument. The purge - first verifies each of them is actually gone with a strongly consistent - read — a surviving profile means the client cascade has not completed, so - it refuses with CONFLICT and deletes nothing — then sweeps the four - classes the browser cannot reach, reusing the shared deletion_cascade - helpers: invites for the account's profiles, inbound shares on other - owners' profiles, S3 report objects, and payment-QR S3 objects. Invites - and reports are keyed by profileId, which survives the deleted profile - rows. - - Call this after the client's per-entity deletes, so a failure there leaves - the account intact rather than half-deleted. - - Args: - event: AppSync event with identity and arguments - context: Lambda context - - Returns: - True if the account was purged successfully - - Raises: - AppError: If not admin, self-purge is attempted, the user does not - exist, a supplied profile still exists (CONFLICT), or a deletion - error occurs. An absent Cognito user is treated as idempotent - success. When the sweep succeeded but the Cognito delete failed, - the error names the partial state so the operator knows the - account needs a retry to complete. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - profile_ids = _validate_profile_ids_argument(event.get("arguments", {}).get("profileIds")) - - # get_caller_id reads the raw `identity.sub` (no normalization), so this - # guard's decision is unchanged from the inline read it replaced; it only - # hardens a malformed event to None instead of raising (#660). - caller_id = get_caller_id(event) - _check_not_self_deletion(str(caller_id), account_id) - - user_pool_id = get_required_env("USER_POOL_ID") - cognito = get_cognito_client() - - username, email = _find_cognito_user_by_sub(cognito, user_pool_id, account_id, logger) - account_exists = _account_exists_in_dynamodb(account_id, logger, actor_sub) - - if not username and not account_exists: - raise AppError(ErrorCode.NOT_FOUND, f"User not found: {account_id}") - - # The purge runs after the client's per-entity deletes (#521). It verifies - # rather than trusts: while any supplied profile still exists the cascade - # has not completed, so nothing is deleted and the account stays intact. - _assert_profiles_deleted(account_id, profile_ids, logger, actor_sub) - - profiles_for_sweep = [{"profileId": profile_id} for profile_id in profile_ids] - # The sweep runs before the Cognito delete and before the accounts row, so - # a retry after any failure below converges. Both phases and their failure - # classification live in the shared helper so the two deletion paths report - # them identically; ``log_context`` carries the actor attribution every - # admin audit line must have (#507). - run_deletion_steps( - sweep=lambda: _sweep_purge_residue(profiles_for_sweep, account_id, logger), - delete_cognito=lambda: _delete_cognito_user_after_sweep( - cognito, user_pool_id, username, email or "", account_id, actor_sub, logger - ), - logger=logger, - log_context={"account_id": account_id, "actor_sub": actor_sub}, - sweep_log_message=( - "Data sweep failed before the Cognito delete; the Cognito user and the accounts record are untouched" - ), - sweep_retry_message=( - "Data sweep failed before the Cognito delete; the Cognito user and the " - "accounts record are untouched. Retry the purge to complete the deletion." - ), - sweep_fatal_message=( - "Data sweep failed before the Cognito delete; the Cognito user and the accounts record are untouched." - ), - partial_retry_message=( - "Account data was swept but the Cognito user could not be confirmed deleted; " - "the accounts record still exists. Retry the purge to complete the deletion." - ), - partial_fatal_message=( - "Account data was swept but the Cognito user could not be confirmed deleted; " - "the accounts record still exists and the Cognito user may also survive — " - "complete the deletion manually." - ), - ) - - if account_exists: - db_account_id = normalize_account_id(account_id) - try: - tables.accounts.delete_item(Key={"accountId": db_account_id}) - logger.info("Deleted account record", account_id=db_account_id, actor_sub=actor_sub) - except ClientError as e: - logger.error("Failed to delete account record", error=str(e), account_id=db_account_id) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to delete account record") from e - else: - logger.info("Account record already absent", account_id=account_id, actor_sub=actor_sub) - - logger.info("User account purged", account_id=account_id, email=mask_email(email), actor_sub=actor_sub) - return True - - -def _validate_profile_ids_argument(profile_ids: Any) -> list[str]: - """Validate the client-supplied profileIds list for the purge (#521). - - The schema requires ``[ID!]!``; this guards the Lambda entry point itself, - which a caller can reach without going through GraphQL validation. - """ - if not isinstance(profile_ids, list) or not all( - isinstance(profile_id, str) and profile_id.strip() for profile_id in profile_ids - ): - raise AppError(ErrorCode.INVALID_INPUT, "profileIds must be a list of profile IDs") - return profile_ids - - -def _assert_profiles_deleted(account_id: str, profile_ids: list[str], logger: Any, actor_sub: str = "") -> None: - """Refuse the purge while any client-reported-deleted profile still exists (#521). - - The strongly consistent read is required because the profile rows were - just deleted by a different writer (the client's cascade moments earlier). - """ - db_account_id = normalize_account_id(account_id) - for profile_id in profile_ids: - try: - response = tables.profiles.get_item( - Key={"ownerAccountId": db_account_id, "profileId": profile_id}, - ConsistentRead=True, - ) - except ClientError as e: - _raise_batch_lookup_error("verify profile deletion", logger, e, profile_id=profile_id) - if "Item" in response: - logger.warning("Purge refused: profile still present", profile_id=profile_id, actor_sub=actor_sub) - raise AppError( - ErrorCode.CONFLICT, - "The account's profiles must be deleted before the account can be purged", - ) - - def _parse_product_price(price: Any) -> Decimal: """Convert a product price (number or numeric string) to a validated Decimal. diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index d83f15c45..288bb32d2 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -23,10 +23,10 @@ import pytest from botocore.exceptions import BotoCoreError, ClientError, EndpointConnectionError +from src.handlers.admin_account_purge import admin_purge_user_account from src.handlers.admin_operations import ( _batch_get_campaign_catalogs, _extract_unique_catalog_ids, - admin_purge_user_account, admin_reset_user_password, create_managed_catalog, lambda_handler, @@ -182,7 +182,7 @@ def test_dispatch_admin_purge_user_account( "arguments": {"accountId": target_account_id, "profileIds": []}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": target_account_id}]} mock_cognito.admin_delete_user.return_value = {} @@ -1605,7 +1605,7 @@ def test_deletes_account_record_and_cognito_user( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -1631,11 +1631,11 @@ def test_does_not_touch_profiles_campaigns_or_catalogs( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_account_purge.tables") as mock_tables: mock_tables.accounts.get_item.return_value = {"Item": {"accountId": "ACCOUNT#target-user-123"}} mock_tables.profiles.get_item.return_value = {} result = admin_purge_user_account(event, lambda_context) @@ -1680,7 +1680,7 @@ def test_catalog_survives_a_purge( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -1716,7 +1716,7 @@ def test_refuses_while_a_profile_remains( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -1745,7 +1745,7 @@ def test_succeeds_once_the_profiles_are_gone( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -1771,7 +1771,7 @@ def test_cognito_user_already_gone_is_idempotent( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = ClientError( {"Error": {"Code": "UserNotFoundException", "Message": "User does not exist."}}, @@ -1800,7 +1800,7 @@ def test_cognito_delete_failure_aborts_the_purge( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = ClientError( {"Error": {"Code": "NotAuthorizedException", "Message": "Access denied."}}, @@ -1839,7 +1839,7 @@ def test_purge_cognito_throttle_is_retryable_and_attributes_the_actor( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = ClientError( {"Error": {"Code": "TooManyRequestsException", "Message": "Rate exceeded."}}, @@ -1882,7 +1882,7 @@ def test_purge_cognito_failure_logs_the_actor_exactly_once( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = ClientError( {"Error": {"Code": "NotAuthorizedException", "Message": "Access denied."}}, @@ -1919,7 +1919,7 @@ def test_purge_cognito_network_fault_keeps_the_partial_state_narration( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = EndpointConnectionError(endpoint_url="cognito") mock_get_client.return_value = mock_cognito @@ -1959,8 +1959,8 @@ def test_purge_sweep_apperror_is_attributed_and_re_raised_unchanged( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.delete_all_user_qr_codes") as mock_qr, + patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_account_purge.delete_all_user_qr_codes") as mock_qr, ): mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -2003,8 +2003,8 @@ def test_purge_sweep_throttle_is_retryable_and_attributes_the_actor( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.delete_user_s3_reports") as mock_reports, + patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_account_purge.delete_user_s3_reports") as mock_reports, ): mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -2048,8 +2048,8 @@ def test_purge_non_transient_sweep_failure_is_permanent_and_attributed( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} with ( - patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client, - patch("src.handlers.admin_operations.delete_user_s3_reports") as mock_reports, + patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client, + patch("src.handlers.admin_account_purge.delete_user_s3_reports") as mock_reports, ): mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -2094,7 +2094,7 @@ def test_purge_sweeps_invites_for_the_supplied_profiles( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_get_client.return_value = self._mock_cognito(target_account_id) assert admin_purge_user_account(event, lambda_context) is True @@ -2123,7 +2123,7 @@ def test_purge_sweeps_inbound_shares( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_get_client.return_value = self._mock_cognito(target_account_id) assert admin_purge_user_account(event, lambda_context) is True @@ -2165,7 +2165,7 @@ def test_cognito_failure_after_sweep_then_retry_completes( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_cognito.admin_delete_user.side_effect = [ ClientError({"Error": {"Code": "InternalError", "Message": "Internal error"}}, "AdminDeleteUser"), @@ -2215,12 +2215,12 @@ def test_data_sweep_failure_leaves_cognito_user_intact( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito with patch( - "src.handlers.admin_operations.delete_invites_for_owned_profiles", + "src.handlers.admin_account_purge.delete_invites_for_owned_profiles", side_effect=ClientError( {"Error": {"Code": "InternalServerError", "Message": "DynamoDB exploded"}}, "DeleteItem", @@ -2265,7 +2265,7 @@ def test_purge_deletes_s3_reports_for_the_supplied_profiles( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_get_client.return_value = self._mock_cognito(target_account_id) assert admin_purge_user_account(event, lambda_context) is True @@ -2302,7 +2302,7 @@ def test_purge_deletes_payment_qr_codes( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_get_client.return_value = self._mock_cognito(target_account_id) assert admin_purge_user_account(event, lambda_context) is True @@ -2331,7 +2331,7 @@ def test_rejects_malformed_profile_ids( event = {**admin_appsync_event, "arguments": {"accountId": "target-user-123", "profileIds": bad_profile_ids}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_get_client.return_value = mock_cognito @@ -2357,11 +2357,11 @@ def test_profile_verification_throttling_returns_resource_busy( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": ["PROFILE#p1"]}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_account_purge.tables") as mock_tables: mock_tables.accounts.get_item.return_value = {"Item": {"accountId": "ACCOUNT#target-user-123"}} mock_tables.profiles.get_item.side_effect = ClientError( {"Error": {"Code": "ProvisionedThroughputExceededException"}}, "GetItem" @@ -2387,7 +2387,7 @@ def test_absent_account_record_still_deletes_cognito_user( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito @@ -2412,7 +2412,7 @@ def test_self_purge_rejected( "arguments": {"accountId": admin_appsync_event["identity"]["sub"], "profileIds": []}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_get_client.return_value = MagicMock() result = admin_purge_user_account(event, lambda_context) @@ -2432,7 +2432,7 @@ def test_not_found_when_no_cognito_user_and_no_account( event = {**admin_appsync_event, "arguments": {"accountId": "ghost-user", "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -2459,7 +2459,7 @@ def test_absent_cognito_user_still_deletes_account_record( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -2484,11 +2484,11 @@ def test_account_delete_failure_returns_internal_error( event = {**admin_appsync_event, "arguments": {"accountId": target_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = self._mock_cognito(target_account_id) mock_get_client.return_value = mock_cognito - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_account_purge.tables") as mock_tables: mock_tables.accounts.get_item.return_value = {"Item": {"accountId": "ACCOUNT#target-user-123"}} mock_tables.profiles.get_item.return_value = {} mock_tables.accounts.delete_item.side_effect = ClientError( @@ -2513,7 +2513,7 @@ def test_unsafe_account_id_rejected_before_cognito( event = {**admin_appsync_event, "arguments": {"accountId": bad_account_id, "profileIds": []}} - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_account_purge.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_get_client.return_value = mock_cognito @@ -3716,7 +3716,7 @@ def test_delete_inbound_shares_no_shares( def test_find_cognito_user_by_sub_unprefixed(self) -> None: """_find_cognito_user_by_sub queries Cognito with raw sub.""" - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -3745,7 +3745,7 @@ def test_find_cognito_user_by_sub_unprefixed(self) -> None: def test_find_cognito_user_by_sub_prefixed(self) -> None: """_find_cognito_user_by_sub strips ACCOUNT# prefix before querying Cognito.""" - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.return_value = { @@ -3774,7 +3774,7 @@ def test_find_cognito_user_by_sub_prefixed(self) -> None: def test_find_cognito_user_by_sub_not_found(self) -> None: """_find_cognito_user_by_sub returns (None, None) when user not found.""" - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} @@ -3788,7 +3788,7 @@ def test_find_cognito_user_by_sub_not_found(self) -> None: def test_find_cognito_user_by_sub_client_error(self) -> None: """A non-retryable lookup failure surfaces as INTERNAL_ERROR.""" - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.side_effect = ClientError( @@ -3806,7 +3806,7 @@ def test_find_cognito_user_by_sub_client_error(self) -> None: def test_find_cognito_user_by_sub_throttle_is_retryable(self) -> None: """A throttled lookup is retryable RESOURCE_BUSY with a retry-promise message.""" - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.side_effect = ClientError( @@ -3828,7 +3828,7 @@ def test_find_cognito_user_by_sub_transport_fault_is_retryable(self) -> None: The lookup runs before the sweep and before the Cognito delete, so a transport fault here has mutated nothing and a retry converges. """ - from src.handlers.admin_operations import _find_cognito_user_by_sub + from src.handlers.admin_account_purge import _find_cognito_user_by_sub mock_cognito = MagicMock() mock_cognito.list_users.side_effect = BotoCoreError() @@ -3843,7 +3843,7 @@ def test_find_cognito_user_by_sub_transport_fault_is_retryable(self) -> None: def test_account_exists_in_dynamodb_unprefixed(self, dynamodb_table: Any) -> None: """_account_exists_in_dynamodb normalizes unprefixed account ID.""" - from src.handlers.admin_operations import _account_exists_in_dynamodb + from src.handlers.admin_account_purge import _account_exists_in_dynamodb accounts_table = get_accounts_table() target_account_id = "11111111-1111-1111-1111-111111111111" @@ -3858,7 +3858,7 @@ def test_account_exists_in_dynamodb_unprefixed(self, dynamodb_table: Any) -> Non def test_account_exists_in_dynamodb_prefixed(self, dynamodb_table: Any) -> None: """_account_exists_in_dynamodb normalizes ACCOUNT#-prefixed account ID.""" - from src.handlers.admin_operations import _account_exists_in_dynamodb + from src.handlers.admin_account_purge import _account_exists_in_dynamodb accounts_table = get_accounts_table() target_account_id = "11111111-1111-1111-1111-111111111111" @@ -3873,15 +3873,15 @@ def test_account_exists_in_dynamodb_prefixed(self, dynamodb_table: Any) -> None: def test_account_exists_in_dynamodb_not_found(self, dynamodb_table: Any) -> None: """_account_exists_in_dynamodb returns False when account not in DynamoDB.""" - from src.handlers.admin_operations import _account_exists_in_dynamodb + from src.handlers.admin_account_purge import _account_exists_in_dynamodb assert _account_exists_in_dynamodb("non-existent-user", MagicMock()) is False def test_account_exists_in_dynamodb_client_error(self) -> None: """_account_exists_in_dynamodb propagates ClientError.""" - from src.handlers.admin_operations import _account_exists_in_dynamodb + from src.handlers.admin_account_purge import _account_exists_in_dynamodb - with patch("src.handlers.admin_operations.tables.accounts.get_item") as mock_get: + with patch("src.handlers.admin_account_purge.tables.accounts.get_item") as mock_get: mock_get.side_effect = ClientError( {"Error": {"Code": "ResourceNotFoundException", "Message": "Table not found"}}, "GetItem", @@ -7199,7 +7199,7 @@ def test_purge_records_the_actor_on_every_deletion_line( mock_cognito.list_users.return_value = {"Users": [{"Username": target_account_id}]} mock_cognito.admin_delete_user.return_value = {} - with patch("src.handlers.admin_operations.get_cognito_client", return_value=mock_cognito): + with patch("src.handlers.admin_account_purge.get_cognito_client", return_value=mock_cognito): assert admin_purge_user_account(event, lambda_context) is True for message in ("Deleted user from Cognito", "Deleted account record", "User account purged"): diff --git a/tests/unit/test_cognito_filter_call_sites.py b/tests/unit/test_cognito_filter_call_sites.py index e7929f0a6..3f12bd49d 100644 --- a/tests/unit/test_cognito_filter_call_sites.py +++ b/tests/unit/test_cognito_filter_call_sites.py @@ -14,7 +14,7 @@ import pytest -from src.handlers import account_operations, admin_operations, admin_user_operations, pre_signup +from src.handlers import account_operations, admin_account_purge, admin_operations, admin_user_operations, pre_signup from src.utils.errors import AppError, ErrorCode POOL = "us-east-1_TEST123" @@ -48,7 +48,7 @@ ), ( "admin _find_cognito_user_by_sub", - admin_operations._find_cognito_user_by_sub, + admin_account_purge._find_cognito_user_by_sub, "account_id", f"ACCOUNT#{UUID}", f'sub = "{UUID}"', @@ -183,7 +183,7 @@ def test_find_cognito_user_by_sub_resolves_not_found_for_a_safe_non_uuid() -> No cognito = MagicMock() cognito.list_users.return_value = {"Users": []} - result: Optional[Tuple[None, None]] = admin_operations._find_cognito_user_by_sub( + result: Optional[Tuple[None, None]] = admin_account_purge._find_cognito_user_by_sub( cognito, POOL, "ACCOUNT#definitely-not-a-uuid", MagicMock() ) From aa953239b94fee1d0e288431d49c6393e3ef51e8 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 02:06:05 -0400 Subject: [PATCH 3/6] refactor(handlers): extract createManagedCatalog handler into admin_catalog_operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slice 3 of the #773 admin_operations split: move the managed-catalog creation concern (the createManagedCatalog handler plus its private validation/build/persist helpers) from admin_operations into its own module src/handlers/admin_catalog_operations.py, with zero behavior change. - The seven functions (_parse_product_price, _validate_and_process_product, _validate_catalog_input, _build_catalog_item, _persist_catalog, _require_admin_and_get_actor_sub, create_managed_catalog) move verbatim; the only textual difference is the file-boundary newline, because create_managed_catalog is now the module's last function. - This is the cleanest of the three extractions: every private helper it uses, including _require_admin_and_get_actor_sub, is used only by the createManagedCatalog handler, so the whole concern travels to the new module with no function-level import from admin_operations at all. - The dispatcher map in admin_operations stays the single registry: it imports create_managed_catalog back at module load (safe — the new module imports nothing from this module), and the AppSync handler mapping plus the Lambda entry point are unchanged. No IaC change: the Lambda payload ships the whole src/ tree. - admin_operations drops its now-unused `import uuid` and the `Decimal`/`InvalidOperation` imports. - Tests: the create_managed_catalog import in test_admin_operations.py is repointed to the new module, and the two `tables` patch targets in TestCreateManagedCatalog follow the code to src.handlers.admin_catalog_operations; no test logic changed. Verified: uv run pytest tests/unit -q (1841 passed, 2 skipped, 100% coverage), ruff check src/ tests/, ruff format --check src/, mypy src/ (strict), xenon A/B complexity gate, cspell on the changed files, and tofu validate (dev). --- src/handlers/admin_catalog_operations.py | 194 +++++++++++++++++++++++ src/handlers/admin_operations.py | 166 ++----------------- tests/unit/test_admin_operations.py | 6 +- 3 files changed, 206 insertions(+), 160 deletions(-) create mode 100644 src/handlers/admin_catalog_operations.py diff --git a/src/handlers/admin_catalog_operations.py b/src/handlers/admin_catalog_operations.py new file mode 100644 index 000000000..65d11f2fd --- /dev/null +++ b/src/handlers/admin_catalog_operations.py @@ -0,0 +1,194 @@ +""" +Admin managed-catalog handler (extracted from admin_operations, #773). + +Provides: +- createManagedCatalog: Create an ADMIN_MANAGED global catalog + +The handler stays registered on the shared admin-operations Lambda: +``admin_operations`` imports it for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +import uuid +from datetime import datetime, timezone +from decimal import Decimal, InvalidOperation +from typing import TYPE_CHECKING, Any, Dict + +from botocore.exceptions import ClientError + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.appsync_types import get_caller_id + from utils.auth import require_admin_mfa + from utils.dynamodb import tables + from utils.errors import AppError, ErrorCode + from utils.logging import get_logger +except ModuleNotFoundError: # pragma: no cover + from ..utils.appsync_types import get_caller_id + from ..utils.auth import require_admin_mfa + from ..utils.dynamodb import tables + from ..utils.errors import AppError, ErrorCode + from ..utils.logging import get_logger + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _parse_product_price(price: Any) -> Decimal: + """Convert a product price (number or numeric string) to a validated Decimal. + + A missing price, a non-numeric/unparsable value, or a negative price + raises INVALID_INPUT. + """ + if price is None: + raise AppError(ErrorCode.INVALID_INPUT, "Valid product price is required") + + try: + price_decimal = Decimal(str(price)) + except InvalidOperation, ValueError, TypeError: + raise AppError(ErrorCode.INVALID_INPUT, "Product price must be a valid number") + + if price_decimal < 0: + raise AppError(ErrorCode.INVALID_INPUT, "Valid product price is required") + return price_decimal + + +def _validate_and_process_product(product: Dict[str, Any]) -> Dict[str, Any]: + """Validate and process a catalog product, returning processed product dict. + + Price may arrive as a number or a numeric string (e.g. from AppSync JSON + deserialization); it is converted to Decimal before validation and storage. + Non-numeric or unparsable values raise INVALID_INPUT. + """ + product_name = product.get("productName", "").strip() + + if not product_name: + raise AppError(ErrorCode.INVALID_INPUT, "Product name is required") + + processed_product: Dict[str, Any] = { + "productId": f"PRODUCT#{uuid.uuid4()}", + "productName": product_name, + "price": _parse_product_price(product.get("price")), + "sortOrder": product.get("sortOrder", 0), + } + + description = product.get("description", "").strip() + if description: + processed_product["description"] = description + + return processed_product + + +def _validate_catalog_input(catalog_input: Dict[str, Any]) -> tuple[str, bool, list[Dict[str, Any]]]: + """Validate catalog input and extract name, isPublic, products.""" + catalog_name = catalog_input.get("catalogName", "").strip() + is_public = catalog_input.get("isPublic", True) + products = catalog_input.get("products", []) + + if not catalog_name: + raise AppError(ErrorCode.INVALID_INPUT, "Catalog name is required") + if not products: + raise AppError(ErrorCode.INVALID_INPUT, "Products array cannot be empty") + + return catalog_name, is_public, products + + +def _build_catalog_item( + catalog_id: str, + catalog_name: str, + is_public: bool, + caller_id: str, + processed_products: list[Dict[str, Any]], + now: str, +) -> Dict[str, Any]: + """Build the catalog item dictionary for DynamoDB.""" + return { + "catalogId": catalog_id, + "catalogName": catalog_name, + "catalogType": "ADMIN_MANAGED", # Key difference from regular createCatalog + "ownerAccountId": f"ACCOUNT#{caller_id}", # Track who created it + "isPublic": is_public, + "isPublicStr": "true" if is_public else "false", # For GSI queries + "products": processed_products, + "createdAt": now, + "updatedAt": now, + } + + +def _persist_catalog(catalog_item: Dict[str, Any], logger: Any) -> None: + """Persist catalog to DynamoDB.""" + try: + tables.catalogs.put_item(Item=catalog_item) + except ClientError as e: + logger.error("Failed to create catalog", error=str(e)) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to create catalog") + + +def _require_admin_and_get_actor_sub(event: Dict[str, Any]) -> str: + """Require admin + MFA and return the acting administrator's own ``sub`` (#507). + + The actor counterpart to `_require_admin_and_get_target_account_id`: this + one returns the caller, whose sub is what the audit line must record. + """ + require_admin_mfa(event) + + caller_id = get_caller_id(event) + if not caller_id: + raise AppError(ErrorCode.UNAUTHORIZED, "Authentication required") + + return str(caller_id) + + +@with_error_handling(error_message="Failed to create catalog") +def create_managed_catalog(event: Dict[str, Any], context: Any) -> Dict[str, Any]: + """ + Create an ADMIN_MANAGED catalog (admin only). + + AppSync Lambda resolver for createManagedCatalog mutation. + Creates a global catalog that is available to all users. + + Args: + event: AppSync event with identity and arguments + context: Lambda context + + Returns: + Created Catalog object + + Raises: + AppError: If not admin, validation fails, or creation error + """ + logger = get_logger(__name__) + + caller_id = _require_admin_and_get_actor_sub(event) + + arguments = event.get("arguments", {}) + catalog_input = arguments.get("input", {}) + + catalog_name, is_public, products = _validate_catalog_input(catalog_input) + + catalog_id = f"CATALOG#{uuid.uuid4()}" + now = datetime.now(timezone.utc).isoformat() + + processed_products = [_validate_and_process_product(product) for product in products] + + catalog_item = _build_catalog_item(catalog_id, catalog_name, is_public, caller_id, processed_products, now) + + _persist_catalog(catalog_item, logger) + + logger.info( + "Created managed catalog", + catalog_id=catalog_id, + catalog_name=catalog_name, + actor_sub=caller_id, + ) + + return catalog_item diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index e263f87f0..88ab3c740 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -3,11 +3,11 @@ Provides: - adminResetUserPassword: Send password reset email to user -- createManagedCatalog: Create an ADMIN_MANAGED global catalog -The adminListUsers / adminSearchUser handlers live in ``admin_user_operations`` -and the adminPurgeUserAccount handler lives in ``admin_account_purge`` (#773); -both are imported below so the dispatcher map stays the single registry. +The adminListUsers / adminSearchUser handlers live in ``admin_user_operations``, +the adminPurgeUserAccount handler lives in ``admin_account_purge``, and the +createManagedCatalog handler lives in ``admin_catalog_operations`` (#773); all +three are imported below so the dispatcher map stays the single registry. User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs @@ -28,9 +28,7 @@ principals. """ -import uuid from datetime import datetime, timezone -from decimal import Decimal, InvalidOperation from typing import TYPE_CHECKING, Any, Dict, NoReturn, Optional, cast from botocore.exceptions import ClientError @@ -91,6 +89,11 @@ # `admin_account_purge` only imports from this module inside functions. from .admin_account_purge import admin_purge_user_account +# The managed-catalog handler was extracted to its own module (#773) but stays +# registered on this Lambda. The import is safe at module load because +# `admin_catalog_operations` does not import from this module at all. +from .admin_catalog_operations import create_managed_catalog + # The user listing & search handlers were extracted to their own module (#773) but # stay registered on this Lambda. The import is safe at module load because # `admin_user_operations` only imports from this module inside functions. @@ -306,157 +309,6 @@ def admin_reset_user_password(event: Dict[str, Any], context: Any) -> bool: return True -def _parse_product_price(price: Any) -> Decimal: - """Convert a product price (number or numeric string) to a validated Decimal. - - A missing price, a non-numeric/unparsable value, or a negative price - raises INVALID_INPUT. - """ - if price is None: - raise AppError(ErrorCode.INVALID_INPUT, "Valid product price is required") - - try: - price_decimal = Decimal(str(price)) - except InvalidOperation, ValueError, TypeError: - raise AppError(ErrorCode.INVALID_INPUT, "Product price must be a valid number") - - if price_decimal < 0: - raise AppError(ErrorCode.INVALID_INPUT, "Valid product price is required") - return price_decimal - - -def _validate_and_process_product(product: Dict[str, Any]) -> Dict[str, Any]: - """Validate and process a catalog product, returning processed product dict. - - Price may arrive as a number or a numeric string (e.g. from AppSync JSON - deserialization); it is converted to Decimal before validation and storage. - Non-numeric or unparsable values raise INVALID_INPUT. - """ - product_name = product.get("productName", "").strip() - - if not product_name: - raise AppError(ErrorCode.INVALID_INPUT, "Product name is required") - - processed_product: Dict[str, Any] = { - "productId": f"PRODUCT#{uuid.uuid4()}", - "productName": product_name, - "price": _parse_product_price(product.get("price")), - "sortOrder": product.get("sortOrder", 0), - } - - description = product.get("description", "").strip() - if description: - processed_product["description"] = description - - return processed_product - - -def _validate_catalog_input(catalog_input: Dict[str, Any]) -> tuple[str, bool, list[Dict[str, Any]]]: - """Validate catalog input and extract name, isPublic, products.""" - catalog_name = catalog_input.get("catalogName", "").strip() - is_public = catalog_input.get("isPublic", True) - products = catalog_input.get("products", []) - - if not catalog_name: - raise AppError(ErrorCode.INVALID_INPUT, "Catalog name is required") - if not products: - raise AppError(ErrorCode.INVALID_INPUT, "Products array cannot be empty") - - return catalog_name, is_public, products - - -def _build_catalog_item( - catalog_id: str, - catalog_name: str, - is_public: bool, - caller_id: str, - processed_products: list[Dict[str, Any]], - now: str, -) -> Dict[str, Any]: - """Build the catalog item dictionary for DynamoDB.""" - return { - "catalogId": catalog_id, - "catalogName": catalog_name, - "catalogType": "ADMIN_MANAGED", # Key difference from regular createCatalog - "ownerAccountId": f"ACCOUNT#{caller_id}", # Track who created it - "isPublic": is_public, - "isPublicStr": "true" if is_public else "false", # For GSI queries - "products": processed_products, - "createdAt": now, - "updatedAt": now, - } - - -def _persist_catalog(catalog_item: Dict[str, Any], logger: Any) -> None: - """Persist catalog to DynamoDB.""" - try: - tables.catalogs.put_item(Item=catalog_item) - except ClientError as e: - logger.error("Failed to create catalog", error=str(e)) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to create catalog") - - -def _require_admin_and_get_actor_sub(event: Dict[str, Any]) -> str: - """Require admin + MFA and return the acting administrator's own ``sub`` (#507). - - The actor counterpart to `_require_admin_and_get_target_account_id`: this - one returns the caller, whose sub is what the audit line must record. - """ - require_admin_mfa(event) - - caller_id = get_caller_id(event) - if not caller_id: - raise AppError(ErrorCode.UNAUTHORIZED, "Authentication required") - - return str(caller_id) - - -@with_error_handling(error_message="Failed to create catalog") -def create_managed_catalog(event: Dict[str, Any], context: Any) -> Dict[str, Any]: - """ - Create an ADMIN_MANAGED catalog (admin only). - - AppSync Lambda resolver for createManagedCatalog mutation. - Creates a global catalog that is available to all users. - - Args: - event: AppSync event with identity and arguments - context: Lambda context - - Returns: - Created Catalog object - - Raises: - AppError: If not admin, validation fails, or creation error - """ - logger = get_logger(__name__) - - caller_id = _require_admin_and_get_actor_sub(event) - - arguments = event.get("arguments", {}) - catalog_input = arguments.get("input", {}) - - catalog_name, is_public, products = _validate_catalog_input(catalog_input) - - catalog_id = f"CATALOG#{uuid.uuid4()}" - now = datetime.now(timezone.utc).isoformat() - - processed_products = [_validate_and_process_product(product) for product in products] - - catalog_item = _build_catalog_item(catalog_id, catalog_name, is_public, caller_id, processed_products, now) - - _persist_catalog(catalog_item, logger) - - logger.info( - "Created managed catalog", - catalog_id=catalog_id, - catalog_name=catalog_name, - actor_sub=caller_id, - ) - - return catalog_item - - def _user_profiles_for(account_id: str) -> list[Dict[str, Any]]: """Sweep the profiles table once for a per-domain admin deletion.""" return get_user_profiles(normalize_account_id(account_id)) diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index 288bb32d2..b18ff3333 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -24,11 +24,11 @@ from botocore.exceptions import BotoCoreError, ClientError, EndpointConnectionError from src.handlers.admin_account_purge import admin_purge_user_account +from src.handlers.admin_catalog_operations import create_managed_catalog from src.handlers.admin_operations import ( _batch_get_campaign_catalogs, _extract_unique_catalog_ids, admin_reset_user_password, - create_managed_catalog, lambda_handler, ) from src.handlers.admin_user_operations import ( @@ -2857,7 +2857,7 @@ def test_dynamodb_error_raises( }, } - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_catalog_operations.tables") as mock_tables: mock_catalogs = MagicMock() mock_catalogs.put_item.side_effect = ClientError( {"Error": {"Code": "InternalServerError", "Message": "DB error"}}, @@ -2888,7 +2888,7 @@ def test_unexpected_exception_handled( }, } - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_catalog_operations.tables") as mock_tables: mock_tables.catalogs.put_item.side_effect = RuntimeError("Unexpected") result = create_managed_catalog(event, lambda_context) From 755194e9c26e7ddf6353fbee5d64c411138747a3 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 02:50:52 -0400 Subject: [PATCH 4/6] refactor(handlers): extract adminDeleteUser* per-entity handlers into admin_user_deletion Slice 4 of the #773 admin_operations split: move the per-entity user deletion concern (the four adminDeleteUser* handlers plus their shared profile-sweep helper) from admin_operations into its own module src/handlers/admin_user_deletion.py, with zero behavior change. - The five functions (_user_profiles_for, admin_delete_user_orders, admin_delete_user_campaigns, admin_delete_user_shares, admin_delete_user_profiles) move byte-identical; the only textual difference is the function-level import of _require_admin_and_get_target_account_id from .admin_operations in each handler (same convention as admin_account_purge, since a top-level import would be circular). - admin_operations keeps the dispatcher as the single registry (it imports the four handlers back at module load) and drops the four delete_user_* names from its .deletion_cascade import. The AppSync mapping and the Lambda entry point are unchanged; no IaC change (the payload ships the whole src/ tree). - Line counts: src/handlers/admin_operations.py 839 -> 775; src/handlers/admin_user_deletion.py 123 (new). Tests: - tests/unit/test_admin_operations.py: repoint the 21 function-local imports of the four handlers from src.handlers.admin_operations to src.handlers.admin_user_deletion; no test logic and no patch target changed (the suites mock src.handlers.deletion_cascade.tables / src.handlers.campaign_operations.tables, which stay put). - Full unit suite green (1841 passed, 2 skipped, 100% coverage), ruff check src/ tests/ + ruff format --check src/, mypy src/ (strict), xenon A/B complexity gate, cspell on the changed files, and tofu validate (dev). --- src/handlers/admin_operations.py | 92 ++++----------------- src/handlers/admin_user_deletion.py | 123 ++++++++++++++++++++++++++++ tests/unit/test_admin_operations.py | 42 +++++----- 3 files changed, 158 insertions(+), 99 deletions(-) create mode 100644 src/handlers/admin_user_deletion.py diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index 88ab3c740..f974c027a 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -5,9 +5,10 @@ - adminResetUserPassword: Send password reset email to user The adminListUsers / adminSearchUser handlers live in ``admin_user_operations``, -the adminPurgeUserAccount handler lives in ``admin_account_purge``, and the -createManagedCatalog handler lives in ``admin_catalog_operations`` (#773); all -three are imported below so the dispatcher map stays the single registry. +the adminPurgeUserAccount handler lives in ``admin_account_purge``, the +createManagedCatalog handler lives in ``admin_catalog_operations``, and the +per-entity adminDeleteUser* handlers live in ``admin_user_deletion`` (#773); +all four are imported below so the dispatcher map stays the single registry. User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs @@ -36,10 +37,6 @@ # Sibling handler modules use a same-package relative import, which resolves both # in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). from .deletion_cascade import ( - delete_user_campaigns, - delete_user_orders, - delete_user_profiles, - delete_user_shares, get_user_profiles, normalize_account_id, ) @@ -94,6 +91,16 @@ # `admin_catalog_operations` does not import from this module at all. from .admin_catalog_operations import create_managed_catalog +# The per-entity user deletion handlers were extracted to their own module (#773) +# but stay registered on this Lambda. The import is safe at module load because +# `admin_user_deletion` only imports from this module inside functions. +from .admin_user_deletion import ( + admin_delete_user_campaigns, + admin_delete_user_orders, + admin_delete_user_profiles, + admin_delete_user_shares, +) + # The user listing & search handlers were extracted to their own module (#773) but # stay registered on this Lambda. The import is safe at module load because # `admin_user_operations` only imports from this module inside functions. @@ -309,77 +316,6 @@ def admin_reset_user_password(event: Dict[str, Any], context: Any) -> bool: return True -def _user_profiles_for(account_id: str) -> list[Dict[str, Any]]: - """Sweep the profiles table once for a per-domain admin deletion.""" - return get_user_profiles(normalize_account_id(account_id)) - - -@with_error_handling(error_message="Failed to delete user orders") -def admin_delete_user_orders(event: Dict[str, Any], context: Any) -> int: - """ - Delete all orders for all campaigns of all profiles owned by a user (admin only). - - Verifies with strongly consistent reads that each deleted order is gone - from the orders table before returning success. - - Returns the count of deleted orders. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - deleted_count = delete_user_orders(_user_profiles_for(account_id), logger) - logger.info("Admin deleted user orders", account_id=account_id, count=deleted_count, actor_sub=actor_sub) - return deleted_count - - -@with_error_handling(error_message="Failed to delete user campaigns") -def admin_delete_user_campaigns(event: Dict[str, Any], context: Any) -> int: - """ - Delete all campaigns for all profiles owned by a user (admin only). - - Verifies with strongly consistent reads that each deleted campaign is gone - from the campaigns table before returning success. - - Returns the count of deleted campaigns. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - deleted_count = delete_user_campaigns(_user_profiles_for(account_id), logger) - logger.info("Admin deleted user campaigns", account_id=account_id, count=deleted_count, actor_sub=actor_sub) - return deleted_count - - -@with_error_handling(error_message="Failed to delete user shares") -def admin_delete_user_shares(event: Dict[str, Any], context: Any) -> int: - """ - Delete all shares for all profiles owned by a user (admin only). - - Returns the count of deleted shares. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - deleted_count = delete_user_shares(_user_profiles_for(account_id), logger) - logger.info("Admin deleted user shares", account_id=account_id, count=deleted_count, actor_sub=actor_sub) - return deleted_count - - -@with_error_handling(error_message="Failed to delete user profiles") -def admin_delete_user_profiles(event: Dict[str, Any], context: Any) -> int: - """ - Delete all profiles owned by a user (admin only). - - Returns the count of deleted profiles. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - deleted_count = delete_user_profiles(account_id, _user_profiles_for(account_id), logger) - logger.info("Admin deleted user profiles", account_id=account_id, count=deleted_count, actor_sub=actor_sub) - return deleted_count - - @with_error_handling(error_message="Failed to get user profiles") def admin_get_user_profiles(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: """ diff --git a/src/handlers/admin_user_deletion.py b/src/handlers/admin_user_deletion.py new file mode 100644 index 000000000..5119636de --- /dev/null +++ b/src/handlers/admin_user_deletion.py @@ -0,0 +1,123 @@ +""" +Admin per-entity user deletion handlers (extracted from admin_operations, #773). + +Provides: +- adminDeleteUserOrders: Delete all orders for a user's campaigns +- adminDeleteUserCampaigns: Delete all campaigns for a user's profiles +- adminDeleteUserShares: Delete all shares for a user's profiles +- adminDeleteUserProfiles: Delete all profiles owned by a user + +The handlers stay registered on the shared admin-operations Lambda: +``admin_operations`` imports them for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +from typing import TYPE_CHECKING, Any, Dict + +# Sibling handler modules use a same-package relative import, which resolves both +# in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). +from .deletion_cascade import ( + delete_user_campaigns, + delete_user_orders, + delete_user_profiles, + delete_user_shares, + get_user_profiles, + normalize_account_id, +) + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.logging import get_logger +except ModuleNotFoundError: # pragma: no cover + from ..utils.logging import get_logger + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _user_profiles_for(account_id: str) -> list[Dict[str, Any]]: + """Sweep the profiles table once for a per-domain admin deletion.""" + return get_user_profiles(normalize_account_id(account_id)) + + +@with_error_handling(error_message="Failed to delete user orders") +def admin_delete_user_orders(event: Dict[str, Any], context: Any) -> int: + """ + Delete all orders for all campaigns of all profiles owned by a user (admin only). + + Verifies with strongly consistent reads that each deleted order is gone + from the orders table before returning success. + + Returns the count of deleted orders. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + deleted_count = delete_user_orders(_user_profiles_for(account_id), logger) + logger.info("Admin deleted user orders", account_id=account_id, count=deleted_count, actor_sub=actor_sub) + return deleted_count + + +@with_error_handling(error_message="Failed to delete user campaigns") +def admin_delete_user_campaigns(event: Dict[str, Any], context: Any) -> int: + """ + Delete all campaigns for all profiles owned by a user (admin only). + + Verifies with strongly consistent reads that each deleted campaign is gone + from the campaigns table before returning success. + + Returns the count of deleted campaigns. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + deleted_count = delete_user_campaigns(_user_profiles_for(account_id), logger) + logger.info("Admin deleted user campaigns", account_id=account_id, count=deleted_count, actor_sub=actor_sub) + return deleted_count + + +@with_error_handling(error_message="Failed to delete user shares") +def admin_delete_user_shares(event: Dict[str, Any], context: Any) -> int: + """ + Delete all shares for all profiles owned by a user (admin only). + + Returns the count of deleted shares. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + deleted_count = delete_user_shares(_user_profiles_for(account_id), logger) + logger.info("Admin deleted user shares", account_id=account_id, count=deleted_count, actor_sub=actor_sub) + return deleted_count + + +@with_error_handling(error_message="Failed to delete user profiles") +def admin_delete_user_profiles(event: Dict[str, Any], context: Any) -> int: + """ + Delete all profiles owned by a user (admin only). + + Returns the count of deleted profiles. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + deleted_count = delete_user_profiles(account_id, _user_profiles_for(account_id), logger) + logger.info("Admin deleted user profiles", account_id=account_id, count=deleted_count, actor_sub=actor_sub) + return deleted_count diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index b18ff3333..d7004eb95 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -2912,7 +2912,7 @@ def test_success_deletes_all_orders( monkeypatch.setenv("CAMPAIGNS_TABLE_NAME", "kernelworx-campaigns-ue1-dev") monkeypatch.setenv("ORDERS_TABLE_NAME", "kernelworx-orders-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -2978,7 +2978,7 @@ def test_success_with_campaign_having_no_orders( monkeypatch.setenv("CAMPAIGNS_TABLE_NAME", "kernelworx-campaigns-ue1-dev") monkeypatch.setenv("ORDERS_TABLE_NAME", "kernelworx-orders-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3041,7 +3041,7 @@ def test_throttled_returns_resource_busy( from botocore.exceptions import ClientError - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3081,7 +3081,7 @@ def test_non_admin_forbidden( lambda_context: Any, ) -> None: """Test that non-admin users cannot delete user orders.""" - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders event = { **non_admin_appsync_event, @@ -3100,7 +3100,7 @@ def test_missing_account_id( lambda_context: Any, ) -> None: """Test that missing account ID raises error.""" - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders event = { **admin_appsync_event, @@ -3122,7 +3122,7 @@ def test_unexpected_error_handled( """Test that unexpected exceptions are handled.""" monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_orders + from src.handlers.admin_user_deletion import admin_delete_user_orders event = { **admin_appsync_event, @@ -3152,7 +3152,7 @@ def test_success_deletes_all_campaigns( monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") monkeypatch.setenv("CAMPAIGNS_TABLE_NAME", "kernelworx-campaigns-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3205,7 +3205,7 @@ def test_throttled_returns_resource_busy( from botocore.exceptions import ClientError - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3239,7 +3239,7 @@ def test_non_admin_forbidden( lambda_context: Any, ) -> None: """Test that non-admin users cannot delete user campaigns.""" - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns event = { **non_admin_appsync_event, @@ -3258,7 +3258,7 @@ def test_missing_account_id( lambda_context: Any, ) -> None: """Test that missing account ID raises error.""" - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns event = { **admin_appsync_event, @@ -3284,7 +3284,7 @@ def test_profile_with_no_campaigns_deletes_nothing( """ monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns event = { **admin_appsync_event, @@ -3316,7 +3316,7 @@ def test_unexpected_error_handled( """Test that unexpected exceptions are handled.""" monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_campaigns + from src.handlers.admin_user_deletion import admin_delete_user_campaigns event = { **admin_appsync_event, @@ -3346,7 +3346,7 @@ def test_success_deletes_all_shares( monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") monkeypatch.setenv("SHARES_TABLE_NAME", "kernelworx-shares-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_shares + from src.handlers.admin_user_deletion import admin_delete_user_shares target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3394,7 +3394,7 @@ def test_profile_with_no_shares_deletes_nothing( """ monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_shares + from src.handlers.admin_user_deletion import admin_delete_user_shares event = { **admin_appsync_event, @@ -3420,7 +3420,7 @@ def test_non_admin_forbidden( lambda_context: Any, ) -> None: """Test that non-admin users cannot delete user shares.""" - from src.handlers.admin_operations import admin_delete_user_shares + from src.handlers.admin_user_deletion import admin_delete_user_shares event = { **non_admin_appsync_event, @@ -3439,7 +3439,7 @@ def test_missing_account_id( lambda_context: Any, ) -> None: """Test that missing account ID raises error.""" - from src.handlers.admin_operations import admin_delete_user_shares + from src.handlers.admin_user_deletion import admin_delete_user_shares event = { **admin_appsync_event, @@ -3461,7 +3461,7 @@ def test_unexpected_error_handled( """Test that unexpected exceptions are handled.""" monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_shares + from src.handlers.admin_user_deletion import admin_delete_user_shares event = { **admin_appsync_event, @@ -3490,7 +3490,7 @@ def test_success_deletes_all_profiles( """Test successful deletion of all profiles for a user.""" monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_profiles + from src.handlers.admin_user_deletion import admin_delete_user_profiles target_account_id = "target-user-123" db_account_id = f"ACCOUNT#{target_account_id}" @@ -3525,7 +3525,7 @@ def test_non_admin_forbidden( lambda_context: Any, ) -> None: """Test that non-admin users cannot delete user profiles.""" - from src.handlers.admin_operations import admin_delete_user_profiles + from src.handlers.admin_user_deletion import admin_delete_user_profiles event = { **non_admin_appsync_event, @@ -3544,7 +3544,7 @@ def test_missing_account_id( lambda_context: Any, ) -> None: """Test that missing account ID raises error.""" - from src.handlers.admin_operations import admin_delete_user_profiles + from src.handlers.admin_user_deletion import admin_delete_user_profiles event = { **admin_appsync_event, @@ -3566,7 +3566,7 @@ def test_unexpected_error_handled( """Test that unexpected exceptions are handled.""" monkeypatch.setenv("PROFILES_TABLE_NAME", "kernelworx-profiles-ue1-dev") - from src.handlers.admin_operations import admin_delete_user_profiles + from src.handlers.admin_user_deletion import admin_delete_user_profiles event = { **admin_appsync_event, From 1ca6ea64254676f5210d31db522d1d3c12559a60 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 05:55:05 -0400 Subject: [PATCH 5/6] refactor(handlers): extract adminGetUser*, share/campaign, and credential reset handlers Slice 5 of the #773 admin_operations split: move the remaining read and update handlers (the account-scoped adminGetUser* reads, the profile-share and campaign shared-code mutations, and adminResetUserPassword) from admin_operations into their own modules, with zero behavior change. - The 24 functions move byte-identical; the only textual difference is the in-function import of the shared helpers (_require_admin_and_get_target_account_id / _actor_sub) from .admin_operations in the 8 handlers that need them, the same convention admin_account_purge / admin_user_deletion already use (a top-level import would be circular). - Three new modules: - admin_user_reads.py: adminGetUser{Profiles,Catalogs,Campaigns, SharedCampaigns} plus the campaign-catalog batch helpers. - admin_share_campaign_ops.py: adminGetProfileShares, adminDeleteShare, adminUpdateCampaignSharedCode plus their helpers. - admin_credential_reset.py: adminResetUserPassword plus its helpers. - admin_operations keeps the dispatcher as the single registry (it imports the eight handlers back at module load) and drops the 24 moved functions and the now-unused imports. The AppSync mapping and the Lambda entry point are unchanged; no IaC change (the Lambda package ships the whole src/ tree). - Line counts: admin_operations.py 775 -> 271; admin_user_reads.py 287 (new), admin_share_campaign_ops.py 247 (new), admin_credential_reset.py 126 (new). Tests: - tests/unit/test_admin_operations.py: 34 function-local patch/import targets repointed off src.handlers.admin_operations (16 to admin_credential_reset, 11 to admin_user_reads, 6 to admin_share_campaign_ops, and 1 to deletion_cascade.tables where that handler reads its tables, the base's admin_operations.tables patch there being a no-op), plus the module-level handler/helper imports; no test logic changed. - tests/unit/test_cognito_filter_call_sites.py: the _find_user_by_email row now points at admin_credential_reset. - Full unit suite green (1841 passed, 2 skipped, 100% coverage), ruff check src/ tests/ + ruff format --check, mypy src/ (strict), xenon A/B complexity gate. No IaC or frontend change. --- src/handlers/admin_credential_reset.py | 126 +++++ src/handlers/admin_operations.py | 558 +------------------ src/handlers/admin_share_campaign_ops.py | 247 ++++++++ src/handlers/admin_user_reads.py | 287 ++++++++++ tests/unit/test_admin_operations.py | 77 ++- tests/unit/test_cognito_filter_call_sites.py | 10 +- 6 files changed, 732 insertions(+), 573 deletions(-) create mode 100644 src/handlers/admin_credential_reset.py create mode 100644 src/handlers/admin_share_campaign_ops.py create mode 100644 src/handlers/admin_user_reads.py diff --git a/src/handlers/admin_credential_reset.py b/src/handlers/admin_credential_reset.py new file mode 100644 index 000000000..b909b748f --- /dev/null +++ b/src/handlers/admin_credential_reset.py @@ -0,0 +1,126 @@ +""" +Admin password-reset handler (extracted from admin_operations, #773). + +Provides: +- adminResetUserPassword: Send password reset email to user + +The handler stays registered on the shared admin-operations Lambda: +``admin_operations`` imports it for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +from typing import TYPE_CHECKING, Any, Dict + +from botocore.exceptions import ClientError + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.auth import require_admin_mfa + from utils.boto import get_cognito_client + from utils.cognito_filters import cognito_user_filter + from utils.dynamodb import get_required_env + from utils.errors import AppError, ErrorCode + from utils.logging import get_logger, mask_email +except ModuleNotFoundError: # pragma: no cover + from ..utils.auth import require_admin_mfa + from ..utils.boto import get_cognito_client + from ..utils.cognito_filters import cognito_user_filter + from ..utils.dynamodb import get_required_env + from ..utils.errors import AppError, ErrorCode + from ..utils.logging import get_logger, mask_email + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _find_user_by_email(cognito: Any, user_pool_id: str, email: str, logger: Any) -> str: + """Find username by email. Returns username.""" + try: + response = cognito.list_users( + UserPoolId=user_pool_id, + # Built in one step so the value that was checked is the value sent (#124, #560). + Filter=cognito_user_filter("email", email), + Limit=1, + ) + except ClientError as e: + logger.error("Cognito list_users failed", error=str(e)) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to search for user") + + users = response.get("Users", []) + if not users: + raise AppError(ErrorCode.NOT_FOUND, f"User with email '{email}' not found") + + return str(users[0]["Username"]) + + +def _initiate_password_reset(cognito: Any, user_pool_id: str, username: str, email: str, logger: Any) -> None: + """Initiate Cognito password reset for user.""" + try: + cognito.admin_reset_user_password( + UserPoolId=user_pool_id, + Username=username, + ) + except ClientError as e: + error_code = e.response.get("Error", {}).get("Code", "") + if error_code == "UserNotFoundException": + raise AppError(ErrorCode.NOT_FOUND, f"User with email '{email}' not found") + if error_code == "InvalidParameterException": + raise AppError(ErrorCode.INVALID_INPUT, "Cannot reset password for this user type") + logger.error("Cognito admin_reset_user_password failed", error=str(e)) + raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to initiate password reset") + + +@with_error_handling(error_message="Failed to reset password") +def admin_reset_user_password(event: Dict[str, Any], context: Any) -> bool: + """ + Send password reset email to user (admin only). + + AppSync Lambda resolver for adminResetUserPassword mutation. + + Args: + event: AppSync event with identity and arguments + context: Lambda context + + Returns: + True if password reset was initiated successfully + + Raises: + AppError: If not admin, user not found, or Cognito error + """ + from .admin_operations import _actor_sub + + logger = get_logger(__name__) + + # Verify caller is admin and extract email + require_admin_mfa(event) + + arguments = event.get("arguments", {}) + email = arguments.get("email", "").strip().lower() + + if not email: + raise AppError(ErrorCode.INVALID_INPUT, "Email is required") + + user_pool_id = get_required_env("USER_POOL_ID") + cognito = get_cognito_client() + + # Find user and initiate reset + username = _find_user_by_email(cognito, user_pool_id, email, logger) + _initiate_password_reset(cognito, user_pool_id, username, email, logger) + + actor_sub = _actor_sub(event) + logger.info( + "Password reset initiated", + email=mask_email(email), + username=mask_email(username), + actor_sub=actor_sub, + ) + return True diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index f974c027a..f426d20b5 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -29,45 +29,21 @@ principals. """ -from datetime import datetime, timezone -from typing import TYPE_CHECKING, Any, Dict, NoReturn, Optional, cast +from typing import TYPE_CHECKING, Any, Dict, NoReturn from botocore.exceptions import ClientError -# Sibling handler modules use a same-package relative import, which resolves both -# in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). -from .deletion_cascade import ( - get_user_profiles, - normalize_account_id, -) - # Handle both Lambda (absolute) and unit test (relative) imports try: # pragma: no cover from utils.appsync_types import get_caller_id from utils.auth import require_admin_mfa - from utils.boto import get_cognito_client - from utils.cognito_filters import cognito_user_filter - from utils.dynamodb import ( - TRANSIENT_ERROR_CODES, - batch_get_chunked, - get_required_env, - tables, - ) + from utils.dynamodb import TRANSIENT_ERROR_CODES from utils.errors import AppError, ErrorCode - from utils.logging import get_logger, mask_email except ModuleNotFoundError: # pragma: no cover from ..utils.appsync_types import get_caller_id from ..utils.auth import require_admin_mfa - from ..utils.boto import get_cognito_client - from ..utils.cognito_filters import cognito_user_filter - from ..utils.dynamodb import ( - TRANSIENT_ERROR_CODES, - batch_get_chunked, - get_required_env, - tables, - ) + from ..utils.dynamodb import TRANSIENT_ERROR_CODES from ..utils.errors import AppError, ErrorCode - from ..utils.logging import get_logger, mask_email # The decorator stays typed for mypy via the relative import below; at runtime # the absolute import resolves in the Lambda zip (package `utils`) and the @@ -91,6 +67,21 @@ # `admin_catalog_operations` does not import from this module at all. from .admin_catalog_operations import create_managed_catalog +# The password-reset handler was extracted to its own module (#773) but stays +# registered on this Lambda. The import is safe at module load because +# `admin_credential_reset` only imports from this module inside functions. +from .admin_credential_reset import admin_reset_user_password + +# The profile-share and campaign shared-code handlers were extracted to their +# own module (#773) but stay registered on this Lambda. The import is safe at +# module load because `admin_share_campaign_ops` only imports from this module +# inside functions. +from .admin_share_campaign_ops import ( + admin_delete_share, + admin_get_profile_shares, + admin_update_campaign_shared_code, +) + # The per-entity user deletion handlers were extracted to their own module (#773) # but stay registered on this Lambda. The import is safe at module load because # `admin_user_deletion` only imports from this module inside functions. @@ -106,16 +97,15 @@ # `admin_user_operations` only imports from this module inside functions. from .admin_user_operations import admin_list_users, admin_search_user -# Handle both Lambda (absolute) and unit test (relative) imports. mypy sees the -# relative path it can resolve; the runtime fallback tries absolute first for Lambda. -if TYPE_CHECKING: # pragma: no cover - from ..utils.pagination import query_all_items -else: # pragma: no cover - try: - from utils.pagination import query_all_items - except ModuleNotFoundError: - from ..utils.pagination import query_all_items - +# The account-scoped admin user read handlers were extracted to their own module +# (#773) but stay registered on this Lambda. The import is safe at module load +# because `admin_user_reads` only imports from this module inside functions. +from .admin_user_reads import ( + admin_get_user_campaigns, + admin_get_user_catalogs, + admin_get_user_profiles, + admin_get_user_shared_campaigns, +) # DynamoDB/Cognito throttling codes: the lookup is retryable, so surface a # RESOURCE_BUSY error instead of silently incomplete admin data (#291, #456). @@ -234,500 +224,6 @@ def _require_admin_and_get_target_account_id(event: Dict[str, Any]) -> tuple[str return account_id, actor_sub -def _find_user_by_email(cognito: Any, user_pool_id: str, email: str, logger: Any) -> str: - """Find username by email. Returns username.""" - try: - response = cognito.list_users( - UserPoolId=user_pool_id, - # Built in one step so the value that was checked is the value sent (#124, #560). - Filter=cognito_user_filter("email", email), - Limit=1, - ) - except ClientError as e: - logger.error("Cognito list_users failed", error=str(e)) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to search for user") - - users = response.get("Users", []) - if not users: - raise AppError(ErrorCode.NOT_FOUND, f"User with email '{email}' not found") - - return str(users[0]["Username"]) - - -def _initiate_password_reset(cognito: Any, user_pool_id: str, username: str, email: str, logger: Any) -> None: - """Initiate Cognito password reset for user.""" - try: - cognito.admin_reset_user_password( - UserPoolId=user_pool_id, - Username=username, - ) - except ClientError as e: - error_code = e.response.get("Error", {}).get("Code", "") - if error_code == "UserNotFoundException": - raise AppError(ErrorCode.NOT_FOUND, f"User with email '{email}' not found") - if error_code == "InvalidParameterException": - raise AppError(ErrorCode.INVALID_INPUT, "Cannot reset password for this user type") - logger.error("Cognito admin_reset_user_password failed", error=str(e)) - raise AppError(ErrorCode.INTERNAL_ERROR, "Failed to initiate password reset") - - -@with_error_handling(error_message="Failed to reset password") -def admin_reset_user_password(event: Dict[str, Any], context: Any) -> bool: - """ - Send password reset email to user (admin only). - - AppSync Lambda resolver for adminResetUserPassword mutation. - - Args: - event: AppSync event with identity and arguments - context: Lambda context - - Returns: - True if password reset was initiated successfully - - Raises: - AppError: If not admin, user not found, or Cognito error - """ - logger = get_logger(__name__) - - # Verify caller is admin and extract email - require_admin_mfa(event) - - arguments = event.get("arguments", {}) - email = arguments.get("email", "").strip().lower() - - if not email: - raise AppError(ErrorCode.INVALID_INPUT, "Email is required") - - user_pool_id = get_required_env("USER_POOL_ID") - cognito = get_cognito_client() - - # Find user and initiate reset - username = _find_user_by_email(cognito, user_pool_id, email, logger) - _initiate_password_reset(cognito, user_pool_id, username, email, logger) - - actor_sub = _actor_sub(event) - logger.info( - "Password reset initiated", - email=mask_email(email), - username=mask_email(username), - actor_sub=actor_sub, - ) - return True - - -@with_error_handling(error_message="Failed to get user profiles") -def admin_get_user_profiles(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Get all profiles owned by a user (admin only). - - Returns the list of profiles with full details. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - db_account_id = normalize_account_id(account_id) - - # Query profiles by ownerAccountId - profiles = query_all_items( - tables.profiles, - { - "KeyConditionExpression": "ownerAccountId = :owner", - "ExpressionAttributeValues": {":owner": db_account_id}, - }, - ) - - logger.info("Retrieved user profiles", account_id=account_id, count=len(profiles), actor_sub=actor_sub) - return profiles - - -@with_error_handling(error_message="Failed to get user catalogs") -def admin_get_user_catalogs(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Get all catalogs owned by a user (admin only). - - Returns the list of catalogs with full details. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - db_account_id = normalize_account_id(account_id) - - # Query catalogs by ownerAccountId using GSI - catalogs = query_all_items( - tables.catalogs, - { - "IndexName": "ownerAccountId-index", - "KeyConditionExpression": "ownerAccountId = :owner", - "FilterExpression": "attribute_not_exists(isDeleted) OR isDeleted = :false", - "ExpressionAttributeValues": { - ":owner": db_account_id, - ":false": False, - }, - }, - ) - - logger.info("Retrieved user catalogs", account_id=account_id, count=len(catalogs), actor_sub=actor_sub) - return catalogs - - -def _extract_unique_catalog_ids(campaigns: list[Dict[str, Any]]) -> list[str]: - """Extract the de-duplicated catalogIds from a campaign array, preserving first-seen order.""" - catalog_ids: list[str] = [] - seen: set[str] = set() - for campaign in campaigns: - catalog_id = campaign.get("catalogId") - if catalog_id and catalog_id not in seen: - seen.add(catalog_id) - catalog_ids.append(catalog_id) - return catalog_ids - - -def _batch_get_catalog_items( - catalog_ids: list[str], catalogs_table_name: str, logger: Any -) -> Dict[str, Dict[str, Any]]: - """Batch-get the catalogs by id; the shared helper owns the 100-key chunking. - - Raises AppError if keys stay unprocessed after the retries (#557). - """ - catalog_map: Dict[str, Dict[str, Any]] = {} - - def _store_catalog(item: Dict[str, Any]) -> None: - catalog_map[item["catalogId"]] = item - - batch_get_chunked( - catalogs_table_name, - [{"catalogId": catalog_id} for catalog_id in catalog_ids], - _store_catalog, - consistent_read=False, - logger=logger, - ) - return catalog_map - - -def _campaign_catalog_value(catalog: Dict[str, Any] | None, treat_deleted_as_null: bool) -> Dict[str, Any] | None: - """Map a fetched catalog to its Campaign/SharedCampaign catalog contract value.""" - if catalog is not None and treat_deleted_as_null and catalog.get("isDeleted") is True: - return None - return catalog - - -def _attach_campaign_catalogs( - campaigns: list[Dict[str, Any]], catalog_map: Dict[str, Dict[str, Any]], treat_deleted_as_null: bool -) -> None: - """Attach each campaign's fetched catalog, mapping missing/soft-deleted per contract.""" - for campaign in campaigns: - catalog_id = campaign.get("catalogId") - if not catalog_id: - continue - campaign["catalog"] = _campaign_catalog_value(catalog_map.get(catalog_id), treat_deleted_as_null) - - -def _batch_get_campaign_catalogs(campaigns: list[Dict[str, Any]], treat_deleted_as_null: bool, logger: Any) -> None: - """Batch-fetch the catalogs referenced by campaigns and attach each as `catalog`. - - Mirrors the #332 list-query pipeline batching (batch_get_catalogs_fn.js / - batch_get_shared_campaign_catalogs_fn.js) inside the admin Lambda, which - can chunk and loop where AppSync pipeline functions cannot. AppSync always - executes the attached Campaign.catalog / SharedCampaign.catalog field - resolver; since check_source_catalog_fn.js short-circuits when the source - already carries `catalog`, attaching it here replaces the per-item GetItem - (N+1) the admin campaign lists previously paid. - - Deleted-catalog contract matches the field resolvers: Campaign.catalog - returns the raw item including soft-deleted catalogs (treat_deleted_as_null - = False); SharedCampaign.catalog maps soft-deleted or missing catalogs to - None (treat_deleted_as_null = True). - """ - catalog_ids = _extract_unique_catalog_ids(campaigns) - if not catalog_ids: - return - - catalogs_table_name = get_required_env("CATALOGS_TABLE_NAME") - # batch_get_chunked already translates every lookup failure (retryable - # RESOURCE_BUSY on a throttle, INTERNAL_ERROR otherwise), so the typed - # AppError propagates unchanged (#557). - catalog_map = _batch_get_catalog_items(catalog_ids, catalogs_table_name, logger) - - _attach_campaign_catalogs(campaigns, catalog_map, treat_deleted_as_null) - - -def _get_campaigns_for_profiles(profiles: list[Dict[str, Any]], logger: Any, actor_sub: str) -> list[Dict[str, Any]]: - """Query campaigns for each profile.""" - all_campaigns = [] - for profile in profiles: - profile_id = profile.get("profileId") - if profile_id: - campaigns = query_all_items( - tables.campaigns, - { - "KeyConditionExpression": "profileId = :pid", - "ExpressionAttributeValues": {":pid": profile_id}, - }, - ) - all_campaigns.extend(campaigns) - logger.info( - "Retrieved campaigns for profile", - profile_id=profile_id, - count=len(campaigns), - actor_sub=actor_sub, - ) - return all_campaigns - - -@with_error_handling(error_message="Failed to get user campaigns") -def admin_get_user_campaigns(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Get all campaigns for a user's profiles. - - Admin-only operation. Returns all campaigns across all profiles owned by the account. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - db_account_id = normalize_account_id(account_id) - - # First, get all profiles owned by this account - profiles = get_user_profiles(db_account_id) - logger.info("Retrieved user profiles", account_id=account_id, count=len(profiles), actor_sub=actor_sub) - - # Now query campaigns for each profile - all_campaigns = _get_campaigns_for_profiles(profiles, logger, actor_sub) - # Pre-resolve catalogs in one chunked BatchGetItem so the Campaign.catalog - # field resolver short-circuits instead of doing per-item GetItem reads (#332). - _batch_get_campaign_catalogs(all_campaigns, treat_deleted_as_null=False, logger=logger) - logger.info("Retrieved user campaigns", account_id=account_id, count=len(all_campaigns), actor_sub=actor_sub) - return all_campaigns - - -@with_error_handling(error_message="Failed to get user shared campaigns") -def admin_get_user_shared_campaigns(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Get all shared campaigns created by a user. - - Admin-only operation. - """ - logger = get_logger(__name__) - - account_id, actor_sub = _require_admin_and_get_target_account_id(event) - db_account_id = normalize_account_id(account_id) - - # Query shared campaigns by createdBy using GSI1 - campaigns = query_all_items( - tables.shared_campaigns, - { - "IndexName": "GSI1", - "KeyConditionExpression": "createdBy = :creator", - "ExpressionAttributeValues": { - ":creator": db_account_id, - }, - }, - ) - - # Pre-resolve catalogs in one chunked BatchGetItem so the SharedCampaign.catalog - # field resolver short-circuits instead of doing per-item GetItem reads (#332). - _batch_get_campaign_catalogs(campaigns, treat_deleted_as_null=True, logger=logger) - - logger.info( - "Retrieved user shared campaigns", - account_id=account_id, - count=len(campaigns), - actor_sub=actor_sub, - ) - return campaigns - - -def _convert_permissions_to_lists(shares: list[Dict[str, Any]]) -> None: - """Convert DynamoDB permission sets to lists for JSON serialization.""" - for share in shares: - if "permissions" in share and isinstance(share["permissions"], set): - share["permissions"] = list(share["permissions"]) - - -def _query_profile_shares(db_profile_id: str) -> list[Dict[str, Any]]: - """Query all shares for a profile.""" - return query_all_items( - tables.shares, - { - "KeyConditionExpression": "profileId = :pid", - "ExpressionAttributeValues": {":pid": db_profile_id}, - }, - ) - - -def _require_admin_and_get_target_profile_id(event: Dict[str, Any]) -> tuple[str, str]: - """Require admin + MFA, returning ``(db_profile_id, actor_sub)`` (#507). - - ``db_profile_id`` is the target profile named by the request; ``actor_sub`` - is the calling administrator, returned alongside so the audit line can - attribute the operation. - """ - require_admin_mfa(event) - actor_sub = _actor_sub(event) - - arguments = event.get("arguments", {}) - profile_id = arguments.get("profileId", "").strip() - - if not profile_id: - raise AppError(ErrorCode.INVALID_INPUT, "Profile ID is required") - - # Add PROFILE# prefix if not present - db_profile_id = profile_id if profile_id.startswith("PROFILE#") else f"PROFILE#{profile_id}" - return db_profile_id, actor_sub - - -@with_error_handling(error_message="Failed to get profile shares") -def admin_get_profile_shares(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: - """ - Get all shares for a specific profile. - - Admin-only operation. Returns all users who have access to this profile. - """ - logger = get_logger(__name__) - - db_profile_id, actor_sub = _require_admin_and_get_target_profile_id(event) - - # Query shares and convert sets to lists - shares = _query_profile_shares(db_profile_id) - _convert_permissions_to_lists(shares) - - logger.info("Retrieved profile shares", profile_id=db_profile_id, count=len(shares), actor_sub=actor_sub) - return shares - - -def _normalize_profile_and_account_ids(profile_id: str, target_account_id: str) -> tuple[str, str]: - """Normalize profile ID and target account ID with prefixes.""" - db_profile_id = profile_id if profile_id.startswith("PROFILE#") else f"PROFILE#{profile_id}" - db_target_id = target_account_id if target_account_id.startswith("ACCOUNT#") else f"ACCOUNT#{target_account_id}" - return db_profile_id, db_target_id - - -def _require_admin_and_get_share_ids(event: Dict[str, Any]) -> tuple[str, str, str]: - """Require admin + MFA, returning ``(profile_id, target_account_id, actor_sub)`` (#507). - - The first two are the share the request names; the third is the calling - administrator, carried so the revocation can be attributed. - """ - require_admin_mfa(event) - actor_sub = _actor_sub(event) - - arguments = event.get("arguments", {}) - profile_id = arguments.get("profileId", "").strip() - target_account_id = arguments.get("targetAccountId", "").strip() - - if not profile_id or not target_account_id: - raise AppError(ErrorCode.INVALID_INPUT, "Profile ID and target account ID are required") - - return profile_id, target_account_id, actor_sub - - -@with_error_handling(error_message="Failed to delete share") -def admin_delete_share(event: Dict[str, Any], context: Any) -> bool: - """ - Delete a specific share (revoke access). - - Admin-only operation. Allows admin to remove someone's access to a profile. - """ - logger = get_logger(__name__) - - profile_id, target_account_id, actor_sub = _require_admin_and_get_share_ids(event) - db_profile_id, db_target_id = _normalize_profile_and_account_ids(profile_id, target_account_id) - - tables.shares.delete_item(Key={"profileId": db_profile_id, "targetAccountId": db_target_id}) - - logger.info( - "Deleted share", - profile_id=profile_id, - target_account_id=target_account_id, - actor_sub=actor_sub, - ) - return True - - -def _get_campaign_profile_id(db_campaign_id: str, campaign_id: str) -> str: - """Get profile ID for a campaign via GSI lookup.""" - response = tables.campaigns.query( - IndexName="campaignId-index", - KeyConditionExpression="campaignId = :cid", - ExpressionAttributeValues={":cid": db_campaign_id}, - ) - - items = response.get("Items", []) - if not items: - raise AppError(ErrorCode.NOT_FOUND, f"Campaign not found: {campaign_id}") - - return str(items[0]["profileId"]) - - -def _update_campaign_shared_code( - profile_id: str, db_campaign_id: str, shared_campaign_code: Optional[str] -) -> Dict[str, Any]: - """Update campaign's sharedCampaignCode field and return the stored item.""" - updated_at = datetime.now(timezone.utc).isoformat() - - if shared_campaign_code is None: - update_expr = "REMOVE sharedCampaignCode SET updatedAt = :updated" - expr_vals = {":updated": updated_at} - else: - update_expr = "SET sharedCampaignCode = :code, updatedAt = :updated" - expr_vals = {":code": shared_campaign_code, ":updated": updated_at} - - response = tables.campaigns.update_item( - Key={"profileId": profile_id, "campaignId": db_campaign_id}, - UpdateExpression=update_expr, - ExpressionAttributeValues=expr_vals, - ReturnValues="ALL_NEW", - ) - - return cast(Dict[str, Any], response.get("Attributes", {})) - - -def _require_admin_and_get_target_campaign_id(event: Dict[str, Any]) -> tuple[str, Optional[str], str]: - """Require admin + MFA, returning ``(campaign_id, shared_campaign_code, actor_sub)`` (#507). - - ``campaign_id`` is the target campaign named by the request; ``actor_sub`` - is the calling administrator, carried so the update can be attributed. - """ - require_admin_mfa(event) - actor_sub = _actor_sub(event) - - arguments = event.get("arguments", {}) - campaign_id = arguments.get("campaignId", "").strip() - shared_campaign_code = arguments.get("sharedCampaignCode") - - if not campaign_id: - raise AppError(ErrorCode.INVALID_INPUT, "Campaign ID is required") - - return campaign_id, shared_campaign_code, actor_sub - - -@with_error_handling(error_message="Failed to update campaign shared code") -def admin_update_campaign_shared_code(event: Dict[str, Any], context: Any) -> Dict[str, Any]: - """ - Update a campaign's sharedCampaignCode field and return the stored campaign item. - - Admin-only operation. Allows associating a campaign with a shared campaign. - """ - logger = get_logger(__name__) - - campaign_id, shared_campaign_code, actor_sub = _require_admin_and_get_target_campaign_id(event) - - # Add CAMPAIGN# prefix if not present - db_campaign_id = campaign_id if campaign_id.startswith("CAMPAIGN#") else f"CAMPAIGN#{campaign_id}" - - # Get campaign profile, update shared code, and return the stored item - profile_id = _get_campaign_profile_id(db_campaign_id, campaign_id) - updated_campaign = _update_campaign_shared_code(profile_id, db_campaign_id, shared_campaign_code) - - logger.info( - "Updated campaign shared code", - campaign_id=campaign_id, - shared_campaign_code=shared_campaign_code, - actor_sub=actor_sub, - ) - return updated_campaign - - # Operation dispatcher map _OPERATION_HANDLERS = { "adminListUsers": admin_list_users, diff --git a/src/handlers/admin_share_campaign_ops.py b/src/handlers/admin_share_campaign_ops.py new file mode 100644 index 000000000..1020369b6 --- /dev/null +++ b/src/handlers/admin_share_campaign_ops.py @@ -0,0 +1,247 @@ +""" +Admin profile-share & campaign shared-code handlers (extracted from admin_operations, #773). + +Provides: +- adminGetProfileShares: Get all shares for a profile +- adminDeleteShare: Delete a specific share (revoke access) +- adminUpdateCampaignSharedCode: Update a campaign's sharedCampaignCode field + +The handlers stay registered on the shared admin-operations Lambda: +``admin_operations`` imports them for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. +""" + +from datetime import datetime, timezone +from typing import TYPE_CHECKING, Any, Dict, Optional, cast + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.auth import require_admin_mfa + from utils.dynamodb import tables + from utils.errors import AppError, ErrorCode + from utils.logging import get_logger +except ModuleNotFoundError: # pragma: no cover + from ..utils.auth import require_admin_mfa + from ..utils.dynamodb import tables + from ..utils.errors import AppError, ErrorCode + from ..utils.logging import get_logger + +# query_all_items must be typed for mypy (warn_return_any): mypy reads only the +# TYPE_CHECKING branch; the runtime fallback tries the absolute import first so it +# resolves in the Lambda zip (package `utils`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.pagination import query_all_items +else: # pragma: no cover + try: + from utils.pagination import query_all_items + except ModuleNotFoundError: + from ..utils.pagination import query_all_items + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +def _convert_permissions_to_lists(shares: list[Dict[str, Any]]) -> None: + """Convert DynamoDB permission sets to lists for JSON serialization.""" + for share in shares: + if "permissions" in share and isinstance(share["permissions"], set): + share["permissions"] = list(share["permissions"]) + + +def _query_profile_shares(db_profile_id: str) -> list[Dict[str, Any]]: + """Query all shares for a profile.""" + return query_all_items( + tables.shares, + { + "KeyConditionExpression": "profileId = :pid", + "ExpressionAttributeValues": {":pid": db_profile_id}, + }, + ) + + +def _require_admin_and_get_target_profile_id(event: Dict[str, Any]) -> tuple[str, str]: + """Require admin + MFA, returning ``(db_profile_id, actor_sub)`` (#507). + + ``db_profile_id`` is the target profile named by the request; ``actor_sub`` + is the calling administrator, returned alongside so the audit line can + attribute the operation. + """ + from .admin_operations import _actor_sub + + require_admin_mfa(event) + actor_sub = _actor_sub(event) + + arguments = event.get("arguments", {}) + profile_id = arguments.get("profileId", "").strip() + + if not profile_id: + raise AppError(ErrorCode.INVALID_INPUT, "Profile ID is required") + + # Add PROFILE# prefix if not present + db_profile_id = profile_id if profile_id.startswith("PROFILE#") else f"PROFILE#{profile_id}" + return db_profile_id, actor_sub + + +@with_error_handling(error_message="Failed to get profile shares") +def admin_get_profile_shares(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Get all shares for a specific profile. + + Admin-only operation. Returns all users who have access to this profile. + """ + logger = get_logger(__name__) + + db_profile_id, actor_sub = _require_admin_and_get_target_profile_id(event) + + # Query shares and convert sets to lists + shares = _query_profile_shares(db_profile_id) + _convert_permissions_to_lists(shares) + + logger.info("Retrieved profile shares", profile_id=db_profile_id, count=len(shares), actor_sub=actor_sub) + return shares + + +def _normalize_profile_and_account_ids(profile_id: str, target_account_id: str) -> tuple[str, str]: + """Normalize profile ID and target account ID with prefixes.""" + db_profile_id = profile_id if profile_id.startswith("PROFILE#") else f"PROFILE#{profile_id}" + db_target_id = target_account_id if target_account_id.startswith("ACCOUNT#") else f"ACCOUNT#{target_account_id}" + return db_profile_id, db_target_id + + +def _require_admin_and_get_share_ids(event: Dict[str, Any]) -> tuple[str, str, str]: + """Require admin + MFA, returning ``(profile_id, target_account_id, actor_sub)`` (#507). + + The first two are the share the request names; the third is the calling + administrator, carried so the revocation can be attributed. + """ + from .admin_operations import _actor_sub + + require_admin_mfa(event) + actor_sub = _actor_sub(event) + + arguments = event.get("arguments", {}) + profile_id = arguments.get("profileId", "").strip() + target_account_id = arguments.get("targetAccountId", "").strip() + + if not profile_id or not target_account_id: + raise AppError(ErrorCode.INVALID_INPUT, "Profile ID and target account ID are required") + + return profile_id, target_account_id, actor_sub + + +@with_error_handling(error_message="Failed to delete share") +def admin_delete_share(event: Dict[str, Any], context: Any) -> bool: + """ + Delete a specific share (revoke access). + + Admin-only operation. Allows admin to remove someone's access to a profile. + """ + logger = get_logger(__name__) + + profile_id, target_account_id, actor_sub = _require_admin_and_get_share_ids(event) + db_profile_id, db_target_id = _normalize_profile_and_account_ids(profile_id, target_account_id) + + tables.shares.delete_item(Key={"profileId": db_profile_id, "targetAccountId": db_target_id}) + + logger.info( + "Deleted share", + profile_id=profile_id, + target_account_id=target_account_id, + actor_sub=actor_sub, + ) + return True + + +def _get_campaign_profile_id(db_campaign_id: str, campaign_id: str) -> str: + """Get profile ID for a campaign via GSI lookup.""" + response = tables.campaigns.query( + IndexName="campaignId-index", + KeyConditionExpression="campaignId = :cid", + ExpressionAttributeValues={":cid": db_campaign_id}, + ) + + items = response.get("Items", []) + if not items: + raise AppError(ErrorCode.NOT_FOUND, f"Campaign not found: {campaign_id}") + + return str(items[0]["profileId"]) + + +def _update_campaign_shared_code( + profile_id: str, db_campaign_id: str, shared_campaign_code: Optional[str] +) -> Dict[str, Any]: + """Update campaign's sharedCampaignCode field and return the stored item.""" + updated_at = datetime.now(timezone.utc).isoformat() + + if shared_campaign_code is None: + update_expr = "REMOVE sharedCampaignCode SET updatedAt = :updated" + expr_vals = {":updated": updated_at} + else: + update_expr = "SET sharedCampaignCode = :code, updatedAt = :updated" + expr_vals = {":code": shared_campaign_code, ":updated": updated_at} + + response = tables.campaigns.update_item( + Key={"profileId": profile_id, "campaignId": db_campaign_id}, + UpdateExpression=update_expr, + ExpressionAttributeValues=expr_vals, + ReturnValues="ALL_NEW", + ) + + return cast(Dict[str, Any], response.get("Attributes", {})) + + +def _require_admin_and_get_target_campaign_id(event: Dict[str, Any]) -> tuple[str, Optional[str], str]: + """Require admin + MFA, returning ``(campaign_id, shared_campaign_code, actor_sub)`` (#507). + + ``campaign_id`` is the target campaign named by the request; ``actor_sub`` + is the calling administrator, carried so the update can be attributed. + """ + from .admin_operations import _actor_sub + + require_admin_mfa(event) + actor_sub = _actor_sub(event) + + arguments = event.get("arguments", {}) + campaign_id = arguments.get("campaignId", "").strip() + shared_campaign_code = arguments.get("sharedCampaignCode") + + if not campaign_id: + raise AppError(ErrorCode.INVALID_INPUT, "Campaign ID is required") + + return campaign_id, shared_campaign_code, actor_sub + + +@with_error_handling(error_message="Failed to update campaign shared code") +def admin_update_campaign_shared_code(event: Dict[str, Any], context: Any) -> Dict[str, Any]: + """ + Update a campaign's sharedCampaignCode field and return the stored campaign item. + + Admin-only operation. Allows associating a campaign with a shared campaign. + """ + logger = get_logger(__name__) + + campaign_id, shared_campaign_code, actor_sub = _require_admin_and_get_target_campaign_id(event) + + # Add CAMPAIGN# prefix if not present + db_campaign_id = campaign_id if campaign_id.startswith("CAMPAIGN#") else f"CAMPAIGN#{campaign_id}" + + # Get campaign profile, update shared code, and return the stored item + profile_id = _get_campaign_profile_id(db_campaign_id, campaign_id) + updated_campaign = _update_campaign_shared_code(profile_id, db_campaign_id, shared_campaign_code) + + logger.info( + "Updated campaign shared code", + campaign_id=campaign_id, + shared_campaign_code=shared_campaign_code, + actor_sub=actor_sub, + ) + return updated_campaign diff --git a/src/handlers/admin_user_reads.py b/src/handlers/admin_user_reads.py new file mode 100644 index 000000000..1a6c489c7 --- /dev/null +++ b/src/handlers/admin_user_reads.py @@ -0,0 +1,287 @@ +""" +Admin user read handlers (extracted from admin_operations, #773). + +Provides: +- adminGetUserProfiles: Get all profiles owned by a user +- adminGetUserCatalogs: Get all catalogs owned by a user +- adminGetUserCampaigns: Get all campaigns across a user's profiles +- adminGetUserSharedCampaigns: Get all shared campaigns created by a user + +The handlers stay registered on the shared admin-operations Lambda: +``admin_operations`` imports them for its operation dispatcher map, so the +Lambda entry point (``handlers.admin_operations.lambda_handler``) and the +AppSync handler mapping are unchanged. + +All four are account-scoped reads guarded by ``_require_admin_and_get_target_account_id``; +the two campaign reads share the ``_batch_get_campaign_catalogs`` chain, which +pre-resolves each campaign's ``catalog`` so the Campaign/SharedCampaign field +resolver short-circuits instead of doing a per-item GetItem (#332). +""" + +from typing import TYPE_CHECKING, Any, Dict + +# Sibling handler modules use a same-package relative import, which resolves both +# in the Lambda zip (package `handlers`) and in unit tests (package `src.handlers`). +from .deletion_cascade import ( + get_user_profiles, + normalize_account_id, +) + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.dynamodb import batch_get_chunked, get_required_env, tables + from utils.logging import get_logger +except ModuleNotFoundError: # pragma: no cover + from ..utils.dynamodb import batch_get_chunked, get_required_env, tables + from ..utils.logging import get_logger + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + + +# Handle both Lambda (absolute) and unit test (relative) imports. mypy sees the +# relative path it can resolve; the runtime fallback tries absolute first for Lambda. +if TYPE_CHECKING: # pragma: no cover + from ..utils.pagination import query_all_items +else: # pragma: no cover + try: + from utils.pagination import query_all_items + except ModuleNotFoundError: + from ..utils.pagination import query_all_items + + +@with_error_handling(error_message="Failed to get user profiles") +def admin_get_user_profiles(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Get all profiles owned by a user (admin only). + + Returns the list of profiles with full details. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + db_account_id = normalize_account_id(account_id) + + # Query profiles by ownerAccountId + profiles = query_all_items( + tables.profiles, + { + "KeyConditionExpression": "ownerAccountId = :owner", + "ExpressionAttributeValues": {":owner": db_account_id}, + }, + ) + + logger.info("Retrieved user profiles", account_id=account_id, count=len(profiles), actor_sub=actor_sub) + return profiles + + +@with_error_handling(error_message="Failed to get user catalogs") +def admin_get_user_catalogs(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Get all catalogs owned by a user (admin only). + + Returns the list of catalogs with full details. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + db_account_id = normalize_account_id(account_id) + + # Query catalogs by ownerAccountId using GSI + catalogs = query_all_items( + tables.catalogs, + { + "IndexName": "ownerAccountId-index", + "KeyConditionExpression": "ownerAccountId = :owner", + "FilterExpression": "attribute_not_exists(isDeleted) OR isDeleted = :false", + "ExpressionAttributeValues": { + ":owner": db_account_id, + ":false": False, + }, + }, + ) + + logger.info("Retrieved user catalogs", account_id=account_id, count=len(catalogs), actor_sub=actor_sub) + return catalogs + + +def _extract_unique_catalog_ids(campaigns: list[Dict[str, Any]]) -> list[str]: + """Extract the de-duplicated catalogIds from a campaign array, preserving first-seen order.""" + catalog_ids: list[str] = [] + seen: set[str] = set() + for campaign in campaigns: + catalog_id = campaign.get("catalogId") + if catalog_id and catalog_id not in seen: + seen.add(catalog_id) + catalog_ids.append(catalog_id) + return catalog_ids + + +def _batch_get_catalog_items( + catalog_ids: list[str], catalogs_table_name: str, logger: Any +) -> Dict[str, Dict[str, Any]]: + """Batch-get the catalogs by id; the shared helper owns the 100-key chunking. + + Raises AppError if keys stay unprocessed after the retries (#557). + """ + catalog_map: Dict[str, Dict[str, Any]] = {} + + def _store_catalog(item: Dict[str, Any]) -> None: + catalog_map[item["catalogId"]] = item + + batch_get_chunked( + catalogs_table_name, + [{"catalogId": catalog_id} for catalog_id in catalog_ids], + _store_catalog, + consistent_read=False, + logger=logger, + ) + return catalog_map + + +def _campaign_catalog_value(catalog: Dict[str, Any] | None, treat_deleted_as_null: bool) -> Dict[str, Any] | None: + """Map a fetched catalog to its Campaign/SharedCampaign catalog contract value.""" + if catalog is not None and treat_deleted_as_null and catalog.get("isDeleted") is True: + return None + return catalog + + +def _attach_campaign_catalogs( + campaigns: list[Dict[str, Any]], catalog_map: Dict[str, Dict[str, Any]], treat_deleted_as_null: bool +) -> None: + """Attach each campaign's fetched catalog, mapping missing/soft-deleted per contract.""" + for campaign in campaigns: + catalog_id = campaign.get("catalogId") + if not catalog_id: + continue + campaign["catalog"] = _campaign_catalog_value(catalog_map.get(catalog_id), treat_deleted_as_null) + + +def _batch_get_campaign_catalogs(campaigns: list[Dict[str, Any]], treat_deleted_as_null: bool, logger: Any) -> None: + """Batch-fetch the catalogs referenced by campaigns and attach each as `catalog`. + + Mirrors the #332 list-query pipeline batching (batch_get_catalogs_fn.js / + batch_get_shared_campaign_catalogs_fn.js) inside the admin Lambda, which + can chunk and loop where AppSync pipeline functions cannot. AppSync always + executes the attached Campaign.catalog / SharedCampaign.catalog field + resolver; since check_source_catalog_fn.js short-circuits when the source + already carries `catalog`, attaching it here replaces the per-item GetItem + (N+1) the admin campaign lists previously paid. + + Deleted-catalog contract matches the field resolvers: Campaign.catalog + returns the raw item including soft-deleted catalogs (treat_deleted_as_null + = False); SharedCampaign.catalog maps soft-deleted or missing catalogs to + None (treat_deleted_as_null = True). + """ + catalog_ids = _extract_unique_catalog_ids(campaigns) + if not catalog_ids: + return + + catalogs_table_name = get_required_env("CATALOGS_TABLE_NAME") + # batch_get_chunked already translates every lookup failure (retryable + # RESOURCE_BUSY on a throttle, INTERNAL_ERROR otherwise), so the typed + # AppError propagates unchanged (#557). + catalog_map = _batch_get_catalog_items(catalog_ids, catalogs_table_name, logger) + + _attach_campaign_catalogs(campaigns, catalog_map, treat_deleted_as_null) + + +def _get_campaigns_for_profiles(profiles: list[Dict[str, Any]], logger: Any, actor_sub: str) -> list[Dict[str, Any]]: + """Query campaigns for each profile.""" + all_campaigns = [] + for profile in profiles: + profile_id = profile.get("profileId") + if profile_id: + campaigns = query_all_items( + tables.campaigns, + { + "KeyConditionExpression": "profileId = :pid", + "ExpressionAttributeValues": {":pid": profile_id}, + }, + ) + all_campaigns.extend(campaigns) + logger.info( + "Retrieved campaigns for profile", + profile_id=profile_id, + count=len(campaigns), + actor_sub=actor_sub, + ) + return all_campaigns + + +@with_error_handling(error_message="Failed to get user campaigns") +def admin_get_user_campaigns(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Get all campaigns for a user's profiles. + + Admin-only operation. Returns all campaigns across all profiles owned by the account. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + db_account_id = normalize_account_id(account_id) + + # First, get all profiles owned by this account + profiles = get_user_profiles(db_account_id) + logger.info("Retrieved user profiles", account_id=account_id, count=len(profiles), actor_sub=actor_sub) + + # Now query campaigns for each profile + all_campaigns = _get_campaigns_for_profiles(profiles, logger, actor_sub) + # Pre-resolve catalogs in one chunked BatchGetItem so the Campaign.catalog + # field resolver short-circuits instead of doing per-item GetItem reads (#332). + _batch_get_campaign_catalogs(all_campaigns, treat_deleted_as_null=False, logger=logger) + logger.info("Retrieved user campaigns", account_id=account_id, count=len(all_campaigns), actor_sub=actor_sub) + return all_campaigns + + +@with_error_handling(error_message="Failed to get user shared campaigns") +def admin_get_user_shared_campaigns(event: Dict[str, Any], context: Any) -> list[Dict[str, Any]]: + """ + Get all shared campaigns created by a user. + + Admin-only operation. + """ + from .admin_operations import _require_admin_and_get_target_account_id + + logger = get_logger(__name__) + + account_id, actor_sub = _require_admin_and_get_target_account_id(event) + db_account_id = normalize_account_id(account_id) + + # Query shared campaigns by createdBy using GSI1 + campaigns = query_all_items( + tables.shared_campaigns, + { + "IndexName": "GSI1", + "KeyConditionExpression": "createdBy = :creator", + "ExpressionAttributeValues": { + ":creator": db_account_id, + }, + }, + ) + + # Pre-resolve catalogs in one chunked BatchGetItem so the SharedCampaign.catalog + # field resolver short-circuits instead of doing per-item GetItem reads (#332). + _batch_get_campaign_catalogs(campaigns, treat_deleted_as_null=True, logger=logger) + + logger.info( + "Retrieved user shared campaigns", + account_id=account_id, + count=len(campaigns), + actor_sub=actor_sub, + ) + return campaigns diff --git a/tests/unit/test_admin_operations.py b/tests/unit/test_admin_operations.py index d7004eb95..0cfa5fc00 100644 --- a/tests/unit/test_admin_operations.py +++ b/tests/unit/test_admin_operations.py @@ -25,12 +25,8 @@ from src.handlers.admin_account_purge import admin_purge_user_account from src.handlers.admin_catalog_operations import create_managed_catalog -from src.handlers.admin_operations import ( - _batch_get_campaign_catalogs, - _extract_unique_catalog_ids, - admin_reset_user_password, - lambda_handler, -) +from src.handlers.admin_credential_reset import admin_reset_user_password +from src.handlers.admin_operations import lambda_handler from src.handlers.admin_user_operations import ( _batch_get_display_names, _batch_get_user_groups, @@ -39,6 +35,7 @@ admin_list_users, admin_search_user, ) +from src.handlers.admin_user_reads import _batch_get_campaign_catalogs, _extract_unique_catalog_ids from src.utils.errors import AppError, ErrorCode from tests.unit.fixtures import accept_batch_deletes @@ -145,7 +142,7 @@ def test_dispatch_admin_reset_user_password( } # Mock Cognito - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.return_value = {} @@ -963,7 +960,7 @@ def test_success( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.return_value = {} @@ -992,7 +989,7 @@ def test_email_trimmed_and_lowercased( "arguments": {"email": " TEST@EXAMPLE.COM "}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.return_value = {} @@ -1064,7 +1061,7 @@ def test_user_not_found( "arguments": {"email": "notfound@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": []} mock_get_client.return_value = mock_cognito @@ -1109,7 +1106,7 @@ def test_cognito_list_users_error( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.side_effect = ClientError( {"Error": {"Code": "InternalErrorException", "Message": "Test error"}}, @@ -1137,7 +1134,7 @@ def test_cognito_reset_user_not_found( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.side_effect = ClientError( @@ -1166,7 +1163,7 @@ def test_cognito_invalid_parameter_exception( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.side_effect = ClientError( @@ -1195,7 +1192,7 @@ def test_cognito_other_error_during_reset( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.side_effect = ClientError( @@ -1224,7 +1221,7 @@ def test_unexpected_exception_handled( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.side_effect = RuntimeError("Unexpected error") mock_get_client.return_value = mock_cognito @@ -1249,7 +1246,7 @@ def test_email_with_quote_rejected_before_filter( "arguments": {"email": 'evil"@example.com'}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_get_client.return_value = mock_cognito @@ -1274,7 +1271,7 @@ def test_email_with_backslash_rejected_before_filter( "arguments": {"email": "evil\\@example.com\\"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_get_client.return_value = mock_cognito @@ -1299,7 +1296,7 @@ def test_malformed_email_rejected_before_filter( "arguments": {"email": "not-an-email"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_get_client.return_value = mock_cognito @@ -1364,7 +1361,7 @@ def test_admin_with_mfa_passes( "arguments": {"email": "test@example.com"}, } - with patch("src.handlers.admin_operations.get_cognito_client") as mock_get_client: + with patch("src.handlers.admin_credential_reset.get_cognito_client") as mock_get_client: mock_cognito = MagicMock() mock_cognito.list_users.return_value = {"Users": [{"Username": "test-user-123"}]} mock_cognito.admin_reset_user_password.return_value = {} @@ -3129,7 +3126,7 @@ def test_unexpected_error_handled( "arguments": {"accountId": "target-user-123"}, } - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.deletion_cascade.tables") as mock_tables: mock_tables.profiles.query.side_effect = RuntimeError("Unexpected") result = admin_delete_user_orders(event, lambda_context) @@ -5538,7 +5535,7 @@ def test_get_user_profiles_paginates( lambda_context: Any, ) -> None: """Test that profile retrieval follows DynamoDB pagination.""" - with patch("src.handlers.admin_operations.query_all_items") as mock_query_all: + with patch("src.handlers.admin_user_reads.query_all_items") as mock_query_all: mock_query_all.return_value = [{"profileId": f"PROFILE#{i}", "sellerName": f"Scout {i}"} for i in range(3)] admin_appsync_event["info"]["fieldName"] = "adminGetUserProfiles" @@ -5652,7 +5649,7 @@ def test_get_user_catalogs_paginates( lambda_context: Any, ) -> None: """Test that catalog retrieval follows DynamoDB pagination.""" - with patch("src.handlers.admin_operations.query_all_items") as mock_query_all: + with patch("src.handlers.admin_user_reads.query_all_items") as mock_query_all: mock_query_all.return_value = [ {"catalogId": f"CATALOG#{i}", "catalogName": f"Catalog {i}"} for i in range(3) ] @@ -5773,7 +5770,7 @@ def test_get_user_campaigns_profile_without_profile_id( with ( patch("src.handlers.deletion_cascade.tables") as mock_cascade_tables, - patch("src.handlers.admin_operations.tables") as mock_tables, + patch("src.handlers.admin_user_reads.tables") as mock_tables, ): # Mock profiles query returning a malformed profile without profileId mock_cascade_tables.profiles.query.return_value = { @@ -5937,7 +5934,7 @@ def test_get_user_shared_campaigns_paginates( lambda_context: Any, ) -> None: """Test that shared campaign retrieval follows DynamoDB pagination.""" - with patch("src.handlers.admin_operations.query_all_items") as mock_query_all: + with patch("src.handlers.admin_user_reads.query_all_items") as mock_query_all: mock_query_all.return_value = [ {"sharedCampaignCode": f"CODE{i}", "campaignName": f"Campaign {i}"} for i in range(3) ] @@ -5965,9 +5962,9 @@ def test_get_user_shared_campaigns_batch_resolves_catalogs( ] with ( - patch("src.handlers.admin_operations.query_all_items", return_value=campaigns), + patch("src.handlers.admin_user_reads.query_all_items", return_value=campaigns), patch("src.utils.dynamodb.get_dynamodb_resource") as mock_resource, - patch("src.handlers.admin_operations.tables") as mock_tables, + patch("src.handlers.admin_user_reads.tables") as mock_tables, ): mock_resource.return_value.batch_get_item.return_value = { "Responses": {"kernelworx-catalogs-ue1-dev": [catalog]}, @@ -5994,7 +5991,7 @@ def test_get_user_shared_campaigns_chunks_batch_get_item( campaigns = [{"sharedCampaignCode": f"CODE{i}", "catalogId": f"CATALOG#{i}"} for i in range(150)] with ( - patch("src.handlers.admin_operations.query_all_items", return_value=campaigns), + patch("src.handlers.admin_user_reads.query_all_items", return_value=campaigns), patch("src.utils.dynamodb.get_dynamodb_resource") as mock_resource, ): mock_resource.return_value.batch_get_item.return_value = { @@ -6226,7 +6223,7 @@ def test_get_profile_shares_permissions_set_conversion( admin_appsync_event["info"]["fieldName"] = "adminGetProfileShares" admin_appsync_event["arguments"] = {"profileId": "test-profile"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_share_campaign_ops.tables") as mock_tables: # Return shares where permissions is a Python set mock_tables.shares.query.return_value = { "Items": [ @@ -6254,7 +6251,7 @@ def test_get_profile_shares_permissions_already_list( admin_appsync_event["info"]["fieldName"] = "adminGetProfileShares" admin_appsync_event["arguments"] = {"profileId": "test-profile"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_share_campaign_ops.tables") as mock_tables: # Return shares where permissions is already a list (not a set) mock_tables.shares.query.return_value = { "Items": [ @@ -6294,7 +6291,7 @@ def test_get_profile_shares_paginates( lambda_context: Any, ) -> None: """Test that share retrieval follows DynamoDB pagination.""" - with patch("src.handlers.admin_operations.query_all_items") as mock_query_all: + with patch("src.handlers.admin_share_campaign_ops.query_all_items") as mock_query_all: mock_query_all.return_value = [ { "profileId": "PROFILE#test", @@ -6565,7 +6562,7 @@ def test_get_user_profiles_unexpected_error( admin_appsync_event["info"]["fieldName"] = "adminGetUserProfiles" admin_appsync_event["arguments"] = {"accountId": "test-user"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_reads.tables") as mock_tables: mock_tables.profiles.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6583,7 +6580,7 @@ def test_get_user_catalogs_unexpected_error( admin_appsync_event["info"]["fieldName"] = "adminGetUserCatalogs" admin_appsync_event["arguments"] = {"accountId": "test-user"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_reads.tables") as mock_tables: mock_tables.catalogs.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6601,7 +6598,7 @@ def test_get_user_campaigns_unexpected_error( admin_appsync_event["info"]["fieldName"] = "adminGetUserCampaigns" admin_appsync_event["arguments"] = {"accountId": "test-user"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_reads.tables") as mock_tables: mock_tables.profiles.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6619,7 +6616,7 @@ def test_get_user_shared_campaigns_unexpected_error( admin_appsync_event["info"]["fieldName"] = "adminGetUserSharedCampaigns" admin_appsync_event["arguments"] = {"accountId": "test-user"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_user_reads.tables") as mock_tables: mock_tables.shared_campaigns.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6637,7 +6634,7 @@ def test_get_profile_shares_unexpected_error( admin_appsync_event["info"]["fieldName"] = "adminGetProfileShares" admin_appsync_event["arguments"] = {"profileId": "test-profile"} - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_share_campaign_ops.tables") as mock_tables: mock_tables.shares.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6658,7 +6655,7 @@ def test_delete_share_unexpected_error( "targetAccountId": "target-user", } - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_share_campaign_ops.tables") as mock_tables: mock_tables.shares.delete_item.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -6679,7 +6676,7 @@ def test_update_campaign_shared_code_unexpected_error( "sharedCampaignCode": "CODE123", } - with patch("src.handlers.admin_operations.tables") as mock_tables: + with patch("src.handlers.admin_share_campaign_ops.tables") as mock_tables: mock_tables.campaigns.query.side_effect = RuntimeError("Unexpected failure") result = lambda_handler(admin_appsync_event, lambda_context) @@ -7221,12 +7218,12 @@ def test_reset_password_records_the_actor( event = {**admin_appsync_event, "arguments": {"email": "target@example.com"}} with ( - patch("src.handlers.admin_operations.get_cognito_client"), + patch("src.handlers.admin_credential_reset.get_cognito_client"), patch( - "src.handlers.admin_operations._find_user_by_email", + "src.handlers.admin_credential_reset._find_user_by_email", return_value="target", ), - patch("src.handlers.admin_operations._initiate_password_reset"), + patch("src.handlers.admin_credential_reset._initiate_password_reset"), ): assert admin_reset_user_password(event, lambda_context) is True diff --git a/tests/unit/test_cognito_filter_call_sites.py b/tests/unit/test_cognito_filter_call_sites.py index 3f12bd49d..6c7b91437 100644 --- a/tests/unit/test_cognito_filter_call_sites.py +++ b/tests/unit/test_cognito_filter_call_sites.py @@ -14,7 +14,13 @@ import pytest -from src.handlers import account_operations, admin_account_purge, admin_operations, admin_user_operations, pre_signup +from src.handlers import ( + account_operations, + admin_account_purge, + admin_credential_reset, + admin_user_operations, + pre_signup, +) from src.utils.errors import AppError, ErrorCode POOL = "us-east-1_TEST123" @@ -55,7 +61,7 @@ ), ( "admin _find_user_by_email", - admin_operations._find_user_by_email, + admin_credential_reset._find_user_by_email, "email", "user@example.com", 'email = "user@example.com"', From bba89e0ad408a758bc0287ca9692eceabfd81500 Mon Sep 17 00:00:00 2001 From: David Meiser Date: Mon, 26 Oct 2026 06:52:40 -0400 Subject: [PATCH 6/6] refactor(handlers): extract the admin dispatcher into admin_dispatcher Slice 6 (final) of #773. The operation map and the lambda_handler entry point move byte-identical from admin_operations.py into a new admin_dispatcher.py module. admin_operations.py keeps the shared helpers the admin_* modules import (audit/auth helpers, throttling and batch-lookup-error classifiers) and re-exports lambda_handler, so the Terraform-wired Lambda entry point handlers.admin_operations.lambda_handler is unchanged: no IaC change, no AppSync change. Also corrects two stale module pointers in AGENTS.md (the dispatcher pattern example and the admin list handler locations). --- AGENTS.md | 4 +- src/handlers/admin_dispatcher.py | 105 +++++++++++++++++++++++++ src/handlers/admin_operations.py | 131 +++---------------------------- 3 files changed, 120 insertions(+), 120 deletions(-) create mode 100644 src/handlers/admin_dispatcher.py diff --git a/AGENTS.md b/AGENTS.md index 846522dfe..61b9930df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -151,7 +151,7 @@ Read paths: `listMyProfiles` is a pipeline (`list_my_profiles` → `batch_latest ### Batch-resolving `Campaign.catalog`/`SharedCampaign.catalog` (#332, admin Lambda side) -The `Campaign.catalog`/`SharedCampaign.catalog` field resolvers are pipelines whose first function, `check_source_catalog_fn.js`, early-returns whenever the parent already attached `catalog` to the source item — so any list-returning resolver can kill the per-item GetItem N+1 by batch-attaching catalogs itself. The #332-era list pipelines do this in AppSync (`batch_get_catalogs_fn.js`, `batch_get_shared_campaign_catalogs_fn.js`, shared helpers in `js-resolvers/lib/batch_catalogs.js`), where >100 distinct ids on a page must skip/error because a pipeline function cannot loop. The admin list handlers `admin_get_user_campaigns`/`admin_get_user_shared_campaigns` in `src/handlers/admin_operations.py` do the same inside the Lambda (`_batch_get_campaign_catalogs`: chunked BatchGetItem with 100-key chunks and UnprocessedKeys retry) — Lambdas can loop, so there is no page-size cap. Contracts differ per type and must be preserved when touching either side: Campaign.catalog returns the raw item including soft-deleted catalogs; SharedCampaign.catalog maps soft-deleted or missing catalogs to null. Missing catalog ids always map to `catalog = null` (never omit the key — omission would re-trigger the field resolver's GetItem). +The `Campaign.catalog`/`SharedCampaign.catalog` field resolvers are pipelines whose first function, `check_source_catalog_fn.js`, early-returns whenever the parent already attached `catalog` to the source item — so any list-returning resolver can kill the per-item GetItem N+1 by batch-attaching catalogs itself. The #332-era list pipelines do this in AppSync (`batch_get_catalogs_fn.js`, `batch_get_shared_campaign_catalogs_fn.js`, shared helpers in `js-resolvers/lib/batch_catalogs.js`), where >100 distinct ids on a page must skip/error because a pipeline function cannot loop. The admin list handlers `admin_get_user_campaigns`/`admin_get_user_shared_campaigns` in `src/handlers/admin_user_reads.py` do the same inside the Lambda (`_batch_get_campaign_catalogs`: chunked BatchGetItem with 100-key chunks and UnprocessedKeys retry) — Lambdas can loop, so there is no page-size cap. Contracts differ per type and must be preserved when touching either side: Campaign.catalog returns the raw item including soft-deleted catalogs; SharedCampaign.catalog maps soft-deleted or missing catalogs to null. Missing catalog ids always map to `catalog = null` (never omit the key — omission would re-trigger the field resolver's GetItem). ### AppSync resolver bundling prerequisite (#277/#282/#288) @@ -188,7 +188,7 @@ This is a conscious, documented security posture. If schema-level owner authoriz ### Lambda exception-handling decorator (#294, contract changed by #329) -`src/utils/handlers.py` provides the `lambda_handler` decorator (imported in handler modules as `with_error_handling`). Since #329 it does NOT re-raise: both `AppError` and unexpected exceptions are converted to a returned structured payload `{"__isError": True, "errorCode": ..., "message": ...}` (re-raising would let Lambda serialize only `errorType`/`errorMessage` and lose the code). The 16 admin-operations UNIT resolvers (everything on the `admin_operations` Lambda datasource in `tofu/application/modules/appsync/resolvers_mutations.tf`/`resolvers_queries.tf`) run `lambda_passthrough_resolver.js`, whose `request()` forwards `{arguments, identity, info, prev, stash}` to the handler (the dispatcher needs `info.fieldName`; `is_admin` needs `identity.claims`) and whose `response()` detects `ctx.result.__isError` and calls `util.error(message, errorCode, null, { errorCode })` so the code lands in GraphQL extensions; the frontend's `getErrorCode()` reads `extensions.errorCode`, `extensions.code`, and `extensions.errorType` (#329). The 11 direct Lambda UNIT resolvers on the other decorator-wrapped handlers (listCatalogsInUse, listUnitCatalogs, getUnitReport, listUnitCampaignCatalogs, deleteMyAccount, transferProfileOwnership, requestCampaignReport, request/deletePaymentMethodQRCode*, and the API-key `publicGetOrderOffer`/`publicGetOrderReceipt` fields on the public-orders Lambda, where `identity` is null under API_KEY auth and is never read) run `lambda_unit_resolver.js`, which mirrors the default whole-context Invoke payload and applies the same `__isError` detection. The pipeline functions invoking decorated handlers detect `__isError` too: `delete_campaign_orders_lambda_fn.js` util.errors to abort the deleteCampaign pipeline so `delete_campaign` never runs after a failed order-deletion step, `delete_profile_cascade_fn.js`, `confirm_qr_upload_fn.js`, and `batch_qr_urls_fn.js` util.error, the best-effort `delete_payment_method_qr_code_fn.js` swallows it exactly like `ctx.error`, and the best-effort public-orders mailer step `send_public_order_emails_fn.js` swallows both invoke-level `ctx.error` and a structured `__isError`, reporting `confirmationEmailSent: false` instead of failing the order. The generic path logs an error naming the function and returns `INTERNAL_ERROR`; use `@lambda_handler(error_message="...")` for a client-facing message. Only the generic unexpected-exception path is covered — handler-specific typed errors (e.g. retryable `RESOURCE_BUSY` in `_raise_batch_lookup_error`) must stay in the handler/helpers, which is why those helpers' tests still assert `pytest.raises(AppError)`. All AppSync resolver handlers are migrated (#337); handler modules import it under `TYPE_CHECKING` for mypy (relative) with an absolute/relative runtime fallback (see `admin_operations.py`). The Cognito triggers are deliberately NOT decorated: `pre_signup.py` must propagate `FederatedIdentityLinkedException` and original error messages to Cognito, and `post_authentication.py` deliberately swallows exceptions and returns the event so auth never fails on DynamoDB issues. They still log through the project's structured logger — `get_logger(__name__)` from `src/utils/logging.py` (absolute import with a relative fallback) with message + keyword fields, never `logging.getLogger()` at module scope, which mutates the root logger and emits plain-text lines that are not machine-parseable (#565). The single canonical error-code vocabulary is `ErrorCode` in `src/utils/errors.py`; JS resolvers and VTL templates should emit those exact strings (e.g. `INVALID_INPUT`, `NOT_FOUND`, `FORBIDDEN`, `UNAUTHORIZED`, `INTERNAL_ERROR`), not HTTP-style spellings like `BadRequest` (#329 aligned the HTTP-style spellings; legacy types such as `ConflictException` (also the payment-method optimistic-lock conflict from #433), `CatalogInUse`, `RateLimitExceeded`, and `ALREADY_SHARED` remain in some share/invite/usage resolvers). `UNAUTHORIZED` means "session invalid, sign in again" (frontend `isAuthError`/`mapErrorCodeToMessage`): reserve it for a missing/invalid caller identity, and use `FORBIDDEN` for a failed ownership/permission condition on an already-authenticated caller (#439, #571). Overload stub lines in `src/utils/handlers.py` carry `pragma: no cover` (type-checking-only); the module must stay at 100% coverage. +`src/utils/handlers.py` provides the `lambda_handler` decorator (imported in handler modules as `with_error_handling`). Since #329 it does NOT re-raise: both `AppError` and unexpected exceptions are converted to a returned structured payload `{"__isError": True, "errorCode": ..., "message": ...}` (re-raising would let Lambda serialize only `errorType`/`errorMessage` and lose the code). The 16 admin-operations UNIT resolvers (everything on the `admin_operations` Lambda datasource in `tofu/application/modules/appsync/resolvers_mutations.tf`/`resolvers_queries.tf`) run `lambda_passthrough_resolver.js`, whose `request()` forwards `{arguments, identity, info, prev, stash}` to the handler (the dispatcher needs `info.fieldName`; `is_admin` needs `identity.claims`) and whose `response()` detects `ctx.result.__isError` and calls `util.error(message, errorCode, null, { errorCode })` so the code lands in GraphQL extensions; the frontend's `getErrorCode()` reads `extensions.errorCode`, `extensions.code`, and `extensions.errorType` (#329). The 11 direct Lambda UNIT resolvers on the other decorator-wrapped handlers (listCatalogsInUse, listUnitCatalogs, getUnitReport, listUnitCampaignCatalogs, deleteMyAccount, transferProfileOwnership, requestCampaignReport, request/deletePaymentMethodQRCode*, and the API-key `publicGetOrderOffer`/`publicGetOrderReceipt` fields on the public-orders Lambda, where `identity` is null under API_KEY auth and is never read) run `lambda_unit_resolver.js`, which mirrors the default whole-context Invoke payload and applies the same `__isError` detection. The pipeline functions invoking decorated handlers detect `__isError` too: `delete_campaign_orders_lambda_fn.js` util.errors to abort the deleteCampaign pipeline so `delete_campaign` never runs after a failed order-deletion step, `delete_profile_cascade_fn.js`, `confirm_qr_upload_fn.js`, and `batch_qr_urls_fn.js` util.error, the best-effort `delete_payment_method_qr_code_fn.js` swallows it exactly like `ctx.error`, and the best-effort public-orders mailer step `send_public_order_emails_fn.js` swallows both invoke-level `ctx.error` and a structured `__isError`, reporting `confirmationEmailSent: false` instead of failing the order. The generic path logs an error naming the function and returns `INTERNAL_ERROR`; use `@lambda_handler(error_message="...")` for a client-facing message. Only the generic unexpected-exception path is covered — handler-specific typed errors (e.g. retryable `RESOURCE_BUSY` in `_raise_batch_lookup_error`) must stay in the handler/helpers, which is why those helpers' tests still assert `pytest.raises(AppError)`. All AppSync resolver handlers are migrated (#337); handler modules import it under `TYPE_CHECKING` for mypy (relative) with an absolute/relative runtime fallback (see `admin_dispatcher.py`). The Cognito triggers are deliberately NOT decorated: `pre_signup.py` must propagate `FederatedIdentityLinkedException` and original error messages to Cognito, and `post_authentication.py` deliberately swallows exceptions and returns the event so auth never fails on DynamoDB issues. They still log through the project's structured logger — `get_logger(__name__)` from `src/utils/logging.py` (absolute import with a relative fallback) with message + keyword fields, never `logging.getLogger()` at module scope, which mutates the root logger and emits plain-text lines that are not machine-parseable (#565). The single canonical error-code vocabulary is `ErrorCode` in `src/utils/errors.py`; JS resolvers and VTL templates should emit those exact strings (e.g. `INVALID_INPUT`, `NOT_FOUND`, `FORBIDDEN`, `UNAUTHORIZED`, `INTERNAL_ERROR`), not HTTP-style spellings like `BadRequest` (#329 aligned the HTTP-style spellings; legacy types such as `ConflictException` (also the payment-method optimistic-lock conflict from #433), `CatalogInUse`, `RateLimitExceeded`, and `ALREADY_SHARED` remain in some share/invite/usage resolvers). `UNAUTHORIZED` means "session invalid, sign in again" (frontend `isAuthError`/`mapErrorCodeToMessage`): reserve it for a missing/invalid caller identity, and use `FORBIDDEN` for a failed ownership/permission condition on an already-authenticated caller (#439, #571). Overload stub lines in `src/utils/handlers.py` carry `pragma: no cover` (type-checking-only); the module must stay at 100% coverage. ### DynamoDB lookup failures: one classification, and never a silent partial answer (#291, #456, #556) diff --git a/src/handlers/admin_dispatcher.py b/src/handlers/admin_dispatcher.py new file mode 100644 index 000000000..8b9b7fc21 --- /dev/null +++ b/src/handlers/admin_dispatcher.py @@ -0,0 +1,105 @@ +""" +Dispatcher for the shared admin-operations Lambda (#773). + +Owns the operation map and the ``lambda_handler`` entry point: one Lambda +serves every admin operation, and this module routes on the GraphQL field +name to the handler that owns it. The individual handlers live in the +sibling ``admin_*`` modules extracted in #773; this module only imports +them, so the operation map is the single registry of what the admin Lambda +serves. + +``admin_operations`` re-exports ``lambda_handler`` so the Terraform-wired +Lambda entry point (``handlers.admin_operations.lambda_handler``) is +unchanged by the split; the AppSync resolver mapping is untouched. +""" + +from typing import TYPE_CHECKING, Any, Dict + +# Handle both Lambda (absolute) and unit test (relative) imports +try: # pragma: no cover + from utils.errors import AppError, ErrorCode +except ModuleNotFoundError: # pragma: no cover + from ..utils.errors import AppError, ErrorCode + +# The decorator stays typed for mypy via the relative import below; at runtime +# the absolute import resolves in the Lambda zip (package `utils`) and the +# relative fallback resolves in unit tests (package `src.handlers`). +if TYPE_CHECKING: # pragma: no cover + from ..utils.handlers import lambda_handler as with_error_handling +else: # pragma: no cover + try: + from utils.handlers import lambda_handler as with_error_handling + except ModuleNotFoundError: + from ..utils.handlers import lambda_handler as with_error_handling + +# The operation handlers were extracted to their own modules (#773) but stay +# registered on this Lambda. The imports are safe at module load because +# every `admin_*` module imports its shared helpers from `admin_operations` +# inside functions, never at module scope. +from .admin_account_purge import admin_purge_user_account +from .admin_catalog_operations import create_managed_catalog +from .admin_credential_reset import admin_reset_user_password +from .admin_share_campaign_ops import ( + admin_delete_share, + admin_get_profile_shares, + admin_update_campaign_shared_code, +) +from .admin_user_deletion import ( + admin_delete_user_campaigns, + admin_delete_user_orders, + admin_delete_user_profiles, + admin_delete_user_shares, +) +from .admin_user_operations import admin_list_users, admin_search_user +from .admin_user_reads import ( + admin_get_user_campaigns, + admin_get_user_catalogs, + admin_get_user_profiles, + admin_get_user_shared_campaigns, +) + +# Operation dispatcher map +_OPERATION_HANDLERS = { + "adminListUsers": admin_list_users, + "adminSearchUser": admin_search_user, + "adminResetUserPassword": admin_reset_user_password, + "adminPurgeUserAccount": admin_purge_user_account, + "adminDeleteUserOrders": admin_delete_user_orders, + "adminDeleteUserCampaigns": admin_delete_user_campaigns, + "adminDeleteUserShares": admin_delete_user_shares, + "adminDeleteUserProfiles": admin_delete_user_profiles, + "createManagedCatalog": create_managed_catalog, + "adminGetUserProfiles": admin_get_user_profiles, + "adminGetUserCatalogs": admin_get_user_catalogs, + "adminGetUserCampaigns": admin_get_user_campaigns, + "adminGetUserSharedCampaigns": admin_get_user_shared_campaigns, + "adminGetProfileShares": admin_get_profile_shares, + "adminDeleteShare": admin_delete_share, + "adminUpdateCampaignSharedCode": admin_update_campaign_shared_code, +} + + +@with_error_handling(error_message="Failed to execute admin operation") +def lambda_handler(event: Dict[str, Any], context: Any) -> Any: + """ + Main Lambda handler that dispatches to specific admin operations. + + Determines which operation to call based on the GraphQL field name. + Wrapped in ``with_error_handling`` (#329) so dispatch-level errors also + return the structured ``__isError`` payload instead of raising. + + Args: + event: AppSync event + context: Lambda context + + Returns: + Result from the specific operation handler, or a structured error + payload when the operation fails. + """ + field_name = event.get("info", {}).get("fieldName", "") + handler = _OPERATION_HANDLERS.get(field_name) + + if not handler: + raise AppError(ErrorCode.INVALID_INPUT, f"Unknown admin operation: {field_name}") + + return handler(event, context) diff --git a/src/handlers/admin_operations.py b/src/handlers/admin_operations.py index f426d20b5..0c6c920a9 100644 --- a/src/handlers/admin_operations.py +++ b/src/handlers/admin_operations.py @@ -1,14 +1,13 @@ """ -Admin operations handlers for superadmin functionality. +Shared helpers for the admin-operations handlers. -Provides: -- adminResetUserPassword: Send password reset email to user - -The adminListUsers / adminSearchUser handlers live in ``admin_user_operations``, -the adminPurgeUserAccount handler lives in ``admin_account_purge``, the -createManagedCatalog handler lives in ``admin_catalog_operations``, and the -per-entity adminDeleteUser* handlers live in ``admin_user_deletion`` (#773); -all four are imported below so the dispatcher map stays the single registry. +The #773 split moved every admin operation handler out of this module into +its own module; what remains here is what those modules import from it — the +admin auth/audit helpers and the throttling/batch-lookup-error classifiers — +plus the dispatcher re-export. ``admin_dispatcher`` owns the operation map +and ``lambda_handler``; this module re-exports it so the Terraform-wired +Lambda entry point (``handlers.admin_operations.lambda_handler``) is +unchanged. User deletion is client-side by decision (#521). The client issues the per-entity ``adminDeleteUser*`` mutations it can reach, reads the profile IDs @@ -29,7 +28,7 @@ principals. """ -from typing import TYPE_CHECKING, Any, Dict, NoReturn +from typing import Any, Dict, NoReturn from botocore.exceptions import ClientError @@ -45,67 +44,10 @@ from ..utils.dynamodb import TRANSIENT_ERROR_CODES from ..utils.errors import AppError, ErrorCode -# The decorator stays typed for mypy via the relative import below; at runtime -# the absolute import resolves in the Lambda zip (package `utils`) and the -# relative fallback resolves in unit tests (package `src.handlers`). -if TYPE_CHECKING: # pragma: no cover - from ..utils.handlers import lambda_handler as with_error_handling -else: # pragma: no cover - try: - from utils.handlers import lambda_handler as with_error_handling - except ModuleNotFoundError: - from ..utils.handlers import lambda_handler as with_error_handling - - -# The account purge handler was extracted to its own module (#773) but stays -# registered on this Lambda. The import is safe at module load because -# `admin_account_purge` only imports from this module inside functions. -from .admin_account_purge import admin_purge_user_account - -# The managed-catalog handler was extracted to its own module (#773) but stays -# registered on this Lambda. The import is safe at module load because -# `admin_catalog_operations` does not import from this module at all. -from .admin_catalog_operations import create_managed_catalog - -# The password-reset handler was extracted to its own module (#773) but stays -# registered on this Lambda. The import is safe at module load because -# `admin_credential_reset` only imports from this module inside functions. -from .admin_credential_reset import admin_reset_user_password - -# The profile-share and campaign shared-code handlers were extracted to their -# own module (#773) but stay registered on this Lambda. The import is safe at -# module load because `admin_share_campaign_ops` only imports from this module -# inside functions. -from .admin_share_campaign_ops import ( - admin_delete_share, - admin_get_profile_shares, - admin_update_campaign_shared_code, -) - -# The per-entity user deletion handlers were extracted to their own module (#773) -# but stay registered on this Lambda. The import is safe at module load because -# `admin_user_deletion` only imports from this module inside functions. -from .admin_user_deletion import ( - admin_delete_user_campaigns, - admin_delete_user_orders, - admin_delete_user_profiles, - admin_delete_user_shares, -) - -# The user listing & search handlers were extracted to their own module (#773) but -# stay registered on this Lambda. The import is safe at module load because -# `admin_user_operations` only imports from this module inside functions. -from .admin_user_operations import admin_list_users, admin_search_user - -# The account-scoped admin user read handlers were extracted to their own module -# (#773) but stay registered on this Lambda. The import is safe at module load -# because `admin_user_reads` only imports from this module inside functions. -from .admin_user_reads import ( - admin_get_user_campaigns, - admin_get_user_catalogs, - admin_get_user_profiles, - admin_get_user_shared_campaigns, -) +# The dispatcher was extracted to ``admin_dispatcher`` (#773). The Lambda +# entry point stays ``handlers.admin_operations.lambda_handler`` (wired in +# ``tofu/application/modules/lambda/main.tf``), so it is re-exported here. +from .admin_dispatcher import lambda_handler # noqa: F401 # DynamoDB/Cognito throttling codes: the lookup is retryable, so surface a # RESOURCE_BUSY error instead of silently incomplete admin data (#291, #456). @@ -222,50 +164,3 @@ def _require_admin_and_get_target_account_id(event: Dict[str, Any]) -> tuple[str raise AppError(ErrorCode.INVALID_INPUT, "Account ID is required") return account_id, actor_sub - - -# Operation dispatcher map -_OPERATION_HANDLERS = { - "adminListUsers": admin_list_users, - "adminSearchUser": admin_search_user, - "adminResetUserPassword": admin_reset_user_password, - "adminPurgeUserAccount": admin_purge_user_account, - "adminDeleteUserOrders": admin_delete_user_orders, - "adminDeleteUserCampaigns": admin_delete_user_campaigns, - "adminDeleteUserShares": admin_delete_user_shares, - "adminDeleteUserProfiles": admin_delete_user_profiles, - "createManagedCatalog": create_managed_catalog, - "adminGetUserProfiles": admin_get_user_profiles, - "adminGetUserCatalogs": admin_get_user_catalogs, - "adminGetUserCampaigns": admin_get_user_campaigns, - "adminGetUserSharedCampaigns": admin_get_user_shared_campaigns, - "adminGetProfileShares": admin_get_profile_shares, - "adminDeleteShare": admin_delete_share, - "adminUpdateCampaignSharedCode": admin_update_campaign_shared_code, -} - - -@with_error_handling(error_message="Failed to execute admin operation") -def lambda_handler(event: Dict[str, Any], context: Any) -> Any: - """ - Main Lambda handler that dispatches to specific admin operations. - - Determines which operation to call based on the GraphQL field name. - Wrapped in ``with_error_handling`` (#329) so dispatch-level errors also - return the structured ``__isError`` payload instead of raising. - - Args: - event: AppSync event - context: Lambda context - - Returns: - Result from the specific operation handler, or a structured error - payload when the operation fails. - """ - field_name = event.get("info", {}).get("fieldName", "") - handler = _OPERATION_HANDLERS.get(field_name) - - if not handler: - raise AppError(ErrorCode.INVALID_INPUT, f"Unknown admin operation: {field_name}") - - return handler(event, context)