Skip to content

Feature/manual scan result detail#166

Open
AaronAlijani wants to merge 10 commits into
mainfrom
feature/manual-scan-result-detail
Open

Feature/manual scan result detail#166
AaronAlijani wants to merge 10 commits into
mainfrom
feature/manual-scan-result-detail

Conversation

@AaronAlijani
Copy link
Copy Markdown
Collaborator

@AaronAlijani AaronAlijani commented Apr 15, 2026

Summary

Add the manual_scan_result_detail table to store extra information for manually verified compliance controls. This table links back to scan_result with a one-to-one relationship, to keep scan_result as the single source of truth for all results (automated and manual) and use a detail table for manual-specific fields.
Update :
Add the manual_scan_result_detail table and full CRUD API for manually verified compliance controls. This table links back to scan_result with a one-to-one relationship, keeping scan_result as the single source of truth for all results (automated and manual) while using a detail table for manual-specific fields. Includes Pydantic schemas for request/response validation and API endpoints (POST, GET, PATCH, DELETE) registered in the application router.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactor / code cleanup
  • Documentation
  • CI/CD / infrastructure
  • Security

Affected Components

  • /backend-api
  • /frontend
  • /engine (collectors / policies)
  • /security
  • /infrastructure
  • /.github/workflows
  • /docs

Motivation

Replaces the previous manual_verification table (PR #148) based on team lead feedback. scan_result already holds scan_id, control_id, and status, so a separate table with the same columns creates duplication. This new design keeps scan_result as the single source for all results and adds a small detail table keyed to scan_result.id for manual-specific fields (user_id, comment).

Planner task: 25T3-RES-x-0xx — Build backend for manual control verification

Testing Done

  • Unit tests pass locally

  • Tested manually — describe how:

    • Rebuilt full Docker Compose stack with docker compose --profile all up --build -d
    • Verified migration ran via docker compose exec db psql -U autoaudit -d autoaudit -c "\d manual_scan_result_detail"
    • Confirmed correct columns, primary key, unique constraint on scan_result_id, and foreign keys with CASCADE to scan_result and user
    • All 7 services running healthy via docker compose ps
      Update:
    • Verified all 5 endpoints visible in Swagger docs at localhost:8000/docs (POST, GET by ID, GET by scan_result_id, PATCH, DELETE)
  • No tests required — explain why:

Security Considerations

No security impact. The table stores user_id as a foreign key reference and an optional text comment. No secrets, tokens, or sensitive data involved.

Breaking Changes

  • No breaking changes
  • Yes — describe below:

Rollback Plan

  • Revert commit is sufficient
  • Requires additional steps — describe below:
    • Run docker compose exec backend-api uv run alembic downgrade -1 to drop the manual_scan_result_detail table

Checklist

  • Code follows project conventions
  • No secrets, credentials, or tokens committed
  • Relevant documentation updated (if applicable)
  • CI/CD workflows pass on this branch
  • PR is focused on one thing

Screenshots

Table verification in PostgreSQL:
Screenshot 2026-04-15 125315

Swagger docs screenshot: Prove all 5 endpoints are live and working in the running application.
Screenshot 2026-04-16 120026

Copy link
Copy Markdown
Collaborator

@du-dhartley du-dhartley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronAlijani Pivoting away from the mostly duplicate manual table is good and this structure looks much better. A couple of things to introduce before we can safely commit this code though.

  1. It doesn't appear that there's any ownership enforcement on the endpoints - we should enforce that users can only retrieve and/or update their own scans and scan results. It's worth testing this and including a screenshot, then making the change and making the test again, showing that it no longer works.
  2. You've mentioned that no tests are required in the PR notes, endpoints that require authentication definitely do need tests wrapped around them to prevent the case I described in #1. We need tests that show a successful response when a record is pulled out / patched / deleted and so on, and the expected failure cases (duplicates, nonexistent records, etc)
  3. Is there a way for a user to remove a comment, or have we deliberately prevented this?

@du-dhartley
Copy link
Copy Markdown
Collaborator

@AaronAlijani Did you come back to this PR after the feedback above? The 3 points above need to be addressed, though #3 is more a question at this stage

@AaronAlijani
Copy link
Copy Markdown
Collaborator Author

Thanks David, I appreciate the feedback. I'll address all three:

I'll add ownership checks to the GET, PATCH, and DELETE endpoints so users can only access their own manual verifications. Will include screenshots showing it blocks access to other users' records.
I'll add tests covering successful CRUD operations, rejection of duplicate scan_result_id values, handling of nonexistent records, and enforcement of authentication.
Currently, a user can PATCH with an empty comment, which removes the comment. I think that's fine. If they want to clear their comment, they should be able to. But happy to change this if you'd prefer comments to be permanent once set.
Will push these changes shortly.

@AaronAlijani
Copy link
Copy Markdown
Collaborator Author

I've addressed all three points:

1-Added ownership enforcement, a _check_ownership helper checks detail.user_id against current_user.id and returns 403 Forbidden if they don't match. Applied to GET, PATCH, and DELETE endpoints.
2-Added unit tests in backend-api/tests/test_manual_verification.py
covering successful CRUD operations, duplicate scan_result_id rejection, nonexistent record 404s, and ownership enforcement 403s for GET, PATCH, and DELETE.
3-Regarding comment removal, currently a user can PATCH with a new comment to update it, but cannot set it to null since the update only applies if update. Comment is None. If you'd prefer users to be able to clear their comments, I can change that.

@du-dhartley
Copy link
Copy Markdown
Collaborator

@AaronAlijani Those ownership checks are looking good. The only one left to enforce is the POST - right now it appears as if any user can post a payload such as

{
  scan_result_id: <anything>
}

and become the owner of that manual verification. We should ensure that the POST checks the underlying scan_result record also.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

CI: Backend API

Job Result
Security analysis (CodeQL + Bandit) failure
Lint success

One or more checks failed. View logs

@AaronAlijani
Copy link
Copy Markdown
Collaborator Author

Implemented the POST ownership validation for manual verification records.

The endpoint now:

  • Verifies the referenced "scan_result" exists
  • Checks ownership of the parent's scan`
  • Returns 403 for unauthorised access
  • Handles duplicate manual verification creation with 409

Also added and ran integration tests for:

  • GET nonexistent → 404
  • PATCH nonexistent → 404
  • DELETE nonexistent → 404
  • GET by scan result nonexistent → 404

All manual verification tests are passing locally.
The remaining Bandit warnings are from pre-existing unrelated files (auth.py, evidence.py, init_db.py) outside the scope of this PR.

@AaronAlijani AaronAlijani requested a review from du-dhartley May 27, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants