Skip to content
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-847342: Add connection property for setting browser timeout value #1824

Merged

Conversation

sfc-gh-ext-simba-jf
Copy link
Collaborator

Overview

SNOW-847342

Pre-review self checklist

  • PR branch is updated with all the changes from master branch
  • The code is correctly formatted (run mvn -P check-style validate)
  • New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
  • The pull request name is prefixed with SNOW-XXXX:
  • Code is in compliance with internal logging requirements

External contributors - please answer these questions before submitting a pull request. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes #NNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency or upgrading an existing one
    • I am adding new public/protected component not marked with @SnowflakeJdbcInternalApi (note that public/protected methods/fields in classes marked with this annotation are already internal)
  3. Please describe how your code solves the related issue.

    Added new connection property to allow setting a time out for external browser authentication.

@sfc-gh-ext-simba-jf sfc-gh-ext-simba-jf requested a review from a team as a code owner July 11, 2024 11:16
@sfc-gh-ext-simba-jf
Copy link
Collaborator Author

sfc-gh-ext-simba-jf commented Jul 24, 2024

Test app:
`import net.snowflake.client.jdbc.SnowflakeSQLLoggedException;

import java.sql.Connection;
import java.sql.DriverManager;
import java.util.Properties;

public class ExternalBrowserTest {

public static String AUTHENTICATOR = "";
public static String ACCOUNTID = "";
public static String WAREHOUSE = "";
public static String DATABASE = "";
public static String SCHEMA = "";
public static String BROWSER_RESPONSE_TIMEOUT = "10";
public static String USER = "";
public static String PWD = "";


public static final void main(String arg[]) throws Throwable {

    String mfaConnectionUrl = "jdbc:snowflake://"+ ACCOUNTID +".snowflakecomputing.com/?CLIENT_REQUEST_MFA_TOKEN=false";

    Properties _connectionProperties = new Properties();
    _connectionProperties.put("user", USER);
    _connectionProperties.put("password", PWD);
    _connectionProperties.put("authenticator", AUTHENTICATOR);
    _connectionProperties.put("warehouse", WAREHOUSE);
    _connectionProperties.put("database", DATABASE);
    _connectionProperties.put("schema", SCHEMA);
    _connectionProperties.put("BROWSER_RESPONSE_TIMEOUT", BROWSER_RESPONSE_TIMEOUT);

    Class.forName("net.snowflake.client.jdbc.SnowflakeDriver");

    try (Connection con = DriverManager.getConnection(mfaConnectionUrl, _connectionProperties)) {
        //When browser window opens, close it and an error should be thrown within the specified time, or by default within 120 seconds.
    } catch (SnowflakeSQLLoggedException e) {
        System.out.println("exception was thrown");
        e.printStackTrace();
    }

}

}`

The test should launch an auth attempt in an external browser. If you then close the browser window you should now see an error being thrown within the browser timeout specified or 120 seconds by default net.snowflake.client.jdbc.SnowflakeSQLLoggedException: JDBC driver encountered communication error. Message: Accept timed out.

Previously the driver would just hang infinitely.

@sfc-gh-dprzybysz
Copy link
Collaborator

@sfc-gh-ext-simba-jf please include the sample as a manual test in JDBC

@sfc-gh-dprzybysz sfc-gh-dprzybysz changed the title Add connection property for setting browser timeout value SNOW-847342: Add connection property for setting browser timeout value Jul 24, 2024
@sfc-gh-ext-simba-jf
Copy link
Collaborator Author

@sfc-gh-ext-simba-jf please include the sample as a manual test in JDBC

I've added a manual test in ConnectionLatestIT. It can be run locally and will be ignored when running the test suite. If there is a better or preferred place to put this test, please let me know.

@sfc-gh-dprzybysz sfc-gh-dprzybysz merged commit fc29e81 into master Jul 31, 2024
139 of 141 checks passed
@sfc-gh-dprzybysz sfc-gh-dprzybysz deleted the Snow-847342-Add-Timeout-for-external-browser-auth branch July 31, 2024 06:38
@github-actions github-actions bot locked and limited conversation to collaborators Jul 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants