diff --git a/CHANGELOG.md b/CHANGELOG.md index ee3faca4e..982b7c6df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **Authentication settings: startup-config save no longer offers a downloadable `config.json` fallback when the backend refuses the save as invalid.** `SettingsView.saveSettings()` previously wrapped `apiService.saveStartupConfig` in a bare `catch {}` and treated every failure as a disk-persistence problem — offering the user a downloadable `config.json` containing the *server-rejected* values so they could save it manually. That bypasses the new backend admin-existence guard entirely: a user who tries to enable the login screen with no admin user gets the backend's 400, the FE catches it, and the FE offers a download of the same `AuthenticationRequired=true` config the server just refused. The catch now inspects the thrown error's `status`: 4xx responses are validation refusals and surface as a hard error toast (no download offered); 5xx and network failures fall through to the existing download fallback, which is the right escape hatch for "server wants to save but can't write to disk." +- **Authentication settings: enabling the login screen now refuses to persist when no admin user exists.** `ConfigurationService.SaveStartupConfigAsync` queries `IUserService.GetAdminUsersAsync` whenever the incoming save *transitions* `AuthenticationRequired` from disabled to enabled, and throws if the admin user list is empty. This closes the carveout left by the credential-visibility and admin-provisioning fixes below: the settings DTO clears blank fields before save, so a user who flips "Enable login screen" with empty (or username-only) admin credentials silently skipped provisioning entirely and still reached the startup-config write, locking themselves out of an admin-less instance (recoverable by editing `config/config.json` back to `"AuthenticationRequired": "false"`, but a confusing first-time-setup trap). The check is scoped to the transition: subsequent saves while auth is already on (API key regenerations, port changes, log-level tweaks) don't re-query the admin list, and the common "just updating other startup fields with auth off" path stays unaffected. The admin block in `SaveApplicationSettings` runs before the startup-config write in the same save flow, so the typical "supply credentials and enable login in the same save" sequence has the admin row in place by the time the check runs. +- **Authentication settings: admin provisioning failures no longer silently let the auth-required toggle proceed.** `ConfigurationService.SaveApplicationSettingsAsync` previously caught any exception from `CreateUserAsync` / `UpdatePasswordAsync`, logged it, and returned successfully — so when admin credentials were supplied but the user-service rejected them (password policy violation, repo I/O error, concurrent-write race), `SettingsView.saveSettings()` would still go on to persist `AuthenticationRequired=true` on its second request. The result was an instance that required login but had no working admin account — exactly the lockout shape the credential-visibility fix below was meant to prevent. The catch now re-throws the failure so the caller aborts before the auth-toggle write. The settings row itself is still saved before the admin block (non-admin changes like notification triggers and webhooks shouldn't disappear because admin provisioning failed), and the no-credentials path remains an unchanged silent skip. +- **Authentication settings: corrected misleading description on the "Enable login screen" toggle.** Previously said *"Changes here are local and will not modify server files — edit config/config.json on the host to persist"*, which was demonstrably wrong: `SettingsView` actually writes `authenticationRequired` back to the server's startup config on save. The description now accurately states the toggle persists, and prompts the user to set admin credentials in the same save. +- **Authentication settings: admin credential fields are always visible.** Previously the *Admin Account Management* row was gated by `v-if="authEnabledComputed"` in `AuthenticationSection.vue`, which meant the only way to surface the username/password inputs was to first toggle on the login screen. If a user enabled `AuthenticationRequired` via `config.json` on the host (e.g., for the very first time) and then opened settings, the toggle reflected the server state (on), but if they instead opened settings *with auth still off*, the fields were hidden — and once they ticked the toggle and saved, the login screen activated immediately on the next page load, locking them out before they could create a user. The fields now render unconditionally so credentials can be configured before or after enabling auth. Help text and the password placeholder were updated to reflect the create-or-update semantics (blank password = keep existing). + ## [0.2.71] - 2026-04-17 ### Added diff --git a/fe/src/__tests__/AuthenticationSection.spec.ts b/fe/src/__tests__/AuthenticationSection.spec.ts index 2b47d0c65..e0d41e4d3 100644 --- a/fe/src/__tests__/AuthenticationSection.spec.ts +++ b/fe/src/__tests__/AuthenticationSection.spec.ts @@ -27,6 +27,23 @@ describe('AuthenticationSection', () => { vi.restoreAllMocks() }) + it('renders the admin credential inputs even when authEnabled is false', async () => { + // Regression guard: the admin form must be visible while auth is disabled + // so a user can configure credentials before turning the login screen on. + // Previously this was gated by `v-if="authEnabledComputed"`, which made + // first-time setup require enabling auth (and the login screen) before + // any UI affordance for credentials existed — a lockout. + const { default: AuthenticationSection } = + await import('@/components/settings/AuthenticationSection.vue') + const wrapper = mount(AuthenticationSection, { + props: { settings: { adminUsername: '', adminPassword: '' }, authEnabled: false }, + global: { components: { PasswordInput } }, + }) + + expect(wrapper.find('input[type="text"][placeholder="Admin username"]').exists()).toBe(true) + expect(wrapper.findComponent(PasswordInput).exists()).toBe(true) + }) + it('emits update:authEnabled when checkbox toggled', async () => { const { default: AuthenticationSection } = await import('@/components/settings/AuthenticationSection.vue') diff --git a/fe/src/components/settings/AuthenticationSection.vue b/fe/src/components/settings/AuthenticationSection.vue index 84748a939..d962ea6c9 100644 --- a/fe/src/components/settings/AuthenticationSection.vue +++ b/fe/src/components/settings/AuthenticationSection.vue @@ -23,7 +23,7 @@ @@ -36,9 +36,8 @@
diff --git a/fe/src/views/SettingsView.vue b/fe/src/views/SettingsView.vue index 8e66a4ad1..11e4e2ca5 100644 --- a/fe/src/views/SettingsView.vue +++ b/fe/src/views/SettingsView.vue @@ -860,31 +860,49 @@ const saveSettings = async () => { startupConfig.value = newCfg startupConfigSaved = true toast.success('Startup config', 'Startup configuration saved (config.json)') - } catch { - // If server can't persist startup config (e.g., permission denied), offer a fallback download of the config JSON - toast.info( - 'Startup config', - 'Could not persist startup config to disk. Preparing downloadable startup config so you can save it manually.', - ) - try { - const blob = new Blob([JSON.stringify(newCfg, null, 2)], { type: 'application/json' }) - const url = URL.createObjectURL(blob) - const a = document.createElement('a') - a.href = url - a.download = 'config.json' - document.body.appendChild(a) - a.click() - a.remove() - URL.revokeObjectURL(url) - toast.info( - 'Startup config', - 'Download started. Save the file to the server config directory to persist the change.', - ) - } catch { + } catch (err) { + // Distinguish a server *validation refusal* (e.g. attempting to enable + // the login screen when no admin user exists — backend returns 400 + // with an actionable message) from a *disk-persistence failure* (e.g. + // permission denied writing config.json — backend wants to save but + // can't). For validation refusals we must NOT offer the download + // fallback: letting the user manually save a server-rejected config + // would defeat the backend guard entirely (see PR #623). For genuine + // persistence failures, the download fallback is still the right + // escape hatch so the operator can save the file by hand. + const status = (err as { status?: number } | null)?.status + const isValidationRefusal = typeof status === 'number' && status >= 400 && status < 500 + if (isValidationRefusal) { + const message = + err instanceof Error && err.message + ? err.message + : 'Startup configuration refused by the server.' + toast.error('Startup config refused', message) + } else { toast.info( 'Startup config', - 'Also failed to prepare a download. Edit config/config.json on the host to make the change persistent.', + 'Could not persist startup config to disk. Preparing downloadable startup config so you can save it manually.', ) + try { + const blob = new Blob([JSON.stringify(newCfg, null, 2)], { type: 'application/json' }) + const url = URL.createObjectURL(blob) + const a = document.createElement('a') + a.href = url + a.download = 'config.json' + document.body.appendChild(a) + a.click() + a.remove() + URL.revokeObjectURL(url) + toast.info( + 'Startup config', + 'Download started. Save the file to the server config directory to persist the change.', + ) + } catch { + toast.info( + 'Startup config', + 'Also failed to prepare a download. Edit config/config.json on the host to make the change persistent.', + ) + } } } // If authentication has just been enabled and persistence succeeded, ensure we diff --git a/listenarr.application/Common/ConfigurationService.cs b/listenarr.application/Common/ConfigurationService.cs index 30dd74184..82581298e 100644 --- a/listenarr.application/Common/ConfigurationService.cs +++ b/listenarr.application/Common/ConfigurationService.cs @@ -267,7 +267,17 @@ public async Task SaveApplicationSettingsAsync(ApplicationSettings settings) } catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) { - logger.LogError(ex, "Failed to create or update admin user '{Username}' from application settings. Settings will still be saved.", settings.AdminUsername); + // Admin provisioning failed after credentials were supplied. Surface + // the failure to the caller — SettingsView relies on this throwing + // before it persists AuthenticationRequired=true on its second + // request, otherwise the user can be locked out of an instance + // that has no working admin (password-policy rejection, repo I/O + // error, race against a concurrent admin write, etc.). The + // settings row above was already saved, which is intentional: + // non-admin changes (notification triggers, webhooks, etc.) are + // worth preserving even when credential provisioning fails. + logger.LogError(ex, "Failed to create or update admin user '{Username}' from application settings; surfacing failure to caller", settings.AdminUsername); + throw; } } catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) @@ -404,7 +414,43 @@ public async Task SaveStartupConfigAsync(StartupConfig config) { try { - await startupConfigService.SaveAsync(config); + // Defense-in-depth backstop against the auth-enable lockout. + // SaveApplicationSettingsAsync's throw-on-failure (above) only + // covers the case where admin credentials were *supplied* but + // provisioning failed. The settings DTO clears blank fields + // before save, so a user who flips the login-screen toggle + // with empty (or username-only) credentials silently skips + // provisioning entirely — and without this check would still + // reach the startup-config write below, locking themselves + // out of an instance that has no working admin to log in as. + // + // Only enforced on the *transition* from auth-disabled to + // auth-enabled. Once auth is already on, the admin must + // already exist (or no one could have toggled it on through + // this same check), and every subsequent unrelated save + // — API key regenerations, port changes, log-level tweaks — + // shouldn't have to re-prove the admin row is still there. + // Demotion or deletion of the last admin row while auth is + // enabled is a separate concern and belongs in the user + // management path, not here. + if (config != null && config.IsAuthenticationEnabled()) + { + var currentConfig = startupConfigService.GetConfig(); + var wasAuthEnabled = currentConfig?.IsAuthenticationEnabled() == true; + if (!wasAuthEnabled) + { + var admins = await userService.GetAdminUsersAsync(); + if (admins == null || admins.Count == 0) + { + throw new InvalidOperationException( + "Cannot enable the login screen: no admin user exists. " + + "Set an admin username and password in the same save to " + + "create one, or leave the login screen disabled."); + } + } + } + + await startupConfigService.SaveAsync(config!); } catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) { diff --git a/tests/Features/Api/Services/ConfigurationServiceTests.cs b/tests/Features/Api/Services/ConfigurationServiceTests.cs index 68384b002..f75190fca 100644 --- a/tests/Features/Api/Services/ConfigurationServiceTests.cs +++ b/tests/Features/Api/Services/ConfigurationServiceTests.cs @@ -18,6 +18,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.EntityFrameworkCore; +using Moq; using Xunit; using Listenarr.Domain.Models; using Listenarr.Domain.Common; @@ -30,6 +31,8 @@ namespace Listenarr.Tests.Features.Api.Services { + [Trait("Name", "ConfigurationServiceTests")] + [Trait("Category", "ConfigurationService")] public class ConfigurationServiceTests : BaseTests { [Fact] @@ -162,5 +165,199 @@ await svc.SaveApplicationSettingsAsync(new ApplicationSettings var stored = await _applicationSettingsRepository.GetAsync(); Assert.False(string.IsNullOrWhiteSpace(stored.ProwlarrApiKeyEncrypted)); } + + [Fact] + public async Task SaveApplicationSettings_AdminProvisioningFailure_PropagatesToCaller() + { + // When the caller supplies admin credentials but the user-service + // can't honour the request (password policy violation, repo I/O + // error, race with a concurrent admin write), the failure must + // reach the caller. SettingsView.saveSettings() persists + // AuthenticationRequired=true *after* the call to + // SaveApplicationSettingsAsync; if the admin failure is swallowed + // here, the operator ends up with an instance that requires + // login and has no working admin — a hard lockout. + // + // Regression coverage for the `kevinheneveld:fix/auth-admin-credentials-always-visible` + // upstream PR review feedback. + var failingUserService = new Mock(MockBehavior.Strict); + failingUserService.Setup(u => u.GetByUsernameAsync(It.IsAny())) + .ReturnsAsync((User?)null); + failingUserService.Setup(u => u.CreateUserAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ThrowsAsync(new InvalidOperationException("password rejected by policy")); + + Init(b => b.WithScoped(_ => failingUserService.Object)); + + var svc = _provider.GetRequiredService(); + var settings = await svc.GetApplicationSettingsAsync(); + settings.AdminUsername = "admin"; + settings.AdminPassword = "weakpass"; + // Bundle a non-admin change in the same payload so we can verify + // it still lands — non-admin settings are saved before the admin + // block, and that ordering is intentional. + settings.OutputPath = FileUtils.GetAbsolutePath("admin-fail-output"); + + var ex = await Assert.ThrowsAsync( + () => svc.SaveApplicationSettingsAsync(settings)); + Assert.Equal("password rejected by policy", ex.Message); + + // Non-admin changes saved before the admin block remain — the + // settings row write is intentionally outside the admin try/catch. + var afterFail = await svc.GetApplicationSettingsAsync(); + Assert.Equal(FileUtils.GetAbsolutePath("admin-fail-output"), afterFail.OutputPath); + + failingUserService.Verify( + u => u.CreateUserAsync("admin", "weakpass", null, true), + Times.Once); + } + + [Fact] + public async Task SaveApplicationSettings_NoAdminCredentials_DoesNotInvokeUserService() + { + // Carveout check: when no credentials are supplied (the common + // "I'm just updating notification triggers" path), the admin + // block must remain a silent skip — neither invoking the user + // service nor throwing. + var userService = new Mock(MockBehavior.Strict); + + Init(b => b.WithScoped(_ => userService.Object)); + + var svc = _provider.GetRequiredService(); + var settings = await svc.GetApplicationSettingsAsync(); + settings.AdminUsername = null; + settings.AdminPassword = null; + settings.OutputPath = FileUtils.GetAbsolutePath("no-creds-output"); + + await svc.SaveApplicationSettingsAsync(settings); + + userService.VerifyNoOtherCalls(); + } + + [Fact] + public async Task SaveStartupConfig_RefusesAuthEnableTransition_WhenNoAdminExists() + { + // Defense-in-depth backstop for the auth-enable lockout. The + // SaveApplicationSettings throw-on-failure only covers the case + // where credentials were *supplied* and rejected; the FE strips + // blank fields before save, so a user can tick "Enable login + // screen" with empty (or username-only) credentials, the admin + // block silently no-ops, and without this check the startup + // config would still be persisted with AuthenticationRequired=true + // — locking the operator out of an admin-less instance. + // + // Regression coverage for the upstream #623 review follow-up. + var emptyAdminUserService = new Mock(); + emptyAdminUserService.Setup(u => u.GetAdminUsersAsync()) + .ReturnsAsync(new List()); + + // Current startup config must be present and have auth *off* so + // the new save constitutes a transition from disabled to enabled. + var currentConfigDisabled = new Mock(); + currentConfigDisabled.Setup(s => s.GetConfig()) + .Returns(new StartupConfig { AuthenticationRequired = "false" }); + + Init(b => b + .WithScoped(_ => emptyAdminUserService.Object) + .WithSingleton(currentConfigDisabled.Object)); + + var svc = _provider.GetRequiredService(); + var startup = new StartupConfig { AuthenticationRequired = "true" }; + + var ex = await Assert.ThrowsAsync( + () => svc.SaveStartupConfigAsync(startup)); + Assert.Contains("Cannot enable the login screen", ex.Message); + + emptyAdminUserService.Verify(u => u.GetAdminUsersAsync(), Times.Once); + // The file write must NOT have been reached. + currentConfigDisabled.Verify(s => s.SaveAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task SaveStartupConfig_AllowsAuthEnableTransition_WhenAdminExists() + { + // The typical "supply credentials + enable login in the same save" + // flow runs SaveApplicationSettings (which creates the admin user) + // before SaveStartupConfig. By the time this check fires the + // admin row exists, so the backstop passes through. + var withAdminUserService = new Mock(); + withAdminUserService.Setup(u => u.GetAdminUsersAsync()) + .ReturnsAsync(new List + { + new() { Username = "admin", IsAdmin = true }, + }); + + var currentConfigDisabled = new Mock(); + currentConfigDisabled.Setup(s => s.GetConfig()) + .Returns(new StartupConfig { AuthenticationRequired = "false" }); + + Init(b => b + .WithScoped(_ => withAdminUserService.Object) + .WithSingleton(currentConfigDisabled.Object)); + + var svc = _provider.GetRequiredService(); + var startup = new StartupConfig { AuthenticationRequired = "true" }; + + await svc.SaveStartupConfigAsync(startup); + + withAdminUserService.Verify(u => u.GetAdminUsersAsync(), Times.Once); + currentConfigDisabled.Verify(s => s.SaveAsync(startup), Times.Once); + } + + [Fact] + public async Task SaveStartupConfig_SkipsAdminCheck_WhenAuthAlreadyEnabled() + { + // Once auth is already on, the admin must already exist (or the + // transition check above wouldn't have let it land), so every + // subsequent unrelated save — API key regenerations, port + // changes, log-level tweaks — must NOT re-query the admin list. + // This keeps the backstop scoped to the lockout vector Robbie + // identified during first-time setup, and avoids breaking the + // session-cookie integration tests that stub auth-on factories + // without populating IUserService. + var userService = new Mock(MockBehavior.Strict); + var currentConfigEnabled = new Mock(); + currentConfigEnabled.Setup(s => s.GetConfig()) + .Returns(new StartupConfig { AuthenticationRequired = "true" }); + + Init(b => b + .WithScoped(_ => userService.Object) + .WithSingleton(currentConfigEnabled.Object)); + + var svc = _provider.GetRequiredService(); + + await svc.SaveStartupConfigAsync(new StartupConfig + { + AuthenticationRequired = "true", + ApiKey = "regenerated-key", + }); + + userService.VerifyNoOtherCalls(); + currentConfigEnabled.Verify(s => s.SaveAsync(It.IsAny()), Times.Once); + } + + [Fact] + public async Task SaveStartupConfig_SkipsAdminCheck_WhenAuthDisabled() + { + // Carveout check: the admin-count query only fires when auth is + // actually being enabled. Persisting any startup config with + // AuthenticationRequired=false (or blank, or any non-truthy + // value) doesn't need an admin and must not block on one — the + // common path is "just updating other startup fields." + var userService = new Mock(MockBehavior.Strict); + + Init(b => b.WithScoped(_ => userService.Object)); + + var svc = _provider.GetRequiredService(); + + await svc.SaveStartupConfigAsync(new StartupConfig { AuthenticationRequired = "false" }); + await svc.SaveStartupConfigAsync(new StartupConfig { AuthenticationRequired = null }); + await svc.SaveStartupConfigAsync(new StartupConfig { AuthenticationRequired = "" }); + + userService.VerifyNoOtherCalls(); + } } }