Skip to content

Block unauthorized Sales Order Agent setup access - #10232

Open
attilatoury wants to merge 1 commit into
mainfrom
private/attilatoury/fix-640825-soa-setup-permission
Open

Block unauthorized Sales Order Agent setup access#10232
attilatoury wants to merge 1 commit into
mainfrom
private/attilatoury/fix-640825-soa-setup-permission

Conversation

@attilatoury

@attilatoury attilatoury commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Problem

A user without Sales Order Agent configuration rights can navigate directly to page 4400 and activate or reconfigure the agent, bypassing the authorization applied by the normal Agents pages.

Root cause

The SOA Setup page trusts its caller. Its OnOpenPage trigger resolves the target agent but does not verify that the current user can manage that agent.

Fix

Validate the resolved agent user security ID with Agent System Permissions.CurrentUserCanManageAgent. This preserves per-agent Can Configure access and requires Configure All Agents for first-time setup or unfiltered direct navigation.

Validation

  • AL diagnostics for the changed file: clean
  • git diff --check: clean
  • Built a local-compatible Sales Order Agent package from the exact source exposed by the NAV_master environment, with this authorization change applied
  • Published the package to NAV_master and downloaded its symbols back; verified that the deployed page contains both the CurrentUserCanManageAgent guard and authorization error
  • The local environment's Base Application predates the current-main Contact List.OnBeforeFindRecord event, so GitHub validation remains authoritative for the complete current-main app build

Risk

Low. The change is limited to the setup page entry boundary and uses the existing agent authorization API.

Fixes AB#640825

@attilatoury
attilatoury requested a review from a team August 13, 2026 13:26
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 SCM GitHub request for SCM area labels Aug 13, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 13, 2026
if not Evaluate(UserSecurityID, UserSecurityIDFilter) then
Clear(UserSecurityID);

if not AgentSystemPermissions.CurrentUserCanManageAgent(UserSecurityID) then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Breaking\ Changes}$

Adding Error(NotAuthorizedToConfigureAgentErr) in OnOpenPage means users who previously could open the Sales Order Agent setup page can no longer do so unless they hold agent-management permission (Configure All Agents, or Can Curr. User Configure Agent on the resolved Agent record). For a page reached only via first-time setup (gated separately by AllowCreateNewSOAgent requiring Configure All Agents) this is consistent, but for the edit path any previously-permitted viewer is now blocked at open time rather than at a modify/configure action, which is a behavior change worth calling out explicitly in the PR description.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4

if not Evaluate(UserSecurityID, UserSecurityIDFilter) then
Clear(UserSecurityID);

if not AgentSystemPermissions.CurrentUserCanManageAgent(UserSecurityID) then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Telemetry}$

The new permission-denied branch throws Error(NotAuthorizedToConfigureAgentErr) before the existing FeatureTelemetry.LogUptake(...Discovered) call executes, so a user who opens the Sales Order Agent setup page but lacks permission to manage the agent no longer records the feature-discovery telemetry event. Per the feature-uptake lifecycle guidance, Discovered should be logged when the user encounters the feature even if the flow cannot continue past that point. Move the LogUptake(Discovered) call (or an equivalent call) ahead of the new permission check, or emit it inside the error branch.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if not AgentSystemPermissions.CurrentUserCanManageAgent(UserSecurityID) then begin
            FeatureTelemetry.LogUptake('0000QIK', SOASetupCU.GetFeatureName(), Enum::"Feature Uptake Status"::Discovered);
            Error(NotAuthorizedToConfigureAgentErr);
        end;

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This PR adds a permission check when page 4400 SOA Setup opens. The check uses the resolved User Security ID, so an unfiltered direct URL requires Configure All Agents, while a filtered existing agent can still be opened by a user who can configure that agent.

The fix addresses the root cause at the page entry boundary, before setup data is loaded or changed. It also matches the existing Agent Card pattern, which uses Agent System Permissions.CurrentUserCanManageAgent for the same agent-management decision.

Suggestions

S1 - Add a permission regression test
The diff adds an authorization gate on page 4400, but no test covers the direct URL path. Add a test for an unauthorized user opening SOA Setup without a valid agent-management permission, and a positive test for a user who can configure the target agent.

Risk assessment and necessity

Risk: This is an authorization boundary for Sales Order Agent setup. A wrong check could either reopen the direct-access bypass or block valid setup users, but the code uses the existing agent permission API and leaves the update flow unchanged.

Necessity: The work item and PR description describe a concrete unauthorized direct-navigation bug. The scope is targeted: it blocks access immediately after resolving the target agent user security ID and before setup state can be changed.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10232 round=1 by=alexei-dobriansky at=2026-08-14T08:44:21Z lastSha=6be92686d64926ed37b0dbbaaa808a4101768f70 reviewKey=na suggestions=S1@7961888b

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.


🔄 How to trigger a new status check:

  1. 📤 Push a new commit to the PR branch, or
  2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants