SNOW-715504: MFA token cache support #988

sfc-gh-jmartinezramirez · 2024-07-04T14:53:26Z

Description

Added support for MFA token cache.

Checklist

Code compiles correctly
Code is formatted according to Coding Conventions
Created tests which fail without the change (if possible)
All tests passing (dotnet test)
Extended the README / documentation, if necessary
Provide JIRA issue id (if possible) or GitHub issue id in PR name

codecov · 2024-07-09T22:23:37Z

Codecov Report

Attention: Patch coverage is 85.40268% with 87 lines in your changes missing coverage. Please review.

Project coverage is 86.11%. Comparing base (a33bb45) to head (f5e1862).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...ager/Infrastructure/SFCredentialManagerFileImpl.cs	85.39%	21 Missing and 5 partials ⚠️
...astructure/SFCredentialManagerWindowsNativeImpl.cs	81.17%	14 Missing and 2 partials ⚠️
Snowflake.Data/Core/Session/SessionPool.cs	82.00%	3 Missing and 6 partials ⚠️
...e.Data/Client/SnowflakeCredentialManagerFactory.cs	86.88%	4 Missing and 4 partials ⚠️
Snowflake.Data/Core/Session/SFSession.cs	78.94%	6 Missing and 2 partials ⚠️
Snowflake.Data/Core/Tools/UnixOperations.cs	72.22%	4 Missing and 1 partial ⚠️
...e.Data/Core/Authenticator/MFACacheAuthenticator.cs	78.94%	3 Missing and 1 partial ⚠️
Snowflake.Data/Core/Tools/FileOperations.cs	55.55%	3 Missing and 1 partial ⚠️
Snowflake.Data/Core/Tools/StringUtils.cs	75.00%	1 Missing and 1 partial ⚠️
...Core/Authenticator/ExternalBrowserAuthenticator.cs	0.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
- Coverage   86.32%   86.11%   -0.21%     
==========================================
  Files         121      129       +8     
  Lines       11845    12404     +559     
  Branches     1212     1273      +61     
==========================================
+ Hits        10225    10682     +457     
- Misses       1331     1406      +75     
- Partials      289      316      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerWindowsNativeImpl.cs

Snowflake.Data/Core/Authenticator/IAuthenticator.cs

Snowflake.Data/Core/Authenticator/MFACacheAuthenticator.cs

Snowflake.Data/Core/Session/SessionPool.cs

Snowflake.Data/Client/SFCredentialManagerFactory.cs

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerWindowsNativeImpl.cs

Snowflake.Data/Core/Session/ConnectionCacheManager.cs

Snowflake.Data/Core/Session/ConnectionPoolManager.cs

Snowflake.Data/Core/Session/SessionPool.cs

Snowflake.Data/Core/Session/SFSession.cs

Snowflake.Data/Core/Tools/StringUtils.cs

Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerTest.cs

Snowflake.Data.Tests/UnitTests/SFSessionTest.cs

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs

Snowflake.Data/Core/Session/SessionPool.cs

Snowflake.Data/Core/Tools/UnixOperations.cs

Snowflake.Data/Core/SFError.cs

Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs

Snowflake.Data/Core/Session/SFSessionProperty.cs

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

Snowflake.Data/Core/Session/SessionPool.cs

Snowflake.Data/Client/SFCredentialManagerFactory.cs

Snowflake.Data/Core/Session/SessionPool.cs

Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs

Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs

Snowflake.Data/Core/Tools/StringUtils.cs

…each credential manager implementation.

Added write file validator for file permissions. Additional PR suggestions

Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs

sfc-gh-mkubik · 2025-01-02T16:24:27Z

Snowflake.Data/Client/SnowflakeCredentialManagerFactory.cs

+                {
+                    if (s_credentialManager == null)
+                    {
+                        s_credentialManager = s_defaultCredentialManager;


Is it necessary to check if s_credentialManager == null inside lock does lock mean possibly waiting for another thread setting this value? Also why are we checking it on Get instead of setting defaultCredentialManager as a default value of s_credentialManager?

The reason to check if manager is null inside the lock is that even though it was null before the lock could stop the thread for a while so after waking up the situation can be different (another thread could initialise the manager).

Your second question is why not to initialise the s_credentialManager with the default value in the constructor? Maybe we could do so. The idea could be that we wanted to have lazy initialisation...
I changed it.

sfc-gh-mkubik · 2025-01-03T08:57:58Z

Snowflake.Data/Core/Authenticator/IAuthenticator.cs

@@ -101,6 +101,24 @@ protected void Login()
        /// <param name="data">The login request data to update.</param>
        protected abstract void SetSpecializedAuthenticatorData(ref LoginRequestData data);

+        protected void SetSecondaryAuthenticationData(ref LoginRequestData data)


nit: could we add a doc string to this method? Probably it's because I'm reading it in the morning but I understood secondary authentication data as some less important data instead of data related to the second authentication factor

"less important data" - then it would be AuthtneticationSecondaryData not SecondaryAuthenticationData. I don't like commenting what a method is doing. I would prefer give it a better name if you could propose anything better.

sfc-gh-mkubik · 2025-01-03T09:56:35Z

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

+
+        internal KeyTokenDict ReadJsonFile()
+        {
+            var contentFile = _fileOperations.ReadAllText(_fileStorage.JsonCacheFilePath, ValidateFilePermissions);


Should we make sure that the file exists first? I see that we're checking it before each call to this method but if we're returning new KeyTokenDict for the deserialization (corrupted file) then it'd make sense to return the same for non existing file?

I added handling the case if the file was deleted just after we checked it.

sfc-gh-mkubik · 2025-01-03T10:10:17Z

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

+                    KeyTokenDict keyTokenPairs = _fileOperations.Exists(_fileStorage.JsonCacheFilePath) ? ReadJsonFile() : new KeyTokenDict();
+                    keyTokenPairs[key] = token;
+                    var credentials = new CredentialsFileContent { Tokens = keyTokenPairs };
+                    string jsonString = JsonConvert.SerializeObject(credentials);


really really nit: JsonConvert.SerializeObject(credentials) could be passed directly to WriteToJsonFile to match line#149

I changed it

Snowflake.Data/Core/Tools/FileOperations.cs

Snowflake.Data/Core/Tools/UnixOperations.cs

doc/Connecting.md

Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerInMemoryImplTest.cs

Snowflake.Data.Tests/UnitTests/CredentialManager/SFCredentialManagerWindowsNativeImplTest.cs

Snowflake.Data.Tests/UnitTests/SFSessionTest.cs

Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 5 times, most recently from 6eba99f to 69d80c7 Compare July 5, 2024 21:54

sfc-gh-jmartinezramirez changed the title ~~Snow 715504 mfa token cache~~ SNOW-715504: MFA token cache support Jul 8, 2024

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from a8dff4b to 67089fc Compare July 22, 2024 21:36

sfc-gh-knozderko requested changes Jul 29, 2024

View reviewed changes

sfc-gh-knozderko reviewed Sep 3, 2024

View reviewed changes

Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 4 times, most recently from 6af9d1a to e36f8ed Compare September 4, 2024 13:31

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 2 times, most recently from e795b5f to 9316bbc Compare September 18, 2024 23:08

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from 9316bbc to 43adef9 Compare October 3, 2024 21:15

sfc-gh-jmartinezramirez marked this pull request as ready for review October 4, 2024 02:42

sfc-gh-jmartinezramirez requested a review from a team as a code owner October 4, 2024 02:42

sfc-gh-knozderko requested changes Oct 8, 2024

View reviewed changes

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from 3894dae to b1ec82b Compare October 8, 2024 16:20

sfc-gh-knozderko requested changes Oct 9, 2024

View reviewed changes

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from 0be7aa9 to ce1b619 Compare October 9, 2024 22:49

sfc-gh-knozderko requested changes Oct 17, 2024

View reviewed changes

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch 3 times, most recently from 5d32b9c to be6f19d Compare October 19, 2024 03:51

sfc-gh-knozderko requested changes Oct 22, 2024

View reviewed changes

sfc-gh-knozderko reviewed Oct 22, 2024

View reviewed changes

Snowflake.Data/Core/Tools/StringUtils.cs Outdated Show resolved Hide resolved

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from a9cec2f to 10f124b Compare October 22, 2024 21:00

sfc-gh-jmartinezramirez added 5 commits November 26, 2024 18:20

Additional comments

534f6fb

Using file validator mechanism

cd03870

Removed commented code.

1aae3a1

Moved logic to secure credential manager key when created instead of …

e542b76

…each credential manager implementation.

Added lock mechanism when applying credential manager operations.

9ae4120

Added write file validator for file permissions. Additional PR suggestions

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from 7675dcf to 2f1c811 Compare November 27, 2024 00:21

sfc-gh-knozderko reviewed Nov 27, 2024

View reviewed changes

Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs Outdated Show resolved Hide resolved

Applying PR suggestions

c709726

sfc-gh-jmartinezramirez force-pushed the SNOW-715504-MFA-Token-Cache branch from 2f1c811 to c709726 Compare November 28, 2024 14:15

sfc-gh-knozderko added 14 commits December 6, 2024 12:42

Change cache synchronization

efff0c3

Consistent approach to caching with other drivers

f0315b2

Change key and file format

5a46335

Fix file overriding

5eb2569

remove lock directory if too old

aba9cf4

Merge branch 'master' into SNOW-715504-MFA-Token-Cache

26f52ee

Fix ignored mfa tests

bada922

Fix test

d41d854

trying to fix a test

1865330

Fix tests

ef7b2ee

Log exceptions when operating with token cache

f5e1862

Support for xdg_cache_home env variable, enforce secure dir permissions

47b6333

Merge branch 'master' into SNOW-715504-MFA-Token-Cache

99907c3

test creating a directory with permissions

867e953

sfc-gh-mkubik reviewed Jan 3, 2025

View reviewed changes

sfc-gh-knozderko approved these changes Jan 3, 2025

View reviewed changes

little refactor

f1ab9a1

sfc-gh-pbulawa requested changes Jan 6, 2025

View reviewed changes

sfc-gh-knozderko added 2 commits January 7, 2025 11:02

improvements from review

a31a7ff

improvements from review

850db55

sfc-gh-pbulawa approved these changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-715504: MFA token cache support #988

SNOW-715504: MFA token cache support #988

sfc-gh-jmartinezramirez commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

sfc-gh-mkubik Jan 2, 2025

sfc-gh-knozderko Jan 3, 2025 •

edited

Loading

sfc-gh-mkubik Jan 3, 2025

sfc-gh-knozderko Jan 3, 2025

sfc-gh-mkubik Jan 3, 2025

sfc-gh-knozderko Jan 3, 2025

sfc-gh-mkubik Jan 3, 2025

sfc-gh-knozderko Jan 3, 2025

SNOW-715504: MFA token cache support #988

Are you sure you want to change the base?

SNOW-715504: MFA token cache support #988

Conversation

sfc-gh-jmartinezramirez commented Jul 4, 2024 • edited Loading

Description

Checklist

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

sfc-gh-mkubik Jan 2, 2025

Choose a reason for hiding this comment

sfc-gh-knozderko Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

sfc-gh-mkubik Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-knozderko Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-mkubik Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-knozderko Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-mkubik Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-knozderko Jan 3, 2025

Choose a reason for hiding this comment

sfc-gh-jmartinezramirez commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

sfc-gh-knozderko Jan 3, 2025 •

edited

Loading