Merge pull request #15 from umbraco/bugfix/ensure-secret-encryptor-is-optional

Ensure that when optional secret key for token encryption is not provided token is stored without encryption.

Author: acoumb

README.md

@@ -97,6 +97,8 @@ Not all values are required for all services.  Those that are required are marke
 
 Provides an optional key used to encrypt and decrypt tokens when they are saved and retrieved from storage respectively.
 
+If not provided, the value stored in configuration at `Umbraco:CMS:Global:Id` will be used.
+
 ##### Services
 
 The collection of services available for authorization and usage.
@@ -297,15 +299,15 @@ Responsible for creating a dictionary of parameters provided in the request to r
 
 Responsible for encrypting and decrypting stored tokens (or other values).
 
-It has two implementations:
+It has three implementations:
 
 - `AesSecretEncryptor` - default implementation that is using a standard `AES` cryptographic algorithm for encrypting/decrypting values based on the provided `TokenEncryptionKey`.
+- `NoopSecretEncryptor` - provides no encryption saving the provided token as is.
 - `DataProtectionSecretEncryptor` - additional implementation that uses the `IDataProtectionProvider` interface for providing data protection services.
 
-Switching the encryption engine to `DataProtectionSecretEncryptor` can be done in code, adding these two lines:
+Switching the encryption engine to for example `DataProtectionSecretEncryptor` can be done in code, adding these two lines:
 
 ```
-builder.Services.AddDataProtection();
 builder.Services.AddUnique<ISecretEncryptor, DataProtectionSecretEncrytor>();
 ```
 

src/Umbraco.AuthorizedServices/AuthorizedServicesComposer.cs

@@ -1,11 +1,14 @@
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using Umbraco.AuthorizedServices.Configuration;
 using Umbraco.AuthorizedServices.Manifests;
 using Umbraco.AuthorizedServices.Migrations;
 using Umbraco.AuthorizedServices.Services;
 using Umbraco.AuthorizedServices.Services.Implement;
 using Umbraco.Cms.Core.Composing;
+using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Notifications;
 using Umbraco.Extensions;
@@ -16,7 +19,8 @@ internal class AuthorizedServicesComposer : IComposer
 {
     public void Compose(IUmbracoBuilder builder)
     {
-        IConfigurationSection configSection = builder.Config.GetSection("Umbraco:AuthorizedServices");
+        const string ConfigurationRoot = "Umbraco:AuthorizedServices";
+        IConfigurationSection configSection = builder.Config.GetSection(ConfigurationRoot);
         builder.Services.Configure<AuthorizedServiceSettings>(configSection);
 
         // manifest filter
@@ -38,9 +42,24 @@ public void Compose(IUmbracoBuilder builder)
 
         builder.Services.AddUnique<ISecretEncryptor>(factory =>
             {
+                ILogger<AuthorizedServicesComposer> logger = factory.GetRequiredService<ILogger<AuthorizedServicesComposer>>();
                 var tokenEncryptionKey = configSection.GetValue<string>(nameof(AuthorizedServiceSettings.TokenEncryptionKey));
 
-                return new AesSecretEncryptor(tokenEncryptionKey ?? string.Empty);
+                if (string.IsNullOrWhiteSpace(tokenEncryptionKey))
+                {
+                    logger.LogWarning($"No encryption key was found in configuration at {ConfigurationRoot}:{nameof(AuthorizedServiceSettings.TokenEncryptionKey)}. Falling back to using the Umbraco:CMS:{nameof(GlobalSettings)}:{nameof(GlobalSettings.Id)} value.");
+
+                    IOptions<GlobalSettings> globalSettings = factory.GetRequiredService<IOptions<GlobalSettings>>();
+                    tokenEncryptionKey = globalSettings.Value.Id;
+                }
+
+                if (string.IsNullOrWhiteSpace(tokenEncryptionKey))
+                {
+                    logger.LogWarning($"Could not fallback back to using the Umbraco:CMS:{nameof(GlobalSettings)}:{nameof(GlobalSettings.Id)} value as it has not been completed. Access tokens will not be encrypted when stored in the local database.");
+                    return new NoopSecretEncryptor();
+                }
+
+                return new AesSecretEncryptor(tokenEncryptionKey);
             });
 
         builder.Services.AddUnique<ITokenFactory, TokenFactory>();

src/Umbraco.AuthorizedServices/ClientApp/src/backoffice/AuthorizedServices/edit.controller.ts

@@ -34,6 +34,9 @@ function AuthorizedServiceEditController(this: any, $routeParams, $location, aut
     authorizedServiceResource.sendSampleRequest(serviceAlias, vm.sampleRequest)
       .then(function (response) {
         vm.sampleRequestResponse = "Request: " + vm.sampleRequest + "\r\nResponse: " + JSON.stringify(response.data, null, 2);
+      })
+      .catch(function (e) {
+        notificationsService.error("Authorized Services", "The sample request did not complete: " + e.data.ExceptionMessage);
       });
   };
 

src/Umbraco.AuthorizedServices/Services/Implement/AesSecretEncryptor.cs

@@ -62,7 +62,17 @@ public string Decrypt(string value)
             throw new ArgumentException("The value to be decrypted cannot be empty.", nameof(value));
         }
 
-        var combined = Convert.FromBase64String(value);
+        byte[] combined;
+        try
+        {
+            combined = Convert.FromBase64String(value);
+        }
+        catch (FormatException)
+        {
+            // Can't convert from Base64 string. Probably means we've previously stored the token unencrypted, so we should return that.
+            return value;
+        }
+
         var buffer = new byte[combined.Length];
         var hash = SHA512.Create();
         var aesKey = new byte[24];

src/Umbraco.AuthorizedServices/Services/Implement/DataProtectionSecretEncrytor.cs

@@ -2,13 +2,13 @@
 
 namespace Umbraco.AuthorizedServices.Services.Implement;
 
-internal sealed class DataProtectionSecretEncrytor : ISecretEncryptor
+internal sealed class DataProtectionSecretEncryptor : ISecretEncryptor
 {
     private readonly IDataProtector _protector;
 
     private const string Purpose = "UmbracoAuthorizedServiceTokens";
 
-    public DataProtectionSecretEncrytor(IDataProtectionProvider dataProtectionProvider)
+    public DataProtectionSecretEncryptor(IDataProtectionProvider dataProtectionProvider)
     {
         _protector = dataProtectionProvider.CreateProtector(Purpose);
     }

src/Umbraco.AuthorizedServices/Services/Implement/NoopSecretEncryptor.cs

@@ -0,0 +1,8 @@
+namespace Umbraco.AuthorizedServices.Services.Implement;
+
+internal sealed class NoopSecretEncryptor : ISecretEncryptor
+{
+    public string Encrypt(string value) => value;
+
+    public string Decrypt(string value) => value;
+}

tests/Umbraco.AuthorizedServices.Tests/Services/AesSecretEncryptorTests.cs

@@ -0,0 +1,13 @@
+using Umbraco.AuthorizedServices.Services;
+using Umbraco.AuthorizedServices.Services.Implement;
+
+namespace Umbraco.AuthorizedServices.Tests.Services;
+
+internal class AesSecretEncryptorTests : EncryptorTestsBase
+{
+    protected override ISecretEncryptor CreateEncrytor()
+    {
+        const string SecretKey = "secret";
+        return new AesSecretEncryptor(SecretKey);
+    }
+}

tests/Umbraco.AuthorizedServices.Tests/Services/DataProtectionSecretEncrytorTests.cs

@@ -0,0 +1,14 @@
+using Microsoft.AspNetCore.DataProtection;
+using Umbraco.AuthorizedServices.Services;
+using Umbraco.AuthorizedServices.Services.Implement;
+
+namespace Umbraco.AuthorizedServices.Tests.Services;
+
+internal class DataProtectionSecretEncrytorTests : EncryptorTestsBase
+{
+    protected override ISecretEncryptor CreateEncrytor()
+    {
+        var dataProtectionProvider = new EphemeralDataProtectionProvider();
+        return new DataProtectionSecretEncryptor(dataProtectionProvider);
+    }
+}

tests/Umbraco.AuthorizedServices.Tests/Services/EncryptorTestsBase.cs

@@ -0,0 +1,24 @@
+using Umbraco.AuthorizedServices.Services;
+
+namespace Umbraco.AuthorizedServices.Tests.Services;
+
+internal abstract class EncryptorTestsBase
+{
+    protected const string Message = "When seagulls follow the trawler, it is because they think sardines will be thrown into the sea.";
+
+    [Test]
+    public void Encrypt_And_Decrypt()
+    {
+        // Arrange
+        ISecretEncryptor sut = CreateEncrytor();
+
+        // Act
+        var encryptedMessage = sut.Encrypt(Message);
+        var decryptedMessage = sut.Decrypt(encryptedMessage);
+
+        // Assert
+        decryptedMessage.Should().Be(Message);
+    }
+
+    protected abstract ISecretEncryptor CreateEncrytor();
+}

tests/Umbraco.AuthorizedServices.Tests/Services/NoopSecretEncrytorTests.cs

@@ -0,0 +1,9 @@
+using Umbraco.AuthorizedServices.Services;
+using Umbraco.AuthorizedServices.Services.Implement;
+
+namespace Umbraco.AuthorizedServices.Tests.Services;
+
+internal class NoopSecretEncrytorTests : EncryptorTestsBase
+{
+    protected override ISecretEncryptor CreateEncrytor() => new NoopSecretEncryptor();
+}