Skip dashboard instance-store queries without workflows/instances:view

[sfmskywalker]

Purpose

Same Greptile 4/5 fix as #8572, targeted at release/3.9.0: a caller who can view runtime status but not workflow instances no longer hits the instance store on every overview load.

Scope

Select one primary concern:

• Bug fix (behavior change)
• Refactor (no behavior change)
• Documentation update
• Formatting / code cleanup
• Dependency / build update
• New feature

Description

Problem

WorkflowDashboardContributor declares both workflows/runtime:view and workflows/instances:view. The overview composer still invoked it for a runtime-only caller, and GetOverviewAsync always ran one SummarizeMany plus five Count queries. DefaultDashboardProvider then withheld the instance section. The figures were thrown away.

Solution

• DashboardContext keeps its original five positional parameters (the 3.8 constructor and Deconstruct). CanRead is an init-only property on the record body; null means unrestricted.
• DefaultDashboardProvider.CreateContext sets CanRead with an object initializer.
• WorkflowDashboardContributor skips the instance-store queries on the overview path when the caller cannot read workflows/instances:view. It still returns an empty instance section so the provider withholds it as Unauthorized.
• The findings-path skip is removed: GET /dashboard/needs-attention already requires dashboard:view or workflows/instances:view.
• Tests: GetOverviewAsync_WithRuntimeOnlyCaller_DoesNotQueryTheInstanceStore and a contributor-level counterpart.

Docs fix (verbatim 3.9 wording, also on #8572)

Pre-upgrade refresh tokens stay valid for the remaining time under their issued expiry. The old "at most RefreshTokenLifetime after the upgrade" bound is wrong if that lifetime is lowered during the upgrade. Same change in doc/migrations/authorization-model.md and doc/wiki/identity-tenancy-security.md. The migration guide also notes that a contributor can check DashboardContext.CanRead to skip queries, and that null means unrestricted.

Companion on main: #8572.

Verification

HEAD: 40d54a4e3

dotnet test test/unit/Elsa.Dashboard.Api.UnitTests/Elsa.Dashboard.Api.UnitTests.csproj — 114 passed.

Checklist

• The PR is focused on a single concern
• Commit messages follow the recommended convention
• Tests added or updated (if applicable)
• No unrelated cleanup included
• All tests pass

  

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: ba61df94-7947-4d37-9444-92354de4f844

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[sfmskywalker]

@greptileai

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Dashboard permission checks skip unnecessary database queries.

No outstanding findings block merging.

Summary

The dashboard overview avoids workflow-instance-store queries when a caller can read runtime status but not workflow instances. The existing DashboardContext positional API is preserved, the callback is documented, and the refresh-token expiry documentation is corrected.

_{Reviews (2) · Last reviewed commit: "fix(dashboard): keep DashboardContext bi..."}

[sfmskywalker]

Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ 5090663

Code Review, Round 1/4

This backports the dashboard skip fix to release/3.9.0 as a single commit (5090663) on the current branch tip. The behaviour is right, it fails closed, and the scope is narrow. But for a frozen 3.9 it changes the public API in a way that breaks binary compatibility, and it adds a guard the shipped endpoint never reaches. Both are small to fix.

Matches #8572

The five changed files are byte-identical to #8572 @ 989065d: IDashboardContributor.cs, DefaultDashboardProvider.cs, WorkflowDashboardContributor.cs, and the two Dashboard test files. The diff against the base is identical line for line to #8572's two fix commits combined. No branch-specific changes were needed, and the port includes the empty-section correction from #8572's second commit.

Suitable for 3.9?

Yes, once the items below are fixed.

• 3.9 introduced the problem. In 3.8.4 the overview needed dashboard:view and had no per-section permissions. 3.9 opens GET /dashboard/overview to any signed-in user and withholds each section by permission. As a result, a caller with only workflows/runtime:view makes the workflow contributor run one unpaged summary query and five counts on every overview poll, and then those results are thrown away.
• The migration guide promises otherwise. It says the per-contributor skip exists so that an account without dashboard permissions "costs no database counts". This fix makes the runtime-only case keep that promise before 3.9 ships.
• The scope is tight. There are 3 source files and 2 test files, with no behaviour change for callers who can read instances, and no wire or contract change.
• The catch. It currently changes a public record that shipped in 3.8 (item 1), which a frozen branch must not do.

Blocking

1. Keep DashboardContext binary-compatible.
DashboardContext has shipped with five positional parameters since 3.8.0. Adding CanRead as a sixth removes the five-argument constructor and the five-value Deconstruct, so an extension built against 3.8 throws MissingMethodException on 3.9. Greptile verified this by swapping the DLL under an unchanged consumer. Make CanRead an init-only property on the record body and leave the positional list alone:

public record DashboardContext(
    DashboardRange Range,
    bool IncludeSystem,
    CancellationToken CancellationToken,
    string? TenantId = null,
    string? EnvironmentName = null)
{
    /// <summary>
    /// Whether the caller may read data guarded by a permission, with the dashboard's default rule already applied
    /// (<c>dashboard:view</c> reads everything, and data with no declared permission needs it). It only tells a
    /// contributor which queries it can skip; the provider still filters what the contributor returns.
    /// <c>null</c> means unrestricted.
    /// </summary>
    public Func<DashboardPermission?, bool>? CanRead { get; init; }
}

In DefaultDashboardProvider.CreateContext, use new(range, includeSystem, cancellationToken, EnvironmentName: environment.EnvironmentName) { CanRead = canRead }. I tried this on this branch: it builds and all 114 Dashboard tests pass, with the tests' with { CanRead = ... } unchanged. This resolves Greptile's "Preserve existing record signatures" thread.

2. Document the hook (Greptile's "Document the contributor callback" thread).
The doc comment above covers the API. Also add one sentence to the "Modules that add to the dashboard" paragraph of doc/migrations/authorization-model.md: a contributor can check DashboardContext.CanRead to skip queries for data the caller can't read, and null means unrestricted.

3. Drop the GetFindingsAsync guard (Greptile's "Cover findings query guard" thread).
GET /dashboard/needs-attention has RequireAnyPermission(dashboard:view, workflows/instances:view). Every API caller that reaches GetFindingsAsync can therefore read instances, so the guard never fires through the shipped API. Greptile confirmed no test covers it. On a frozen branch, remove it rather than add a test for a path the shipped API doesn't take. It also makes the diff smaller. Keep the overview guard.

Make the same three changes on #8572 so the two branches stay identical.

Verified

• Fails closed. The skip only saves work. The provider passes the same canRead it filters with, and MergeSection still decides what the caller sees. A wrong or missing CanRead can at worst cost queries; it can't expose data. null falls back to querying, which matches DashboardAccess.WithDefault(null).
• Empty section, not omitted. A runtime-only caller gets WorkflowInstances back as Unauthorized, as the migration guide documents. If the section were left out, it would read Available with zero counts.
• The tests mean something. I ran two controls on this branch. Reverting the contributor fails both new tests. Leaving the section out fails the provider test and the existing GetOverviewAsync_WithFailingInstanceStore_... theory. The store returns 99 for counts, so Running == 0 really shows nothing was queried. Keeping only the provider-level test would be enough, but this is not blocking.
• Local run. Elsa.Dashboard.Api.UnitTests passes 114/114 at this head.

Merge gate

core needs APPROVE + HIGH, green CI and Greptile 5/5 on the tip.

• Review: REQUEST_CHANGES + HIGH (items 1–3).
• CI at 5090663: ubuntu-latest (build and tests) was still running when I checked. GitGuardian, CLA and CodeRabbit passed. CodeRabbit is disabled for this base branch.
• Greptile: 4/5 on 5090663, with three open threads, all covered by items 1–3. A fresh 5/5 is needed on the new tip.
• Threads: 3 unresolved, all from Greptile.
• 3.9 is frozen, so the release owner should confirm this counts as a blocker. My read: yes, once it is binary-compatible, because 3.9 introduced the wasted queries and the migration guide promises they don't happen.

[cursor]

@greptileai

[sfmskywalker]

Elsa 3 Code Review: APPROVE + HIGH @ 40d54a4

Code Review, Round 2/4

The only change since Round 1 is 40d54a4, a single commit on top of 5090663. It is on the current release/3.9.0 tip (61aa0f7), and it resolves all three Round 1 items. The diff against the base is still small: three dashboard source files, two dashboard test files and two doc files.

Round 1 items

1. Binary compatibility: fixed, checked against the shipped package.
DashboardContext keeps its five positional parameters, and CanRead is an init-only property on the record body. I compared the public surface of DashboardContext in three builds: the Elsa.Dashboard.Abstractions 3.8.4 package from nuget.org, the release/3.9.0 base, and this head.

• 3.8.4 and the base are identical.
• This head only adds the CanRead property, get_CanRead, and set_CanRead (init). The five-argument constructor and the five-value Deconstruct keep their exact signatures.
• Runtime check: I compiled a consumer against the 3.8.4 DLL; it constructs with five arguments, deconstructs, uses with, and compares. It runs unchanged against this head's DLL. Against the Round 1 head (5090663), the same consumer throws MissingMethodException.

2. Documentation: done.

• XML doc: CanRead has a doc comment saying the default rule is already applied, that it only tells a contributor which queries it can skip, that the provider still filters, and that null means unrestricted.
• Migration guide: it gains one accurate sentence about the hook.
• Optional nit: that sentence would read better at the end of its paragraph, after the OverviewPermissions explanation.

3. Findings guard: removed. GetFindingsAsync is back to the base code, and the overview guard stays. That makes the 3.9 change smaller.

Also in this commit: the refresh-token wording fix from #8572, in doc/migrations/authorization-model.md and doc/wiki/identity-tenancy-security.md. Tokens stay valid "for the remaining time under their issued expiry, which can outlast the current RefreshTokenLifetime if that setting was lowered during the upgrade". That's accurate, since the lifetime validator checks only exp. It's a docs-only correction, and it keeps both branches saying the same thing, so it's fine on a frozen branch.

What the CanRead default means

• null is unrestricted, not fail-closed. A null CanRead means "unrestricted": the contributor queries, as it did in 3.8. That's correct, because extensions and custom providers that use the old constructor never set it, and denying on null would hide instance figures from callers who may read them.
• Where fail-closed lives. Fail-closed is in DefaultDashboardProvider, which filters every section with the same canRead it passes in. A missing or wrong value can only cost queries; it can't expose data.
• Construction sites. The only production construction site is DefaultDashboardProvider.CreateContext, used by the overview and needs-attention paths, and it sets CanRead = canRead.
  • That value comes from DashboardAccess.WithDefault over CreateReadCheck(HttpContext).
  • The check returns true for workflows/instances:view and dashboard:view callers.
  • It also returns true for everyone when endpoint security is disabled.
  • So no caller who may read instances is skipped.

Tests still catch regressions

The Dashboard suite passes 114/114 on this head. I applied five mutations and confirmed each one is caught:

Mutation	What catches it
Revert the contributor to the base	Both runtime-only tests
Provider stops setting CanRead	The provider runtime-only test
Leave out the empty instance section	Both runtime-only tests plus the failing-store theory
Treat null as deny	The contributor failing-store test
Skip whenever CanRead is set	The provider failing-store theory

Non-blocking: the over-hiding mutations are caught only by failing-store tests. A positive provider test for a workflows/instances:view caller would pin that path directly. That belongs on main, not in 3.9.

Branch parity with #8572

• 40d54a4 and #8572's c228bc2 change the same lines.
• The three dashboard source files and the two test files are byte-identical between the two heads.
• The built DLLs expose the same DashboardContext surface.

Suitable for 3.9

• Narrow: 3 source files and 2 test files, plus a docs correction.
• Compatible: no binary or wire contract change; the API change is purely additive.
• Unchanged for readers of instance data: callers who can read instances see no behaviour change.
• A 3.9 regression: it fixes wasted instance-store queries that 3.9 introduced, which go against the migration guide's promise that callers who can't read instance data cost no database counts.
• The release owner still decides what counts as a blocker on a frozen branch. My read: this qualifies.

Merge gate

core needs APPROVE + HIGH, green CI and Greptile 5/5 on the tip.

• Review: APPROVE + HIGH @ 40d54a4.
• CI at 40d54a4: green. ubuntu-latest (build and tests), GitGuardian and CLA passed. CodeRabbit is disabled for this base branch. Merge state is CLEAN.
• Greptile: 5/5 on 40d54a4, with no outstanding findings.
• Threads: 0 unresolved (3 of 3 resolved).
• The gate is met at this head.