fix(dashboard): de-duplicate dashboard_tags typed into the API-key create form

The form split tags with the shared ParseList and attached the result verbatim,
so "team-a, TEAM-A" persisted as two entries and the constraints column read
dashboard_tags=[team-a, TEAM-A] — one grant reported as two on the page whose
job is to show what a key was granted. Enforcement never saw it (a session holds
its tags in a case-insensitive set), which is exactly why the display was the
only place it could surface.

De-duplicates ordinal-ignore-case at the attach point only, first spelling
winning, matching ApiKeyAdminCommandLineParser.ParseDashboardTags. ParseList is
untouched: the five glob lists are matched literally, so near-duplicates there
are not necessarily the same rule and must survive verbatim — pinned by a test.

The help text claimed to mirror the CLI flag; it now claims only the shared
separators and the dedupe, since the form still drops an empty segment silently
where the CLI hard-fails. A browser form has no exit code to fail with, so that
difference stays, and Authorization.md now records it.
This commit is contained in:
Joseph Doherty
2026-08-18 05:19:02 -04:00
parent 1ea6f60ea2
commit fab600d3b0
3 changed files with 85 additions and 3 deletions
+4 -1
View File
@@ -197,7 +197,10 @@ Tags are set at key creation with
`apikey create-key --dashboard-tags team-a,team-b` (repeatable; segments are `apikey create-key --dashboard-tags team-a,team-b` (repeatable; segments are
trimmed and de-duplicated ordinal-ignore-case). The dashboard API Keys page sets trimmed and de-duplicated ordinal-ignore-case). The dashboard API Keys page sets
them too: its create form has a **Dashboard tags** field alongside the data-access them too: its create form has a **Dashboard tags** field alongside the data-access
constraints, split on the same separators the other constraint fields use. constraints, split on the same separators the other constraint fields use and
de-duplicated ordinal-ignore-case as the CLI does. The two differ on one point: a
stray separator drops an empty segment silently on the form rather than failing
the command, because a browser form has no exit code to fail with.
That page's constraints column names `dashboard_tags` like any other member. It That page's constraints column names `dashboard_tags` like any other member. It
has to: `IsEmpty` counts the tags, so a key whose only recorded policy is a has to: `IsEmpty` counts the tags, so a key whose only recorded policy is a
@@ -121,7 +121,8 @@ else
aria-describedby="dashboardTagsHelp" aria-describedby="dashboardTagsHelp"
@bind="CreateModel.DashboardTags" @bind:event="oninput"></textarea> @bind="CreateModel.DashboardTags" @bind:event="oninput"></textarea>
<div id="dashboardTagsHelp" class="form-text small"> <div id="dashboardTagsHelp" class="form-text small">
Comma- or newline-separated; mirrors <code>apikey create-key --dashboard-tags</code>. Split on the same separators as the constraint fields above, and
de-duplicated case-insensitively — two spellings of a tag are one grant.
Matched case-insensitively against the viewer grants in Matched case-insensitively against the viewer grants in
<code>Dashboard:GroupToTag</code>. Scopes dashboard event visibility only — <code>Dashboard:GroupToTag</code>. Scopes dashboard event visibility only —
never what the key may read, write, or browse. Empty leaves the key's sessions never what the key may read, write, or browse. Empty leaves the key's sessions
@@ -478,7 +479,7 @@ else
{ {
// Init-only rather than positional (it was bolted onto the record after the // Init-only rather than positional (it was bolted onto the record after the
// eight-member constructor shipped), so it is attached here instead. // eight-member constructor shipped), so it is attached here instead.
DashboardTags = ParseList(CreateModel.DashboardTags), DashboardTags = ParseDashboardTags(CreateModel.DashboardTags),
}); });
return true; return true;
@@ -575,6 +576,18 @@ else
} }
} }
// Dashboard tags alone are de-duplicated ordinal-ignore-case, which is how the enforcement site
// compares them — two spellings are one grant, and ApiKeyAdminCommandLineParser.ParseDashboardTags
// collapses them the same way, first spelling winning. Persisting both would not change who can
// see what (GatewaySession holds its tags in a case-insensitive set) but the constraints column
// would report one grant twice, and a security grant that reads as two is the wrong kind of
// wrong on an audit surface. The other five list fields keep ParseList's verbatim behaviour:
// their globs are matched literally, so near-duplicates there are not necessarily the same rule.
private static IReadOnlyList<string> ParseDashboardTags(string? value)
{
return [.. ParseList(value).Distinct(StringComparer.OrdinalIgnoreCase)];
}
private static IReadOnlyList<string> ParseList(string? value) private static IReadOnlyList<string> ParseList(string? value)
{ {
return (value ?? string.Empty) return (value ?? string.Empty)
@@ -111,6 +111,72 @@ public sealed class ApiKeysPageDashboardTagsTests
Assert.False(request.Constraints.HasWriteConstraints); Assert.False(request.Constraints.HasWriteConstraints);
} }
/// <summary>
/// Two spellings of one tag persist as one grant, matching how
/// <c>ApiKeyAdminCommandLineParser.ParseDashboardTags</c> collapses them and how the
/// enforcement site compares them.
/// </summary>
/// <remarks>
/// Enforcement would survive the duplicate — a session holds its tags in a
/// case-insensitive set — but the API Keys page's constraints column would render
/// <c>dashboard_tags=[team-a, TEAM-A]</c> and report one grant as two, which is the
/// reading an audit surface can least afford.
/// </remarks>
[Fact]
public void TryBuildCreateRequest_DeduplicatesDashboardTagsIgnoringCase()
{
ApiKeysPage page = new();
page.CreateModel.DashboardTags = "team-a, TEAM-A, team-a";
bool built = page.TryBuildCreateRequest(out DashboardApiKeyManagementRequest? request, out string? error);
Assert.True(built, error);
Assert.NotNull(request);
// First spelling typed wins, so what the operator wrote is what the audit surface shows.
Assert.Equal(["team-a"], request.Constraints.DashboardTags);
}
/// <summary>
/// A dedupe that spanned fields would be a bug of its own: the five glob lists are matched
/// literally, so near-duplicates there are not necessarily the same rule and must survive
/// verbatim.
/// </summary>
[Fact]
public void TryBuildCreateRequest_DoesNotDeduplicateTheOtherConstraintLists()
{
ApiKeysPage page = new();
page.CreateModel.ReadSubtrees = "Area1, AREA1";
bool built = page.TryBuildCreateRequest(out DashboardApiKeyManagementRequest? request, out string? error);
Assert.True(built, error);
Assert.NotNull(request);
Assert.Equal(["Area1", "AREA1"], request.Constraints.ReadSubtrees);
}
/// <summary>
/// <c>Reset</c> clears the new field, so the next key minted in the same dialog does not
/// inherit the previous key's tag grant.
/// </summary>
[Fact]
public void Reset_ClearsDashboardTagsAlongsideTheOtherConstraintFields()
{
ApiKeysPage page = new();
page.CreateModel.DashboardTags = "team-a";
// A sibling field pins the assertion to Reset itself: were Reset a no-op, both would
// survive and the failure would name the method rather than the one line.
page.CreateModel.ReadSubtrees = "Area1";
page.CreateModel.KeyId = "leftover";
page.CreateModel.Reset();
Assert.Equal(string.Empty, page.CreateModel.DashboardTags);
Assert.Equal(string.Empty, page.CreateModel.ReadSubtrees);
Assert.Equal(string.Empty, page.CreateModel.KeyId);
}
/// <summary>An empty field leaves the key untagged rather than inventing a tag.</summary> /// <summary>An empty field leaves the key untagged rather than inventing a tag.</summary>
[Fact] [Fact]
public void TryBuildCreateRequest_WhenDashboardTagsIsBlank_LeavesTheKeyUntagged() public void TryBuildCreateRequest_WhenDashboardTagsIsBlank_LeavesTheKeyUntagged()