Least-privilege monitoring grants: document the verified set, harden three paths (#1823)

[erikdarlingdata]

Summary

Third field report on #1823: the user provisioned their AD service login exactly as the README said and got five distinct collector failures. Every one reproduced on SQL Server 2025 with a scratch login carrying only the documented grants, and every fix below was then verified live with that login before being written down.

The docs were wrong, not the user. SQLAgentReaderRole gates the sp_help_job* procedure interface — which no code path in this repo calls (grepped; every Agent read is a direct three-part SELECT) — and grants no SELECT on the base tables. MS Learn's own SCOM low-privilege script grants explicit table SELECTs for exactly this reason. A silent second gap surfaced during verification: with CONNECT ANY DATABASE but no VIEW ANY DEFINITION, per-database catalog views return zero rows with no error (sys.tables: 0 of 22 visible), so the index/object collectors on a minimal login collect nothing while looking healthy.

Docs (both README.md and Darling/README.md, plus Azure contained-user guidance):

• msdb: direct SELECT on sysjobs, sysjobactivity, sysjobhistory, sysjobschedules, syscategories, syssessions + EXECUTE on agent_datetime (scalar function; no read role covers EXECUTE) — replacing SQLAgentReaderRole
• CONNECT ANY DATABASE for the per-database collectors (current + future databases, no per-DB users), with the per-DB-user loop documented as the fallback for shops that reject it
• VIEW ANY DEFINITION promoted from AG-only to core, with the silent-zero-rows failure mode spelled out
• ALTER TRACE documented as the sys.traces requirement (VIEW SERVER STATE does not cover it, 2016–2025 alike) with its real cost stated: not read-only, implies SHOWPLAN — withholding it is a supported choice
• Azure contained users gain VIEW DEFINITION for the same catalog-visibility reason

Code, mirroring proven patterns:

• database_scoped_config enumeration gains AND HAS_DBACCESS(d.name) = 1 — the same self-skip index_object_stats and database_size_stats already had — killing the per-database-per-cycle 916 warnings. On-prem only: from master on Azure SQL DB, HAS_DBACCESS answers falsely for every user database, and the new tests pin the asymmetry both ways. query_store_stats deliberately unchanged — its enumeration probe already swallows inaccessible databases per-item (empty CATCH, QueryStoreCollector.cs:183-185).
• Error 8189 (You do not have permission to run 'SYS.TRACES') now classifies as PERMISSIONS instead of ERROR in both Lite and Darling — parity change, same comment both sides.
• The failed-job skip message in Darling and the deprecated Dashboard stops recommending the role that provably does not work and names the grants that do.

Test plan

• All five field failures reproduced on SQL 2025 under EXECUTE AS with the documented grants; all pass with the new set; both Agent roles dropped to prove the explicit grants suffice alone
• Lite.Tests + Darling.Tests + deprecated Dashboard build: 0 warnings, 0 errors
• Definition + classifier test classes: 78/78 pass, including new pins (on-prem contains HAS_DBACCESS, Azure must not)
• Scratch login darling_permtest dropped from sql2025 after verification

🤖 Generated with Claude Code

[claude]

Lite/Darling parity drift. Darling's skip message here is correctly rewritten to point at the direct table SELECTs instead of SQLAgentReaderRole. But Lite's equivalent code path — Lite/Services/RemoteCollectorService.RunningJobs.cs (GetRecentlyFailedJobsAsync) — wasn't touched by this PR and still says the old, now-disproven thing:

/// SQLAgentReaderRole access, a transient failure, etc.) returns an empty list rather than
...
/* Login lacks msdb / SQLAgentReaderRole access — expected for read-only monitoring
   accounts; skip quietly so a permission gap doesn't fail the whole alert cycle. */

Since the whole premise of #1823 is that SQLAgentReaderRole grants nothing useful here, Lite's doc comment and log path should get the same correction the PR description calls out for Darling and the deprecated Dashboard.

[claude]

Good fix here, but it's not applied consistently within this same file: the "FinOps Index Analysis (per-database grants)" section further down (README.md:470) still reads:

The grants above (VIEW SERVER STATE, plus optional SQLAgentReaderRole on msdb) are not sufficient on their own...

That line still presents SQLAgentReaderRole as a legitimate optional grant belonging to "the grants above" — but this PR just replaced that grant with direct table SELECTs precisely because it doesn't work. Worth updating that reference too so it doesn't contradict the corrected guidance a few paragraphs above it.

[claude]

This rewrite is thorough, but two other spots later in this same README still describe the old, disproven grant as if it's viable and weren't updated:

• Line 268: "so losing the SQLAgentReaderRole grant later moves those collectors from running to skipped" — HasMsdbAccess is really just HAS_DBACCESS('msdb'), i.e. any msdb access at all, not tied to this specific role.
• Line 602 (Troubleshooting): "the login has no msdb / SQLAgentReaderRole access... grant the role if you want job alerts." This directly contradicts the fix above — it's telling operators to grant the exact role this PR's field investigation proved insufficient.

Worth a follow-up pass over the rest of the file for other stale SQLAgentReaderRole mentions.

[claude]

Review summary

Solid, well-verified fix — the msdb table grants, CONNECT ANY DATABASE/VIEW ANY DEFINITION promotion, and the 8189→PERMISSIONS reclassification are all backed by live testing against a scratch login, and the SQL/comment style in the rewritten README grant blocks matches the T-SQL conventions (block comments, no more --).

Verified correct:

• The new HAS_DBACCESS(d.name) = 1 filter in DatabaseScopedConfigCollector.cs mirrors the existing pattern in IndexObjectStatsCollector.cs / DatabaseSizeStatsCollector.cs, is correctly applied only to the on-prem query (not Azure, where it would always return 0), and is pinned by new tests in both directions.
• DatabaseScopedConfigCollector is shared between Lite and Darling (PerformanceMonitor.Collectors), so the fix covers both apps without needing a duplicate change.
• The documented msdb table list (sysjobs, sysjobactivity, sysjobhistory, sysjobschedules, syscategories, syssessions, agent_datetime) matches every table actually read by JobHistoryCollector, RunningJobsCollector, AgentStatusCollector, and FailedJobsQuery — nothing over- or under-granted.
• The 8189 classification change is applied to the shared/generic collector-error catch in both Lite (RemoteCollectorService.cs) and Darling (DarlingWorker.cs's per-collector runner), so it's symmetric, and AzureDmvPermissionHint.For() safely no-ops for error numbers other than 300.

Left three inline comments, all in the same vein — the PR fixes SQLAgentReaderRole guidance in most places but misses a few:

1. Lite/Darling parity drift: Lite/Services/RemoteCollectorService.RunningJobs.cs (GetRecentlyFailedJobsAsync) has the exact same doc comment + catch-block message as the Darling method this PR rewrote, but wasn't touched — it still tells the reader SQLAgentReaderRole is the relevant access.
2. README.md:470 (FinOps Index Analysis section) still lists SQLAgentReaderRole as part of "the grants above," contradicting the corrected grant block just above it.
3. Darling/README.md:268 and :602 (troubleshooting) still reference SQLAgentReaderRole as the thing to check/grant for the failed-job skip message — :602 in particular tells operators to "grant the role if you want job alerts," which this PR's own field investigation proved doesn't work.

Not flagged inline, minor: deprecated/Dashboard/README.md still documents ALTER ROLE [SQLAgentReaderRole] ADD MEMBER even though the deprecated Dashboard's own C# skip message (DatabaseService.NocHealth.cs, updated in this PR) now says that role alone isn't enough — same category of staleness, just in a component outside the PR's stated doc scope (only README.md + Darling/README.md were mentioned). Worth a follow-up if the deprecated app's README is still maintained.

No correctness bugs in the query/error-handling logic itself, and no security concerns (the new SQL is static GRANT DDL in docs plus one added static-column predicate, no dynamic SQL or new input surface).