Fix challenge deletions (#17)

* Fixed null reference triggered by 'finally' clause in LetsEncryptRenewalService
Implemented deletion in FileChallengePersistenceStrategy
Improved deletion in MemoryChallengePersistenceStrategy

* Improved readability of DistributedCacheChallengePersistenceStrategy by implementing IChallengePersistenceStrategy instead of deriving from CustomChallengePersistenceStrategy
Improved deletion in DistributedCacheChallengePersistenceStrategy

Author: killswtch

src/FluffySpoon.AspNet.LetsEncrypt.Redis/DistributedCacheChallengePersistenceStrategy.cs

@@ -11,39 +11,60 @@
 
 namespace FluffySpoon.AspNet.LetsEncrypt.Redis
 {
-	public class DistributedCacheChallengePersistenceStrategy : CustomChallengePersistenceStrategy
+	public class DistributedCacheChallengePersistenceStrategy : IChallengePersistenceStrategy
 	{
 		private const string Key = "FluffySpoon_Challenges";
 
+		private readonly ILogger<DistributedCacheChallengePersistenceStrategy> _logger;
+		private readonly IDistributedCache _cache;
+		private readonly TimeSpan _expiry;
+
 		public DistributedCacheChallengePersistenceStrategy(
 			ILogger<DistributedCacheChallengePersistenceStrategy> logger,
 			IDistributedCache cache, 
-			TimeSpan expiry) : base(
-				new ChallengeType[] { ChallengeType.Http01 },
-				async (challenges) =>
-				{
-					var json = challenges == null ? null : JsonConvert.SerializeObject(challenges.ToArray());
-					logger.LogDebug("Persisting challenges {0}", json);
-
-					var bytes = json == null ? null : Encoding.UTF8.GetBytes(json);
-
-					await cache.SetAsync(Key, bytes, new DistributedCacheEntryOptions()
-					{
-						AbsoluteExpirationRelativeToNow = expiry
-					});
-				},
-				async () => {
-					var bytes = await cache.GetAsync(Key);
-					var json = Encoding.UTF8.GetString(bytes);
-					var challenges = JsonConvert.DeserializeObject<IEnumerable<ChallengeDto>>(json);
-
-					return challenges;
-				},
-				async (challenges) =>
-				{
-					await cache.RemoveAsync(Key);
-				})
+			TimeSpan expiry)
 		{
+			_logger = logger;
+			_cache = cache;
+			_expiry = expiry;
+		}
+
+		public bool CanHandleChallengeType(ChallengeType challengeType)
+		{
+			return challengeType == ChallengeType.Http01;
+		}
+
+		public async Task DeleteAsync(IEnumerable<ChallengeDto> challenges)
+		{
+			var persistedChallenges = await RetrieveAsync();
+			var challengesToPersist = persistedChallenges
+				.Where(x =>
+					!challenges.Any(y => y.Token == x.Token))
+				.ToList();
+
+			await PersistAsync(challengesToPersist);
+		}
+
+		public async Task PersistAsync(IEnumerable<ChallengeDto> challenges)
+		{
+			var json = challenges == null ? null : JsonConvert.SerializeObject(challenges.ToArray());
+			_logger.LogDebug("Persisting challenges {0}", json);
+
+			var bytes = json == null ? null : Encoding.UTF8.GetBytes(json);
+
+			await _cache.SetAsync(Key, bytes, new DistributedCacheEntryOptions()
+			{
+				AbsoluteExpirationRelativeToNow = _expiry
+			});
+		}
+
+		public async Task<IEnumerable<ChallengeDto>> RetrieveAsync()
+		{
+			var bytes = await _cache.GetAsync(Key);
+			var json = Encoding.UTF8.GetString(bytes);
+			var challenges = JsonConvert.DeserializeObject<IEnumerable<ChallengeDto>>(json);
+
+			return challenges;
 		}
 	}
 }

src/FluffySpoon.AspNet.LetsEncrypt/LetsEncryptRenewalService.cs

@@ -278,7 +278,7 @@ private async Task ValidateOrderAsync(string[] domains, IOrderContext order)
 			}
 			finally
 			{
-				await _persistenceService.PersistChallengesAsync(null);
+				await _persistenceService.DeleteChallengesAsync(challengeDtos);
 			}
 		}
 

src/FluffySpoon.AspNet.LetsEncrypt/Persistence/FileChallengePersistenceStrategy.cs

@@ -18,9 +18,15 @@ public FileChallengePersistenceStrategy(string relativeFilePath)
 			_relativeFilePath = relativeFilePath;
 		}
 
-		public Task DeleteAsync(IEnumerable<ChallengeDto> challenges)
+		public async Task DeleteAsync(IEnumerable<ChallengeDto> challenges)
 		{
-			throw new System.NotImplementedException();
+			var persistedChallenges = await RetrieveAsync();
+			var challengesToPersist = persistedChallenges
+				.Where(x => 
+					!challenges.Any(y => y.Token == x.Token))
+				.ToList();
+
+			await PersistAsync(challengesToPersist);
 		}
 
 		public bool CanHandleChallengeType(ChallengeType challengeType)
@@ -37,7 +43,7 @@ public Task PersistAsync(IEnumerable<ChallengeDto> challenges)
 			lock (typeof(FileChallengePersistenceStrategy))
 			{
 				File.WriteAllBytes(
-					GetCertificatePath(),
+					GetChallengesStorePath(),
 					bytes);
 			}
 
@@ -48,18 +54,18 @@ public Task<IEnumerable<ChallengeDto>> RetrieveAsync()
 		{
 			lock (typeof(FileChallengePersistenceStrategy))
 			{
-				if (!File.Exists(GetCertificatePath()))
+				if (!File.Exists(GetChallengesStorePath()))
 					return Task.FromResult<IEnumerable<ChallengeDto>>(new List<ChallengeDto>());
 
-				var bytes = File.ReadAllBytes(GetCertificatePath());
+				var bytes = File.ReadAllBytes(GetChallengesStorePath());
 				var json = Encoding.UTF8.GetString(bytes);
 				var challenges = JsonConvert.DeserializeObject<IEnumerable<ChallengeDto>>(json);
 
 				return Task.FromResult(challenges);
 			}
 		}
 
-		private string GetCertificatePath()
+		private string GetChallengesStorePath()
 		{
 			return _relativeFilePath + "_Challenges";
 		}

src/FluffySpoon.AspNet.LetsEncrypt/Persistence/MemoryChallengePersistenceStrategy.cs

@@ -1,5 +1,6 @@
 using FluffySpoon.AspNet.LetsEncrypt.Persistence.Models;
 using System.Collections.Generic;
+using System.Linq;
 using System.Threading.Tasks;
 
 namespace FluffySpoon.AspNet.LetsEncrypt.Persistence
@@ -15,7 +16,10 @@ public MemoryChallengePersistenceStrategy()
 
 		public Task DeleteAsync(IEnumerable<ChallengeDto> challenges)
 		{
-			_challenges = new List<ChallengeDto>();
+			_challenges = _challenges
+				.Where(x =>
+					!challenges.Any(y => y.Token == x.Token))
+				.ToList();
 
 			return Task.CompletedTask;
 		}