-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNOW-715524: Add SSO token cache #921
base: master
Are you sure you want to change the base?
Changes from 22 commits
7de3ea6
4457077
15a58be
632a6b0
7f28fa8
a317158
625e04b
383fe5e
8ce9d10
a460e4b
e0b65d0
6606823
4e6869d
3788474
c597c64
39b90b2
b4ab4ed
13ba839
b334fb3
623ce6a
976bac2
feab579
61b2317
cb1c84f
7f0f801
780d213
575c0a4
aa00982
ecdcf37
6f31fe6
cdc9f80
9622f5c
a2f9b50
739c40c
c502e80
465da80
94bce01
83119f3
4e57147
61855f2
8b38fed
194eafa
8666194
403dbd2
33ebec5
512de5b
6ef9b35
d616dcc
045fc04
5c9d8d7
adb3218
da0cffb
44c746b
590e98b
2abcad4
dd24c76
f8b3230
7a384e8
7672901
2cdbae4
524b78f
6fdf045
2d2870d
f8f7336
8af5ba8
e56b086
2ba9aaf
7e9c098
e49ed64
c434184
67a8fbf
f8fcf2b
a46f1ad
52bfd3e
9edfd79
f6d8a0f
77a69b3
9038ba1
d800e6f
58b74d8
8f919e7
fbbac23
d53dd8f
98df45e
359cfa3
85d04ed
2d39171
f7be152
2f20edd
ac1d659
8c65f4d
330fca9
fc928fc
d53955a
d6ff05a
a1a6a31
74963c7
126e337
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
/* | ||
/* | ||
* Copyright (c) 2012-2023 Snowflake Computing Inc. All rights reserved. | ||
*/ | ||
|
||
|
@@ -1020,6 +1020,52 @@ public void TestSSOConnectionTimeoutAfter10s() | |
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (waitSeconds + 5) * 1000); | ||
} | ||
|
||
[Test] | ||
[Ignore("This test requires manual interaction and therefore cannot be run in CI")] | ||
public void TestSSOConnectionWithTokenCaching() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with ignored tests is that they will not be executed so in the future something can break and we will not notice that. You provide here a specific user. Also maybe we would need information that we should change it when testing on particular environment. But we should not write about any connection details in this repo since it is publicly visible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood, I'll put more details on the comments on how to do the test As for the user, I'll replace it so the details are not exposed |
||
{ | ||
using (IDbConnection conn = new SnowflakeDbConnection()) | ||
{ | ||
conn.ConnectionString | ||
= ConnectionStringWithoutAuth | ||
+ ";authenticator=externalbrowser;[email protected];allow_sso_token_caching=true;"; | ||
|
||
// Authenticate to retrieve and store the token if doesn't exist or invalid | ||
conn.Open(); | ||
Assert.AreEqual(ConnectionState.Open, conn.State); | ||
|
||
conn.Close(); | ||
Assert.AreEqual(ConnectionState.Closed, conn.State); | ||
|
||
// Authenticate using the token | ||
conn.Open(); | ||
Assert.AreEqual(ConnectionState.Open, conn.State); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the connection pool is enabled by default so when you open the connection for the second time you get exactly the same session - so you are not testing getting the token from the credential manager. You should open the second connection before closing the first one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
} | ||
|
||
[Test] | ||
[Ignore("This test requires manual interaction and therefore cannot be run in CI")] | ||
public void TestSSOConnectionWithInvalidCachedToken() | ||
{ | ||
using (IDbConnection conn = new SnowflakeDbConnection()) | ||
{ | ||
conn.ConnectionString | ||
= ConnectionStringWithoutAuth | ||
+ ";authenticator=externalbrowser;[email protected];allow_sso_token_caching=true;"; | ||
|
||
var key = SnowflakeCredentialManagerFactory.BuildCredentialKey(testConfig.host, testConfig.user, TokenType.IdToken.ToString()); | ||
var credentialManager = SnowflakeCredentialManagerInMemoryImpl.Instance; | ||
credentialManager.SaveCredentials(key, "wrongToken"); | ||
|
||
SnowflakeCredentialManagerFactory.SetCredentialManager(credentialManager); | ||
|
||
conn.Open(); | ||
Assert.AreEqual(ConnectionState.Open, conn.State); | ||
|
||
SnowflakeCredentialManagerFactory.UseDefaultCredentialManager(); | ||
} | ||
} | ||
|
||
[Test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to write a test which would not be ignored? Maybe with something mocked or something that simulates the browser action? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added tests for external browser authentication using a mock browser |
||
[Ignore("This test requires manual interaction and therefore cannot be run in CI")] | ||
public void TestSSOConnectionWithWrongUser() | ||
|
@@ -2231,17 +2277,42 @@ public void TestConnectStringWithQueryTag() | |
{ | ||
string expectedQueryTag = "Test QUERY_TAG 12345"; | ||
conn.ConnectionString = ConnectionString + $";query_tag={expectedQueryTag}"; | ||
|
||
conn.Open(); | ||
var command = conn.CreateCommand(); | ||
// This query itself will be part of the history and will have the query tag | ||
command.CommandText = "SELECT QUERY_TAG FROM table(information_schema.query_history_by_session())"; | ||
var queryTag = command.ExecuteScalar(); | ||
|
||
Assert.AreEqual(expectedQueryTag, queryTag); | ||
} | ||
} | ||
|
||
|
||
[Test] | ||
[Ignore("This test requires manual interaction and therefore cannot be run in CI")] | ||
public void TestSSOConnectionWithTokenCachingAsync() | ||
{ | ||
using (SnowflakeDbConnection conn = new SnowflakeDbConnection()) | ||
{ | ||
conn.ConnectionString | ||
= ConnectionStringWithoutAuth | ||
+ ";authenticator=externalbrowser;[email protected];allow_sso_token_caching=true;"; | ||
|
||
// Authenticate to retrieve and store the token if doesn't exist or invalid | ||
Task connectTask = conn.OpenAsync(CancellationToken.None); | ||
connectTask.Wait(); | ||
Assert.AreEqual(ConnectionState.Open, conn.State); | ||
|
||
connectTask = conn.CloseAsync(CancellationToken.None); | ||
connectTask.Wait(); | ||
Assert.AreEqual(ConnectionState.Closed, conn.State); | ||
|
||
// Authenticate using the token | ||
connectTask = conn.OpenAsync(CancellationToken.None); | ||
connectTask.Wait(); | ||
Assert.AreEqual(ConnectionState.Open, conn.State); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to update the PR with changes from master - this table was moved to another file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that it's in
Connecting.md
now so I put this line there