feat(cloud): workspace model + role-based access (C1)#365
Merged
Conversation
First slice of Track C (cloud multitenancy). Adds Workspace + WorkspaceUser models (owner/editor/viewer membership, unique per user/workspace), core/permissions.py with a role hierarchy, check_workspace_access (pure, unit-tested), and a require_workspace_access FastAPI dependency factory for workspace-scoped routes. Migration 003 creates both tables; conftest registers the models. Tests cover relationships, the unique constraint, the role hierarchy, and access checks. (Single-user auto 'Personal' workspace bootstrap + route wiring are C2.) Verified: web-backend pytest 4/4 pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds workspace multitenancy support with new ORM models, a migration for the underlying tables, hierarchical workspace access helpers, and tests covering relationships, constraints, and authorization behavior. ChangesWorkspace Multitenancy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…068) Sonar flagged the literal placeholder assigned to hashed_password as a hard-coded credential (python:S2068), tripping the security-rating gate. Generate the throwaway hash with secrets.token_hex instead — same intent (no real auth in these tests), no literal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web-backend/core/permissions.py`:
- Around line 42-70: The workspace authorization helpers currently ignore
Workspace.owner_id and only derive access from WorkspaceUser membership, so
ownership can drift from the actual permission check. Update
user_role_in_workspace and check_workspace_access to treat the workspace owner
as having owner-level access when Workspace.owner_id matches the user, likely by
querying Workspace alongside WorkspaceUser and mapping owners to the highest
WorkspaceRole before applying role_satisfies. Ensure the logic stays consistent
with existing role handling and continues to fall back to membership-based roles
for non-owners.
In `@apps/web-backend/migrations/versions/003_create_workspaces.py`:
- Around line 37-51: The workspace owner relationship in the migration is using
a cascading foreign key that can delete the whole workspace when a user is
removed. Update the foreign key defined in the workspace table migration so the
owner reference in the create_workspaces migration no longer uses
ondelete="CASCADE"; keep the constraint but make deletions non-cascading and
handle owner transfer or explicit workspace deletion in application logic
instead. Use the owner_id foreign key and the workspace table definition to
locate the change.
- Around line 62-76: The workspace membership table currently accepts any string
in the role column, which allows invalid values to persist even though
WorkspaceRole in core.permissions is a closed set. Update the migration that
defines the workspace membership table to enforce the allowed role values at the
database level, using the existing role column in the membership table
definition and a schema constraint or equivalent DB enum check so only valid
WorkspaceRole entries can be stored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3fb9e4a-c62d-4eb6-9a0a-13f09252b1df
📒 Files selected for processing (5)
apps/web-backend/api/models/workspace.pyapps/web-backend/core/permissions.pyapps/web-backend/migrations/versions/003_create_workspaces.pyapps/web-backend/tests/conftest.pyapps/web-backend/tests/test_workspaces.py
…1 review) Address review on the workspace foundation: - permissions: user_role_in_workspace now treats Workspace.owner_id as owner-level access, even without a WorkspaceUser row — so the single-user 'Personal' workspace owner actually satisfies every check (was previously membership-only, contradicting the docstring). - schema (model + migration 003): owner_id FK is RESTRICT, not CASCADE, so deleting a user no longer silently destroys their whole workspace; ownership transfer/deletion is explicit app logic (Track C). - schema (model + migration 003): workspace_users.role gets a CHECK constraint mirroring the WorkspaceRole closed set, so invalid roles cannot persist. Added tests: owner-without-membership access, and DB rejection of an invalid role (IntegrityError). 6/6 pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-import) conftest imports Workspace/WorkspaceUser purely to register them with Base.metadata. CodeQL's py/unused-import ignores ruff's # noqa, so it flagged them as unused — unlike User/GitRepository, which were already in __all__. Add the two models to __all__ (re-export = used), matching the existing pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web-backend/tests/test_workspaces.py`:
- Around line 109-120: Tighten
test_workspace_owner_has_owner_access_without_membership so it explicitly
verifies the workspace owner does not have an owner membership row before
asserting access. Use the existing test_db setup plus the Workspace and
user_role_in_workspace/check_workspace_access flow, and add an assertion against
the membership source used by workspace role resolution to confirm the
owner-level access is coming from Workspace.owner_id rather than an auto-created
membership.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: db3deddd-49fb-4cfb-bda2-5f12fcc4ed45
📒 Files selected for processing (5)
apps/web-backend/api/models/workspace.pyapps/web-backend/core/permissions.pyapps/web-backend/migrations/versions/003_create_workspaces.pyapps/web-backend/tests/conftest.pyapps/web-backend/tests/test_workspaces.py
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
First slice of Track C (cloud multitenancy — see docs/strategy/roadmap.md).
api/models/workspace.py):Workspace(tenant boundary, owner) +WorkspaceUser(membership withowner/editor/viewerrole, unique per (workspace, user)).core/permissions.py): hierarchicalWorkspaceRole,check_workspace_access(pure, unit-tested), and arequire_workspace_access(required_role)FastAPI dependency factory for workspace-scoped routes.003_create_workspaces.pycreates both tables (FKs + unique constraint + indexes); conftest registers the models forcreate_all.tests/test_workspaces.py): relationships, unique constraint, role hierarchy, access checks — 4/4 pass; ruff clean.Single-user mode reuses this with one auto-created "Personal" workspace; the bootstrap + route wiring (and per-workspace isolation) are the next slices (C2+).
🤖 Generated with Claude Code
Summary by CodeRabbit