Fix API test authentication in BCApps - #10085
Conversation
GitHub Actions
Inactive
github-actions
Bot
added
Build: scripts & configs
AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
5dee938 to
d459cb4
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| local procedure TryGetContainerPassword(var Password: SecretText) | ||
| var | ||
| File: DotNet File; | ||
| PasswordText: Text; |
There was a problem hiding this comment.
The new always-on auth-injection subscriber materializes the API test password in a plain Text variable (PasswordText: Text) before assigning it into the SecretText output parameter. BCQuality's SecretText guidance requires credentials to flow as SecretText end-to-end from retrieval to consumption, because a Text password is visible in the debugger, in variable watches, and in any error output that prints the variable. Read the file contents directly into a construct that stays SecretText (or use a SecretText-returning API) instead of round-tripping through Text.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| User: Record User; | ||
| Password: SecretText; | ||
| begin | ||
| if not User.Get(UserSecurityId()) then |
There was a problem hiding this comment.
The new "Library - Graph Auth Mgt." subscriber has no EventSubscriberInstance = Manual gating (unlike the deleted helper, which required an explicit BindSubscription in only 4 test codeunits), so it now fires globally for every codeunit that uses "Library - Graph Mgt" HTTP requests across the whole test suite. It also dropped the previous EnvironmentInfo.IsSaaSInfrastructure() short-circuit: it only checks whether the user's Windows Security ID is blank, then unconditionally tries the container-password-file and Key Vault paths and calls Error() if both fail. Any test run in an environment without the BCApps container password bridge or the NavServerUserPassword secret configured (for example a SaaS/cloud sandbox using NavUserPassword-style auth) will now fail at this subscriber before the actual test assertion runs, instead of the previous silent no-op. Reintroduce an explicit environment/auth-mode guard (e.g. the previous SaaS short-circuit, or an equivalent check) before attempting password retrieval.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
There was a problem hiding this comment.
Pull request overview
This PR centralizes Basic authentication injection for Library - Graph Mgt-based API tests so they can run in NavUserPassword (UserPassword) containers, and then re-enables several previously disabled API/E2E test suites. It replaces an app-specific Expense Agent auth helper with a shared TestLibraries subscriber and adds a container-side “credential bridge” file so the test runner can obtain the password without requiring Azure Key Vault.
Changes:
- Added
Library - Graph Auth Mgt.as an event subscriber to inject Basic auth for non-Windows test users, sourcing the password from a container file or (fallback) Azure Key Vault. - Removed the Expense Agent test-only auth helper + manual subscription binding; tests now rely on the shared subscriber.
- Re-enabled multiple API/E2E test suites by removing entries from various
*.DisabledTest.jsonfiles (and deleting the APIV1/APIV2 exclusion lists), and updated container provisioning to create the API-test password bridge file.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Layers/W1/Tests/TestLibraries/LibraryGraphAuthMgt.Codeunit.al | New shared subscriber that injects Basic auth into Library - Graph Mgt requests for NavUserPassword scenarios. |
| build/scripts/NewBcContainer.ps1 | Writes the container password bridge file and sets ACLs so server-side AL can read it. |
| src/Apps/W1/ExpenseAgent/test/src/Helper/ExpenseAPITestAuthHelper.Codeunit.al | Removes app-specific auth injection helper in favor of shared TestLibraries implementation. |
| src/Apps/W1/ExpenseAgent/test/src/API/ExpenseUsersAPITest.Codeunit.al | Drops manual subscription binding to the removed helper. |
| src/Apps/W1/ExpenseAgent/test/src/API/ExpenseProjectsAPITest.Codeunit.al | Drops manual subscription binding to the removed helper. |
| src/Apps/W1/ExpenseAgent/test/src/API/ExpensePerDiemLocationsTest.Codeunit.al | Drops manual subscription binding to the removed helper. |
| src/Apps/W1/ExpenseAgent/test/src/API/ExpenseCapabilitiesAPITest.Codeunit.al | Drops manual subscription binding to the removed helper. |
| src/DisabledTests/Tests-Integration/Tests-Integration.DisabledTest.json | Re-enables specific integration API tests by removing disable entries. |
| src/DisabledTests/Tests-Graph/Tests-Graph.DisabledTest.json | Re-enables specific Graph E2E tests by removing disable entries. |
| src/DisabledTests/Sustainability_Tests/Sustainability_Tests.DisabledTest.json | Re-enables Sustainability API tests by removing disable entries. |
| src/DisabledTests/Quality_Management-Tests/Quality_Management-Tests.DisabledTest.json | Re-enables Quality Management API tests by removing disable entry. |
| src/DisabledTests/IRS_Forms_Tests/IRS_Forms_Tests.DisabledTest.json | Re-enables IRS 1099 API test by removing disable entry. |
| src/DisabledTests/E-Document_Core_Tests/E-Document Core Tests.DisabledTest.json | Re-enables E-Document API tests by removing disable entry. |
| src/DisabledTests/_Exclude_APIV2__Tests/_Exclude_APIV2__Tests.DisabledTest.json | Deletes the APIV2 exclusion list (re-enables APIV2 suite). |
| src/DisabledTests/_Exclude_APIV1__Tests/_Exclude_APIV1__Tests.DisabledTest.json | Deletes the APIV1 exclusion list (re-enables APIV1 suite). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| New-BcContainer @parameters | ||
|
|
||
| $apiTestPasswordFile = 'C:\Run\my\ApiTestPassword' | ||
| $apiTestPassword = $parameters.credential.GetNetworkCredential().Password |
| $apiTestPasswordFile = 'C:\Run\my\ApiTestPassword' | ||
| $apiTestPassword = $parameters.credential.GetNetworkCredential().Password | ||
| $hostPasswordFile = Join-Path ([System.IO.Path]::GetTempPath()) "BCAppsApiTestPassword-$([Guid]::NewGuid().ToString('N'))" | ||
| try { |
There was a problem hiding this comment.
NewBcContainer.ps1 only guarantees cleanup of the host-side temp password file via its try/finally block. After Copy-FileToBcContainer succeeds, a failure in the subsequent Invoke-ScriptInBcContainer ACL-hardening step leaves the plaintext password file at C:\Run\my\ApiTestPassword inside the container with default (non-hardened) inherited permissions, and nothing removes it. Wrap the copy-plus-ACL sequence so a failure in the ACL step also removes the copied secret file from the container.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| if User."Windows Security ID" <> '' then | ||
| exit; | ||
|
|
||
| if not TryGetContainerPassword(Password) then |
There was a problem hiding this comment.
AddAuthentication collapses every failure from TryGetContainerPassword and TryGetNavEnlistmentPassword into the same generic MissingPasswordErr. Whether the container file is missing, unreadable, empty, or Key Vault access failed, the caller only ever sees a 'configure the bridge or secret' message with no diagnostic detail about which source failed and why, making container/CI failures harder to triage.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
| // ------------------------------------------------------------------------------------------------ | ||
|
|
||
| codeunit 131022 "Library - Graph Auth Mgt." |
There was a problem hiding this comment.
The new codeunit 131022 "Library - Graph Auth Mgt." subscribes to "Library - Graph Mgt".OnAfterInitializeWebRequestWithURL without EventSubscriberInstance = Manual, so it is a static, always-on subscriber that now fires for every HTTP request initialized through the shared W1 test library "Library - Graph Mgt" - not just Expense Agent tests. The PR simultaneously deletes the previous helper (codeunit 148332, Access = Internal, EventSubscriberInstance = Manual) and removes the explicit BindSubscription calls that scoped it to exactly 4 Expense API test codeunits. The new subscriber's AddAuthentication procedure calls Error(MissingPasswordErr) when neither the container password file nor the NavServerUserPassword Key Vault secret is available, so any other test suite across the repository that uses Library - Graph Mgt's HTTP helper and runs under NavUserPassword authentication without Windows Security ID will now unexpectedly fail unless the container/secret bridge happens to be configured for it - a materially wider blast radius than the previous manual, opt-in binding. Keep this subscriber manual (EventSubscriberInstance = Manual) and bind/unbind it explicitly only around the test suites that require the credential bridge, mirroring the pattern the deleted helper used.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| PasswordRetrievalFailedErr: Label 'The API test password could not be retrieved.', Locked = true; | ||
|
|
||
| [NonDebuggable] | ||
| [EventSubscriber(ObjectType::Codeunit, Codeunit::"Library - Graph Mgt", OnAfterInitializeWebRequestWithURL, '', false, false)] |
There was a problem hiding this comment.
This PR removes over a thousand disabled API-test entries (and deletes the two _Exclude_APIV1/APIV2 files wholesale) and makes the new global subscriber in "Library - Graph Auth Mgt." the only auth bridge for those suites, but adds no focused test proving request initialization succeeds through both password sources (the container file at C:\Run\my\ApiTestPassword and the NavServerUserPassword Key Vault fallback). If either branch regresses, the many newly re-enabled tests will fail with generic infrastructure errors and no small, isolated test will pinpoint the auth bridge as the cause. Add a dedicated test exercising both password-retrieval branches of AddAuthentication before relying on it to re-enable this many suites at once.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
| // ------------------------------------------------------------------------------------------------ | ||
|
|
||
| codeunit 131022 "Library - Graph Auth Mgt." |
There was a problem hiding this comment.
The new helper codeunit "Library - Graph Auth Mgt." is added without Access = Internal;, exposing an implementation-detail test helper as a public object contract, unlike the codeunit it replaces which declared Access = Internal;. Unless dependent apps are expected to bind to this codeunit directly, mark it internal so it can evolve without creating an accidental supported API.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
codeunit 131022 "Library - Graph Auth Mgt."
{
Access = Internal;
varKnowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| User: Record User; | ||
| Password: SecretText; | ||
| begin | ||
| if not User.Get(UserSecurityId()) then |
There was a problem hiding this comment.
Library - Graph Auth Mgt. now installs an automatic OnAfterInitializeWebRequestWithURL subscriber that starts with User.Get(UserSecurityId()). Because this subscriber runs for every Library - Graph Mgt web request, the change moves a database lookup into the hot path for all graph-based tests instead of only the Expense tests that previously bound the helper manually. Add a cheap opt-in guard (or keep the subscriber manually scoped) before the Get so unrelated requests do not pay the extra database round-trip and password-resolution work.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
|
|
||
| New-BcContainer @parameters | ||
|
|
||
| $apiTestPasswordFile = 'C:\Run\my\ApiTestPassword' |
There was a problem hiding this comment.
The build script writes the reusable NavUserPassword container admin credential to C:\Run\my\ApiTestPassword inside the container and the AL library reads it back later, leaving a plaintext password at rest inside the shared test container. Any code running under the allowed service/admin identities (SYSTEM, NETWORK SERVICE, Administrators per the ACL set in NewBcContainer.ps1) can read and replay that file. Prefer a secret-preserving bridge (for example Key Vault, or one-shot secret injection without persisting to disk) instead of a persistent plaintext file, and delete the file immediately after first use if a temporary bridge is unavoidable.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| MissingPasswordErr: Label 'API tests require a password when the server uses NavUserPassword authentication. Configure the BCApps container credential bridge or the NavServerUserPassword secret.'; | ||
| PasswordRetrievalFailedErr: Label 'The API test password could not be retrieved.', Locked = true; | ||
|
|
||
| [NonDebuggable] |
There was a problem hiding this comment.
Beyond the events/performance framing, from a security standpoint: replacing the manually-bound Expense-only helper with a static subscriber on Library - Graph Mgt.OnAfterInitializeWebRequestWithURL broadens Basic-auth injection to every LibraryGraphMgt request in the whole test suite, and the previous IsSaaSInfrastructure() guard is gone. This is flagged here as a security-impact severity note in addition to the events/performance findings above; the underlying architectural fix is the same (restore manual binding or add explicit scope/SaaS checks).
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| NavServerUserPasswordKeyTok: Label 'NavServerUserPassword', Locked = true; | ||
| CurrentUserNotFoundErr: Label 'The current test user could not be found.'; | ||
| MissingContainerPasswordFileErr: Label 'The API test password file is not available.', Locked = true; | ||
| MissingPasswordErr: Label 'API tests require a password when the server uses NavUserPassword authentication. Configure the BCApps container credential bridge or the NavServerUserPassword secret.'; |
There was a problem hiding this comment.
MissingPasswordErr is a translatable error label but it embeds the exact machine token NavServerUserPassword. In a localized build that token can be translated or altered, leaving the setup guidance wrong. Keep the secret name in a separate Locked = true token label and inject it through a placeholder documented with a Comment.
Knowledge:
- microsoft/knowledge/style/label-locked-for-non-translatable.md
- microsoft/knowledge/style/label-comment-explains-placeholders.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
Summary
Library - Graph MgtAPI tests in UserPassword containersTesting
AB#646383