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-715619: Support authentication with external browser #813

Draft
wants to merge 13 commits into
base: SNOW-715551v2
Choose a base branch
from

Conversation

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

No description provided.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 16.41337% with 275 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (1c9842a) to head (5670642).

Files with missing lines Patch % Lines
cpp/lib/IAuth.cpp 9.50% 257 Missing ⚠️
lib/client.c 18.18% 9 Missing ⚠️
cpp/lib/Authenticator.cpp 86.20% 4 Missing ⚠️
lib/snowflake_util.h 0.00% 4 Missing ⚠️
include/snowflake/IAuth.hpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           SNOW-715551v2     #813      +/-   ##
=================================================
- Coverage          78.39%   77.96%   -0.44%     
=================================================
  Files                107      108       +1     
  Lines               9484     9814     +330     
=================================================
+ Hits                7435     7651     +216     
- Misses              2049     2163     +114     

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

@sfc-gh-ext-simba-jy sfc-gh-ext-simba-jy changed the base branch from master to SNOW-715551v2 February 3, 2025 18:52
@@ -248,6 +248,7 @@ endif ()

if (APPLE)
# macOS
find_library(CORESERVICES_LIBRARY CoreServices)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For static library we should not add extra linking libs here but should be in the link options for test cases:
https://github.com/snowflakedb/libsnowflakeclient/blob/master/tests/CMakeLists.txt#L235
@sfc-gh-ext-simba-jl Just a heads up you might need to adjust build options for dynamic lib when merging your PR with the changes here.
Also CoreServices seems to be a larger one FYI ODBC are using following build options on MacOS maybe we could use the same?

if (APPLE)
    target_link_libraries(snowflakeclient INTERFACE
        "-framework CoreFoundation"
        "-framework SystemConfiguration"
        "-framework ApplicationServices"
        "-framework Security"
        )
endif ()

@@ -122,6 +135,101 @@ namespace IAuth
std::string oneTimeToken;
std::string m_samlResponse;
};

class IAuthWebServer : public AuthErrorHandler
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need IAuthWebServer defined in external header file? Does it include driver specific logic that needs to be overridden on ODBC or any other driver side?
For my understanding in ODBC IAuthWebServer is defined as pure interface without any implementation, for test purpose only so it can be overridden in unit test cases.
If we want to keep test cases on both ODBC and libsfclient side, keep IAuthWebServer as a pure interface would be better. Or if test it on libsfclient side is enough, we could move the definition to internal header files.

@@ -0,0 +1,250 @@
/*
Copy link
Collaborator

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check ODBC test cases and take cases as much as possible to here. https://github.com/snowflakedb/snowflake-odbc/blob/master/Tests/UnitTests/UnitAuthenticatorTest/UnitAuthenticatorTest.cpp
e.g. SF_CON_BROWSER_RESPONSE_TIMEOUT and SF_CON_DISABLE_CONSOLE_LOGIN need to be tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants