-
Notifications
You must be signed in to change notification settings - Fork 853
Default telemetry API to disabled when frontend has auth #14990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -73,6 +73,16 @@ public void PostConfigure(string? name, DashboardOptions options) | |||||||
| // If an API key is configured, default to ApiKey auth mode instead of Unsecured. | ||||||||
| options.Mcp.AuthMode ??= string.IsNullOrEmpty(options.Mcp.PrimaryApiKey) ? McpAuthMode.Unsecured : McpAuthMode.ApiKey; | ||||||||
| options.Api.AuthMode ??= string.IsNullOrEmpty(options.Api.PrimaryApiKey) ? ApiAuthMode.Unsecured : ApiAuthMode.ApiKey; | ||||||||
|
|
||||||||
| // When the frontend requires authentication (e.g. OpenID Connect or BrowserToken) but the API | ||||||||
| // has no key configured (and would therefore default to Unsecured), disable the API by default | ||||||||
| // to prevent unauthenticated access to telemetry data on the same port. | ||||||||
| if (options.Api.Enabled is null && | ||||||||
| options.Frontend.AuthMode is not FrontendAuthMode.Unsecured && | ||||||||
| options.Api.AuthMode is ApiAuthMode.Unsecured) | ||||||||
|
||||||||
| options.Api.AuthMode is ApiAuthMode.Unsecured) | |
| string.IsNullOrEmpty(options.Api.PrimaryApiKey) && | |
| string.IsNullOrEmpty(options.Api.SecondaryApiKey)) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -82,6 +82,86 @@ public async Task Configuration_ApiKeyExplicit_OverridesMcp() | |||||
| Assert.Equal(apiKey.Length, options.Api.GetPrimaryApiKeyBytesOrNull()!.Length); | ||||||
| } | ||||||
|
|
||||||
| [Fact] | ||||||
| public async Task Configuration_ApiDefaultsToDisabled_WhenFrontendIsBrowserToken() | ||||||
| { | ||||||
| // Arrange - BrowserToken frontend with no API key configured | ||||||
| await using var app = IntegrationTestHelpers.CreateDashboardWebApplication(_testOutputHelper, config => | ||||||
| { | ||||||
| config[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey] = FrontendAuthMode.BrowserToken.ToString(); | ||||||
| // Don't set any Api config — no API key | ||||||
| }); | ||||||
| await app.StartAsync().DefaultTimeout(); | ||||||
|
|
||||||
| // Assert - API should default to disabled when frontend has auth but no API key | ||||||
| var options = app.Services.GetRequiredService<IOptionsMonitor<DashboardOptions>>().CurrentValue; | ||||||
| Assert.False(options.Api.Enabled); | ||||||
| } | ||||||
|
Comment on lines
+96
to
+99
|
||||||
|
|
||||||
| [Fact] | ||||||
| public async Task Configuration_ApiStaysEnabled_WhenExplicitlyEnabled() | ||||||
| { | ||||||
| // Arrange - BrowserToken frontend, explicitly enable API with API key auth | ||||||
| var apiKey = "TestKey123!"; | ||||||
| await using var app = IntegrationTestHelpers.CreateDashboardWebApplication(_testOutputHelper, config => | ||||||
| { | ||||||
| config[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey] = FrontendAuthMode.BrowserToken.ToString(); | ||||||
| config[DashboardConfigNames.DashboardApiEnabledName.ConfigKey] = "true"; | ||||||
| config[DashboardConfigNames.DashboardApiAuthModeName.ConfigKey] = ApiAuthMode.ApiKey.ToString(); | ||||||
| config[DashboardConfigNames.DashboardApiPrimaryApiKeyName.ConfigKey] = apiKey; | ||||||
| }); | ||||||
| await app.StartAsync().DefaultTimeout(); | ||||||
|
|
||||||
| // Assert - API should be enabled because explicitly configured | ||||||
| var options = app.Services.GetRequiredService<IOptionsMonitor<DashboardOptions>>().CurrentValue; | ||||||
| Assert.True(options.Api.Enabled); | ||||||
|
||||||
| Assert.True(options.Api.Enabled); | |
| Assert.Equal(true, options.Api.Enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note "API keys are shared with MCP configuration—setting either configures both" doesn’t match the current implementation in
PostConfigureDashboardOptions(it only copies Api -> Mcp, not the other direction). With the updated defaulting rules forApi:Enabled, this doc can further confuse hosters who set only MCP keys and expect the API to be treated as keyed. Update the note to reflect the one-way fallback (or implement bidirectional behavior).