283 external mode web api contract config fetch external result ingestion#295
Conversation
There was a problem hiding this comment.
Pull request overview
Adds M2M-protected Web API support for “external/private mode” integrations (e.g., GitHub Action) by introducing bearer-token auth plus endpoints to fetch configs by internal ID and ingest externally computed grading results.
Changes:
- Added Bearer-token auth dependency for integration-only endpoints.
- Added
GET /api/v1/configs/id/{config_id}for internal-ID config fetch. - Added
POST /api/v1/submissions/external-resultsplus Pydantic schemas and web tests/docs for external result ingestion.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
web/config/auth.py |
Adds integration token config loader/caching. |
web/api/deps.py |
Introduces require_integration_token FastAPI dependency using HTTPBearer. |
web/api/v1/grading_configs.py |
Adds protected config fetch endpoint by internal DB ID. |
web/api/v1/submissions.py |
Adds protected endpoint to ingest externally computed results into submissions + submission_results. |
web/schemas/submission.py |
Adds ExternalResult* request/response schemas and status enum. |
web/schemas/__init__.py |
Exports the new external-result schemas. |
tests/web/test_external_results.py |
Adds tests for config-by-id and external result ingestion behavior. |
tests/web/test_integration_auth.py |
Adds tests ensuring protected endpoints require token and public endpoints remain public. |
docs/API.md |
Documents auth setup and the new protected endpoints. |
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | ||
| cfg.token = TEST_TOKEN | ||
| auth_module.integration_auth_config = cfg | ||
| yield | ||
| auth_module.integration_auth_config = cfg |
There was a problem hiding this comment.
The _set_integration_token autouse fixture sets auth_module.integration_auth_config but does not restore the previous value in teardown (it reassigns the same cfg). This can leak state across test modules and mask failures around missing/invalid env configuration; capture the old value before overriding and restore it after yield (often back to None).
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | |
| cfg.token = TEST_TOKEN | |
| auth_module.integration_auth_config = cfg | |
| yield | |
| auth_module.integration_auth_config = cfg | |
| old_integration_auth_config = getattr(auth_module, "integration_auth_config", None) | |
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | |
| cfg.token = TEST_TOKEN | |
| auth_module.integration_auth_config = cfg | |
| yield | |
| auth_module.integration_auth_config = old_integration_auth_config |
| When the variable is **not set or empty**, the server will reject all requests to protected endpoints with `401 Unauthorized`. The token **must** be configured before using integration endpoints. | ||
|
|
There was a problem hiding this comment.
This paragraph states that when AUTOGRADER_INTEGRATION_TOKEN is unset or empty the server will reject protected requests with 401, but the implementation currently raises a ValueError when the env var is missing (which surfaces as a 500 unless handled) and it does not explicitly treat empty values as misconfiguration. Please update the docs to match actual behavior, or adjust the implementation to match the docs.
| raise ValueError( | ||
| "AUTOGRADER_INTEGRATION_TOKEN environment variable must be set" | ||
| ) | ||
| self.token: str = os.environ["AUTOGRADER_INTEGRATION_TOKEN"] |
There was a problem hiding this comment.
IntegrationAuthConfig currently only checks that AUTOGRADER_INTEGRATION_TOKEN exists, but it allows an empty string. An empty token effectively locks out all integration requests and is easy to misconfigure; consider validating that the env var is non-empty (e.g., after .strip()), and fail fast with a clear error.
| self.token: str = os.environ["AUTOGRADER_INTEGRATION_TOKEN"] | |
| token = os.environ["AUTOGRADER_INTEGRATION_TOKEN"].strip() | |
| if not token: | |
| raise ValueError( | |
| "AUTOGRADER_INTEGRATION_TOKEN environment variable must be non-empty" | |
| ) | |
| self.token: str = token |
| credentials: HTTPAuthorizationCredentials | None = Depends(_bearer_scheme), | ||
| ) -> None: | ||
| """Enforce Bearer-token auth on integration endpoints.""" | ||
| config = get_integration_auth_config() |
There was a problem hiding this comment.
get_integration_auth_config() can raise ValueError when the integration token env var is unset, but require_integration_token does not handle this. That will surface as a 500 error on the protected endpoints and contradicts the documented behavior; either initialize/validate the auth config during app startup so misconfiguration fails fast, or catch the exception here and return a deterministic HTTP error with a clear message.
| config = get_integration_auth_config() | |
| try: | |
| config = get_integration_auth_config() | |
| except ValueError as exc: | |
| raise HTTPException( | |
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | |
| detail="Integration authentication is not configured", | |
| ) from exc |
| now = datetime.now(timezone.utc) | ||
|
|
||
| # Create submission record (no files — externally graded) | ||
| submission_repo = SubmissionRepository(session) | ||
| db_submission = await submission_repo.create( | ||
| grading_config_id=grading_config.id, | ||
| external_user_id=payload.external_user_id, | ||
| username=payload.username, | ||
| submission_files={}, | ||
| language=payload.language, | ||
| status=submission_status, | ||
| submission_metadata=payload.submission_metadata, | ||
| ) | ||
| db_submission.graded_at = now |
There was a problem hiding this comment.
now = datetime.now(timezone.utc) produces a timezone-aware datetime, but elsewhere the code persists graded_at as a naive UTC timestamp (e.g., grading_service uses datetime.now(timezone.utc).replace(tzinfo=None)). Storing/returning mixed naive/aware datetimes can cause inconsistent API responses and can error on some DB backends; align this endpoint with the existing persistence convention for graded_at.
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | ||
| cfg.token = TEST_TOKEN | ||
| auth_module.integration_auth_config = cfg | ||
| yield | ||
| auth_module.integration_auth_config = cfg |
There was a problem hiding this comment.
The _set_integration_token autouse fixture sets auth_module.integration_auth_config but does not restore the previous value in teardown (it reassigns the same cfg). This can leak state across test modules and mask failures around missing/invalid env configuration; capture the old value before overriding and restore it after yield (often back to None).
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | |
| cfg.token = TEST_TOKEN | |
| auth_module.integration_auth_config = cfg | |
| yield | |
| auth_module.integration_auth_config = cfg | |
| old_integration_auth_config = auth_module.integration_auth_config | |
| cfg = IntegrationAuthConfig.__new__(IntegrationAuthConfig) | |
| cfg.token = TEST_TOKEN | |
| auth_module.integration_auth_config = cfg | |
| yield | |
| auth_module.integration_auth_config = old_integration_auth_config |
|
|
||
| import pytest | ||
| from httpx import AsyncClient, ASGITransport | ||
| from unittest.mock import Mock, patch, AsyncMock |
There was a problem hiding this comment.
AsyncMock is imported but not used in this test module. Removing unused imports keeps tests easier to maintain and avoids lint failures in stricter CI setups.
| from unittest.mock import Mock, patch, AsyncMock | |
| from unittest.mock import Mock, patch |
|
@ArthurCRodrigues ready for review :D |
|
Will review soon. |
[External Mode] Web API contract + M2M authentication for cloud integration
Summary
Adds two new protected endpoints to support GitHub Action external/private mode, along with a mandatory bearer-token authentication layer for machine-to-machine cloud integration.
What changed
New endpoints
GET /api/v1/configs/id/{config_id}Fetches a full grading configuration by its internal database ID.
Required by the GitHub Action when operating in external mode, where only the
grading_config_idis available.Returns all fields needed to call
build_pipeline(...):template_name,criteria_config,setup_config,feedback_config,include_feedback,languages,external_assignment_id,version,is_active.POST /api/v1/submissions/external-resultsIngests a grading result computed externally (e.g. inside the GitHub Action runner).
Creates a
submissionsrow and a matchingsubmission_resultsrow. The result is immediately visible through the existingGET /submissions/{id}andGET /submissions/user/{external_user_id}endpoints.Request payload:
grading_config_idexternal_user_idusernamelanguagestatuscompletedorfailedfinal_score>= 0execution_time_ms>= 0feedbackresult_treefocuspipeline_executionerror_messagesubmission_metadataM2M authentication
Both new endpoints are protected with a Bearer token dependency.
AUTOGRADER_INTEGRATION_TOKENenvironment variable (required — server raisesValueErrorat startup if unset)hmac.compare_digestto prevent timing attacks401for missing or invalid tokenNew schemas
ExternalResultStatus—completed | failedExternalResultCreate— request body with Pydantic validationExternalResultResponse— response bodyDocs
docs/API.mdupdated with:Tests
tests/web/test_external_results.pytests/web/test_integration_auth.pyAll 51 web tests pass (
test_external_results,test_integration_auth,test_routes,test_api_endpoints).Files changed