From 881727bd21d42dee043d91ba7ab06b9427fbbaef Mon Sep 17 00:00:00 2001 From: Fesaa <77553571+Fesaa@users.noreply.github.com> Date: Tue, 5 Aug 2025 15:38:46 +0200 Subject: [PATCH] A few bug fixes (#3980) --- .github/copilot-instructions.md | 19 ++++ API.Tests/Services/OidcServiceTests.cs | 12 ++- API/API.csproj | 1 + API/Helpers/Builders/AppUserBuilder.cs | 7 ++ API/Services/OidcService.cs | 87 +++++++++++-------- API/Services/Plus/ExternalMetadataService.cs | 9 +- API/Services/TaskScheduler.cs | 36 +++++++- API/Services/Tasks/Metadata/CoverDbService.cs | 4 +- .../app/_pipes/cbl-conflict-reason.pipe.ts | 4 +- .../user-scrobble-history.component.html | 2 +- .../user-scrobble-history.component.ts | 3 +- .../manage-matched-metadata.component.html | 2 +- .../manage-matched-metadata.component.ts | 3 +- .../manage-users/manage-users.component.html | 4 +- .../all-filters/all-filters.component.html | 2 +- .../app/all-filters/all-filters.component.ts | 3 +- UI/Web/src/app/app.component.html | 17 ++-- .../infinite-scroller.component.ts | 39 +++++++-- .../manga-reader/manga-reader.component.ts | 2 + .../reading-list-item.component.html | 2 +- .../reading-list-item.component.ts | 3 +- .../library-settings-modal.component.ts | 2 +- .../manage-reading-profiles.component.ts | 80 +++++++++-------- .../scrobbling-holds.component.html | 2 +- .../user-holds/scrobbling-holds.component.ts | 2 + UI/Web/src/assets/langs/en.json | 8 +- 26 files changed, 245 insertions(+), 110 deletions(-) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..dd64e8100 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,19 @@ +## Coding Guidelines +- Anytime you don't use {} on an if statement, it must be on one line and MUST be a jump operation (return/continue/break). All other times, you must use {} and be on 2 lines. +- Use var whenever possible +- return statements should generally have a newline above them +Examples: +```csharp +# Case when okay - simple logic flow +var a = 2 + 3; +return a; + +# Case when needs newline - complex logic is grouped together +var a = b + c; +_imageService.Resize(...); + +return; +``` +- Operation (+,-,*, etc) should always have spaces around it; I.e. `a + b` not `a+b`. +- Comma's `,` should always be followed by a space +- When setting href directectly (not using Angulars routing) it should always be prefixed with baseURL diff --git a/API.Tests/Services/OidcServiceTests.cs b/API.Tests/Services/OidcServiceTests.cs index 1619c1956..d7dfa2cbc 100644 --- a/API.Tests/Services/OidcServiceTests.cs +++ b/API.Tests/Services/OidcServiceTests.cs @@ -28,7 +28,9 @@ public class OidcServiceTests: AbstractDbTest await ResetDb(); var (oidcService, _, _, userManager) = await Setup(); - var user = new AppUserBuilder("holo", "holo@localhost").Build(); + var user = new AppUserBuilder("holo", "holo@localhost") + .WithIdentityProvider(IdentityProvider.OpenIdConnect) + .Build(); var res = await userManager.CreateAsync(user); Assert.Empty(res.Errors); Assert.True(res.Succeeded); @@ -426,7 +428,9 @@ public class OidcServiceTests: AbstractDbTest Assert.NotNull(userFromDb); Assert.NotEqual(AgeRating.Teen, userFromDb.AgeRestriction); - var newUser = new AppUserBuilder("NotAnAdmin", "NotAnAdmin@localhost").Build(); + var newUser = new AppUserBuilder("NotAnAdmin", "NotAnAdmin@localhost") + .WithIdentityProvider(IdentityProvider.OpenIdConnect) + .Build(); var res = await userManager.CreateAsync(newUser); Assert.Empty(res.Errors); Assert.True(res.Succeeded); @@ -515,7 +519,9 @@ public class OidcServiceTests: AbstractDbTest var defaultAdmin = new AppUserBuilder("defaultAdmin", "defaultAdmin@localhost") .WithRole(PolicyConstants.AdminRole) .Build(); - var user = new AppUserBuilder("amelia", "amelia@localhost").Build(); + var user = new AppUserBuilder("amelia", "amelia@localhost") + .WithIdentityProvider(IdentityProvider.OpenIdConnect) + .Build(); var roleStore = new RoleStore< AppRole, diff --git a/API/API.csproj b/API/API.csproj index fa6b40bca..c505640f0 100644 --- a/API/API.csproj +++ b/API/API.csproj @@ -80,6 +80,7 @@ + diff --git a/API/Helpers/Builders/AppUserBuilder.cs b/API/Helpers/Builders/AppUserBuilder.cs index 7ffac355e..4fdea81c4 100644 --- a/API/Helpers/Builders/AppUserBuilder.cs +++ b/API/Helpers/Builders/AppUserBuilder.cs @@ -2,6 +2,7 @@ using System.Linq; using API.Data; using API.Entities; +using API.Entities.Enums; using Kavita.Common; namespace API.Helpers.Builders; @@ -68,4 +69,10 @@ public class AppUserBuilder : IEntityBuilder _appUser.UserRoles.Add(new AppUserRole() {Role = new AppRole() {Name = role}}); return this; } + + public AppUserBuilder WithIdentityProvider(IdentityProvider identityProvider) + { + _appUser.IdentityProvider = identityProvider; + return this; + } } diff --git a/API/Services/OidcService.cs b/API/Services/OidcService.cs index e933018b2..67ca14d8c 100644 --- a/API/Services/OidcService.cs +++ b/API/Services/OidcService.cs @@ -68,10 +68,30 @@ public class OidcService(ILogger logger, UserManager userM /// The name of the Auth Cookie set by .NET public const string CookieName = ".AspNetCore.Cookies"; - private OpenIdConnectConfiguration? _discoveryDocument; + /// + /// The ConfigurationManager will refresh the configuration periodically to ensure the data stays up to date + /// We can store the same one indefinitely as the authority does not change unless Kavita is restarted + /// + /// The ConfigurationManager has its own lock, it loads data thread safe + private static readonly ConfigurationManager OidcConfigurationManager; private static readonly ConcurrentDictionary RefreshInProgress = new(); private static readonly ConcurrentDictionary LastFailedRefresh = new(); +#pragma warning disable S3963 + static OidcService() + { + var authority = Configuration.OidcSettings.Authority; + var hasTrailingSlash = authority.EndsWith('/'); + var url = authority + (hasTrailingSlash ? string.Empty : "/") + ".well-known/openid-configuration"; + + OidcConfigurationManager = new ConfigurationManager( + url, + new OpenIdConnectConfigurationRetriever(), + new HttpDocumentRetriever { RequireHttps = url.StartsWith("https") } + ); + } +#pragma warning restore S3963 + public async Task LoginOrCreate(HttpRequest request, ClaimsPrincipal principal) { var settings = (await unitOfWork.SettingsRepository.GetSettingsDtoAsync()).OidcConfig; @@ -83,7 +103,12 @@ public class OidcService(ILogger logger, UserManager userM } var user = await unitOfWork.UserRepository.GetByOidcId(oidcId, AppUserIncludes.UserPreferences); - if (user != null) return user; + if (user != null) + { + await SyncUserSettings(request, settings, principal, user); + + return user; + } var email = principal.FindFirstValue(ClaimTypes.Email); if (string.IsNullOrEmpty(email)) @@ -111,6 +136,8 @@ public class OidcService(ILogger logger, UserManager userM user.OidcId = oidcId; await unitOfWork.CommitAsync(); + await SyncUserSettings(request, settings, principal, user); + return user; } @@ -160,17 +187,17 @@ public class OidcService(ILogger logger, UserManager userM try { user.UpdateLastActive(); + + if (unitOfWork.HasChanges()) + { + await unitOfWork.CommitAsync(); + } } catch (Exception ex) { logger.LogError(ex, "Failed to update last active for {UserName}", user.UserName); } - if (unitOfWork.HasChanges()) - { - await unitOfWork.CommitAsync(); - } - if (string.IsNullOrEmpty(tokenResponse.IdToken)) { logger.LogTrace("The OIDC provider did not return an id token in the refresh response, continuous sync is not supported"); @@ -341,9 +368,17 @@ public class OidcService(ILogger logger, UserManager userM await unitOfWork.CommitAsync(); } + /// + /// Syncs the given user to the principal found in the id token + /// + /// + /// + /// + /// + /// If syncing fails private async Task SyncUserSettings(CookieValidatePrincipalContext ctx, OidcConfigDto settings, string idToken, AppUser user) { - if (!settings.SyncUserSettings) return; + if (!settings.SyncUserSettings || user.IdentityProvider != IdentityProvider.OpenIdConnect) return; try { @@ -366,7 +401,7 @@ public class OidcService(ILogger logger, UserManager userM /// public async Task SyncUserSettings(HttpRequest request, OidcConfigDto settings, ClaimsPrincipal claimsPrincipal, AppUser user) { - if (!settings.SyncUserSettings) return; + if (!settings.SyncUserSettings || user.IdentityProvider != IdentityProvider.OpenIdConnect) return; // Never sync the default user var defaultAdminUser = await unitOfWork.UserRepository.GetDefaultAdminUser(); @@ -565,10 +600,10 @@ public class OidcService(ILogger logger, UserManager userM /// /// /// - private async Task RefreshTokenAsync(OidcConfigDto dto, string refreshToken) + private static async Task RefreshTokenAsync(OidcConfigDto dto, string refreshToken) { - _discoveryDocument ??= await LoadOidcConfiguration(dto.Authority); + var discoveryDocument = await OidcConfigurationManager.GetConfigurationAsync(); var msg = new { @@ -578,7 +613,7 @@ public class OidcService(ILogger logger, UserManager userM client_secret = dto.Secret, }; - var json = await _discoveryDocument.TokenEndpoint + var json = await discoveryDocument.TokenEndpoint .AllowAnyHttpStatus() .PostUrlEncodedAsync(msg) .ReceiveString(); @@ -593,14 +628,15 @@ public class OidcService(ILogger logger, UserManager userM /// /// /// - private async Task ParseIdToken(OidcConfigDto dto, string idToken) + private static async Task ParseIdToken(OidcConfigDto dto, string idToken) { - _discoveryDocument ??= await LoadOidcConfiguration(dto.Authority); + var discoveryDocument = await OidcConfigurationManager.GetConfigurationAsync(); + var tokenValidationParameters = new TokenValidationParameters { - ValidIssuer = _discoveryDocument.Issuer, + ValidIssuer = discoveryDocument.Issuer, ValidAudience = dto.ClientId, - IssuerSigningKeys = _discoveryDocument.SigningKeys, + IssuerSigningKeys = discoveryDocument.SigningKeys, ValidateIssuerSigningKey = true, }; @@ -610,25 +646,6 @@ public class OidcService(ILogger logger, UserManager userM return principal; } - /// - /// Loads OpenIdConnectConfiguration, includes - /// - /// - /// - private static async Task LoadOidcConfiguration(string authority) - { - var hasTrailingSlash = authority.EndsWith('/'); - var url = authority + (hasTrailingSlash ? string.Empty : "/") + ".well-known/openid-configuration"; - - var manager = new ConfigurationManager( - url, - new OpenIdConnectConfigurationRetriever(), - new HttpDocumentRetriever { RequireHttps = url.StartsWith("https") } - ); - - return await manager.GetConfigurationAsync(); - } - /// /// Return a list of claims in the same way the NativeJWT token would map them. /// Optionally include original claims if the claims are needed later in the pipeline diff --git a/API/Services/Plus/ExternalMetadataService.cs b/API/Services/Plus/ExternalMetadataService.cs index d8c197532..6d77e1db7 100644 --- a/API/Services/Plus/ExternalMetadataService.cs +++ b/API/Services/Plus/ExternalMetadataService.cs @@ -1806,10 +1806,15 @@ public class ExternalMetadataService : IExternalMetadataService { // Find highest age rating from mappings mappings ??= new Dictionary(); - mappings = mappings.ToDictionary(k => k.Key.ToNormalized(), k => k.Value); + mappings = mappings + .GroupBy(m => m.Key.ToNormalized()) + .ToDictionary( + g => g.Key, + g => g.Max(m => m.Value) + ); return values - .Select(v => mappings.TryGetValue(v.ToNormalized(), out var mapping) ? mapping : AgeRating.Unknown) + .Select(v => mappings.GetValueOrDefault(v.ToNormalized(), AgeRating.Unknown)) .DefaultIfEmpty(AgeRating.Unknown) .Max(); } diff --git a/API/Services/TaskScheduler.cs b/API/Services/TaskScheduler.cs index 575f89b3b..fa44a5256 100644 --- a/API/Services/TaskScheduler.cs +++ b/API/Services/TaskScheduler.cs @@ -16,6 +16,8 @@ using API.SignalR; using Hangfire; using Kavita.Common.Helpers; using Microsoft.Extensions.Logging; +using Polly; +using Polly.Retry; namespace API.Services; @@ -84,6 +86,8 @@ public class TaskScheduler : ITaskScheduler public const string KavitaPlusStackSyncId = "kavita+-stack-sync"; public const string KavitaPlusWantToReadSyncId = "kavita+-want-to-read-sync"; + private const int BaseRetryDelay = 60; // 1-minute + public static readonly ImmutableArray ScanTasks = ["ScannerService", "ScanLibrary", "ScanLibraries", "ScanFolder", "ScanSeries"]; private static readonly ImmutableArray NonCronOptions = ["disabled", "daily", "weekly"]; @@ -94,6 +98,10 @@ public class TaskScheduler : ITaskScheduler { TimeZone = TimeZoneInfo.Local }; + /// + /// Retry policy, with 3 tries, and a 1-minute base delay + /// + private readonly AsyncRetryPolicy _defaultRetryPolicy; public TaskScheduler(ICacheService cacheService, ILogger logger, IScannerService scannerService, @@ -123,6 +131,24 @@ public class TaskScheduler : ITaskScheduler _smartCollectionSyncService = smartCollectionSyncService; _wantToReadSyncService = wantToReadSyncService; _eventHub = eventHub; + + _defaultRetryPolicy = Policy + .Handle() + .WaitAndRetryAsync( + retryCount: 3, + sleepDurationProvider: attempt => + { + var delay = BaseRetryDelay * 2 * attempt; + var jitter = Random.Shared.Next(0, delay / 4); + + return TimeSpan.FromSeconds(delay + jitter); + }, + onRetry: (ex, timeSpan, attempt) => + { + _logger.LogWarning(ex, "Attempt {Attempt} failed, retrying in {Delay}ms", + attempt, timeSpan.TotalMilliseconds); + } + ); } public async Task ScheduleTasks() @@ -487,9 +513,13 @@ public class TaskScheduler : ITaskScheduler // ReSharper disable once MemberCanBePrivate.Global public async Task CheckForUpdate() { - var update = await _versionUpdaterService.CheckForUpdate(); - if (update == null) return; - await _versionUpdaterService.PushUpdate(update); + await _defaultRetryPolicy.ExecuteAsync(async () => + { + var update = await _versionUpdaterService.CheckForUpdate(); + if (update == null) return; + + await _versionUpdaterService.PushUpdate(update); + }); } public async Task SyncThemes() diff --git a/API/Services/Tasks/Metadata/CoverDbService.cs b/API/Services/Tasks/Metadata/CoverDbService.cs index 080ef2f8b..560161d97 100644 --- a/API/Services/Tasks/Metadata/CoverDbService.cs +++ b/API/Services/Tasks/Metadata/CoverDbService.cs @@ -370,7 +370,7 @@ public class CoverDbService : ICoverDbService private async Task FallbackToKavitaReaderFavicon(string baseUrl) { - const string urlsFileName = "publishers.txt"; + const string urlsFileName = "urls.txt"; var correctSizeLink = string.Empty; var allOverrides = await GetCachedData(urlsFileName) ?? await $"{NewHost}favicons/{urlsFileName}".GetStringAsync(); @@ -384,6 +384,7 @@ public class CoverDbService : ICoverDbService var cleanedBaseUrl = baseUrl.Replace("https://", string.Empty); var externalFile = allOverrides .Split("\n") + .Select(url => url.Trim('\n', '\r')) // Ensure windows line terminators don't mess anything up .FirstOrDefault(url => cleanedBaseUrl.Equals(url.Replace(".png", string.Empty)) || cleanedBaseUrl.Replace("www.", string.Empty).Equals(url.Replace(".png", string.Empty) @@ -410,6 +411,7 @@ public class CoverDbService : ICoverDbService var externalFile = allOverrides .Split("\n") + .Select(url => url.Trim('\n', '\r')) // Ensure windows line terminators don't mess anything up .Select(publisherLine => { var tokens = publisherLine.Split("|"); diff --git a/UI/Web/src/app/_pipes/cbl-conflict-reason.pipe.ts b/UI/Web/src/app/_pipes/cbl-conflict-reason.pipe.ts index 6f5463cf2..1e9229ac7 100644 --- a/UI/Web/src/app/_pipes/cbl-conflict-reason.pipe.ts +++ b/UI/Web/src/app/_pipes/cbl-conflict-reason.pipe.ts @@ -2,6 +2,7 @@ import {inject, Pipe, PipeTransform} from '@angular/core'; import { CblBookResult } from 'src/app/_models/reading-list/cbl/cbl-book-result'; import { CblImportReason } from 'src/app/_models/reading-list/cbl/cbl-import-reason.enum'; import {TranslocoService} from "@jsverse/transloco"; +import {APP_BASE_HREF} from "@angular/common"; const failIcon = ''; const successIcon = ''; @@ -13,6 +14,7 @@ const successIcon = '