From 1180d518a2f91c07193663e62a0fc2a7388c1119 Mon Sep 17 00:00:00 2001 From: Amelia <77553571+Fesaa@users.noreply.github.com> Date: Mon, 30 Jun 2025 14:33:10 +0200 Subject: [PATCH] Authority url validator --- API.Tests/Services/SettingsServiceTests.cs | 2 +- API/Controllers/OidcControlller.cs | 18 +++++++++-- API/Extensions/IdentityServiceExtensions.cs | 10 ++++-- API/Services/OidcService.cs | 22 +++++++++++-- API/Services/SettingsService.cs | 19 ++++++++--- .../manage-open-idconnect.component.html | 8 +++++ .../manage-open-idconnect.component.ts | 32 +++++++++++++++++-- UI/Web/src/app/admin/settings.service.ts | 5 +++ UI/Web/src/assets/langs/en.json | 3 +- 9 files changed, 103 insertions(+), 16 deletions(-) diff --git a/API.Tests/Services/SettingsServiceTests.cs b/API.Tests/Services/SettingsServiceTests.cs index a3c6b67b8..fc5dd2b25 100644 --- a/API.Tests/Services/SettingsServiceTests.cs +++ b/API.Tests/Services/SettingsServiceTests.cs @@ -27,7 +27,7 @@ public class SettingsServiceTests _mockUnitOfWork = Substitute.For(); _settingsService = new SettingsService(_mockUnitOfWork, ds, Substitute.For(), Substitute.For(), - Substitute.For>()); + Substitute.For>(), Substitute.For()); } #region UpdateMetadataSettings diff --git a/API/Controllers/OidcControlller.cs b/API/Controllers/OidcControlller.cs index 2ccc570e3..e0ba62bcc 100644 --- a/API/Controllers/OidcControlller.cs +++ b/API/Controllers/OidcControlller.cs @@ -1,6 +1,7 @@ using System.Threading.Tasks; using API.Data; using API.DTOs.Settings; +using API.Services; using AutoMapper; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; @@ -8,10 +9,11 @@ using Microsoft.Extensions.Logging; namespace API.Controllers; -[AllowAnonymous] -public class OidcController(ILogger logger, IUnitOfWork unitOfWork, IMapper mapper): BaseApiController +public class OidcController(ILogger logger, IUnitOfWork unitOfWork, + IMapper mapper, ISettingsService settingsService): BaseApiController { + [AllowAnonymous] [HttpGet("config")] public async Task> GetOidcConfig() { @@ -19,4 +21,16 @@ public class OidcController(ILogger logger, IUnitOfWork unitOfWo return Ok(mapper.Map(settings.OidcConfig)); } + [Authorize("RequireAdminRole")] + [HttpPost("is-valid-authority")] + public async Task> IsValidAuthority([FromBody] IsValidAuthorityBody authority) + { + return Ok(await settingsService.IsValidAuthority(authority.Authority)); + } + + public class IsValidAuthorityBody + { + public string Authority { get; set; } + } + } diff --git a/API/Extensions/IdentityServiceExtensions.cs b/API/Extensions/IdentityServiceExtensions.cs index 6bf253d1f..8ed9c3e50 100644 --- a/API/Extensions/IdentityServiceExtensions.cs +++ b/API/Extensions/IdentityServiceExtensions.cs @@ -141,7 +141,7 @@ public static class IdentityServiceExtensions options.Events = new JwtBearerEvents { - OnMessageReceived = SetTokenFromQuery + OnMessageReceived = SetTokenFromQuery, }; }); @@ -164,8 +164,12 @@ public static class IdentityServiceExtensions if (ctx.Principal == null) return; var user = await oidcService.LoginOrCreate(ctx.Principal); - if (user == null) return; - + if (user == null) + { + ctx.Principal = null; + await ctx.HttpContext.SignOutAsync(OpenIdConnect); + return; + } var claims = new List { diff --git a/API/Services/OidcService.cs b/API/Services/OidcService.cs index 9ddd239e3..605294379 100644 --- a/API/Services/OidcService.cs +++ b/API/Services/OidcService.cs @@ -27,6 +27,11 @@ public interface IOidcService /// /// if any requirements aren't met Task LoginOrCreate(ClaimsPrincipal principal); + /// + /// Remove from all users + /// + /// + Task ClearOidcIds(); } public class OidcService(ILogger logger, UserManager userManager, @@ -46,7 +51,7 @@ public class OidcService(ILogger logger, UserManager userM var user = await unitOfWork.UserRepository.GetByExternalId(externalId, AppUserIncludes.UserPreferences); if (user != null) { - //await SyncUserSettings(settings, principal, user); + // await SyncUserSettings(settings, principal, user); return user; } @@ -64,7 +69,7 @@ public class OidcService(ILogger logger, UserManager userM user.ExternalId = externalId; - //await SyncUserSettings(settings, principal, user); + await SyncUserSettings(settings, principal, user); var roles = await userManager.GetRolesAsync(user); if (roles.Count > 0 && !roles.Contains(PolicyConstants.LoginRole)) @@ -73,6 +78,17 @@ public class OidcService(ILogger logger, UserManager userM return user; } + public async Task ClearOidcIds() + { + var users = await unitOfWork.UserRepository.GetAllUsersAsync(); + foreach (var user in users) + { + user.ExternalId = null; + } + + await unitOfWork.CommitAsync(); + } + private async Task NewUserFromOpenIdConnect(OidcConfigDto settings, ClaimsPrincipal claimsPrincipal) { if (!settings.ProvisionAccounts) return null; @@ -133,10 +149,12 @@ public class OidcService(ILogger logger, UserManager userM var userRoles = await userManager.GetRolesAsync(user); if (userRoles.Contains(PolicyConstants.AdminRole)) return; + await SyncRoles(claimsPrincipal, user); await SyncLibraries(claimsPrincipal, user); SyncAgeRating(claimsPrincipal, user); + if (unitOfWork.HasChanges()) await unitOfWork.CommitAsync(); } diff --git a/API/Services/SettingsService.cs b/API/Services/SettingsService.cs index 51c02d659..8aa8204ff 100644 --- a/API/Services/SettingsService.cs +++ b/API/Services/SettingsService.cs @@ -17,6 +17,7 @@ using Kavita.Common.EnvironmentInfo; using Kavita.Common.Helpers; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Logging; +using Microsoft.IdentityModel.Protocols.OpenIdConnect; namespace API.Services; @@ -24,6 +25,12 @@ public interface ISettingsService { Task UpdateMetadataSettings(MetadataSettingsDto dto); Task UpdateSettings(ServerSettingDto updateSettingsDto); + /// + /// Check if the server can reach the authority at the given uri + /// + /// + /// + Task IsValidAuthority(string authority); } @@ -34,16 +41,18 @@ public class SettingsService : ISettingsService private readonly ILibraryWatcher _libraryWatcher; private readonly ITaskScheduler _taskScheduler; private readonly ILogger _logger; + private readonly IOidcService _oidcService; public SettingsService(IUnitOfWork unitOfWork, IDirectoryService directoryService, ILibraryWatcher libraryWatcher, ITaskScheduler taskScheduler, - ILogger logger) + ILogger logger, IOidcService oidcService) { _unitOfWork = unitOfWork; _directoryService = directoryService; _libraryWatcher = libraryWatcher; _taskScheduler = taskScheduler; _logger = logger; + _oidcService = oidcService; } /// @@ -347,7 +356,7 @@ public class SettingsService : ISettingsService return updateSettingsDto; } - private async Task IsValidAuthority(string authority) + public async Task IsValidAuthority(string authority) { if (string.IsNullOrEmpty(authority)) { @@ -357,8 +366,8 @@ public class SettingsService : ISettingsService var url = authority + "/.well-known/openid-configuration"; try { - var resp = await url.GetAsync(); - return resp.StatusCode == 200; + await url.GetJsonAsync(); + return true; } catch (Exception e) { @@ -413,6 +422,8 @@ public class SettingsService : ISettingsService setting.Value = updateSettingsDto.OidcConfig.Authority + string.Empty; Configuration.OidcAuthority = setting.Value; _unitOfWork.SettingsRepository.Update(setting); + + await _oidcService.ClearOidcIds(); } if (setting.Key == ServerSettingKey.OidcClientId && diff --git a/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.html b/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.html index 92945cef6..0e56c155a 100644 --- a/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.html +++ b/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.html @@ -21,6 +21,14 @@ + + @if (settingsForm.dirty || !settingsForm.untouched) { +
+ @if (formControl.errors?.invalidUri) { +
{{t('invalidUri')}}
+ } +
+ } } diff --git a/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.ts b/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.ts index 6f1bf569f..ca090938b 100644 --- a/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.ts +++ b/UI/Web/src/app/admin/manage-open-idconnect/manage-open-idconnect.component.ts @@ -2,6 +2,8 @@ import {ChangeDetectorRef, Component, OnInit} from '@angular/core'; import {TranslocoDirective} from "@jsverse/transloco"; import {ServerSettings} from "../_models/server-settings"; import { + AbstractControl, + AsyncValidatorFn, FormControl, FormGroup, ReactiveFormsModule, @@ -12,6 +14,7 @@ import {SettingsService} from "../settings.service"; import {OidcConfig} from "../_models/oidc-config"; import {SettingItemComponent} from "../../settings/_components/setting-item/setting-item.component"; import {SettingSwitchComponent} from "../../settings/_components/setting-switch/setting-switch.component"; +import {map, of} from "rxjs"; @Component({ selector: 'app-manage-open-idconnect', @@ -42,9 +45,7 @@ export class ManageOpenIDConnectComponent implements OnInit { this.serverSettings = data; this.oidcSettings = this.serverSettings.oidcConfig; - - // TODO: Validator for authority, /.well-known/openid-configuration endpoint must be reachable - this.settingsForm.addControl('authority', new FormControl(this.oidcSettings.authority, [])); + this.settingsForm.addControl('authority', new FormControl(this.oidcSettings.authority, [], [this.authorityValidator()])); this.settingsForm.addControl('clientId', new FormControl(this.oidcSettings.clientId, [this.requiredIf('authority')])); this.settingsForm.addControl('provisionAccounts', new FormControl(this.oidcSettings.provisionAccounts, [])); this.settingsForm.addControl('requireVerifiedEmail', new FormControl(this.oidcSettings.requireVerifiedEmail, [])); @@ -72,6 +73,31 @@ export class ManageOpenIDConnectComponent implements OnInit { }) } + authorityValidator(): AsyncValidatorFn { + return (control: AbstractControl) => { + let uri: string = control.value; + if (!uri || uri.trim().length === 0) { + return of(null); + } + + try { + new URL(uri); + } catch { + return of({'invalidUri': {'uri': uri}} as ValidationErrors) + } + + if (uri.endsWith('/')) { + uri = uri.substring(0, uri.length - 1); + } + + return this.settingsService.ifValidAuthority(uri).pipe(map(ok => { + if (ok) return null; + + return {'invalidUri': {'uri': uri}} as ValidationErrors; + })); + } + } + requiredIf(other: string): ValidatorFn { return (control): ValidationErrors | null => { const otherControl = this.settingsForm.get(other); diff --git a/UI/Web/src/app/admin/settings.service.ts b/UI/Web/src/app/admin/settings.service.ts index dc490beb7..c7bb6cf92 100644 --- a/UI/Web/src/app/admin/settings.service.ts +++ b/UI/Web/src/app/admin/settings.service.ts @@ -78,6 +78,11 @@ export class SettingsService { isValidCronExpression(val: string) { if (val === '' || val === undefined || val === null) return of(false); return this.http.get(this.baseUrl + 'settings/is-valid-cron?cronExpression=' + val, TextResonse).pipe(map(d => d === 'true')); + } + ifValidAuthority(authority: string) { + if (authority === '' || authority === undefined || authority === null) return of(false); + + return this.http.post(this.baseUrl + 'oidc/is-valid-authority', {authority}); } } diff --git a/UI/Web/src/assets/langs/en.json b/UI/Web/src/assets/langs/en.json index c6a510e13..3af3db6ef 100644 --- a/UI/Web/src/assets/langs/en.json +++ b/UI/Web/src/assets/langs/en.json @@ -17,10 +17,11 @@ "settings": { "save": "{{common.save}}", "notice": "Notice", - "restart-required": "Changing OpenID Connect settings requires a restart", + "restart-required": "Changing OpenID Connect Authority requires a restart", "provider": "Provider", "behaviour": "Behaviour", "field-required": "{{name}} is required when {{other}} is set", + "invalidUri": "The provider URL is not valid", "authority": "Authority", "authority-tooltip": "The URL to your OpenID Connect provider",