Fix Plex user import crash on unauthorized token (#5418) (#5419)

* Fix Plex user import crash on unauthorized token (#5418)

When the Plex auth token is invalid or expired, plex.tv returns an
Unauthorized response whose body is an <errors> document. Two problems
turned this into hard crashes that obscured the real cause:

- Api.ExecuteRequest deserialized the error body with the success type's
  serializer, throwing a misleading "There is an error in XML document
  (2, 2)" exception. The deserialization is now wrapped so a non-matching
  error payload logs a warning and returns the default value instead.

- PlexUserImporter.ImportAdmin dereferenced the account result without a
  null check, throwing a NullReferenceException when the account could not
  be retrieved. It now logs a warning and skips the admin import.

Added a test covering the unauthorized (null account) path.

* Evict failed responses from the API cache

Previously a non-success response returned a value (including the default
when error-body deserialisation failed) from inside the cache factory, so
the failure was stored for the whole CacheDuration on any cacheable GET.
That contradicted the "don't cache failed responses" comment and meant a
transient 401 could be served from cache long after it resolved.

ExecuteRequest now reports whether the response was successful, and the
caching path removes the entry when it was not, so only successful
responses remain cached.

Author: tidusjar

src/Ombi.Api/Api.cs

@@ -48,32 +48,45 @@ public Api(ILogger<Api> log, HttpClient client, ICacheService cacheService, IHos
 
                 try
                 {
-                    return await _cacheService.GetOrAddAsync(cacheKey, async () =>
+                    var wasSuccessful = true;
+                    var cachedResult = await _cacheService.GetOrAddAsync(cacheKey, async () =>
                     {
                         Logger.LogDebug($"ApiCache: MISS for {request.HttpMethod.Method} {request.FullUri}");
-                        return await ExecuteRequest<T>(request, cancellationToken);
+                        var (value, requestSucceeded) = await ExecuteRequest<T>(request, cancellationToken);
+                        wasSuccessful = requestSucceeded;
+                        return value;
                     }, DateTimeOffset.UtcNow.Add(request.CacheDuration.Value));
+
+                    // Don't keep unsuccessful responses in the cache, otherwise a transient
+                    // failure (e.g. an expired token returning a 401) would be served from the
+                    // cache for the entire cache duration.
+                    if (!wasSuccessful)
+                    {
+                        _cacheService.Remove(cacheKey);
+                    }
+
+                    return cachedResult;
                 }
                 catch (JsonException ex)
                 {
                     // Deserialization failed - evict cache and retry
                     Logger.LogWarning(ex, $"ApiCache: Deserialization failed for {request.FullUri}, evicting and retrying");
                     _cacheService.Remove(cacheKey);
-                    return await ExecuteRequest<T>(request, cancellationToken);
+                    return (await ExecuteRequest<T>(request, cancellationToken)).value;
                 }
                 catch (Exception ex)
                 {
                     // Cache service failed - log and proceed without cache
                     Logger.LogWarning(ex, $"ApiCache: Cache read failed for {request.FullUri}, proceeding without cache");
-                    return await ExecuteRequest<T>(request, cancellationToken);
+                    return (await ExecuteRequest<T>(request, cancellationToken)).value;
                 }
             }
 
             // No caching - execute request directly
-            return await ExecuteRequest<T>(request, cancellationToken);
+            return (await ExecuteRequest<T>(request, cancellationToken)).value;
         }
 
-        private async Task<T> ExecuteRequest<T>(Request request, CancellationToken cancellationToken)
+        private async Task<(T value, bool wasSuccessful)> ExecuteRequest<T>(Request request, CancellationToken cancellationToken)
         {
             using (var httpRequestMessage = new HttpRequestMessage(request.HttpMethod, request.FullUri))
             {
@@ -117,17 +130,27 @@ private async Task<T> ExecuteRequest<T>(Request request, CancellationToken cance
                 // Only cache successful responses
                 if (!httpResponseMessage.IsSuccessStatusCode)
                 {
-                    // For failed responses, don't cache - just deserialize and return
+                    // For failed responses, don't cache. The body is usually an error payload
+                    // (e.g. Plex returns an <errors> document on a 401) that does not match the
+                    // expected success type, so attempt to deserialize it but fall back to the
+                    // default value rather than throwing a misleading deserialization exception.
                     var errorString = await httpResponseMessage.Content.ReadAsStringAsync(cancellationToken);
                     LogDebugContent(errorString);
-                    if (request.ContentType == ContentType.Json)
+                    try
                     {
-                        request.OnBeforeDeserialization?.Invoke(errorString);
-                        return JsonConvert.DeserializeObject<T>(errorString, Settings);
+                        if (request.ContentType == ContentType.Json)
+                        {
+                            request.OnBeforeDeserialization?.Invoke(errorString);
+                            return (JsonConvert.DeserializeObject<T>(errorString, Settings), false);
+                        }
+
+                        return (DeserializeXml<T>(errorString), false);
                     }
-                    else
+                    catch (Exception ex)
                     {
-                        return DeserializeXml<T>(errorString);
+                        Logger.LogWarning(ex,
+                            $"Could not deserialize the error response from {request.FullUri} (Status Code: {httpResponseMessage.StatusCode}) into {typeof(T).Name}, returning default");
+                        return (default, false);
                     }
                 }
 
@@ -137,13 +160,11 @@ private async Task<T> ExecuteRequest<T>(Request request, CancellationToken cance
                 if (request.ContentType == ContentType.Json)
                 {
                     request.OnBeforeDeserialization?.Invoke(receivedString);
-                    return JsonConvert.DeserializeObject<T>(receivedString, Settings);
-                }
-                else
-                {
-                    // XML
-                    return DeserializeXml<T>(receivedString);
+                    return (JsonConvert.DeserializeObject<T>(receivedString, Settings), true);
                 }
+
+                // XML
+                return (DeserializeXml<T>(receivedString), true);
             }
         }
 

src/Ombi.Schedule.Tests/PlexUserImporterTests.cs

@@ -128,6 +128,20 @@ public async Task Import_Only_Imports_Plex_Admin()
             _mocker.Verify<OmbiUserManager>(x => x.CreateAsync(It.IsAny<OmbiUser>()), Times.Once);
         }
 
+        [Test]
+        public async Task Import_Admin_Handles_Null_Account_When_Unauthorized()
+        {
+            // Simulates Plex returning an Unauthorized response, in which case the account cannot be retrieved.
+            _mocker.Setup<ISettingsService<UserManagementSettings>, Task<UserManagementSettings>>(x => x.GetSettingsAsync())
+                .ReturnsAsync(new UserManagementSettings { ImportPlexAdmin = true, ImportPlexUsers = false });
+            _mocker.Setup<IPlexApi, Task<PlexAccount>>(x => x.GetAccount(It.IsAny<string>())).ReturnsAsync((PlexAccount)null);
+
+            Assert.DoesNotThrowAsync(() => _subject.Execute(null));
+
+            _mocker.Verify<OmbiUserManager>(x => x.CreateAsync(It.IsAny<OmbiUser>()), Times.Never);
+            _mocker.Verify<OmbiUserManager>(x => x.UpdateAsync(It.IsAny<OmbiUser>()), Times.Never);
+        }
+
         [Test]
         public async Task Import_Only_Imports_Plex_Admin_Already_Exists()
         {

src/Ombi.Schedule/Jobs/Plex/PlexUserImporter.cs

@@ -202,7 +202,14 @@ private async Task<List<OmbiUser>> ImportPlexUsers(UserManagementSettings userMa
         private async Task<OmbiUser> ImportAdmin(UserManagementSettings settings, PlexServers server, 
             List<OmbiUser> allUsers)
         {
-            var plexAdmin = (await _api.GetAccount(server.PlexAuthToken)).user;
+            var plexAdmin = (await _api.GetAccount(server.PlexAuthToken))?.user;
+
+            if (plexAdmin == null)
+            {
+                // We could not retrieve the Plex account, most likely the auth token is invalid or expired.
+                _log.LogWarning("Could not import the Plex admin, the Plex account could not be retrieved. The Plex auth token may be invalid or expired.");
+                return null;
+            }
 
             // Check if the admin is already in the DB
             var adminUserFromDb = allUsers.FirstOrDefault(x =>