feat(contract): statement-acceptance readback + step-up problem type (Taiga #199) - #13
Open
t0kubetsu wants to merge 4 commits into
Open
feat(contract): statement-acceptance readback + step-up problem type (Taiga #199)#13t0kubetsu wants to merge 4 commits into
t0kubetsu wants to merge 4 commits into
Conversation
…e (Taiga #199) Two additive contract changes from the PR #1 review closure: - GET /statement-responses: the caller's receipts, each restating the statement_key and exact version answered, so the UI can decide whether to show the ToS/AUP prompt without joining against retired statement versions (review comment A22). StatementResponseReceipt gains statement_key and version, which also enriches the POST response. - Step-up problem type: the platform's first minted RFC 9457 type URI, https://testing.nc3.lu/problems/step-up-required, carried by the 403 of the three operations gated on current MFA assurance (API-key create and revoke, verification start). Distinguishes step-up — remedied by fresh authentication with an MFA method — from a plain forbidden (A20 companion). The OpenAPI relabel pass now preserves keys pre-declared under problem+json so the example survives export. api-design §1/§5.1/§14 updated; openapi.json re-exported; smoke case added.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe API adds an authenticated, cursor-paginated statement-response readback endpoint. Receipts now include statement identity and version fields. Protected operations now document structured MFA step-up failures through a shared 403 response contract. ChangesStatement response receipts
MFA step-up error responses
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StatementsAPI
participant Authenticator
Client->>StatementsAPI: GET /api/v1/statement-responses with cursor and limit
StatementsAPI->>Authenticator: Validate caller authentication
Authenticator-->>StatementsAPI: Authentication result
StatementsAPI-->>Client: Page_StatementResponseReceipt with next cursor
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/nc3_testing_platform/core/errors.py`:
- Around line 75-109: Align step-up handling with
require_current_mfa_assurance() by changing step_up_forbidden() to document the
contracted 401 response and RFC 9470 insufficient_user_authentication challenge,
replacing the current 403 and STEP_UP_PROBLEM_TYPE problem-details response
while preserving the existing step-up remedy semantics.
In `@src/nc3_testing_platform/domains/statements/router.py`:
- Around line 24-32: Update the statement submission and retrieval flow around
_sample_receipt and the POST/GET handlers to persist each accepted response
together with its authenticated principal, construct the POST receipt from the
stored response so the submitted version is preserved, and filter GET results to
the requesting principal. Add coverage for submitted-version accuracy and
isolation between authenticated callers.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d74fd58-42fa-44fc-aac1-e172c1d7e827
📒 Files selected for processing (8)
api/openapi.jsondocs/reference/api-design-v4_0_1.mdsrc/nc3_testing_platform/core/errors.pysrc/nc3_testing_platform/domains/api_keys/router.pysrc/nc3_testing_platform/domains/assets/router.pysrc/nc3_testing_platform/domains/statements/router.pysrc/nc3_testing_platform/domains/statements/schemas.pytests/test_smoke_surface.py
…eipt echoes the submission CodeRabbit round 1: - require_current_mfa_assurance() docstring promised 401 + RFC 9470 challenge, contradicting the 403s the three gated operations already declare and the minted step-up problem type; the seam now states the decided contract and why RFC 9470 (acr-based) is not used. - POST /statement-responses builds its receipt from the submitted statement_key and version instead of a fixed sample, so the mock echoes what was answered. Stateless-mock behavior otherwise unchanged: credential verification is an unwired seam, so caller-scoped storage belongs to the implementation phase.
Contributor
Author
|
@coderabbitai review |
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/nc3_testing_platform/core/security.py`:
- Around line 107-109: Update the explanatory comment around
STEP_UP_PROBLEM_TYPE to remove the incorrect RFC 9470 §5.1 and amr attribution.
Either cite the specification that defines amr, or state that the service
evaluates provider-specific amr values and therefore does not emit RFC 9470’s
acr_values challenge.
In `@src/nc3_testing_platform/domains/statements/router.py`:
- Around line 85-91: Update record_statement_response to reject submissions
whose required_context_type is non-null before constructing
StatementResponseReceipt, including context-bound statements identified through
StatementResponseSubmission.scan_target_permission. Preserve the existing
receipt construction for account-level statements and leave context_type and
context_id unset there.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7643cd59-2b89-4c65-8224-8b7b48590a62
📒 Files selected for processing (2)
src/nc3_testing_platform/core/security.pysrc/nc3_testing_platform/domains/statements/router.py
t0kubetsu
commented
Aug 10, 2026
t0kubetsu
left a comment
t0kubetsu
left a comment
Contributor
Author
There was a problem hiding this comment.
ECC review (/code-review, PR mode): clean — no findings above LOW. Validation green on the branch (ruff, pyright, 104 tests, OpenAPI regenerated in sync). The step-up 403 problem type is the right remedy discriminator and the RFC 9470 rejection is well-argued; the readback restating statement_key/version avoids joins against retired versions. Advisory only: merge still waits on Nicky's review (fresh CodeRabbit pass also triggered today). Artifact: .claude/reviews/pr-13-review.md.
…ording The handler now does what its docstring and the API design already promised: a statement whose key carries a required context answers 409, because a per-launch declaration travels in the launch payload. The assurance seam's docstring also disambiguates its section reference — §5.1 is the API design's, reading RFC 8176 amr values, not a section of RFC 9470.
Account-level receipts stay countable on one hand, but context-bound receipts arrive per launch from v4.1, and turning a bare array into the Page envelope later would be a breaking contract change. Same CursorPage dependency and Page shape as every other collection.
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
1 participant
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.
Two additive contract changes carried from the PR #1 review closure (Taiga task #199, "contract additions" group). Both shapes were decided in today's follow-up session; nothing here changes an existing operation's behavior.
1. Statement-acceptance readback (review comment A22)
POST /statement-responseswas write-only: no operation returned the caller's acceptance state, so the UI could not decide whether to show the ToS/AUP prompt, nor learn which version was answered.GET /statement-responses(authenticated): the caller's receipts, account-level and context-bound alike.StatementResponseReceiptgainsstatement_keyandversion— restated on the receipt so the readback needs no join against retired statement versions (GET /statementsonly returns what is currently in force). ThePOSTresponse is enriched the same way.GET /statementsstays fully anonymous and untouched. Client rule: a statement in force whose current version has no receipt is one the caller has yet to answer.Considered and rejected:
accepted_versionper item onGET /statements(mixes caller state into an anonymous resource) and an acceptance projection onGET /account(grows the account resource with legal-domain state).2. Step-up MFA problem type (A20 companion; two CodeRabbit review threads asked for the same)
§5.1 defines the assurance signal (
amrwithinmax_age), but a client could not distinguish step-up required from a plain403— the two have different remedies.typeURI:https://testing.nc3.lu/problems/step-up-required, documented in §1 and §5.1 and carried by the403of the three operations gated on current MFA assurance (POST /api-keys,POST /api-keys/{key_id}/revoke,POST /assets/{asset_id}/verification).about:blank.application/problem+json, so the example survives export. Behavior for every existingproblem_responses()call is unchanged (empty declared dict).Checks
make export-openapicommitted in sync ·make lintclean ·make typecheck0 errors ·make test104/104 (smoke case added for the new operation;test_cases_cover_every_operationgreen).Note
Trivial overlap with #12 in
statements/router.py(that PR drops theGET /statements500) — whichever merges second rebases in seconds.Refs: Taiga #199 (v4.0.1 contract follow-ups from PR #1 review closure).
Summary by CodeRabbit
New Features
Bug Fixes
403responses.Tests