SNOW-2360255 Soteria implementation in LIBSFC #937
Conversation
sfc-gh-ext-simba-jy
requested review from
sfc-gh-jszczerbinski and
sfc-gh-pbulawa
|
|
||
| if (sf->client_store_temporary_credential && getAuthenticatorType(sf->authenticator) == AUTH_EXTERNALBROWSER) | ||
| AuthenticatorType authtype = getAuthenticatorType(sf->authenticator); | ||
| if (sf->client_store_temporary_credential) |
There was a problem hiding this comment.
Please provide a list of supported authentication types that allow for secure credential storage. Then, check if the specified authtype is on this list. This check is necessary to prevent initializing secure storage when an unsupported authenticator is used.
|
|
||
| sf->sso_token = secure_storage_get_credential(sf->token_cache, sf->host, sf->user, ID_TOKEN); | ||
|
|
||
| if (authtype == AUTH_EXTERNALBROWSER) { |
There was a problem hiding this comment.
Can you rewrite it to use switch?
| if (json_copy_string(&auth_token, data, "idToken") == SF_JSON_ERROR_NONE && sf->token_cache) { | ||
| secure_storage_save_credential(sf->token_cache, sf->host, sf->user, ID_TOKEN, auth_token); | ||
| if (sf->token_cache) { | ||
| if (json_copy_string(&auth_token, data, "idToken") == SF_JSON_ERROR_NONE) { |
There was a problem hiding this comment.
Can we have switch here?
| alloc_buffer_and_copy(&sf->oauth_scope, value); | ||
| break; | ||
| case SF_CON_SINGLE_USE_REFRESH_TOKEN: | ||
| alloc_buffer_and_copy(&sf->oauth_scope, value); |
There was a problem hiding this comment.
oauth_scope -> single_use_refresh_token
| *value = sf->oauth_scope; | ||
| break; | ||
| case SF_CON_SINGLE_USE_REFRESH_TOKEN: | ||
| *value = &sf->client_store_temporary_credential; |
There was a problem hiding this comment.
Seems that it's incorrect. Shouldn't it update single_use_refresh_token?
| @@ -0,0 +1,72 @@ | |||
| { | |||
There was a problem hiding this comment.
Rename the file to snowflake_oauth_login_successful.json
| { | ||
| "mappings": [ | ||
| { | ||
| "scenarioName": "Refresh expired access token", |
There was a problem hiding this comment.
Scenario name seems to be detached from the mapping itself. Can we have it renamed? Same for the file, can we use e.g. snowflake_oauth_login_failed_expired_token?
| void startWebBrowser(const std::string& url) override | ||
| { | ||
|
|
||
| CXX_LOG_TRACE("sf::AuthenticationWebBrowserTestRunner::running curl to open a browser::%s", url.c_str()); |
There was a problem hiding this comment.
running curl to open a browser -> running curl instead opening a browser?
Uh oh!
There was an error while loading. Please reload this page.