From 70b837520d26dc10e3d575b2048beb0cbd64477e Mon Sep 17 00:00:00 2001 From: msJinLei Date: Thu, 2 Feb 2023 22:47:16 +0800 Subject: [PATCH] Address review comments --- src/Accounts/Accounts.Test/AutosaveTests.cs | 1 - .../Accounts.Test/ContextCmdletTests.cs | 2 +- .../Accounts.Test/ProfileCmdletTests.cs | 2 +- .../Authentication.Test/StorageHelperTest.cs | 6 +-- .../Authentication/KeyStore/AzKeyStore.cs | 49 ++++++++----------- .../Authentication/KeyStore/IStorage.cs | 1 - .../Authentication/KeyStore/IStorageHelper.cs | 5 +- .../KeyStore/InMemoryKeyCache.cs | 10 ++-- .../KeyStore/KeyStoreNotificationArgs.cs | 2 +- .../Authentication/KeyStore/StorageHelper.cs | 14 +++--- .../Authentication/KeyStore/StorageWrapper.cs | 2 +- 11 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/Accounts/Accounts.Test/AutosaveTests.cs b/src/Accounts/Accounts.Test/AutosaveTests.cs index 09ffbecb149b..0986b9808436 100644 --- a/src/Accounts/Accounts.Test/AutosaveTests.cs +++ b/src/Accounts/Accounts.Test/AutosaveTests.cs @@ -41,7 +41,6 @@ public AutosaveTests(ITestOutputHelper output) XunitTracingInterceptor.AddToContext(new XunitTracingInterceptor(output)); commandRuntimeMock = new MockCommandRuntime(); dataStore = new MemoryDataStore(); - ResetState(); keyStore = SetMockedAzKeyStore(); } diff --git a/src/Accounts/Accounts.Test/ContextCmdletTests.cs b/src/Accounts/Accounts.Test/ContextCmdletTests.cs index 7b977a5a7482..9e4d44bb4cb5 100644 --- a/src/Accounts/Accounts.Test/ContextCmdletTests.cs +++ b/src/Accounts/Accounts.Test/ContextCmdletTests.cs @@ -64,7 +64,7 @@ public ContextCmdletTests(ITestOutputHelper output) Mock storageMocker = new Mock(); AzKeyStore azKeyStore = null; string profilePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), Resources.AzureDirectoryName); - azKeyStore = new AzKeyStore(profilePath, AzureSession.Instance.KeyStoreFile, storageMocker.Object); + azKeyStore = new AzKeyStore(profilePath, AzureSession.Instance.KeyStoreFile, true, storageMocker.Object); AzureSession.Instance.RegisterComponent(AzKeyStore.Name, () => azKeyStore, true); } diff --git a/src/Accounts/Accounts.Test/ProfileCmdletTests.cs b/src/Accounts/Accounts.Test/ProfileCmdletTests.cs index 386998a78ffd..fdd16bf07357 100644 --- a/src/Accounts/Accounts.Test/ProfileCmdletTests.cs +++ b/src/Accounts/Accounts.Test/ProfileCmdletTests.cs @@ -55,7 +55,7 @@ private AzKeyStore SetMockedAzKeyStore() storageMocker.Setup(f => f.Create()).Returns(storageMocker.Object); storageMocker.Setup(f => f.ReadData()).Returns(new byte[0]); storageMocker.Setup(f => f.WriteData(It.IsAny())).Callback((byte[] s) => { }); - var keyStore = new AzKeyStore(AzureSession.Instance.ARMProfileDirectory, "azkeystore", storageMocker.Object); + var keyStore = new AzKeyStore(AzureSession.Instance.ARMProfileDirectory, "azkeystore", false, storageMocker.Object); return keyStore; } diff --git a/src/Accounts/Authentication.Test/StorageHelperTest.cs b/src/Accounts/Authentication.Test/StorageHelperTest.cs index 0e95bee71e32..7cde8dd554e6 100644 --- a/src/Accounts/Authentication.Test/StorageHelperTest.cs +++ b/src/Accounts/Authentication.Test/StorageHelperTest.cs @@ -72,11 +72,7 @@ public void SaveToStorageTest() var helper = StorageHelper.GetStorageHelperAsync(true, keyStoreFileName, profilePath , keystoreMocker.Object, storageMocker.Object).GetAwaiter().GetResult(); - var args = new KeyStoreNotificationArgs() - { - KeyStore = keystoreMocker.Object - }; - helper.WriteToCachedStorage(args); + helper.WriteToCachedStorage(keystoreMocker.Object); string actual = Encoding.UTF8.GetString(storageChecker.ToArray()); Assert.Equal(EXPECTED, actual); diff --git a/src/Accounts/Authentication/KeyStore/AzKeyStore.cs b/src/Accounts/Authentication/KeyStore/AzKeyStore.cs index 499df6b4792b..ade907b83f9e 100644 --- a/src/Accounts/Authentication/KeyStore/AzKeyStore.cs +++ b/src/Accounts/Authentication/KeyStore/AzKeyStore.cs @@ -23,11 +23,11 @@ public class AzKeyStore : IDisposable public string FileName { get; set; } public string Directory { get; set; } - private IKeyCache _inMemoryStore = null; - public IKeyCache InMemoryStore + private IKeyCache _inMemoryKeyCache = null; + public IKeyCache InMemoryKeyCache { - get => _inMemoryStore; - set => _inMemoryStore = value; + get => _inMemoryKeyCache; + set => _inMemoryKeyCache = value; } private IStorage inputStorage = null; @@ -38,21 +38,14 @@ public bool IsProtected private set; } - public bool SyncToStorage - { - get; set; - } - public AzKeyStore(string directory, string fileName, bool enableContextAutoSaving = true, IStorage storage = null) { - InMemoryStore = new InMemoryKeyCache(); - - SyncToStorage = enableContextAutoSaving; + InMemoryKeyCache = new InMemoryKeyCache(); - if (SyncToStorage) + if (enableContextAutoSaving) { - InMemoryStore.SetBeforeAccess(LoadStorage); - InMemoryStore.SetOnUpdate(UpdateStorage); + InMemoryKeyCache.SetBeforeAccess(LoadStorage); + InMemoryKeyCache.SetOnUpdate(UpdateStorage); } FileName = fileName; @@ -60,28 +53,28 @@ public AzKeyStore(string directory, string fileName, bool enableContextAutoSavin inputStorage = storage; - InMemoryKeyCache.RegisterJsonConverter(typeof(ServicePrincipalKey), typeof(ServicePrincipalKey).Name); - InMemoryKeyCache.RegisterJsonConverter(typeof(SecureString), typeof(SecureString).Name, new SecureStringConverter()); + Common.InMemoryKeyCache.RegisterJsonConverter(typeof(ServicePrincipalKey), typeof(ServicePrincipalKey).Name); + Common.InMemoryKeyCache.RegisterJsonConverter(typeof(SecureString), typeof(SecureString).Name, new SecureStringConverter()); } private void LoadStorage(KeyStoreNotificationArgs args) { - var asyncHelper = StorageHelper.GetStorageHelperAsync(true, FileName, Directory, args.KeyStore, inputStorage); + var asyncHelper = StorageHelper.GetStorageHelperAsync(true, FileName, Directory, args.KeyCache, inputStorage); var helper = asyncHelper.GetAwaiter().GetResult(); IsProtected = helper.IsProtected; } private void UpdateStorage(KeyStoreNotificationArgs args) { - var asyncHelper = StorageHelper.GetStorageHelperAsync(false, FileName, Directory, args.KeyStore, inputStorage); + var asyncHelper = StorageHelper.GetStorageHelperAsync(false, FileName, Directory, args.KeyCache, inputStorage); var helper = asyncHelper.GetAwaiter().GetResult(); - helper.WriteToCachedStorage(args.KeyStore); + helper.WriteToCachedStorage(args.KeyCache); } public void Clear() { - InMemoryStore.Clear(); + InMemoryKeyCache.Clear(); } public void Dispose() @@ -91,29 +84,29 @@ public void Dispose() public void SaveSecureString(IKeyStoreKey key, SecureString value) { - InMemoryStore.SaveKey(key, value); + InMemoryKeyCache.SaveKey(key, value); } public SecureString GetSecureString(IKeyStoreKey key) { - return InMemoryStore.GetKey(key); + return InMemoryKeyCache.GetKey(key); } public bool RemoveSecureString(IKeyStoreKey key) { - return InMemoryStore.DeleteKey(key); + return InMemoryKeyCache.DeleteKey(key); } public void EnableSyncToStorage() { - InMemoryStore.SetBeforeAccess(LoadStorage); - InMemoryStore.SetOnUpdate(UpdateStorage); + InMemoryKeyCache.SetBeforeAccess(LoadStorage); + InMemoryKeyCache.SetOnUpdate(UpdateStorage); } public void DisableSyncToStorage() { - InMemoryStore.SetBeforeAccess(null); - InMemoryStore.SetOnUpdate(null); + InMemoryKeyCache.SetBeforeAccess(null); + InMemoryKeyCache.SetOnUpdate(null); } } } diff --git a/src/Accounts/Authentication/KeyStore/IStorage.cs b/src/Accounts/Authentication/KeyStore/IStorage.cs index ab8b6653973c..71b7055c6f06 100644 --- a/src/Accounts/Authentication/KeyStore/IStorage.cs +++ b/src/Accounts/Authentication/KeyStore/IStorage.cs @@ -11,7 +11,6 @@ // See the License for the specific language governing permissions and // limitations under the License. // ---------------------------------------------------------------------------------- -using System; namespace Microsoft.Azure.Commands.ResourceManager.Common { diff --git a/src/Accounts/Authentication/KeyStore/IStorageHelper.cs b/src/Accounts/Authentication/KeyStore/IStorageHelper.cs index 704c327e4cbf..49be99e02563 100644 --- a/src/Accounts/Authentication/KeyStore/IStorageHelper.cs +++ b/src/Accounts/Authentication/KeyStore/IStorageHelper.cs @@ -11,7 +11,6 @@ // See the License for the specific language governing permissions and // limitations under the License. // ---------------------------------------------------------------------------------- -using System; namespace Microsoft.Azure.Commands.ResourceManager.Common { @@ -19,9 +18,9 @@ public interface IStorageHelper { void Clear(); - void LoadFromCachedStorage(IKeyCache keystore); + void LoadFromCachedStorage(IKeyCache keycache); - void WriteToCachedStorage(IKeyCache keystore); + void WriteToCachedStorage(IKeyCache keycache); bool IsProtected { diff --git a/src/Accounts/Authentication/KeyStore/InMemoryKeyCache.cs b/src/Accounts/Authentication/KeyStore/InMemoryKeyCache.cs index 69acb5ca7ae0..a2c8a78a5d73 100644 --- a/src/Accounts/Authentication/KeyStore/InMemoryKeyCache.cs +++ b/src/Accounts/Authentication/KeyStore/InMemoryKeyCache.cs @@ -11,14 +11,12 @@ // See the License for the specific language governing permissions and // limitations under the License. // ---------------------------------------------------------------------------------- -using Microsoft.Identity.Client; using Newtonsoft.Json; using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Text; -using System.Threading.Tasks; namespace Microsoft.Azure.Commands.ResourceManager.Common { @@ -48,7 +46,7 @@ public void SaveKey(IKeyStoreKey key, T value) { var args = new KeyStoreNotificationArgs() { - KeyStore = this + KeyCache = this }; BeforeAccess?.Invoke(args) ; if (!_typeNameMap.ContainsKey(key.GetType()) || !_typeNameMap.ContainsKey(value.GetType())) @@ -63,7 +61,7 @@ public T GetKey(IKeyStoreKey key) { var args = new KeyStoreNotificationArgs() { - KeyStore = this + KeyCache = this }; BeforeAccess?.Invoke(args); @@ -88,7 +86,7 @@ public bool DeleteKey(IKeyStoreKey key) { var args = new KeyStoreNotificationArgs() { - KeyStore = this + KeyCache = this }; BeforeAccess?.Invoke(args); bool ret = false; @@ -159,7 +157,7 @@ public void Clear() { var args = new KeyStoreNotificationArgs() { - KeyStore = this + KeyCache = this }; BeforeAccess?.Invoke(args); _credentials.Clear(); diff --git a/src/Accounts/Authentication/KeyStore/KeyStoreNotificationArgs.cs b/src/Accounts/Authentication/KeyStore/KeyStoreNotificationArgs.cs index 72852c52a9dd..3c87ffe1ba44 100644 --- a/src/Accounts/Authentication/KeyStore/KeyStoreNotificationArgs.cs +++ b/src/Accounts/Authentication/KeyStore/KeyStoreNotificationArgs.cs @@ -16,7 +16,7 @@ namespace Microsoft.Azure.Commands.ResourceManager.Common { public class KeyStoreNotificationArgs { - public IKeyCache KeyStore; + public IKeyCache KeyCache; } public delegate void KeyStoreCallbak(KeyStoreNotificationArgs args); diff --git a/src/Accounts/Authentication/KeyStore/StorageHelper.cs b/src/Accounts/Authentication/KeyStore/StorageHelper.cs index 5874b52b3296..85ed4675b180 100644 --- a/src/Accounts/Authentication/KeyStore/StorageHelper.cs +++ b/src/Accounts/Authentication/KeyStore/StorageHelper.cs @@ -20,7 +20,7 @@ namespace Microsoft.Azure.Commands.ResourceManager.Common { - public class StorageHelper : IStorageHelper + internal class StorageHelper : IStorageHelper { private const string KeyChainServiceName = "Microsoft.Azure.PowerShell"; @@ -88,7 +88,7 @@ private static StorageHelper Create(StorageCreationProperties storageCreationPro } #region Public API - public static async Task GetStorageHelperAsync(bool async, string fileName, string directory, IKeyCache keystore, IStorage storage = null) + public static async Task GetStorageHelperAsync(bool async, string fileName, string directory, IKeyCache keycache, IStorage storage = null) { StorageHelper storageHelper = null; @@ -109,7 +109,7 @@ public static async Task GetStorageHelperAsync(bool async, strin storageHelper = GetFallbackStorageHelper(fileName, directory, storage); storageHelper.VerifyPersistence(); } - storageHelper.LoadFromCachedStorage(keystore); + storageHelper.LoadFromCachedStorage(keycache); asyncLock.SetValue(storageHelper); } return storageHelper; @@ -128,7 +128,7 @@ public void Clear() } } - public void LoadFromCachedStorage(IKeyCache keystore) + public void LoadFromCachedStorage(IKeyCache keycache) { LogMessage(EventLogLevel.Verbose, $"Before access\nAcquiring lock for keystore"); @@ -152,7 +152,7 @@ public void LoadFromCachedStorage(IKeyCache keystore) try { LogMessage(EventLogLevel.Verbose, $"Deserializing the store"); - keystore.Deserialize(cachedStoreData, false); + keycache.Deserialize(cachedStoreData, false); } catch (Exception e) { @@ -165,7 +165,7 @@ public void LoadFromCachedStorage(IKeyCache keystore) } } - public void WriteToCachedStorage(IKeyCache keystore) + public void WriteToCachedStorage(IKeyCache keycache) { using (CreateCrossPlatLock(_storageCreationProperties)) { @@ -174,7 +174,7 @@ public void WriteToCachedStorage(IKeyCache keystore) LogMessage(EventLogLevel.Verbose, $"After access, cache in memory HasChanged"); try { - data = keystore.Serialize(); + data = keycache.Serialize(); } catch (Exception e) { diff --git a/src/Accounts/Authentication/KeyStore/StorageWrapper.cs b/src/Accounts/Authentication/KeyStore/StorageWrapper.cs index aaa15a7600d2..647cabe8acee 100644 --- a/src/Accounts/Authentication/KeyStore/StorageWrapper.cs +++ b/src/Accounts/Authentication/KeyStore/StorageWrapper.cs @@ -16,7 +16,7 @@ namespace Microsoft.Azure.Commands.ResourceManager.Common { - class StorageWrapper : IStorage + internal class StorageWrapper : IStorage { public StorageCreationProperties StorageCreationProperties { get; set; }