Centralize melding token verification - #765
timvanoostrom wants to merge 13 commits into
Conversation
- Centralize token validation, validate before any other operations where token is required. - Remove specific functionality tightly coupled to token validation performed in core. Mostly actions and dependencies - boyscout: Removed unused imports - boyscout: polished tests a little
There was a problem hiding this comment.
🟡 Changes recommended
The test suite will fail to run due to parametrized tests not accepting/using their parametrized arguments, and the form upload dependency currently uses the backoffice media-type allowlist.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes “melding” token verification so that melder-facing endpoints validate the token up front, and downstream actions/dependencies no longer need to be tightly coupled to token verification.
Changes:
- Refactors melder endpoints to use a single dependency (
verify_token_and_retrieve_melding) for token validation + melding retrieval. - Updates dependency wiring to pass
RepositoryItem[Melding]into multiple actions instead of passing a token verifier around. - Adjusts/cleans up API tests to reflect the new error details and token-validation flow.
File summaries
| File | Description |
|---|---|
| tests/api/v1/endpoints/test_melding.py | Updates tests for new “not found” detail messaging and token-first validation; introduces parametrization changes. |
| meldingen/dependencies.py | Removes melder-specific token-verifier dependencies and introduces RepositoryItem[Melding] injection; refactors upload-attachment dependencies. |
| meldingen/authentication.py | Introduces a single dependency to validate token and retrieve melding, mapping domain exceptions to HTTP errors. |
| meldingen/api/v1/endpoints/melding.py | Switches melder endpoints to depend on centralized token+melding verification and removes per-endpoint token exception handling. |
| meldingen/api/v1/endpoints/asset_type.py | Removes an unused HTTP status import. |
| meldingen/actions/melding.py | Refactors retrieval/location actions to rely on RepositoryItem[Melding] rather than token verification. |
| meldingen/actions/form.py | Refactors answer create/update actions to rely on RepositoryItem[Melding] rather than token verification. |
| meldingen/actions/attachment.py | Aligns attachment actions with the new centralized token verification approach. |
| meldingen/actions/asset.py | Removes melder-specific asset action in favor of centralized token verification. |
Review details
Suppressed comments (4)
tests/api/v1/endpoints/test_melding.py:6252
- This test is parametrized with
melding_tokenbut the function doesn’t accept it and the request usesmelding.tokendirectly; pytest will fail collection. Accept/use the parametrized argument (or drop the parametrization).
@pytest.mark.parametrize(["melding_token"], [("supersecrettoken",)])
async def test_add_asset_with_asset_type_that_does_not_exist(
self, app: FastAPI, client: AsyncClient, melding: Melding
) -> None:
tests/api/v1/endpoints/test_melding.py:6272
- This test is parametrized with
melding_tokenbut the function signature doesn’t accept it and the request usesmelding_with_assets.token; pytest will fail collection. Accept/use the parametrized argument (or drop the parametrization).
@pytest.mark.parametrize(["melding_token"], [("supersecrettoken",)])
async def test_add_asset_that_does_not_exist(
self,
app: FastAPI,
client: AsyncClient,
melding_with_assets: Melding,
asset_type: AssetType,
) -> None:
tests/api/v1/endpoints/test_melding.py:6609
- This test is parametrized with
melding_tokenbut the function signature doesn’t accept it and the request usesmelding.token; pytest will fail collection. Accept/use the parametrized argument (or remove the decorator).
@pytest.mark.parametrize(["melding_token"], [("supersecrettoken",)])
async def test_delete_asset_from_melding_with_asset_that_does_not_exist(
self, app: FastAPI, client: AsyncClient, melding: Melding
) -> None:
response = await client.delete(
app.url_path_for(self.get_route_name(), melding_id=melding.id, asset_id=456),
params={"token": melding.token},
)
tests/api/v1/endpoints/test_melding.py:3688
melding_tokenis added to the parametrization and test signature but never used (the request always usesmelding_with_classification.token). Either use the parameter or drop it to avoid dead test data.
@pytest.mark.anyio
@pytest.mark.parametrize(
["melding_token", "time_value", "error_message"],
[
("supersecrettoken", "invalid-time-format", r"String should match pattern '^(?:[01]\d|2[0-3]):[0-5]\d$'"),
("supersecrettoken", "24:00:00", r"String should match pattern '^(?:[01]\d|2[0-3]):[0-5]\d$'"),
("supersecrettoken", "1560", r"String should match pattern '^(?:[01]\d|2[0-3]):[0-5]\d$'"),
("supersecrettoken", "ab:cd", r"String should match pattern '^(?:[01]\d|2[0-3]):[0-5]\d$'"),
("supersecrettoken", 1000, "Input should be a valid string"),
("supersecrettoken", 10.00, "Input should be a valid string"),
- Files reviewed: 9/9 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Critical test-collection and upload-validation regressions must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
meldingen/actions/melding.py:56
- This wrapper is never constructed: both endpoint dependencies use
Depends(melding_repository_item), which returnsRepositoryItem[Melding], even though their annotations nameMeldingRetrieveAction. That leaves this class dead and the endpoint dependency types inaccurate. Either restore a provider that constructs this action or type the endpoints asRepositoryItem[Melding]and remove this wrapper and its stale core import.
meldingen/actions/form.py:486 - The action no longer accepts or validates a token, but its conditions still say that it does. Update the docstring so callers do not rely on authorization being enforced by this action rather than by the endpoint dependency.
tests/api/v1/endpoints/test_melding.py:5311
- With list-form
argnames, the scalar string is interpreted as a multi-value parameter set, so pytest fails collection due to the value count not matching the single declared name. Use the string form for this one parameter.
@pytest.mark.parametrize(["melding_token"], ["supersecuretoken"])
tests/api/v1/endpoints/test_melding.py:5357
- With list-form
argnames, the scalar string is interpreted as a multi-value parameter set, so pytest fails collection due to the value count not matching the single declared name. Use the string form for this one parameter.
@pytest.mark.parametrize(["melding_token"], ["supersecuretoken"])
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The dependency lock and import ordering must be fixed, and centralized verification currently adds redundant database queries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
meldingen/actions/form.py:489
- The action no longer accepts or validates a token, but its first documented condition still says that it does. Update the condition so callers are not led to rely on authentication at this action boundary.
async def __call__(self, melding_id: int, answer_id: int, answer_input: AnswerInputUnion) -> Answer:
"""
Conditions:
1. The provided token must be valid
meldingen/api/v1/endpoints/melding.py:253
- This example no longer matches the declared comma-separated string format: it presents two separate values for a
strquery parameter instead of one valid value. Keepexamplesas a list, but put the comma-separated value inside it so generated API documentation shows a request the endpoint can actually parse.
examples=[MeldingStates.PROCESSING, MeldingStates.COMPLETED],
meldingen/api/v1/endpoints/melding.py:95
- The new dependency imports are not sorted:
download_attachment_actionmust precede themelding_*names, and both upload-action providers belong with the othermelding_*imports after the update actions. Because CI runsisort . --check(.github/workflows/ci.yml:121-122), this block will fail validation; please run isort on the file.
melding_upload_attachment_action_backoffice,
download_attachment_action,
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
|
Needs some work in the core repo |
Centralize token validation, validate before any other operations where token is required.
Remove specific functionality tightly coupled to token validation performed in core. Mostly actions and dependencies
boyscout: Removed unused imports
boyscout: polished tests a little
see also: Amsterdam/meldingen-core#325
Ticket: SIG-7133