Fix wording for expiring API keys email notification (#105)

shishirx34 · web-flow · commit 899603f9fb41 · 2017-02-16T12:10:01.000-08:00
* Fix sql query to get apikey v2 expiring api keys

* Fix email wording and aggregate the emails by users

* format mail

* Fix resend email logic

* add app host

* format messaging

* Nit fixes

* Nit fixes

* address feedback

* Fix exception logging

* undo app host changes
diff --git a/.gitignore b/.gitignore
@@ -7,6 +7,7 @@ nuget.exe
 *.suo
 *.user
 *.sln.docstates
+.vs/config/applicationhost.config
 
 # Build results
 [Dd]ebug/
diff --git a/src/Gallery.CredentialExpiration/Gallery.CredentialExpiration.csproj b/src/Gallery.CredentialExpiration/Gallery.CredentialExpiration.csproj
@@ -96,6 +96,7 @@
     <Reference Include="System.Xml" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="LogEvents.cs" />
     <Compile Include="MyJobArgumentNames.cs" />
     <Compile Include="Models\ExpiredCredentialData.cs" />
     <Compile Include="Job.cs" />
diff --git a/src/Gallery.CredentialExpiration/Job.cs b/src/Gallery.CredentialExpiration/Job.cs
@@ -35,8 +35,9 @@ public class Job : JobBase
         private string _mailFrom;
         private SmtpClient _smtpClient;
 
+        private int _allowEmailResendAfterDays = 7;
         private int _warnDaysBeforeExpiration = 10;
-        
+
         private ILogger _logger;
 
         public override bool Init(IDictionary<string, string> jobArgsDictionary)
@@ -64,15 +65,15 @@ public override bool Init(IDictionary<string, string> jobArgsDictionary)
                 var smtpUri = new SmtpUri(new Uri(smtpConnectionString));
                 _smtpClient = CreateSmtpClient(smtpUri);
 
-                var temp = JobConfigurationManager.TryGetIntArgument(jobArgsDictionary, MyJobArgumentNames.WarnDaysBeforeExpiration);
-                if (temp.HasValue)
-                {
-                    _warnDaysBeforeExpiration = temp.Value;
-                }
+                _warnDaysBeforeExpiration = JobConfigurationManager.TryGetIntArgument(jobArgsDictionary, MyJobArgumentNames.WarnDaysBeforeExpiration) 
+                    ?? _warnDaysBeforeExpiration;
+
+                _allowEmailResendAfterDays = JobConfigurationManager.TryGetIntArgument(jobArgsDictionary, MyJobArgumentNames.AllowEmailResendAfterDays)
+                    ?? _allowEmailResendAfterDays;
             }
             catch (Exception exception)
             {
-                _logger.LogCritical("Failed to initialize job! {Exception}", exception);
+                _logger.LogCritical(LogEvents.JobInitFailed, exception, "Failed to initialize job!");
 
                 return false;
             }
@@ -93,8 +94,8 @@ public override async Task<bool> Run()
                     var contactedUsers = JsonConvert.DeserializeObject<Dictionary<string, DateTimeOffset>>(
                         File.ReadAllText(_cursorFile));
 
-                    // Clean older entries (contacted in last _warnDaysBeforeExpiration * 2 days)
-                    var referenceDate = DateTimeOffset.UtcNow.AddDays(-2 * _warnDaysBeforeExpiration);
+                    // Clean older entries (contacted in last _allowEmailResendAfterDays)
+                    var referenceDate = DateTimeOffset.UtcNow.AddDays(-1 * _allowEmailResendAfterDays);
                     foreach (var kvp in contactedUsers.Where(kvp => kvp.Value >= referenceDate))
                     {
                         _contactedUsers.AddOrUpdate(kvp.Key, kvp.Value, (s, offset) => kvp.Value);
@@ -118,24 +119,51 @@ public override async Task<bool> Run()
                         expiredCredentials.Count);
                 }
 
+                // Add default description for non-scoped API keys
+                expiredCredentials
+                    .Where(cred => string.IsNullOrEmpty(cred.Description))
+                    .ToList()
+                    .ForEach(ecd => ecd.Description = Constants.NonScopedApiKeyDescription);
+
+                // Group credentials for each user
+                var userToExpiredCredsMapping = expiredCredentials
+                    .GroupBy(x => x.Username)
+                    .ToDictionary(user => user.Key, value => value.ToList());
+
                 // Handle expiring credentials
                 var jobRunTime = DateTimeOffset.UtcNow;
-                foreach (var expiredCredential in expiredCredentials)
+                foreach (var userCredMapping in userToExpiredCredsMapping)
                 {
-                    if (!_contactedUsers.ContainsKey(expiredCredential.Username))
+                    var username = userCredMapping.Key;
+                    var credentialList = userCredMapping.Value;
+
+                    // Split credentials into two lists: Expired and Expiring to aggregate messages
+                    var expiringCredentialList = credentialList
+                        .Where(x => (x.Expires - jobRunTime).TotalDays > 0)
+                        .ToList();
+                    var expiredCredentialList = credentialList
+                        .Where(x => (x.Expires - jobRunTime).TotalDays <= 0)
+                        .ToList();
+
+                    DateTimeOffset userContactTime;
+                    if (!_contactedUsers.TryGetValue(username, out userContactTime))
                     {
-                        await HandleExpiredCredentialEmail(expiredCredential, jobRunTime);
+                        // send expiring API keys email notification
+                        await HandleExpiredCredentialEmail(username, expiringCredentialList, jobRunTime, expired: false);
+
+                        // send expired API keys email notification
+                        await HandleExpiredCredentialEmail(username, expiredCredentialList, jobRunTime, expired: true);
                     }
                     else
                     {
-                        _logger.LogDebug("Skipping expired credential for user {Username} - already handled today.",
-                            expiredCredential.Username);
+                        _logger.LogDebug("Skipping expired credential for user {Username} - already handled at {JobRuntime}.",
+                            username, userContactTime);
                     }
                 }
             }
             catch (Exception ex)
             {
-                _logger.LogCritical("Job run failed! {Exception}", ex);
+                _logger.LogCritical(LogEvents.JobRunFailed, ex, "Job run failed!");
 
                 return false;
             }
@@ -149,24 +177,37 @@ public override async Task<bool> Run()
             return true;
         }
 
-        private async Task HandleExpiredCredentialEmail(ExpiredCredentialData expiredCredential, DateTimeOffset jobRunTime)
+        private async Task HandleExpiredCredentialEmail(string username, List<ExpiredCredentialData> credentialList, DateTimeOffset jobRunTime, bool expired)
         {
-            _logger.LogInformation("Handling expired credential for user {Username} (expires: {Expires})...", expiredCredential.Username, expiredCredential.Expires);
+            if (credentialList == null || credentialList.Count == 0)
+            {
+                return;
+            }
+
+            _logger.LogInformation("Handling {Expired} credential(s) for user {Username} (Keys: {Descriptions})...",
+                expired ? "expired" : "expiring",
+                username,
+                string.Join(", ", credentialList.Select(x => x.Description).ToList()));
 
             // Build message
-            var mailMessage = new MailMessage(_mailFrom, expiredCredential.EmailAddress);
+            var userEmail = credentialList.FirstOrDefault().EmailAddress;
+            var mailMessage = new MailMessage(_mailFrom, userEmail);
 
+            var apiKeyExpiryMessageList = credentialList
+                .Select(x => BuildApiKeyExpiryMessage(x.Description, x.Expires, jobRunTime))
+                .ToList();
+
+            var apiKeyExpiryMessage = string.Join(Environment.NewLine, apiKeyExpiryMessageList);
             // Build email body
-            var expiresInDays = expiredCredential.Expires.UtcDateTime - DateTime.UtcNow;
-            if (expiresInDays.TotalDays <= 0)
+            if (expired)
             {
                 mailMessage.Subject = string.Format(Strings.ExpiredEmailSubject, _galleryBrand);
-                mailMessage.Body = string.Format(Strings.ExpiredEmailBody, _galleryBrand, _galleryAccountUrl);
+                mailMessage.Body = string.Format(Strings.ExpiredEmailBody, username, _galleryBrand, apiKeyExpiryMessage, _galleryAccountUrl);
             }
             else
             {
                 mailMessage.Subject = string.Format(Strings.ExpiringEmailSubject, _galleryBrand);
-                mailMessage.Body = string.Format(Strings.ExpiringEmailBody, _galleryBrand, _galleryAccountUrl, (int)expiresInDays.TotalDays);
+                mailMessage.Body = string.Format(Strings.ExpiringEmailBody, username, _galleryBrand, apiKeyExpiryMessage, _galleryAccountUrl);
             }
 
             // Send email
@@ -177,22 +218,37 @@ private async Task HandleExpiredCredentialEmail(ExpiredCredentialData expiredCre
                     await _smtpClient.SendMailAsync(mailMessage);
                 }
 
-                _logger.LogInformation("Handled expired credential for user {Username}.", expiredCredential.Username);
+                _logger.LogInformation("Handled {Expired} credential for user {Username}.",
+                    expired ? "expired" : "expiring",
+                    username);
 
-                _contactedUsers.AddOrUpdate(expiredCredential.Username, jobRunTime, (s, offset) => jobRunTime);
+                _contactedUsers.AddOrUpdate(username, jobRunTime, (s, offset) => jobRunTime);
             }
             catch (SmtpFailedRecipientException ex)
             {
-                _logger.LogWarning("Failed to handle expired credential for user {Username} - recipient failed.", expiredCredential.Username, ex);
+                var logMessage = $"Failed to handle credential for user {username} - recipient failed!";
+                _logger.LogWarning(LogEvents.FailedToSendMail, ex, logMessage);
             }
             catch (Exception ex)
             {
-                _logger.LogCritical("Failed to handle expired credential for user {Username}.", expiredCredential.Username, ex);
+                var logMessage = $"Failed to handle credential for user {username}.";
+                _logger.LogCritical(LogEvents.FailedToHandleExpiredCredential, ex, logMessage);
 
                 throw;
             }
         }
 
+        private static string BuildApiKeyExpiryMessage(string description, DateTimeOffset expiry, DateTimeOffset currentTime)
+        {
+            var expiryInDays = (expiry - currentTime).TotalDays;
+            var message = expiryInDays < 0
+                ? string.Format(Strings.ApiKeyExpired, description)
+                : string.Format(Strings.ApiKeyExpiring, description, (int)expiryInDays);
+
+            // \u2022 - Unicode for bullet point.
+            return "\u2022 "+ message + Environment.NewLine;
+        }
+
         private SmtpClient CreateSmtpClient(SmtpUri smtpUri)
         {
             var smtpClient = new SmtpClient(smtpUri.Host, smtpUri.Port)
diff --git a/src/Gallery.CredentialExpiration/LogEvents.cs b/src/Gallery.CredentialExpiration/LogEvents.cs
@@ -0,0 +1,15 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Logging;
+
+namespace Gallery.CredentialExpiration
+{
+    public class LogEvents
+    {
+        public static EventId FailedToHandleExpiredCredential = new EventId(600, "Failed to handle expired credential");
+        public static EventId FailedToSendMail = new EventId(601, "Failed to deliver email");
+        public static EventId JobRunFailed = new EventId(650, "Job run failed");
+        public static EventId JobInitFailed = new EventId(651, "Job initialization failed");
+    }
+}
diff --git a/src/Gallery.CredentialExpiration/Models/ExpiredCredentialData.cs b/src/Gallery.CredentialExpiration/Models/ExpiredCredentialData.cs
@@ -5,11 +5,17 @@
 
 namespace Gallery.CredentialExpiration.Models
 {
+    public static class Constants
+    {
+        public const string NonScopedApiKeyDescription = "Full access API key";
+    }
+
     public class ExpiredCredentialData
     {
         public string Type { get; set; }
         public string Username { get; set; }
         public string EmailAddress { get; set; }
+        public string Description { get; set; }
         public DateTimeOffset Created { get; set; }
         public DateTimeOffset Expires { get; set; }
     }
diff --git a/src/Gallery.CredentialExpiration/MyJobArgumentNames.cs b/src/Gallery.CredentialExpiration/MyJobArgumentNames.cs
@@ -8,5 +8,6 @@ public class MyJobArgumentNames
         public const string GalleryBrand = "GalleryBrand";
         public const string GalleryAccountUrl = "GalleryAccountUrl";
         public const string WarnDaysBeforeExpiration = "WarnDaysBeforeExpiration";
+        public const string AllowEmailResendAfterDays = "AllowEmailResendAfterDays";
     }
 }
diff --git a/src/Gallery.CredentialExpiration/Strings.Designer.cs b/src/Gallery.CredentialExpiration/Strings.Designer.cs
diff --git a/src/Gallery.CredentialExpiration/Strings.resx b/src/Gallery.CredentialExpiration/Strings.resx
@@ -118,36 +118,40 @@
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
   <data name="ExpiredEmailBody" xml:space="preserve">
-    <value>Hi,
+    <value>Hi {0},
 
-We wanted to inform you that your API key on {0} has expired.
+We wanted to inform you that the following API key(s) on {1} have expired.
 
-Visit {1} to generate a new API key so that you can continue pushing packages.
+{2}
+
+Visit {3} to generate a new API key(s) so that you can continue pushing packages.
 
 Best regards,
 
-{0}</value>
+{1}</value>
   </data>
   <data name="ExpiringEmailBody" xml:space="preserve">
-    <value>Hi,
+    <value>Hi {0},
+
+We wanted to inform you that the following API key(s) on {1} will expire soon: 
 
-We wanted to inform you that your API key on {0} will expire in {2} days.
+{2}
 
-Visit {1} to generate a new API key so that you can continue pushing packages.
+Visit {3} to generate a new API key(s) so that you can continue pushing packages using them.
 
 Best regards,
 
-{0}</value>
+{1}</value>
   </data>
   <data name="GetExpiredCredentialsQuery" xml:space="preserve">
-    <value>SELECT cr.[Type], cr.[Created], cr.[Expires], u.[EmailAddress], u.[Username]
+    <value>SELECT cr.[Type], cr.[Created], cr.[Expires], cr.[Description], u.[EmailAddress], u.[Username]
 FROM [Credentials] AS cr
 INNER JOIN Users AS u ON u.[Key] = cr.[UserKey]
 WHERE u.[EmailAllowed] = 1
   AND u.[EmailAddress] &lt;&gt; ''
   AND cr.[Expires] &lt;= DATEADD(day,{0},GETUTCDATE())
-  AND cr.[Expires] &gt; DATEADD(day,-1,GETUTCDATE())
-  AND cr.[Type] = 'apikey.v1'
+  AND cr.[Expires] &gt; DATEADD(day,-1 * {0},GETUTCDATE())
+  AND (cr.[Type] = 'apikey.v1' or cr.[Type] = 'apikey.v2')
 ORDER BY u.[Username]</value>
   </data>
   <data name="ExpiredEmailSubject" xml:space="preserve">
@@ -156,4 +160,10 @@ ORDER BY u.[Username]</value>
   <data name="ExpiringEmailSubject" xml:space="preserve">
     <value>[{0}] Your API key is about to expire</value>
   </data>
+  <data name="ApiKeyExpiring" xml:space="preserve">
+    <value>{0} - expires in {1} day(s).</value>
+  </data>
+  <data name="ApiKeyExpired" xml:space="preserve">
+    <value>{0} - has expired.</value>
+  </data>
 </root>

Original file line number	Diff line number	Diff line change
`@@ -5,11 +5,17 @@`
`5`	`5`
`6`	`6`	`namespace Gallery.CredentialExpiration.Models`
`7`	`7`	`{`
	`8`	`+ public static class Constants`
	`9`	`+ {`
	`10`	`+ public const string NonScopedApiKeyDescription = "Full access API key";`
	`11`	`+ }`
	`12`	`+`
`8`	`13`	`public class ExpiredCredentialData`
`9`	`14`	`{`
`10`	`15`	`public string Type { get; set; }`
`11`	`16`	`public string Username { get; set; }`
`12`	`17`	`public string EmailAddress { get; set; }`
	`18`	`+ public string Description { get; set; }`
`13`	`19`	`public DateTimeOffset Created { get; set; }`
`14`	`20`	`public DateTimeOffset Expires { get; set; }`
`15`	`21`	`}`
Original file line number	Diff line number	Diff line change
`@@ -8,5 +8,6 @@ public class MyJobArgumentNames`
`8`	`8`	`public const string GalleryBrand = "GalleryBrand";`
`9`	`9`	`public const string GalleryAccountUrl = "GalleryAccountUrl";`
`10`	`10`	`public const string WarnDaysBeforeExpiration = "WarnDaysBeforeExpiration";`
	`11`	`+ public const string AllowEmailResendAfterDays = "AllowEmailResendAfterDays";`
`11`	`12`	`}`
`12`	`13`	`}`