SNOW-1373131 disable connection pooling for external browser authentication #941
Conversation
…some of its settings gets changed and no longer match the pool initial setup
…188-changed-session-behavior
… session interface SnowflakeDbSessionPool, removal of pool config setters
…-changed-session-behavior # Conflicts: # Snowflake.Data/Core/Session/SessionPool.cs
…turned by the server in case of values \"final\"
…rnatively securely provided password)
…d connection string
| + ";authenticator=externalbrowser;[email protected];POOLINGENABLED=TRUE"; | ||
| conn.Open(); | ||
| Assert.AreEqual(ConnectionState.Open, conn.State); | ||
| Assert.AreEqual(true, SnowflakeDbConnectionPool.GetPool(conn.ConnectionString).GetPooling()); |
There was a problem hiding this comment.
why true? the feature was about enforcing it to be false?
| { SFSessionProperty.CHANGEDSESSION, DefaultValue(SFSessionProperty.CHANGEDSESSION) }, | ||
| { SFSessionProperty.WAITINGFORIDLESESSIONTIMEOUT, DefaultValue(SFSessionProperty.WAITINGFORIDLESESSIONTIMEOUT) }, | ||
| { SFSessionProperty.EXPIRATIONTIMEOUT, DefaultValue(SFSessionProperty.EXPIRATIONTIMEOUT) }, | ||
| { SFSessionProperty.POOLINGENABLED, "true" } |
| { SFSessionProperty.POOLINGENABLED, "false" } // by default pooling is disabled for externalbrowser authentication | ||
| } | ||
| }; | ||
| var testCaseWithExternalBrowserAndPoolingEnabled = new TestCase() |
There was a problem hiding this comment.
it works, and we have some tests written in this manner, but I would prefer the way of writing test like TestValidateCorrectAccountNames() - so a dedicated test verifying only the properties which are important from given test point of view.
| @@ -296,6 +296,19 @@ private static void ValidateAuthenticator(SFSessionProperties properties) | |||
| logger.Error(error); | |||
| throw new SnowflakeDbException(SFError.UNKNOWN_AUTHENTICATOR, authenticator); | |||
| } | |||
|
|
|||
| if (authenticator.Equals(ExternalBrowserAuthenticator.AUTH_NAME)) | |||
There was a problem hiding this comment.
I think this logic would fit better to SFSessionHttpClientProperties class.
In SFSessionHttpClientProperties we interpret string values as proper types values and we do a logic of changing the values.
SFSessionProperty is operates only on string values rather without interpreting them.
|
|
||
| if (authenticator.Equals(ExternalBrowserAuthenticator.AUTH_NAME)) | ||
| { | ||
| if (!properties.TryGetValue(SFSessionProperty.POOLINGENABLED, out var poolingEnabledStr)) |
There was a problem hiding this comment.
should all this logic be for old pool as well?
| } | ||
| else if (Boolean.TryParse(poolingEnabledStr, out var poolingEnabled) && poolingEnabled) | ||
| { | ||
| logger.Warn("Connection pooling is enabled for external browser authentication"); |
There was a problem hiding this comment.
we should enforce false for the new pool I belive
sfc-gh-dstempniak
changed the base branch from
pool/SNOW-860872-connection-pool
to
master
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
sfc-gh-dstempniak
changed the base branch from
master
to
pool/SNOW-860872-connection-pool
sfc-gh-knozderko
force-pushed
the
pool/SNOW-860872-connection-pool
branch
from
49d5d13 to
97cceaf
Compare
Description
Disable connection pooling for external browser authentication.
Checklist
dotnet test)