Repository navigation
[API tests] Simplify API test web service key handling - #12890
Prangshuman Das (t-prda) wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
Team: Integrations
15251ff to
ed7ffac
Compare
Prangshuman Das (t-prda)
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Address review feedback on the API test authentication provider: - Create the web service key in the caller's session and commit it, instead of in a background session. API tests commit their fixtures before calling the API, so the key commit adds no new transaction boundary. Removes codeunit 131025 "API Test Web Service Key". - Create keys without an expiry. Missing or expired keys are still replaced. - Remove TransactionModel attributes from the provider tests; the codeunit already runs with test isolation disabled. - Verify provider tests through the HTTP response and the key expiry date instead of reading the key as text. AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
ed7ffac to
a6d8742
Compare
| WebServiceKey := ReadWebServiceKey(ExpiryDate); | ||
| if IsMissingOrExpired(WebServiceKey, ExpiryDate) then | ||
| Error(KeyCreationFailedErr); | ||
| Commit(); |
There was a problem hiding this comment.
Creating a missing or expired web service key now calls Commit() in the request authentication path, which commits every pending database change in the caller's transaction rather than only the key. Any test that initializes records and then makes its first API request can persist partial fixture or business data unexpectedly, and a later failure can no longer roll it back. Restore an isolated key-creation transaction (for example, the removed background-session mechanism) instead of committing from ConfigureAuthentication.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6
| Commit(); | ||
| end; | ||
| if WebServiceKey.IsEmpty() then | ||
| Error(EmptyKeyErr); |
There was a problem hiding this comment.
The empty-key guard raises the developer-facing invariant "An empty API test web service key was returned." with a plain client-visible Error. Raise an ErrorInfo with ErrorType::Internal so the technical detail is retained for telemetry while users receive a generic failure message.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6
| WebServiceKey := ReadWebServiceKey(ExpiryDate); | ||
| if IsMissingOrExpired(WebServiceKey, ExpiryDate) then | ||
| Error(KeyCreationFailedErr); | ||
| Commit(); |
There was a problem hiding this comment.
The provider now calls Commit() when it creates a key, but the test methods that exercise this path no longer declare TransactionModel::AutoCommit. They therefore use the default AutoRollback transaction model, so the first-use and expired-key tests can fail on the commit before reaching their assertions. Set those methods to AutoCommit and run them under a test runner with isolation that restores committed changes.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6
| TargetURL := GetUrl(ClientType::ODataV4); | ||
|
|
||
| // [WHEN] A request uses the default provider without explicit selection | ||
| // [THEN] The request authenticates |
There was a problem hiding this comment.
ExistingKeySurvivesLibraryReset no longer captures or compares the existing key, nor checks its non-expiring status, so two successful requests do not prove that library reset preserved the key as the scenario claims. Capture the key and expiry before the requests and assert both remain unchanged afterward.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change creates missing or expired API test web service keys in the caller's session, commits them without an expiry, removes the background-session helper, and simplifies the provider tests. The direct path works for ordinary requests after a committed fixture, but the shared provider can also run inside a negative request's Problem-solution fitFit: Partial The change simplifies first-use authentication and avoids the test-isolation restriction on background sessions. It does not safely cover first use from existing negative API requests. SuggestionsS1 (🔴 High): Keep authentication outside caller transactions S2 (🟠 Moderate): Keep the valid-key reuse assertion Risk assessment and necessityRisk: This is the default provider for API test requests. On a clean tenant or an individual test run, the first request may be a negative request inside Necessity: Self-contained API authentication is needed to run these suites without pipeline-managed credentials. The scope is appropriate, but key provisioning must not add a transaction boundary inside request setup.
|
Review focus: ``MicrosoftTestAuthProvider`` (about 60 lines) and the enum default; the ``build/`` changes are deletions plus the anti-SSRF line in ``NewBcContainer.ps1``. Defaults API test authentication (`Library - Graph Mgt`) to the current user's web service key. API tests no longer need per-app auth uptake or a container password. - `API Test Authentication` enum: `DefaultImplementation` = `Microsoft Test Auth Provider`. Removes `No API Test Auth Provider` and the explicit `Microsoft Test Environment` value. - `Microsoft Test Auth Provider`: on-premises with NavUserPassword, sends Basic auth with `UserId()` and the web service key. When the key is missing or expired, a background session (`API Test Web Service Key`, CU 131025) creates it with a 24h expiry and commits it, because the API request runs in a separate server session. The caller's transaction is never committed; the provider waits up to 60 s for the session and then re-reads the key. - `Library - Graph Mgt`: adds the `SetLicenseSafeWorkDate` and `AddFieldToIgnoreIfExists` helpers used by later layers. - Build: removes the container password and Key Vault credential plumbing (`ApiTestCredential.psm1`, `Remove-ApiTestPassword.ps1`, `PipelineFinalize` hooks and their tests). Keeps the clean-tenant execution logic. Sets `NavHttpClientAntiSSRFEnabled=false` in the test container, as NAV `RunALTests.ps1` does: since microsoft#12323, API tests call the container's own endpoint through AL `HttpClient`, which anti-SSRF blocks. - CU 139494 `API Test Auth Provider Tests`: covers key creation and renewal. Declares `TestHttpRequestPolicy = AllowAllOutboundRequests` (NAV blocks outbound requests by default; microsoft#12694 covered the other API tests). Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Review focus: the Expense test changes and the auth-helper deletion; the ``enableCleanTestCodeunitExecution`` line. The large JSON diffs are temporary disables, undone in C. Re-enables the Expense Agent API tests on top of the default web service key auth. No Key Vault password and no auth subscriber. - Deletes `Expense API Test Auth Helper` and its usages. - Re-applies the Expense fixes from microsoft#12325 (reverted in microsoft#12514): `TravelRequestApproval`, Expense permission and spend-request tests, the `ExpDetailTest` permission set, and `SpendRequestTests`. - Turns on `enableCleanTestCodeunitExecution`, as microsoft#12325 did, so `RequiredTestIsolation = Disabled` codeunits run in clean-tenant lanes. - Expense disabled tests: keeps only the entries NAV master also disables (12). - Temporarily disables the unrelated APIV1/APIV2 tests that the clean lanes start executing, the same entries microsoft#12325 added. They are re-enabled in the next layers. Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Review focus: mostly mechanical. ``Initialize`` sets a license-safe work date in each API test, and the JSON deletions re-enable tests. A few files have small fixture or isolation tweaks. Re-enables the APIV1, APIV2, IRS 1099 and E-Document API tests that pass without area-specific fixes. Aligned with NAV master: only tests that NAV master also runs are re-enabled. - API test `Initialize` sets a license-safe work date. Adds minimal fixture and isolation adjustments. - Removes the APIV1/APIV2 exclusions, keeping only the entries fixed in the following area PRs and the two `139806` methods NAV master disables. Removes E-Doc `135575`. Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
This branch was successfully deployed
3 participants
Review focus:
Microsoft Test Auth Providerkey creation and the provider tests (codeunit 139494).Review fixes for #12674 (web service key auth), kept as a separate layer so the approved bottom PRs do not need another push:
TransactionModelattributes from the provider tests. The codeunit already runs with test isolation disabled.Related: AB#646383
Stack (each PR shows only its own layer; merge bottom first)
main)