Address second round of Jesper Schulz-Wedde's review on PR #157

- log-writes-must-survive-rollback.good.al: fixed invalid trigger
  OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set.
- exposed-objects-must-be-in-a-permission-set.md: distinguished the three
  exposure mechanisms (page/query web service or API, codeunit published
  as a web service, [ServiceEnabled] bound action on a page) and their
  actual permission targets (page/query "..." = X vs codeunit "..." = X).
- code-must-not-change-workdate.md: scoped from an absolute "never" to
  "not as a side effect of unrelated logic" - verified real WorkDate(x)
  setter usage in BCApps demo-data generators and test codeunits.
- bcpt-scenarios-must-be-app-specific.md: SingleInstance and
  StartScenario/EndScenario reframed as context-dependent patterns, not
  mandatory requirements - BCPT Create Customer uses neither.
- test-feature-scenario-tags.good.al/.bad.al: replaced the invented
  LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real,
  verified price-list-line test using Library - Sales/Library - Inventory/
  Library - Price Calculation.
- page-design-must-match-bc-page-type-conventions.md: scoped the missing
  UsageCategory anti-pattern to pages intended as searchable entry points.
- defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al:
  replaced the VAT registration number "low blast radius" example with a
  genuinely cosmetic field (customer home page URL).
- source-organized-by-feature-not-object-type.md: anti-pattern reframed as
  inconsistency with a repo's own convention, not the object-type scheme
  itself.
- pictures-must-use-media-not-blob.md: removed leftover "image variants"
  wording contradicting the already-corrected MediaSet description.

Proactively fixed while sweeping all fixtures for invented APIs:
- given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder
  called with wrong arity and referenced an undeclared variable.
- ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake
  "Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls
  and the real Customer Card TestPage.

Worklist completeness: added review-skill cues for the 12 of 18 new rules
that had none (al-appsource-review.md, al-data-modeling-review.md,
al-error-handling-review.md, al-security-review.md, al-style-review.md x3,
al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md,
al-web-services-review.md), and fixed test-feature-scenario-tags' cue,
which only matched the compliant (tagged) shape instead of the anti-pattern
(untagged/generic-named test).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer 2026-09-08 20:05:56 +02:00
parent 0120b874b2
commit 3842ef7138
26 changed files with 219 additions and 89 deletions

View file

@ -11,11 +11,17 @@ application-area: [all]
## Description
An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, and each needs to be checked on its own terms: a page or query published through the **Web Services** configuration page; a custom REST endpoint declared with `PageType = API` / `QueryType = API`; or an individual codeunit method exposed with the `[ServiceEnabled]` attribute (a method-level attribute — it does not apply to pages or queries as a property). When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing.
An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, each with its own permission target:
- A page or query published through the **Web Services** configuration page, or a custom REST endpoint declared with `PageType = API` / `QueryType = API` — both need a `page "..." = X` / `query "..." = X` entry for that object.
- A codeunit published through **Web Services** exposes *every* public procedure on it as an OData/SOAP operation automatically — there is no per-method attribute to add. The permission target is the codeunit itself: `codeunit "..." = X`.
- `[ServiceEnabled]` is a method-level attribute used on a *page* procedure to expose it as an OData v4 bound action (for example a `Post` action on an invoice page) — it does not apply to pages, queries, or codeunits as an object-level property, and it does not create its own permission target. The action is still a call into that page object, so the page's own `page "..." = X` entry is what governs it.
When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing.
## Best Practice
Give every exposed object an explicit execute entry (`page "..." = X`, `query "..." = X`) in a permission set shipped by the app. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / `ServiceEnabled`) rather than leaving an orphaned endpoint with no permission-set membership.
Give every exposed object an explicit execute entry in a permission set shipped by the app: `page "..." = X` / `query "..." = X` for a published or API page/query (including one that exposes a `[ServiceEnabled]` bound action), and `codeunit "..." = X` for a codeunit published as a web service. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / the Web Services registration) rather than leaving an orphaned endpoint with no permission-set membership.
See sample: `exposed-objects-must-be-in-a-permission-set.good.al`.