refactor(api.iam): set API Key ONDELETE to RESTRICT #212
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lembic - Add SQLAlchemy 2.0 with asyncpg for async database operations - Add Alembic for schema migrations - Add python-ulid for ULID support instead of UUID - Create read/write engine separation with connection pooling - Create FastAPI dependency injection for database sessions - Create SQLAlchemy declarative base with timestamp mixin - Initialize Alembic with async migration support - Create initial migration for teams table (ULID primary key) - Add comprehensive unit tests for engines and dependencies - Configure Alembic to use settings module for database URL - Enable ruff post-write hook for migration formatting Refs: AIHCM-121
- Add authzed library for SpiceDB integration - Add python-ulid for ULID support - Create ResourceType, RelationType, Permission enums (using Group not Team) - Create AuthorizationProvider protocol for swappable implementations - Implement SpiceDBClient with async methods for relationships and permissions - Create SpiceDB schema (.zed) with Tenant→Workspace→Group hierarchy - Create AuthorizationProbe for domain-oriented observability - Move ObservationContext to shared_kernel (fix architectural boundary) - Add 35 unit tests for types and probes - All 410 tests passing Refs: AIHCM-122
Resolved conflicts in authorization files by accepting remote changes: - shared_kernel/authorization/types.py (docstring fix) - shared_kernel/authorization/spicedb/client.py (_parse_reference helper)
Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…dd route documentation Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…th TDD Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Fix workspace definition in schema.zed and ConfigMap to match the relationships actually created by the IAM outbox translator. Schema changes (workspace definition): - Add `relation tenant: tenant` for organizational ownership - Change `relation parent: tenant` to `relation parent: workspace` for hierarchy - Rename `owner` to `admin` for consistency with tenant/group definitions - Rename `permission delete` to `permission manage` for consistency - Add Phase 3 comments for member/permission usage ConfigMap changes (full sync with schema.zed): - Apply all workspace definition fixes above - Add missing `relation member: user` to tenant definition - Fix tenant `permission view = admin` to `permission view = admin + member` - Add missing `permission administrate = admin` to tenant definition - Add missing `api_key` definition (was in schema.zed but not ConfigMap) - Add future resource type comments Inconsistencies found and documented: 1. Schema had `relation parent: tenant` but translator writes `workspace#tenant@tenant` (relation name 'tenant') and `workspace#parent@workspace` (parent type 'workspace') 2. ConfigMap was missing tenant `member` relation, `administrate` permission, and entire `api_key` definition 3. RelationType.WORKSPACE enum exists but is unused by any translator 4. Permission.DELETE enum value corresponds to removed `permission delete` in workspace; may need cleanup in Phase 3 5. Schema `owner` relation on workspace renamed to `admin` to align with tenant and group naming conventions All 970 unit tests pass (3 pre-existing SSL failures unrelated). Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Remove RelationType.WORKSPACE and Permission.DELETE which have no usage in the codebase. Neither value corresponds to any relation or permission in the current SpiceDB schema. They can be re-added when future resource types (knowledge_graph, data_source) are implemented. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…tion Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…nshift-hyperfleet/kartograph into jsell/feat/AIHCM-146-workspace-authz
Integrates workspace lifecycle with tenant lifecycle (AIHCM-147): - TenantService constructor now accepts workspace_repository - create_tenant auto-provisions a root workspace using settings default_workspace_name, falling back to tenant name - delete_tenant cascades to workspaces before groups/API keys, ensuring WorkspaceDeleted events are emitted for SpiceDB cleanup - TenantServiceProbe updated with workspaces_count parameter - Dependency injection wired up with workspace repository - 8 new unit tests covering create and delete workspace integration Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
The clean_iam_data fixture was missing workspace deletion, causing FK constraint violations when tests created workspaces. Adds workspace cleanup in correct order (children then roots) before tenant deletion, and introduces a TestCleanupProbe following Domain-Oriented Observability to replace silent exception swallowing with structured domain events. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…workspace creation The walking skeleton bootstrap in main.py was creating the default tenant directly via domain aggregates, bypassing TenantService which now auto-creates root workspaces. This resulted in the default tenant having no root workspace. Introduces TenantBootstrapService with minimal dependencies (no authz, no group/API key repos) specifically for startup bootstrap operations. Updates main.py to use this service, ensuring the default tenant gets its root workspace just like tenants created via the API. Also extends StartupProbe with workspace bootstrap observability methods. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SQLAlchemy 2.0 autobegins a transaction on the first query. In create_workspace(), queries were executed before session.begin() was called, causing "A transaction is already begun on this Session" error. Restructured create_workspace() to wrap all operations (validation queries + save) inside a single session.begin() block, matching the pattern used in TenantService.create_tenant(). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The parent_workspace_id foreign key should prevent deleting a parent workspace while children exist. If the database has a different constraint (e.g., SET NULL from an earlier state), this migration fixes it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ldren SQLAlchemy by default sets child foreign keys to NULL before deleting a parent, which bypasses database RESTRICT constraints. Adding passive_deletes=True to the child_workspaces relationship lets the database handle FK constraint enforcement, correctly blocking deletion of workspaces that have children. Also adds integration tests for workspace deletion behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- TenantService: Sort workspaces by depth before deletion (children first) to avoid FK constraint violations - TenantBootstrapService: Use savepoints (begin_nested) for tenant and workspace creation to avoid PendingRollbackError on race conditions - conftest.py: Use cast(CursorResult, ...) for DELETE execute results to satisfy mypy rowcount access - test_workspace_api.py: Add status code assertions before accessing resp.json() in deletion tests - Migration downgrade: Raise NotImplementedError instead of no-op pass Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The migration fdd9505854c5 was unnecessary - the original workspaces table migration (205809969bf4) already specified ON DELETE RESTRICT for the parent_workspace_id FK. The actual fix for parent deletion was adding passive_deletes=True to the SQLAlchemy relationship. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Aligns api_keys FK constraint with the pattern used by groups and workspaces. RESTRICT forces application-layer cascading via TenantService, ensuring APIKeyDeleted events are emitted for SpiceDB cleanup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe changes modify the foreign key constraint on the APIKeyModel.tenant_id field from CASCADE to RESTRICT deletion behavior. This requires tenants to be deleted only after their associated API keys have been removed. A database migration file is added to alter the existing constraint on the api_keys table. Documentation notes are updated to clarify that API key deletion should be handled through service layer events rather than automatic cascade deletion. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit