feat(policies): add policy CRUD API with RBAC and integration tests:#62
feat(policies): add policy CRUD API with RBAC and integration tests:#62
Conversation
bihius
commented
Mar 19, 2026
bihius
commented
- add /policies endpoints for list/detail/create/partial update/delete
- enforce RBAC - admin for write operations, viewer/admin for read operations
- persist createted_by from authenticated admin on policy creation
- register policies router in main.py
- add integration tests for happy paths, auth errors (401, 403), not found (404), and duplicate name conflicts (409)
- fix test DB teardown by guarding rollback with transaction.is_active in tests/conftest.py
Add /policies endpoints for list/detail/create/partial update/delete with admin write access and viewer/admin read access. Save created_by from authenticated admin, register policies router in main app, cover 401/403/404/409 flows in integration tests, and guard fixture rollback with transaction.is_active to remove SQLAlchemy warnings.
There was a problem hiding this comment.
Pull request overview
Adds a new FastAPI router implementing CRUD endpoints for WAF policies, wires it into the application, and introduces integration tests validating RBAC behavior and common error cases. This expands the backend API surface to manage Policy entities consistently with existing auth/dependency patterns.
Changes:
- Implement
/policiesCRUD endpoints with RBAC (admin-only writes; authenticated reads) andcreated_byauditing on create. - Register the policies router in
app/main.py. - Add integration tests for happy paths and key error cases; adjust test DB teardown rollback behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/backend/app/routers/policies.py |
New CRUD router for Policy, including RBAC and conflict handling. |
src/backend/app/main.py |
Includes the new policies router in the FastAPI app. |
src/backend/tests/integration/test_policies_router.py |
New integration test suite covering CRUD, RBAC, and error conditions. |
src/backend/tests/conftest.py |
Guards rollback in fixture teardown with transaction.is_active. |
notes/daily/2026-02-16.md |
Removed a daily note file (appears unrelated to backend API changes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for field, value in body.model_dump(exclude_unset=True).items(): | ||
| setattr(policy, field, value) | ||
|
|
||
| try: | ||
| db.commit() | ||
| except IntegrityError: | ||
| db.rollback() | ||
| raise HTTPException( | ||
| status_code=status.HTTP_409_CONFLICT, | ||
| detail="Policy name already exists", | ||
| ) |
There was a problem hiding this comment.
In update_policy, body.model_dump(exclude_unset=True) will include keys explicitly set to null (e.g. { "name": null }). That will set non-nullable DB columns to None, triggering an IntegrityError that is then reported as 409 Policy name already exists, which is misleading. Consider rejecting null for non-nullable fields (422) and/or handling IntegrityError more precisely (e.g., only map unique-constraint violations to 409).
| def _create_policy( | ||
| client: TestClient, | ||
| headers: dict[str, str], | ||
| *, | ||
| name: str = "Default Policy", | ||
| description: str = "Domyslna polityka", | ||
| paranoia_level: int = 2, | ||
| anomaly_threshold: int = 5, | ||
| ) -> dict: |
There was a problem hiding this comment.
Helper _create_policy is annotated as returning bare dict, which loses type information and can violate strict typing checks. Prefer a parameterized type like dict[str, Any]/Mapping[str, Any] (and optionally narrow the expected keys) to match the typing style used in other integration tests.
| app.include_router(auth.router) | ||
| app.include_router(policies.router) |
There was a problem hiding this comment.
This PR is described as adding the policies CRUD API + tests, but it also deletes notes/daily/2026-02-16.md, which looks unrelated to backend API work. Consider reverting/removing that deletion (or moving it to a separate PR) to keep the change set focused and easier to review/audit.
- Add checking for null data in update_policy and another error code for that - 422 - New helper for testing: _is_policy_name_unique_violation -> other errors will give silenced IntegrityError - _create_now have more specific return type - added new tests for null data
