[29.0][VIES Integration] Per-environmentdaily request rate-limit - #11737
Conversation
Pull request was closed
Replace the background/API session block for codeunit 248 with a per-environment daily VIES lookup quota (table 243 "VAT Reg. No. Lookup Quota"), enforced on SaaS only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Tests-VAT to BaseApp internalsVisibleTo so codeunit 134193 can reach the internal VIES quota test helpers on codeunit 248 (fixes AL0161 in the backport build). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change adds a SaaS-only daily quota for EU VIES VAT registration number lookups. The counter is shared across companies in the environment, guarded by a table lock, and tested for blocking, daily reset, and on-premises bypass. The shared counter and lock shape fit the reported flooding problem, but the current placement changes the transaction boundary of every lookup. The new quota commit happens before the service call and before several existing errors, so a failed validation can now persist unrelated caller work. Problem-solution fitFit: Partial The change targets the right failure mode: one environment can flood the unauthenticated VIES service and affect others sharing the outbound address. The implementation needs adjustment because it can commit caller work on a later failure and can count calls that do not actually send the standard VIES request. SuggestionsS1 (🔴 High): Commit happens before lookup errors S2 (🟠 Moderate): Quota is charged before handled event Risk assessment and necessityRisk: The new table and lock are narrow, but codeunit 248 is a shared validation path. The early commit can persist unrelated caller changes on failed validation, and the pre-event quota check can affect extensions that replace the standard lookup. Necessity: A SaaS-only environment quota is justified because uncontrolled VIES traffic can deny-list a shared outbound address. The scope is appropriate, but the quota must not change existing transaction semantics or charge non-standard requests.
|
|
Thanks alexei-dobriansky — investigated. Note this branch is a line-for-line backport of #11734, so the substantive design points here really apply to the main PR; the fixes will land on #11734 and be mirrored to the backports. S1 (Commit before the lookup can fail) — agree, will address on #11734. The S2 (charge only requests that can reach VIES) — agree. Confirmed the quota is registered in So: S1 and S2 are fair and will be fixed on the main PR (#11734) and ported here; treating them as design refinements rather than backport-specific defects. |
…ndard request path Backport of the review fixes: charge the per-environment daily VIES quota only on the standard request path (after the blank-number check and OnRun IsHandled event), and move the quota read-modify-write and its Commit into dedicated codeunit 247 "VAT Lookup Quota Mgt.". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
alexei-dobriansky Pushed a change addressing the review. S2 - done. The quota is now charged only on the standard VIES request path: S1 - done (dedicated codeunit run). The quota read-modify-write and its One honest note on transaction semantics: a same-session codeunit run + S3 - the quota test seams moved off the shared codeunit 248 onto the dedicated codeunit 247 and remain |
…eached, not blocked The audit entry fires on the last allowed lookup (the one that reaches the daily limit), so the message should describe the limit being reached rather than the lookup being blocked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…st transaction model Address AL review agent feedback: - Mark codeunit 247 "VAT Lookup Quota Mgt." as Access = Internal (implementation detail). - Correct the doc/inline comments so they no longer imply the dedicated codeunit run isolates the transaction: the Commit also commits the caller's ambient transaction (the same boundary codeunit 248 already commits at around the outbound call). - Mark the quota tests with TransactionModel::AutoCommit since they commit the counter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Heads-up on the review state: the Good Sense Reviewer - Round 1 (Request Changes) above was raised against the first version of this work (blocking codeunit 248 in background/API sessions), before the per-environment daily VIES rate-limit was ported here. It no longer reflects the current code on this branch. The current implementation matches main #11734: a SaaS-only, table-backed daily quota (table 243 "VAT Reg. No. Lookup Quota") charged on the standard VIES request path via a dedicated codeunit 247 "VAT Lookup Quota Mgt.". The design review (S1-S5) and the AL review agent findings were addressed on #11734 and mirrored here:
|
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThis round moves the daily VIES quota onto the standard request path, after the handled-event decision and blank-number check. It also moves the locked counter into an internal codeunit, clarifies the audit message, and updates the tests for committed quota writes. The placement change resolves the quota-charging concern. However, the dedicated codeunit does not create an isolated transaction: its explicit Status of previous suggestions
New observations (commits since round 1)None - the new commits address the previous placement concern and refine the quota implementation. Risk assessment and necessityRisk: The locked single-row table addresses concurrent lost updates, and the standard-path placement avoids charging handled or blank requests. The remaining data-integrity risk is that a service or request-preparation error can occur after the quota code commits unrelated ambient changes; codeunit 248 can also be called by extensions outside the known standard callers. Necessity: Protecting the shared VIES endpoint from excessive traffic is necessary, and the per-environment SaaS quota fits that problem. The transaction boundary must still be safe for failed lookups before this can merge. The pull-request build is still running.
|
…a state per test - Rename DailyQuotaReachedMsg -> DailyQuotaReachedTxt (telemetry/locked string convention). - Reset the VIES quota row and SaaS testability flag in the test Initialize() so an AutoCommit quota test that fails mid-way cannot leak committed state into later tests under a non-isolated test runner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThis round only renames the quota telemetry label from The main quota design is still the same. The per-environment SaaS quota is well targeted, but the quota code still commits before request preparation, the outbound VIES call, and later error handling can finish. Status of previous suggestions
New observations (commits since round 2)None - the new commit only renames a telemetry label and resets quota test state between tests. Risk assessment and necessityRisk: The single locked quota row prevents lost counter updates, and the new test reset avoids committed quota state leaking between tests. The remaining data-integrity risk is unchanged: a failure after the quota commit can leave unrelated caller changes saved even though the lookup failed. Necessity: Limiting VIES traffic per online environment is necessary and the quota is the right shape for the reported bulk-flooding problem. The implementation still needs a transaction-safe way to make the quota durable without committing the caller's ambient work before failure-prone lookup code runs.
|
What & why
Codeunit 248 ("VAT Lookup Ext. Data Hndl") calls the EU VIES service over SOAP to validate VAT registration numbers. VIES is unauthenticated and rate-limits by source IP. In Business Central online, many tenants share the same outbound egress IP per app service, so when a single noisy tenant floods VIES, VIES deny-lists the shared IP and every co-located tenant starts seeing "VIES service unavailable" errors.
We have a concrete example: one tenant scheduled a job that called codeunit 248 ~1.66 million times in 8 days, getting the whole app service deny-listed by VIES.
This PR adds a per-tenant (per-environment) daily request rate-limit to codeunit 248 so no single tenant can flood VIES and deny-list the shared outbound IP.
How it works
OnRun(before the SOAP request), the codeunit reads a per-day counter, resets it when the UTC day changes, increments it, then persists and commits it before the outbound call.OnRun, it covers all VIES code paths (interactive, background, API, directCODEUNIT.Run(248), and codeunit 249 field validation, which funnels into 248).Enforcement
When a tenant reaches the daily cap, further lookups are blocked (
Error(DailyQuotaExceededErr)) for the rest of the UTC day; the call that hits the limit also emits a security-audit entry and telemetry, and blocked lookups are not counted. The counter is stored in a dedicated table (243 "VAT Reg. No. Lookup Quota", Access = Internal), so the cap value or enforcement behavior can be adjusted later as a pure code change that can be serviced into release branches.Why this replaces the earlier approach
This PR previously blocked codeunit 248 in background/API sessions. That was incomplete: a foreground "Verify All" over a large customer list (or a PTE action) still reaches VIES, and it would break legitimate low-volume automated callers. A per-environment daily quota is benign to legitimate users (well under the cap) while still stopping the bulk-flooding pattern from every session type.
Linked work
Fixes AB#651032
How I validated this
What I tested and the outcome
New unit tests in
ERM VAT VIES Lookup UT(codeunit 134193) exercise the quota decision logic directly (the check runs before the SOAP call, so no service/mock is needed). They use the Environment Info test library to simulate SaaS and internal test-only seams on codeunit 248 to drive the counter:DailyVIESCallQuotaBlocksWhenLimitReached— lookups beyond the daily limit are blocked and blocked lookups are not counted.DailyVIESCallQuotaResetsOnNewDay— after the day rolls over the counter resets, the customer gets a fresh full daily allowance, and the cap re-applies within the same day.DailyVIESCallQuotaSkippedOnPrem— the quota does not apply on-premises (nothing counted or blocked).Risk & compatibility