fix(contract-service): remove use-case status checks that mask domain auth checks#68
Merged
Merged
Conversation
… auth checks
Sign/Cancel/ConfirmDelivery/Dispute use cases each duplicated a status guard
that the domain method (Contract.sign/cancel/confirmDelivery/dispute) already
performs, but in the wrong order relative to the domain: the use case checked
status first, while the domain checks auth first, then status.
Normally this is invisible because the caller is usually wrong about only one
of {auth, status} at a time. It surfaces when both are wrong simultaneously:
23-seller-dispute-invalid in the Bruno e2e suite expected 403 (seller isn't
the buyer) but got 400, because by the time the request landed, an async
consumer had already moved the contract from DELIVERED to SETTLED — so the
use case's own status check fired and threw 400 before ever reaching the
domain's auth check.
Same root cause as the auth-check duplication fixed in PR #40 (domain as
single source of truth) — that pass cleaned up the auth side but left the
status-check duplication in place. Removed the redundant use-case guards
(including SignContractUseCase's duplicate-signatory check, which the domain
also already performs) so the domain's check order is what actually runs.
Verified: full contract-service test suite passes, and the Bruno e2e suite
(scripts/run-e2e.sh) now passes 47/47 requests / 76/76 tests — previously
46/47 with 23-seller-dispute-invalid as the one known failure. Run twice
back to back with identical results.
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.
No tracked GitHub issue — found and fixed directly while chasing down 23-seller-dispute-invalid's failure in the Bruno e2e suite.
Problem
SignContractUseCase/CancelContractUseCase/ConfirmDeliveryUseCase/DisputeContractUseCaseeach duplicated a status guard that the corresponding domain method (Contract.sign/cancel/confirmDelivery/dispute) already performs — but in the wrong order relative to the domain: the use case checks status first, the domain checks auth first then status.This is invisible in the common case (caller is wrong about only one of {auth, status} at a time). It surfaces when both are wrong at once:
23-seller-dispute-invalidexpected 403 (seller isn't the buyer) but got 400, because by the time the request landed, an async RabbitMQ consumer had already moved the contract from DELIVERED to SETTLED — the use case's own status check fired and threw 400 before ever reaching the domain's auth check.Same root cause as the auth-check duplication fixed in PR #40 (domain as SSOT) — that pass cleaned up the auth side but left this status-check duplication in place.
Fix
Removed the redundant use-case-level guards (including
SignContractUseCase's duplicate-signatory check, which the domain also already performs), so the domain's own check order — auth first, then status — is what actually runs.Test plan
contract-servicetest suite passesscripts/run-e2e.shrun twice back to back: 47/47 requests, 76/76 tests, fully green (previously 46/47, with23-seller-dispute-invalidas the one known failure)