From 2135d53ba38d9f19f5a609b1fbcef18c55b117da Mon Sep 17 00:00:00 2001 From: Kevin Heneveld Date: Wed, 27 May 2026 11:14:29 -0800 Subject: [PATCH 1/5] fix(auth-ui): admin credential fields are always visible in Authentication settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Admin Account Management subpanel was gated by 'v-if="authEnabledComputed"', which made the username/password inputs invisible whenever the login screen toggle was off. Combined with the toggle reflecting the server-side AuthenticationRequired value from config.json, this created a lockout: 1. User toggles 'Enable login screen' on, but the form hasn't appeared yet because there's no admin user to manage (or just because they haven't ticked it before opening the page). 2. User saves anyway. authenticationRequired flips to 'true' in config.json. 3. Next request needs auth. No admin user exists. Login screen rejects every credential. User is locked out of their own instance with no UI affordance to provision an admin. Workaround was a curl POST to /api/v1/configuration/settings with X-Api-Key (which bypasses CSRF) and a JSON body containing AdminUsername / AdminPassword — not something a user should be expected to do. Fix: drop the v-if gate so the credential inputs render whenever the Authentication section is on screen. Updated help text and password placeholder to reflect the actual create-or-update semantics (the backend treats username+password as 'create if missing, update if present', both gated on both fields being non-empty). Added a regression test asserting the inputs render with authEnabled=false. --- CHANGELOG.md | 5 +++++ fe/src/__tests__/AuthenticationSection.spec.ts | 17 +++++++++++++++++ .../settings/AuthenticationSection.vue | 5 ++--- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee3faca4e..4d8781bc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ 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: 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..2cfba72d9 100644 --- a/fe/src/components/settings/AuthenticationSection.vue +++ b/fe/src/components/settings/AuthenticationSection.vue @@ -36,9 +36,8 @@
From cebae54c7813e02a929c96be1e3f3b905d2e5db4 Mon Sep 17 00:00:00 2001 From: Kevin Heneveld Date: Wed, 27 May 2026 14:29:20 -0800 Subject: [PATCH 2/5] fix(auth-ui): correct misleading description on the 'Enable login screen' toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CheckboxCard description claimed: 'Changes here are local and will not modify server files — edit config/config.json on the host to persist.' That's stale and demonstrably wrong. SettingsView.onSave writes authenticationRequired ('true' / 'false') back to the server's startup config on every save, which is then persisted to config.json on the host. The 'local-only' text was probably accurate during early development of the toggle, before the persistence path was wired through. Replaced the text with an accurate description that also points the user at the admin credentials below — the new always-visible admin form makes this prompt useful: the user now knows to set credentials in the same save when they're enabling auth for the first time. --- CHANGELOG.md | 1 + fe/src/components/settings/AuthenticationSection.vue | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d8781bc0..1468c59d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **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 diff --git a/fe/src/components/settings/AuthenticationSection.vue b/fe/src/components/settings/AuthenticationSection.vue index 2cfba72d9..d962ea6c9 100644 --- a/fe/src/components/settings/AuthenticationSection.vue +++ b/fe/src/components/settings/AuthenticationSection.vue @@ -23,7 +23,7 @@ From 3b5c8b6cd69065418fb09654daa6ae9b430fe231 Mon Sep 17 00:00:00 2001 From: Kevin Heneveld <1192102+kevinheneveld@users.noreply.github.com> Date: Fri, 29 May 2026 07:42:49 -0800 Subject: [PATCH 3/5] fix(auth): surface admin-provisioning failures so the auth-required toggle aborts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ConfigurationService.SaveApplicationSettingsAsync` caught any exception from the admin user create/update block, logged it, and returned successfully. That left a hard lockout vector around the credential-visibility fix in this same PR: when admin credentials were supplied but the user-service rejected them (password policy violation, repo I/O error, race with a concurrent admin write), `SettingsView.saveSettings()` would still go on to persist `AuthenticationRequired=true` on its second request — the instance ends up requiring login with no working admin to log in as. Per upstream review feedback on #623: re-throw the failure from the admin block so `SettingsView` aborts before the auth-toggle write. The non-admin settings row is still saved before the admin block runs (intentionally outside the admin try/catch — notification triggers, webhooks, etc. shouldn't be lost because credential provisioning failed) and the no-credentials path remains an unchanged silent skip. Two new ConfigurationService tests: 1. Failing user-service propagates to the caller; non-admin settings bundled in the same payload still land. 2. No-credentials path doesn't touch the user-service. --- CHANGELOG.md | 1 + .../Common/ConfigurationService.cs | 12 +++- .../Api/Services/ConfigurationServiceTests.cs | 72 +++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1468c59d9..da319904e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **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). diff --git a/listenarr.application/Common/ConfigurationService.cs b/listenarr.application/Common/ConfigurationService.cs index 30dd74184..b3a28571f 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) diff --git a/tests/Features/Api/Services/ConfigurationServiceTests.cs b/tests/Features/Api/Services/ConfigurationServiceTests.cs index 68384b002..eb116dacc 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; @@ -162,5 +163,76 @@ 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(); + } } } From f43beb11f97ebf74d34e940794faebc4c33dadb3 Mon Sep 17 00:00:00 2001 From: Kevin Heneveld <1192102+kevinheneveld@users.noreply.github.com> Date: Fri, 29 May 2026 16:23:00 -0800 Subject: [PATCH 4/5] fix(auth): backstop the auth-enable transition with an admin-existence check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Option 1 (the prior commit) re-throws when the operator *supplied* admin credentials but provisioning failed. It doesn't cover the carveout Robbie identified in PR review: the settings DTO strips blank fields before save, so a user who flips "Enable login screen" with empty (or username-only) admin credentials silently skips provisioning entirely and still reaches the startup-config write, locking themselves out of an instance that now requires login but has no working admin. `SaveStartupConfigAsync` now queries `IUserService.GetAdminUsersAsync` when the incoming save transitions `AuthenticationRequired` from disabled (or unset) to enabled, and throws if the admin list is empty. Scoped to the *transition* on purpose: - Once auth is already on, the admin must already exist (or no save would have ever flipped it on through this same check), so every subsequent unrelated save — API key regenerations, port changes, log-level tweaks — must NOT re-query the admin list. This keeps the backstop narrow and avoids breaking the session-cookie integration tests that stub auth-on factories without populating IUserService. - Demotion or deletion of the last admin row while auth is enabled is a separate concern, and belongs in the user management path (DeleteUserAsync / demotion logic), not here. - The common "I'm just updating other startup fields with auth off" path stays unaffected — the admin query only fires on the transition edge. Three new ConfigurationService tests cover: refuse-on-transition-no-admin, allow-on-transition-with-admin, and the "auth-already-on subsequent save" case that the failing-test investigation surfaced. --- CHANGELOG.md | 1 + .../Common/ConfigurationService.cs | 38 +++++- .../Api/Services/ConfigurationServiceTests.cs | 123 ++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da319904e..3adbdc58b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **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). diff --git a/listenarr.application/Common/ConfigurationService.cs b/listenarr.application/Common/ConfigurationService.cs index b3a28571f..82581298e 100644 --- a/listenarr.application/Common/ConfigurationService.cs +++ b/listenarr.application/Common/ConfigurationService.cs @@ -414,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 eb116dacc..f5b6f2e9d 100644 --- a/tests/Features/Api/Services/ConfigurationServiceTests.cs +++ b/tests/Features/Api/Services/ConfigurationServiceTests.cs @@ -234,5 +234,128 @@ public async Task SaveApplicationSettings_NoAdminCredentials_DoesNotInvokeUserSe 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(); + } } } From a0535b4eaa552992f9c01dd23b5f4308a55fbbec Mon Sep 17 00:00:00 2001 From: Kevin Heneveld <1192102+kevinheneveld@users.noreply.github.com> Date: Sat, 30 May 2026 17:19:16 -0800 Subject: [PATCH 5/5] fix(auth): SettingsView distinguishes validation refusals from disk-persistence failures, plus test traits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the upstream review on PR #623: (1) FE bypass. SettingsView.saveSettings() wrapped saveStartupConfig in a bare catch {} that treated every failure as a disk-persistence problem — offering the user a downloadable config.json containing the *server-rejected* values. This bypassed 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 then helpfully offers a download of the same AuthenticationRequired=true config the server just refused. The catch now inspects the error's `status` property: - 4xx → validation refusal, surface as a hard error toast, skip the download. Letting the user manually save a server-rejected config would defeat the backend guard. - 5xx / network → genuine disk-persistence failure, keep the existing download fallback so the operator can save the file by hand. (2) Test conventions. tests/README.md says backend test classes should carry class-level [Trait("Name", ...)] and [Trait("Category", ...)] attributes — added them to ConfigurationServiceTests. The FE behaviour change is testable in principle but the SettingsView mount surface is large and the existing test file doesn't exercise the save flow at all; a regression test for this specific branch would add a fragile multi-mock setup that's likely to break on unrelated changes. Surfacing the trade-off in the PR reply. --- CHANGELOG.md | 1 + fe/src/views/SettingsView.vue | 62 ++++++++++++------- .../Api/Services/ConfigurationServiceTests.cs | 2 + 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3adbdc58b..982b7c6df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [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. 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/tests/Features/Api/Services/ConfigurationServiceTests.cs b/tests/Features/Api/Services/ConfigurationServiceTests.cs index f5b6f2e9d..f75190fca 100644 --- a/tests/Features/Api/Services/ConfigurationServiceTests.cs +++ b/tests/Features/Api/Services/ConfigurationServiceTests.cs @@ -31,6 +31,8 @@ namespace Listenarr.Tests.Features.Api.Services { + [Trait("Name", "ConfigurationServiceTests")] + [Trait("Category", "ConfigurationService")] public class ConfigurationServiceTests : BaseTests { [Fact]