From d1999093c026efd80cfa2cd07a22834dbc9aab78 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Thu, 17 Oct 2024 15:51:15 -0600 Subject: [PATCH] Applying new PR suggestions --- .../IntegrationTests/SFConnectionIT.cs | 35 +++++++++++-------- .../SFCredentialManagerTest.cs | 35 +++++++++++++------ .../SnowflakeCredentialManagerFactory.cs | 25 ++++++++++++- .../SFCredentialManagerFileImpl.cs} | 10 +++--- .../Core/Session/SFSessionProperty.cs | 2 -- Snowflake.Data/Core/Session/SessionPool.cs | 9 +++-- Snowflake.Data/Core/Tools/UnixOperations.cs | 10 +++--- 7 files changed, 82 insertions(+), 44 deletions(-) rename Snowflake.Data/{Client/SnowflakeCredentialManagerFileImpl.cs => Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs} (91%) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 76490e099..e4b4bbaee 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -2,28 +2,25 @@ * Copyright (c) 2012-2023 Snowflake Computing Inc. All rights reserved. */ +using System; +using System.Data; using System.Data.Common; +using System.Diagnostics; using System.Net; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using Snowflake.Data.Client; +using Snowflake.Data.Core; using Snowflake.Data.Core.Session; using Snowflake.Data.Core.Tools; +using Snowflake.Data.Log; +using Snowflake.Data.Tests.Mock; using Snowflake.Data.Tests.Util; namespace Snowflake.Data.Tests.IntegrationTests { - using NUnit.Framework; - using Snowflake.Data.Client; - using System.Data; - using System; - using Snowflake.Data.Core; - using System.Threading.Tasks; - using System.Threading; - using Snowflake.Data.Log; - using System.Diagnostics; - using Snowflake.Data.Tests.Mock; - using System.Runtime.InteropServices; - using System.Net.Http; - using Snowflake.Data.Core.CredentialManager; - using Snowflake.Data.Core.CredentialManager.Infrastructure; [TestFixture] class SFConnectionIT : SFBaseTest @@ -2279,11 +2276,15 @@ public void TestUseMultiplePoolsConnectionPoolByDefault() [Ignore("This test requires manual interaction and therefore cannot be run in CI")] public void TestMFATokenCachingWithPasscodeFromConnectionString() { + // Use a connection with MFA enabled and set value of encode from mfa authenticator in the passcode property. + // ACCOUNT PARAMETER ALLOW_CLIENT_MFA_CACHING should be set to true in the account. + // On Mac/Linux OS default credential manager is in memory so please uncomment following line to use file based credential manager + // SnowflakeCredentialManagerFactory.SetCredentialManager(SnowflakeCredentialManagerFileImpl.Instance); using (SnowflakeDbConnection conn = new SnowflakeDbConnection()) { conn.ConnectionString = ConnectionString - + ";authenticator=username_password_mfa;application=DuoTest;Passcode=123456;"; + + ";authenticator=username_password_mfa;application=DuoTest;minPoolSize=0;"; // Authenticate to retrieve and store the token if doesn't exist or invalid @@ -2297,6 +2298,10 @@ public void TestMFATokenCachingWithPasscodeFromConnectionString() [Ignore("Requires manual steps and environment with mfa authentication enrolled")] // to enroll to mfa authentication edit your user profile public void TestMfaWithPasswordConnectionUsingPasscodeWithSecureString() { + // Use a connection with MFA enabled and set value of encode from mfa authenticator in the passcode property. + // ACCOUNT PARAMETER ALLOW_CLIENT_MFA_CACHING should be set to true in the account. + // On Mac/Linux OS default credential manager is in memory so please uncomment following line to use file based credential manager + // SnowflakeCredentialManagerFactory.SetCredentialManager(SnowflakeCredentialManagerFileImpl.Instance); // arrange using (SnowflakeDbConnection conn = new SnowflakeDbConnection()) { diff --git a/Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerTest.cs b/Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerTest.cs index 8526ce978..663e059fc 100644 --- a/Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerTest.cs +++ b/Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerTest.cs @@ -110,7 +110,7 @@ public class SFFileCredentialManagerTest : SFBaseCredentialManagerTest [SetUp] public void SetUp() { - _credentialManager = SnowflakeCredentialManagerFileImpl.Instance; + _credentialManager = SFCredentialManagerFileImpl.Instance; } } @@ -133,7 +133,7 @@ class SFCredentialManagerTest private const string CustomJsonDir = "testdirectory"; - private static readonly string s_customJsonPath = Path.Combine(CustomJsonDir, SnowflakeCredentialManagerFileImpl.CredentialCacheFileName); + private static readonly string s_customJsonPath = Path.Combine(CustomJsonDir, SFCredentialManagerFileImpl.CredentialCacheFileName); [SetUp] public void SetUp() { @@ -141,7 +141,7 @@ [SetUp] public void SetUp() t_directoryOperations = new Mock(); t_unixOperations = new Mock(); t_environmentOperations = new Mock(); - SnowflakeCredentialManagerFactory.SetCredentialManager(SFCredentialManagerInMemoryImpl.Instance); + SnowflakeCredentialManagerFactory.UseInMemoryCredentialManager(); } [TearDown] public void TearDown() @@ -173,13 +173,26 @@ public void TestUsingDefaultCredentialManager() public void TestSettingCustomCredentialManager() { // arrange - SnowflakeCredentialManagerFactory.SetCredentialManager(SnowflakeCredentialManagerFileImpl.Instance); + SnowflakeCredentialManagerFactory.SetCredentialManager(SFCredentialManagerFileImpl.Instance); // act _credentialManager = SnowflakeCredentialManagerFactory.GetCredentialManager(); // assert - Assert.IsInstanceOf(_credentialManager); + Assert.IsInstanceOf(_credentialManager); + } + + [Test] + public void TestUseFileImplCredentialManager() + { + // arrange + SnowflakeCredentialManagerFactory.UseFileCredentialManager(); + + // act + _credentialManager = SnowflakeCredentialManagerFactory.GetCredentialManager(); + + // assert + Assert.IsInstanceOf(_credentialManager); } [Test] @@ -199,9 +212,9 @@ public void TestThatThrowsErrorWhenCacheFileIsNotCreated() FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR)) .Returns(-1); t_environmentOperations - .Setup(e => e.GetEnvironmentVariable(SnowflakeCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) + .Setup(e => e.GetEnvironmentVariable(SFCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) .Returns(CustomJsonDir); - SnowflakeCredentialManagerFactory.SetCredentialManager(new SnowflakeCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); + SnowflakeCredentialManagerFactory.SetCredentialManager(new SFCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); _credentialManager = SnowflakeCredentialManagerFactory.GetCredentialManager(); // act @@ -228,9 +241,9 @@ public void TestThatThrowsErrorWhenCacheFileCanBeAccessedByOthers() .Setup(u => u.GetFilePermissions(s_customJsonPath)) .Returns(FileAccessPermissions.AllPermissions); t_environmentOperations - .Setup(e => e.GetEnvironmentVariable(SnowflakeCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) + .Setup(e => e.GetEnvironmentVariable(SFCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) .Returns(CustomJsonDir); - SnowflakeCredentialManagerFactory.SetCredentialManager(new SnowflakeCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); + SnowflakeCredentialManagerFactory.SetCredentialManager(new SFCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); _credentialManager = SnowflakeCredentialManagerFactory.GetCredentialManager(); // act @@ -257,14 +270,14 @@ public void TestThatJsonFileIsCheckedIfAlreadyExists() .Setup(u => u.GetFilePermissions(s_customJsonPath)) .Returns(FileAccessPermissions.UserReadWriteExecute); t_environmentOperations - .Setup(e => e.GetEnvironmentVariable(SnowflakeCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) + .Setup(e => e.GetEnvironmentVariable(SFCredentialManagerFileImpl.CredentialCacheDirectoryEnvironmentName)) .Returns(CustomJsonDir); t_fileOperations .SetupSequence(f => f.Exists(s_customJsonPath)) .Returns(false) .Returns(true); - SnowflakeCredentialManagerFactory.SetCredentialManager(new SnowflakeCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); + SnowflakeCredentialManagerFactory.SetCredentialManager(new SFCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object)); _credentialManager = SnowflakeCredentialManagerFactory.GetCredentialManager(); // act diff --git a/Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs b/Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs index 072ab3e05..332e0be1f 100644 --- a/Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs +++ b/Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs @@ -33,10 +33,33 @@ public static void UseDefaultCredentialManager() } public static void SetCredentialManager(ISnowflakeCredentialManager customCredentialManager) + { + SetCredentialManager(customCredentialManager, true); + } + + public static void UseInMemoryCredentialManager() + { + SetCredentialManager(SFCredentialManagerInMemoryImpl.Instance, false); + } + + public static void UseFileCredentialManager() + { + SetCredentialManager(SFCredentialManagerFileImpl.Instance, false); + } + + public static void UseWindowsCredentialManager() + { + SetCredentialManager(SFCredentialManagerWindowsNativeImpl.Instance, false); + } + + internal static void SetCredentialManager(ISnowflakeCredentialManager customCredentialManager, bool isCustomCredential) { lock (credentialManagerLock) { - s_logger.Info($"Setting the custom credential manager: {customCredentialManager.GetType().Name}"); + var customText = isCustomCredential ? "custom " : string.Empty; + s_logger.Info(customCredentialManager == null + ? $"Clearing the {customText}credential manager:" + : $"Setting the {customText}credential manager: {customCredentialManager?.GetType()?.Name}"); s_customCredentialManager = customCredentialManager; } } diff --git a/Snowflake.Data/Client/SnowflakeCredentialManagerFileImpl.cs b/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs similarity index 91% rename from Snowflake.Data/Client/SnowflakeCredentialManagerFileImpl.cs rename to Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs index 5a30a4559..0f57aaebd 100644 --- a/Snowflake.Data/Client/SnowflakeCredentialManagerFileImpl.cs +++ b/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs @@ -13,9 +13,9 @@ using System.Runtime.InteropServices; using KeyTokenDict = System.Collections.Generic.Dictionary; -namespace Snowflake.Data.Core +namespace Snowflake.Data.Core.CredentialManager.Infrastructure { - public class SnowflakeCredentialManagerFileImpl : ISnowflakeCredentialManager + internal class SFCredentialManagerFileImpl : ISnowflakeCredentialManager { internal const string CredentialCacheDirectoryEnvironmentName = "SF_TEMPORARY_CREDENTIAL_CACHE_DIR"; @@ -23,7 +23,7 @@ public class SnowflakeCredentialManagerFileImpl : ISnowflakeCredentialManager internal const string CredentialCacheFileName = "temporary_credential.json"; - private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger(); + private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger(); private readonly string _jsonCacheDirectory; @@ -37,9 +37,9 @@ public class SnowflakeCredentialManagerFileImpl : ISnowflakeCredentialManager private readonly EnvironmentOperations _environmentOperations; - public static readonly SnowflakeCredentialManagerFileImpl Instance = new SnowflakeCredentialManagerFileImpl(FileOperations.Instance, DirectoryOperations.Instance, UnixOperations.Instance, EnvironmentOperations.Instance); + public static readonly SFCredentialManagerFileImpl Instance = new SFCredentialManagerFileImpl(FileOperations.Instance, DirectoryOperations.Instance, UnixOperations.Instance, EnvironmentOperations.Instance); - internal SnowflakeCredentialManagerFileImpl(FileOperations fileOperations, DirectoryOperations directoryOperations, UnixOperations unixOperations, EnvironmentOperations environmentOperations) + internal SFCredentialManagerFileImpl(FileOperations fileOperations, DirectoryOperations directoryOperations, UnixOperations unixOperations, EnvironmentOperations environmentOperations) { _fileOperations = fileOperations; _directoryOperations = directoryOperations; diff --git a/Snowflake.Data/Core/Session/SFSessionProperty.cs b/Snowflake.Data/Core/Session/SFSessionProperty.cs index a75492d47..a9663961d 100644 --- a/Snowflake.Data/Core/Session/SFSessionProperty.cs +++ b/Snowflake.Data/Core/Session/SFSessionProperty.cs @@ -113,8 +113,6 @@ internal enum SFSessionProperty POOLINGENABLED, [SFSessionPropertyAttr(required = false, defaultValue = "false")] DISABLE_SAML_URL_CHECK, - [SFSessionPropertyAttr(required = false, defaultValue = "false")] - CLIENT_REQUEST_MFA_TOKEN, [SFSessionPropertyAttr(required = false, IsSecret = true)] PASSCODE, [SFSessionPropertyAttr(required = false, defaultValue = "false")] diff --git a/Snowflake.Data/Core/Session/SessionPool.cs b/Snowflake.Data/Core/Session/SessionPool.cs index 8164c4999..8ba301b4b 100644 --- a/Snowflake.Data/Core/Session/SessionPool.cs +++ b/Snowflake.Data/Core/Session/SessionPool.cs @@ -182,7 +182,7 @@ internal async Task GetSessionAsync(string connStr, SecureString pass if (!GetPooling()) return await NewNonPoolingSessionAsync(connStr, password, passcode, cancellationToken).ConfigureAwait(false); var isMfaAuthentication = sessionProperties.TryGetValue(SFSessionProperty.AUTHENTICATOR, out var authenticator) && authenticator == MFACacheAuthenticator.AUTH_NAME; - var sessionOrCreateTokens = GetIdleSession(connStr, isMfaAuthentication ? 1 : Int32.MaxValue); + var sessionOrCreateTokens = GetIdleSession(connStr, isMfaAuthentication ? 1 : int.MaxValue); WarnAboutOverridenConfig(); if (sessionOrCreateTokens.Session != null) @@ -254,10 +254,9 @@ private SessionOrCreationTokens GetIdleSession(string connStr, int maxSessions) return new SessionOrCreationTokens(session); } s_logger.Debug("SessionPool::GetIdleSession - no thread was waiting for a session, but could not find any idle session available in the pool" + PoolIdentification()); - var sessionsCount = Math.Min(maxSessions, AllowedNumberOfNewSessionCreations(1)); + var sessionsCount = AllowedNumberOfNewSessionCreations(1, maxSessions); if (sessionsCount > 0) { - s_logger.Debug($"SessionPool::GetIdleSession - register creation of {sessionsCount} sessions" + PoolIdentification()); // there is no need to wait for a session since we can create new ones return new SessionOrCreationTokens(RegisterSessionCreations(sessionsCount)); } @@ -277,7 +276,7 @@ private List RegisterSessionCreations(int sessionsCount) = .Select(_ => _sessionCreationTokenCounter.NewToken()) .ToList(); - private int AllowedNumberOfNewSessionCreations(int atLeastCount) + private int AllowedNumberOfNewSessionCreations(int atLeastCount, int maxSessionsLimit = int.MaxValue) { // we are expecting to create atLeast 1 session in case of opening a connection (atLeastCount = 1) // but we have no expectations when closing a connection (atLeastCount = 0) @@ -292,7 +291,7 @@ private int AllowedNumberOfNewSessionCreations(int atLeastCount) { var maxSessionsToCreate = _poolConfig.MaxPoolSize - currentSize; var sessionsNeeded = Math.Max(_poolConfig.MinPoolSize - currentSize, atLeastCount); - var sessionsToCreate = Math.Min(sessionsNeeded, maxSessionsToCreate); + var sessionsToCreate = Math.Min(maxSessionsLimit, Math.Min(sessionsNeeded, maxSessionsToCreate)); s_logger.Debug($"SessionPool - allowed to create {sessionsToCreate} sessions, current pool size is {currentSize} out of {_poolConfig.MaxPoolSize}" + PoolIdentification()); return sessionsToCreate; } diff --git a/Snowflake.Data/Core/Tools/UnixOperations.cs b/Snowflake.Data/Core/Tools/UnixOperations.cs index 7757a5681..1b369da86 100644 --- a/Snowflake.Data/Core/Tools/UnixOperations.cs +++ b/Snowflake.Data/Core/Tools/UnixOperations.cs @@ -3,14 +3,14 @@ */ +using System.IO; +using System.Security; +using System.Text; +using Mono.Unix; +using Mono.Unix.Native; namespace Snowflake.Data.Core.Tools { - using System.IO; - using System.Security; - using System.Text; - using Mono.Unix; - using Mono.Unix.Native; internal class UnixOperations {