SEP-1875: Warn when a reused sep service account's Grafana role is below Admin

[peter-o-addo]

Summary

Warn when a reused sep Grafana service account has an org role below Admin by probing the freshly minted token.

• sidecar/grafana_service_account.py: thread (id, created) from find_or_create_account and expose reused from mint/mint_with_retry; probe reused accounts after mint and warn on FORBIDDEN, with the same UNREACHABLE stderr diagnostic as keep_persisted_token; share the role-gap message via warn_role_gap(subject); document the probe path on resolve_token
• changelog.d/SEP-1875.changed.md: note the new stderr warning for under-privileged reused accounts
• tests/sidecar/test_grafana_service_account.py: cover reuse/create mint returns, post-mint probe warn/skip paths (FORBIDDEN, UNREACHABLE, healthy Admin), race-recovery reuse, and that VALIDATE uses the minted Bearer token

Tested

• Minting onto a reused Viewer account writes the Admin role warning and still returns the token
• Creating a new account mints a token with no role warning
• Minting onto a reused Admin account returns a token with no role warning

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• All tests pass locally (make test)
• Pre-commit hooks pass (make run-pre-commit)
• Database migrations generated if models changed (make makemigrations)
• User-facing changes documented (README, inline help, UI text)
• Configuration changes documented with examples
• Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)

[Copilot]

🟡 Changes recommended

The user-visible warning needs a changelog fragment, and one new integration test duplicates an existing path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Tracks whether a Grafana service account was reused, then warns when its token lacks Admin privileges.

Changes:

• Return reuse metadata from account/token minting.
• Probe tokens minted for reused accounts.
• Add coverage for creation, reuse, and race-recovery paths.

File summaries

File	Description
sidecar/grafana_service_account.py	Adds reuse tracking and role warning.
tests/sidecar/test_grafana_service_account.py	Tests mint results and post-mint probes.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The core test does not verify that the freshly minted token is actually used for the role probe.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

sidecar/grafana_service_account.py:305

• Keep this docstring scoped to this helper’s return contract. Explaining how callers use the flag couples these reference docs to resolve_token, so the text will become stale if probing moves or another caller is added.

This issue also appears on line 531 of the same file.

    The second element is ``True`` when the id came from a search (initial or
    race-recovery) rather than a create this call performed. Callers that mint
    a token onto a reused account can then probe it for an org role below
    ``Admin``, which this create path cannot leave behind.

tests/sidecar/test_grafana_service_account.py:942

• The queued 403 is returned regardless of which credential the probe sends, so this test would still pass if resolve_token accidentally validated with the admin credentials or another token. Assert the recorded Authorization header here to prove the newly minted token is the one being probed.

    assert grafana_stub.calls(StubRoute.VALIDATE)

sidecar/grafana_service_account.py:534

• resolve_token now has a new observable warning path, but its docstring still describes only token resolution. Document the reused-account probe and warning so the function contract stays synchronized with this behavior change.

            token, reused = await mint_with_retry(
                provider, admin_credentials(), deadline
            )
            if reused:

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation is focused, correctly documented, and adequately tested across relevant paths.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[peter-o-addo]

A local Grafana 11.2 container was started with admin/admin, confirmed healthy on :3000, and a temp working directory was prepared with the sidecar settings.yaml plus empty SEP_STATE_DIR for the mint runs below.

---

Step 1 — Scenario 1: reused Viewer sep (expect warning)

1a — Delete any existing sep service account

Deleted any existing Grafana service account named exactly sep via the admin search/delete API.

Deleted sep account id=2

1b — Create sep with org role Viewer

Created a new sep service account with org role Viewer so the mint path would reuse an under-privileged account.

Created sep id=3 role=Viewer

1c — Clear persisted token and run the mint helper

Cleared $SEP_STATE_DIR/grafana_service_account_token and ran sidecar/grafana_service_account.py against that Grafana with no configured token so it would mint onto the reused Viewer account.

exit=0
stdout_token_chars=46
stderr:
  [grafana-mint] Grafana accepts the freshly minted token but the 'sep' service account ranks below Admin in its org; a re-mint would carry the same role, so the token is kept.
persisted=yes

Expect: exit 0, token on stdout, ranks below Admin on stderr, token persisted.

---

Step 2 — Scenario 2: fresh create (expect no role-gap warning)

2a — Delete the Viewer sep account

Deleted the Viewer sep service account so the next mint would take the create path.

Deleted sep account id=3

2b — Clear persisted token and run the mint helper

Cleared the persisted token again and ran the mint helper so Grafana created a fresh Admin sep account with no prior reuse.

exit=0
stdout_token_chars=46
stderr:
persisted=yes

Expect: exit 0, token on stdout, empty stderr (no ranks below), token persisted. Grafana now has an Admin sep account created by the helper.

---

Step 3 — Scenario 3: reused Admin sep (expect no role-gap warning)

3a — Clear persisted token only (keep the Admin sep account)

Removed only the persisted token file while leaving the Admin sep account in place so the next mint would reuse it.

3b — Run the mint helper again

Ran the mint helper again so it minted onto the existing Admin sep account and probed without a role-gap warning.

exit=0
stdout_token_chars=46
stderr:
persisted=yes

Expect: exit 0, token on stdout, no ranks below warning (probe ACCEPTED), token persisted.

[github-actions]

Coverage report

This PR does not seem to contain any modification to coverable code.

[yyyyyyyan]

@peter-o-addo — the reuse flag is threaded at the source rather than re-derived in resolve_token, so both reuse branches (grafana_service_account.py:315, :329) carry it and only the create path at :337 opts out. That is the right place for the "don't probe a fresh account" carve-out to live. test_a_reused_account_at_admin_probes_without_warning is also the test this shape usually skips: it pins the negative case and still asserts calls(StubRoute.VALIDATE)[0], so it cannot pass vacuously if the probe never fired.

Mint-path probe drops the sibling's UNREACHABLE diagnostic — sidecar/grafana_service_account.py:539. keep_persisted_token splits the same validate_token result two ways, FORBIDDEN at :495 and UNREACHABLE at :502; the new probe handles only the first. A Grafana that goes unreachable in the window between the mint and the probe then costs up to PROBE_TIMEOUT_SECONDS of silent wait with nothing on stderr naming why — the same silence this ticket set out to remove.

One smaller note on the duplicated warning text is inline.

Approving — the findings above are yours to take or leave; resolve the threads when you have.

[peter-o-addo]

@yyyyyyyan , All three threads are addressed, UNREACHABLE on the mint probe, warn_role_gap(subject), and (id, created) on find_or_create_account. Could you take another look?

[yyyyyyyan]

@peter-o-addo — all three points from the last round are addressed cleanly. The mint-path probe now mirrors keep_persisted_token on UNREACHABLE, with test_a_reused_account_with_unreachable_probe_warns_after_mint pinning the diagnostic; warn_role_gap collapses the duplicated role-gap message to a single site; and find_or_create_account reads as (id, created) with the negation confined to mint.

Approving — nothing outstanding from my side.