From f340d6b95549c19459117c59346e7fdf22a27b3b Mon Sep 17 00:00:00 2001 From: Joshua McHenry Date: Mon, 27 Jul 2026 19:32:36 -0400 Subject: [PATCH 1/3] feat(bq_driver): add AllowedProjects DSN option to scope SQLTables Power Query's Navigator opens a connection and immediately calls SQLTables with wildcard catalog/schema arguments. GetFilteredProjectIds answered that by enumerating every project the authenticated principal can reach via the projects.list REST API, then listing datasets for each one. In a large enterprise organization the principal can typically *see* far more projects than it holds BigQuery permissions on. Listing datasets for such a project fails with a permission error, and because GetResultSetForDatasets surfaces the first error from its parallel fan-out, one inaccessible project fails the whole SQLTables call. The result is that the Navigator tree cannot be built at all, even though the user only wanted the handful of projects they do have access to. Enumeration is also slow and quota-heavy at that scale, but the permission failure is the blocking issue. Add an AllowedProjects DSN option naming the projects to expose. When set, GetFilteredProjectIds filters that list and skips projects.list entirely, so no inaccessible project is ever contacted. The existing filter semantics are preserved: entries are matched against the catalog argument with the same LIKE pattern / SQL_ATTR_METADATA_ID rules used elsewhere. AllowedProjects and AdditionalProjects stay orthogonal and composable: if both are set, enumeration starts from the allowlist and still appends the AdditionalProjects ids on top. Also add a pick list to the Advanced Options dialog so non-technical users need not hand-type project ids. "Load Projects" reuses the existing DriverForm::GetCatalogAndDataset path to populate a checkbox list view, preserving ticks across reloads and keeping saved ids the account no longer reports. The DSN key remains usable headlessly for automation. Tests: unit coverage for FilterAllowedProjects and ALLOWEDPROJECTS parsing, a dialog round-trip test, and an integration test asserting that a project the principal can reach but did not allow-list is absent from SQLTables. --- .../bq_driver/internal/driver_adv_opt_form.cc | 164 +++++++++++++++++- .../bq_driver/internal/driver_adv_opt_form.h | 18 +- .../internal/driver_adv_opt_form_test.cc | 38 ++++ .../odbc/bq_driver/internal/driver_form.cc | 1 + .../bq_driver/internal/odbc_conn_handle.cc | 1 + .../bq_driver/internal/odbc_conn_handle.h | 1 + .../internal/odbc_conn_handle_test.cc | 23 +++ .../bq_driver/internal/odbc_sql_tables.cc | 50 +++++- .../odbc/bq_driver/internal/odbc_sql_tables.h | 18 +- .../internal/odbc_sql_tables_test.cc | 45 +++++ google/cloud/odbc/bq_driver/internal/utils.cc | 24 +++ google/cloud/odbc/bq_driver/internal/utils.h | 5 + .../cloud/odbc/bq_driver/odbc_connection.cc | 1 + .../odbc/bq_driver/odbc_driver_metadata.cc | 6 +- google/cloud/odbc/bq_driver/odbc_windows.cc | 4 + google/cloud/odbc/bq_driver/odbc_windows.h | 1 + .../odbc_driver_tests/catalog_test.cc | 57 ++++++ 17 files changed, 439 insertions(+), 18 deletions(-) diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc index ca0699776c..eb0eec0a32 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc @@ -13,8 +13,10 @@ // limitations under the License. #include "google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h" +#include "google/cloud/odbc/bq_driver/internal/driver_form.h" #include "google/cloud/odbc/bq_driver/internal/odbc_internal_commons.h" #include "google/cloud/odbc/bq_driver/internal/trace_utils.h" +#include #include namespace google::cloud::odbc_bq_driver_internal { @@ -53,6 +55,7 @@ std::string AdvanceOptions::rows_per_block_ = kDefaultRowsPerBlock; std::string AdvanceOptions::default_string_length_ = kDefaultStringLength; std::string AdvanceOptions::session_location_; std::string AdvanceOptions::additional_projects_; +std::string AdvanceOptions::allowed_projects_; std::string AdvanceOptions::query_properties_; std::string AdvanceOptions::use_wchar_; std::string AdvanceOptions::enable_session_; @@ -75,6 +78,7 @@ std::string const kLargeResultsTempTableExpirationTime = "LargeResultsTempTableExpirationTime"; std::string const kSessionLocation = "SessionLocation"; std::string const kAdditionalProjects = "AdditionalProjects"; +std::string const kAllowedProjects = "AllowedProjects"; std::string const kQueryProperties = "QueryProperties"; std::string const kUseWChar = "UseWVarChar"; std::string const kEnableSession = "EnableSession"; @@ -97,7 +101,11 @@ int const kButtonWidth = 68; int const kXAxis = 10; int const kOkButtonX = 330; int const kCancelButtonX = 410; -int const kButtonY = 613; +// The allowed-projects pick list and its "Load Projects" button occupy the +// space below the query-properties box, so the OK/Cancel row sits lower than +// the other control offsets in this file would suggest. +int const kButtonY = 753; +int const kAllowedProjectsListHeight = 100; int const kYAxis = 20; int const kEditBoxWidth = 260; int const kEditBoxHeight = 17; @@ -428,6 +436,25 @@ void AdvanceOptions::CreateAdditionalControls(HFONT h_font) { SetWindowSubclass(GetDlgItem(adv_hwnd, kIdcQueryPropertiesEdit), InputSubclassProc, 0, 0); + HWND h_allowed_projects_label = CreateLabel( + adv_hwnd, "Allowed projects (all accessible if none checked):", kXAxis, + kYAxis + 635, kWidth * 6, kHeight, WS_VISIBLE | SS_LEFT); + SendMessage(h_allowed_projects_label, WM_SETFONT, (WPARAM)h_font, TRUE); + + HWND h_load_projects_button = + CreateButton(adv_hwnd, "Load Projects", kXAxis + 400, kYAxis + 633, + kButtonWidth + 22, kButtonHeight, kIdcLoadProjectsButton); + SendMessage(h_load_projects_button, WM_SETFONT, (WPARAM)h_font, TRUE); + + HWND h_allowed_projects_list_view = + CreateListView(adv_hwnd, kXAxis, kYAxis + 655, kWidth + 445, + kAllowedProjectsListHeight, kIdcAllowedProjectsListView); + SendMessage(h_allowed_projects_list_view, WM_SETFONT, (WPARAM)h_font, TRUE); + // Without contacting the account, the ids saved in the DSN are all we know. + // Show them ticked so OK round-trips the value even if the user never + // presses "Load Projects". + PopulateAllowedProjectsListView(h_allowed_projects_list_view, {}); + // This feature is turned off for the private release. It will be restored for // the public release with an accompanying documentation link. // TODO(b/461668255):Restore BigQuery documentation URL @@ -443,6 +470,77 @@ void AdvanceOptions::CreateAdditionalControls(HFONT h_font) { // SendMessage(h_hyperlink, WM_SETFONT, (WPARAM)h_font, TRUE); } +void AdvanceOptions::PopulateAllowedProjectsListView( + HWND h_list_view, std::vector const& project_ids) { + if (!h_list_view) { + return; + } + + // Ticked ids must survive a reload, whether they came from the saved DSN or + // from ticks the user made before pressing "Load Projects". + std::set checked; + for (auto& project_id : Split(allowed_projects_, ",")) { + Trim(project_id); + if (!project_id.empty()) { + checked.insert(project_id); + } + } + int const existing_count = ListView_GetItemCount(h_list_view); + for (int i = 0; i < existing_count; ++i) { + char buffer[256] = {0}; + ListView_GetItemText(h_list_view, i, 0, buffer, + static_cast(sizeof(buffer))); + if (buffer[0] != '\0' && ListView_GetCheckState(h_list_view, i)) { + checked.insert(buffer); + } + } + + // Keep ticked ids the account no longer reports, so reloading the list never + // silently drops a value the user had already saved. + std::vector rows = project_ids; + std::set const listed(project_ids.begin(), project_ids.end()); + for (auto const& project_id : checked) { + if (listed.find(project_id) == listed.end()) { + rows.push_back(project_id); + } + } + + ListView_DeleteAllItems(h_list_view); + for (size_t i = 0; i < rows.size(); ++i) { + LVITEM item = {}; + item.mask = LVIF_TEXT; + item.iItem = static_cast(i); + item.iSubItem = 0; + item.pszText = const_cast(rows[i].c_str()); + int const index = ListView_InsertItem(h_list_view, &item); + if (index >= 0 && checked.find(rows[i]) != checked.end()) { + ListView_SetCheckState(h_list_view, index, TRUE); + } + } +} + +std::string AdvanceOptions::CollectCheckedProjects(HWND h_list_view) { + // Without the control there is nothing to read; keep the value already held + // rather than clearing it. + if (!h_list_view) { + return allowed_projects_; + } + std::vector checked; + int const count = ListView_GetItemCount(h_list_view); + for (int i = 0; i < count; ++i) { + if (!ListView_GetCheckState(h_list_view, i)) { + continue; + } + char buffer[256] = {0}; + ListView_GetItemText(h_list_view, i, 0, buffer, + static_cast(sizeof(buffer))); + if (buffer[0] != '\0') { + checked.push_back(buffer); + } + } + return Join(checked, ","); +} + void AdvanceOptions::CreateButtons(HFONT h_font) { HWND h_ok_button = CreateButton(adv_hwnd, "OK", kOkButtonX + 12, kButtonY + 44, kButtonWidth, @@ -639,6 +737,9 @@ LRESULT CALLBACK AdvanceOptions::AdvanceOptProc(HWND hwnd, UINT u_msg, sizeof(additional_projects_buffer)); additional_projects_ = additional_projects_buffer; + allowed_projects_ = CollectCheckedProjects( + GetDlgItem(hwnd, kIdcAllowedProjectsListView)); + HWND h_query_properties_edit = GetDlgItem(hwnd, kIdcQueryPropertiesEdit); char query_properties_buffer[1024] = {0}; @@ -770,6 +871,59 @@ LRESULT CALLBACK AdvanceOptions::AdvanceOptProc(HWND hwnd, UINT u_msg, break; } + case kIdcLoadProjectsButton: { + if (HIWORD(w_param) != BN_CLICKED) { + break; + } + // The credentials live on the parent DSN dialog; this dialog holds no + // copy of them. + HWND h_parent = GetParent(hwnd); + char key_file_buffer[1024] = {0}; + char auth_buffer[256] = {0}; + if (h_parent) { + GetWindowText(GetDlgItem(h_parent, kIdcKeyfileEdit), + key_file_buffer, sizeof(key_file_buffer)); + GetWindowText(GetDlgItem(h_parent, kIdcAuthBox), auth_buffer, + sizeof(auth_buffer)); + } + if (auth_buffer[0] == '\0') { + ShowErrorWindow(hwnd, + "Select an OAuth mechanism on the main dialog " + "before loading projects."); + break; + } + bool const is_adc = + (strcmp(auth_buffer, "Application Default Credentials") == 0); + if (!is_adc && key_file_buffer[0] == '\0') { + ShowErrorWindow(hwnd, + "Enter a key file path on the main dialog before " + "loading projects."); + break; + } + + auto projects_or = DriverForm::GetCatalogAndDataset( + "Catalog", is_adc ? "" : key_file_buffer, auth_buffer, ""); + if (!projects_or.Ok()) { + LOG(ERROR) << "AdvanceOptions::AdvanceOptProc::GetCatalogAndDataset" + ":: " + << projects_or.GetStatusRecord().message; + MessageBox(hwnd, projects_or.GetStatusRecord().message.c_str(), + "Error", MB_OK | MB_ICONERROR); + break; + } + + std::vector project_ids; + for (auto& project_id : Split(projects_or.GetValue(), ";")) { + Trim(project_id); + if (!project_id.empty()) { + project_ids.push_back(std::move(project_id)); + } + } + PopulateAllowedProjectsListView( + GetDlgItem(hwnd, kIdcAllowedProjectsListView), project_ids); + break; + } + case kIdcCancelButton: DestroyWindow(hwnd); // Close the window break; @@ -831,6 +985,7 @@ void AdvanceOptions::SetValues(Section const& attribute_map) { std::to_string(kDefaultMaxRetries)); session_location_ = GetValueOrDefault(attribute_map, kSessionLocation); additional_projects_ = GetValueOrDefault(attribute_map, kAdditionalProjects); + allowed_projects_ = GetValueOrDefault(attribute_map, kAllowedProjects); query_properties_ = GetValueOrDefault(attribute_map, kQueryProperties); // TODO(b/497725655): Enable UI feature after public release // use_wchar_ = GetValueOrDefault(attribute_map, kUseWChar); @@ -858,6 +1013,7 @@ void AdvanceOptions::ResetToDefaults() { max_retries_ = std::to_string(kDefaultMaxRetries); session_location_.clear(); additional_projects_.clear(); + allowed_projects_.clear(); query_properties_.clear(); // use_wchar_.clear(); enable_session_.clear(); @@ -884,13 +1040,15 @@ void AdvanceOptions::Show(HWND hwnd) { (HBRUSH)(COLOR_WINDOW + 1); // Sets background to white INITCOMMONCONTROLSEX icc; icc.dwSize = sizeof(INITCOMMONCONTROLSEX); - icc.dwICC = ICC_STANDARD_CLASSES; + // ICC_LISTVIEW_CLASSES registers WC_LISTVIEW, which the allowed-projects pick + // list needs; the other controls on this dialog are standard classes. + icc.dwICC = ICC_STANDARD_CLASSES | ICC_LISTVIEW_CLASSES; InitCommonControlsEx(&icc); RegisterClass(&wc_adv); int window_width = 525; - int window_height = 720; + int window_height = 860; int screen_width = GetSystemMetrics(SM_CXSCREEN); int screen_height = GetSystemMetrics(SM_CYSCREEN); int x_pos = (screen_width - window_width) / 2; diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h index 1d0d377ea4..842a5c0518 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h @@ -20,7 +20,7 @@ #pragma comment(lib, "Comctl32.lib") // Link with Comctl32.lib namespace google::cloud::odbc_bq_driver_internal { -// NEXTID:153 +// NEXTID:155 static int const kIdcUseDefaultCheckbox = 128; static int const kIdcDatasetNameEdit = 129; static int const kIdcTempExpirationEdit = 130; @@ -45,6 +45,8 @@ static int const kIdcMaxRetriesEdit = 149; static int const kIdcEnablePscGcdCheckbox = 150; static int const kIdcPrivateServiceNameEdit = 151; static int const kIdcUniverseDomainEdit = 152; +static int const kIdcAllowedProjectsListView = 153; +static int const kIdcLoadProjectsButton = 154; class AdvanceOptions { public: @@ -75,6 +77,9 @@ class AdvanceOptions { inline std::string const& GetAdditionalProjects() const { return additional_projects_; } + inline std::string const& GetAllowedProjects() const { + return allowed_projects_; + } inline std::string const& GetQueryProperties() const { return query_properties_; } @@ -124,6 +129,7 @@ class AdvanceOptions { static std::string default_string_length_; static std::string session_location_; static std::string additional_projects_; + static std::string allowed_projects_; static std::string query_properties_; static std::string use_wchar_; static std::string enable_session_; @@ -137,6 +143,16 @@ class AdvanceOptions { static std::string enable_gcd_; static std::string universe_domain_; + // Fill the allowed-projects pick list with 'project_ids', keeping ticked any + // id that is currently ticked as well as any id already in + // 'allowed_projects_'. Used both when the dialog opens and when the user + // reloads the list from the account. + static void PopulateAllowedProjectsListView( + HWND h_list_view, std::vector const& project_ids); + + // Comma-join the ticked rows of the allowed-projects pick list. + static std::string CollectCheckedProjects(HWND h_list_view); + static LRESULT CALLBACK AdvanceOptProc(HWND hwnd, UINT uMsg, WPARAM w_param, LPARAM l_param); static char const CLASS_NAME[]; diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form_test.cc b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form_test.cc index 7af395755b..4dc9916569 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form_test.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form_test.cc @@ -72,6 +72,31 @@ TEST_F(AdvanceOptionsTest, ShowWindow) { << "Window should be visible after calling Show."; } +TEST_F(AdvanceOptionsTest, AllowedProjectsPickListRoundTripsThroughOk) { + // Opening the dialog with a saved value should tick those ids without + // contacting the account, and OK should save back exactly what is ticked. + advance_options->SetValues({{"AllowedProjects", "project-a,project-b"}}); + + // Show() runs a modal GetMessage loop. Queue WM_QUIT first so it builds the + // controls and returns immediately instead of blocking this test; the window + // itself stays alive for the assertions below. + PostQuitMessage(0); + advance_options->Show(nullptr); + HWND hwnd = advance_options->GetHwnd(); + ASSERT_NE(hwnd, nullptr) << "Window should be created and displayed."; + + HWND h_list = GetDlgItem(hwnd, kIdcAllowedProjectsListView); + ASSERT_NE(h_list, nullptr) << "Allowed projects list should be created."; + ASSERT_EQ(ListView_GetItemCount(h_list), 2); + EXPECT_TRUE(ListView_GetCheckState(h_list, 0)); + EXPECT_TRUE(ListView_GetCheckState(h_list, 1)); + + ListView_SetCheckState(h_list, 1, FALSE); + ClickButton(hwnd, kIdcOKButton); + + EXPECT_EQ(advance_options->GetAllowedProjects(), "project-a"); +} + TEST_F(AdvanceOptionsTest, SetValuesValidinput) { Section attribute_map = {{"SQLDialect", "1"}, {"LargeResultsDatasetId", "dataset1"}, @@ -81,6 +106,7 @@ TEST_F(AdvanceOptionsTest, SetValuesValidinput) { {"LargeResultsTempTableExpirationTime", "3600000"}, {"SessionLocation", "USA"}, {"AdditionalProjects", "projectA,projectB"}, + {"AllowedProjects", "projectC,projectD"}, {"QueryProperties", "property1=value1"}, {"MaxThreads", "10"}, {"MaxRetries", "9"}}; @@ -96,6 +122,7 @@ TEST_F(AdvanceOptionsTest, SetValuesValidinput) { EXPECT_EQ(options.GetTempTableExpiration(), "3600000"); EXPECT_EQ(options.GetSessionLocation(), "USA"); EXPECT_EQ(options.GetAdditionalProjects(), "projectA,projectB"); + EXPECT_EQ(options.GetAllowedProjects(), "projectC,projectD"); EXPECT_EQ(options.GetQueryProperties(), "property1=value1"); EXPECT_EQ(options.GetMaxThreads(), "10"); EXPECT_EQ(options.GetMaxRetries(), "9"); @@ -118,8 +145,19 @@ TEST_F(AdvanceOptionsTest, SetValuesMissingkeys) { EXPECT_EQ(options.GetTempTableExpiration(), "3600000"); EXPECT_EQ(options.GetSessionLocation(), ""); EXPECT_EQ(options.GetAdditionalProjects(), ""); + EXPECT_EQ(options.GetAllowedProjects(), ""); EXPECT_EQ(options.GetQueryProperties(), ""); EXPECT_EQ(options.GetAllowHtapiForLargeResultsCheckbox(), ""); } +TEST_F(AdvanceOptionsTest, ResetToDefaultsClearsAllowedProjects) { + AdvanceOptions options; + options.SetValues({{"AllowedProjects", "projectC,projectD"}}); + ASSERT_EQ(options.GetAllowedProjects(), "projectC,projectD"); + + options.ResetToDefaults(); + + EXPECT_EQ(options.GetAllowedProjects(), ""); +} + } // namespace google::cloud::odbc_bq_driver_internal diff --git a/google/cloud/odbc/bq_driver/internal/driver_form.cc b/google/cloud/odbc/bq_driver/internal/driver_form.cc index 5b190eb23f..930c179326 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_form.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_form.cc @@ -223,6 +223,7 @@ static Section BuildTestConnectionAttributes( attributes_map["SessionLocation"] = adv_form.GetSessionLocation(); attributes_map["MaxThreads"] = adv_form.GetMaxThreads(); attributes_map["AdditionalProjects"] = adv_form.GetAdditionalProjects(); + attributes_map["AllowedProjects"] = adv_form.GetAllowedProjects(); attributes_map["QueryProperties"] = adv_form.GetQueryProperties(); attributes_map["UseWVarChar"] = adv_form.GetUseWchar(); attributes_map["EnableSession"] = adv_form.GetEnableSession(); diff --git a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.cc b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.cc index a4071bb1ec..f1d26a005b 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.cc @@ -153,6 +153,7 @@ void ConnectionHandle::SetUp(Section& dsn_section, dsn_.kms_key_name = dsn_section["KMSKEYNAME"]; dsn_.session_location = dsn_section["SESSIONLOCATION"]; dsn_.additional_projects = dsn_section["ADDITIONALPROJECTS"]; + dsn_.allowed_projects = dsn_section["ALLOWEDPROJECTS"]; dsn_.psc = dsn_section["PRIVATESERVICECONNECTURIS"]; dsn_.enable_gcd = dsn_section["ENABLEGCD"] == "1" || dsn_section["ENABLEGCD"] == "true"; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.h b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.h index 566f0e2027..b08c38eb91 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.h +++ b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle.h @@ -103,6 +103,7 @@ struct Dsn { // Proxy options fields google::cloud::odbc_bigquery_client_interface::ProxyOptions proxy_options; std::string additional_projects; + std::string allowed_projects; std::string psc; bool enable_gcd; std::string universe_domain; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle_test.cc b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle_test.cc index 3eada005c0..89a6eec24e 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_conn_handle_test.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_conn_handle_test.cc @@ -355,6 +355,29 @@ TEST(ConnectionHandle, DsnSetupListProjectsParentSet) { EXPECT_EQ(actual.list_projects_parent, kDsnListProjectsParent); } +TEST(ConnectionHandle, DsnSetupAllowedProjectsNotSet) { + ConnectionHandle conn_handle; + Section dsn_section; + + conn_handle.SetUp(dsn_section, kDsnName); + + Dsn actual = conn_handle.GetDsn(); + EXPECT_TRUE(actual.allowed_projects.empty()); +} + +TEST(ConnectionHandle, DsnSetupAllowedProjectsSet) { + ConnectionHandle conn_handle; + Section dsn_section; + + dsn_section["ALLOWEDPROJECTS"] = "project-1,project-2"; + conn_handle.SetUp(dsn_section, kDsnName); + + Dsn actual = conn_handle.GetDsn(); + EXPECT_EQ(actual.allowed_projects, "project-1,project-2"); + // AdditionalProjects and AllowedProjects are independent settings. + EXPECT_TRUE(actual.additional_projects.empty()); +} + TEST(ConnectionHandle, SetAttributeSuccessSQLUInt) { ConnectionHandle conn_handle; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 88e7f6db84..4e2b9241d0 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -79,9 +79,38 @@ StatusRecord ValidateInputParameters( return StatusRecord::Ok(); } +std::vector FilterAllowedProjects( + std::string const& allowed_projects, std::string const& projects_filter, + SQLULEN metadata_id) { + std::vector project_ids; + auto filter_regex = BuildRegex(projects_filter, metadata_id); + std::vector allowed_ids = Split(allowed_projects, ","); + for (auto& project_id : allowed_ids) { + Trim(project_id); + if (project_id.empty()) { + continue; + } + if ((!metadata_id && projects_filter == kMatchAll) || + re2::RE2::FullMatch(project_id, *filter_regex)) { + project_ids.push_back(std::move(project_id)); + } + } + return project_ids; +} + StatusRecordOr> GetFilteredProjectIds( ODBCBQClient& bq_client, std::string const& projects_filter, - SQLULEN metadata_id) { + SQLULEN metadata_id, std::string const& allowed_projects) { + // An explicit allowlist replaces REST-based project discovery entirely. + // projects.list enumerates every project the authenticated principal can + // reach, which is slow and quota-heavy for accounts with access to many + // projects; when the user has named the projects they care about there is + // nothing to discover. + if (!allowed_projects.empty()) { + return FilterAllowedProjects(allowed_projects, projects_filter, + metadata_id); + } + std::vector project_ids; auto filter_regex = BuildRegex(projects_filter, metadata_id); // For now, we use default options. @@ -346,9 +375,10 @@ ResultSet ProcessStringResults( StatusRecordOr GetResultSetForProjects( ODBCBQClient& bq_client, SQLULEN metadata_id, - std::string const& additional_projects) { - auto project_ids_status = - GetFilteredProjectIds(bq_client, kMatchAll, metadata_id); + std::string const& additional_projects, + std::string const& allowed_projects) { + auto project_ids_status = GetFilteredProjectIds( + bq_client, kMatchAll, metadata_id, allowed_projects); if (!project_ids_status) { LOG(ERROR) << "GetResultSetForProjects::GetFilteredProjectIds:: " << project_ids_status.GetStatusRecord().message; @@ -366,9 +396,10 @@ StatusRecordOr GetResultSetForProjects( StatusRecordOr GetResultSetForDatasets( ODBCBQClient& bq_client, SQLULEN metadata_id, - std::string const& catalog_name, std::string const& additional_projects) { - auto project_ids_status = - GetFilteredProjectIds(bq_client, catalog_name, metadata_id); + std::string const& catalog_name, std::string const& additional_projects, + std::string const& allowed_projects) { + auto project_ids_status = GetFilteredProjectIds( + bq_client, catalog_name, metadata_id, allowed_projects); if (!project_ids_status) { LOG(ERROR) << "GetResultSetForDatasets::GetFilteredProjectIds:: " << project_ids_status.GetStatusRecord().message; @@ -416,6 +447,7 @@ StatusRecordOr GetResultSetForTables( std::string const& project_filter, std::string const& dataset_filter, std::string const& table_filter, std::string const& table_type_filter, SQLULEN metadata_id) { + ConnectionHandle& conn_handle = *(stmt_handle.GetConnectionHandle()); // When SQL_ATTR_METADATA_ID is true, the catalog argument is an exact project // identifier (not a pattern). Skip enumerating every accessible project via // projects.list and use the name directly. @@ -424,7 +456,8 @@ StatusRecordOr GetResultSetForTables( project_list = {project_filter}; } else { auto projects_status_record_or = - GetFilteredProjectIds(bq_client, project_filter, metadata_id); + GetFilteredProjectIds(bq_client, project_filter, metadata_id, + conn_handle.GetDsn().allowed_projects); if (!projects_status_record_or) { LOG(ERROR) << "GetResultSetForTables::GetFilteredProjectIds:: " << projects_status_record_or.GetStatusRecord().message; @@ -432,7 +465,6 @@ StatusRecordOr GetResultSetForTables( } project_list = *projects_status_record_or; } - ConnectionHandle& conn_handle = *(stmt_handle.GetConnectionHandle()); // Append additional projects if any project_list = AppendAdditionalProjectsIfMissing( std::move(project_list), conn_handle.GetDsn().additional_projects); diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h index ae0a45e756..361217ab96 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h @@ -54,12 +54,22 @@ odbc_internal::StatusRecord ValidateInputParameters( const SQLCHAR* table_name, SQLSMALLINT table_name_len, SQLSMALLINT table_type_len, SQLULEN metadata_id); +// Return the project ids from the comma-separated 'allowed_projects' allowlist +// that match 'projects_filter', using the same LIKE-pattern / +// SQL_ATTR_METADATA_ID semantics as GetFilteredProjectIds. Blank entries are +// ignored and surrounding whitespace is trimmed. Makes no REST calls. +std::vector FilterAllowedProjects( + std::string const& allowed_projects, std::string const& projects_filter, + SQLULEN metadata_id); + // Return a list of project ids depending on SQL_ATTR_METADATA_ID and // 'projects_filter'. Returns all project ids if SQL_ATTR_METADATA_ID == // SQL_FALSE and projects_filter == "%". +// When 'allowed_projects' is non-empty it is used as the set of candidate +// projects and projects.list is not called at all. odbc_internal::StatusRecordOr> GetFilteredProjectIds( ODBCBQClient& bq_client, std::string const& projects_filter, - SQLULEN metadata_id); + SQLULEN metadata_id, std::string const& allowed_projects = ""); // Return a list of dataset ids depending on SQL_ATTR_METADATA_ID and // 'datasets_filter'. Returns all dataset ids if SQL_ATTR_METADATA_ID == @@ -108,13 +118,15 @@ ResultSet ProcessStringResults( // Search for all projects and populate ResultSet for it. odbc_internal::StatusRecordOr GetResultSetForProjects( ODBCBQClient& bq_client, SQLULEN metadata_id, - std::string const& additional_projects = ""); + std::string const& additional_projects = "", + std::string const& allowed_projects = ""); // Search for all datasets in all projects and populate ResultSet for it. odbc_internal::StatusRecordOr GetResultSetForDatasets( ODBCBQClient& bq_client, SQLULEN metadata_id, std::string const& catalog_name = kMatchAll, - std::string const& additional_projects = ""); + std::string const& additional_projects = "", + std::string const& allowed_projects = ""); // Search for tables and populate ResultSet according to ODBC spec odbc_internal::StatusRecordOr GetResultSetForTables( diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc index d29c987c3c..6f09eaf419 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc @@ -23,7 +23,9 @@ using ::google::cloud::bigquery_v2_minimal_internal::QueryParameter; using google::cloud::odbc_internal::SQLStates; using google::cloud::odbc_internal::StatusRecord; using google::cloud::odbc_testing_bq_driver_utils::CastToSQLCHAR; +using ::testing::ElementsAre; using ::testing::HasSubstr; +using ::testing::IsEmpty; TEST(ValidateInputParameters, Success) { StatusRecord status = ValidateInputParameters( @@ -189,6 +191,49 @@ TEST(ConstructQuery, ConstructWithTwoClausesEmptystrings) { EXPECT_EQ(2, named_query_params.size()); } +TEST(FilterAllowedProjects, WildcardReturnsWholeAllowlistTrimmed) { + EXPECT_THAT(FilterAllowedProjects(" project-1 ,project-2,\tproject-3 ", "%", + SQL_FALSE), + ElementsAre("project-1", "project-2", "project-3")); +} + +TEST(FilterAllowedProjects, EmptyAllowlistReturnsNothing) { + EXPECT_THAT(FilterAllowedProjects("", "%", SQL_FALSE), IsEmpty()); +} + +TEST(FilterAllowedProjects, BlankEntriesAreIgnored) { + EXPECT_THAT(FilterAllowedProjects(" , project-1 ,, ,", "%", SQL_FALSE), + ElementsAre("project-1")); +} + +TEST(FilterAllowedProjects, LikePatternFiltersAllowlist) { + EXPECT_THAT(FilterAllowedProjects("prod-a,prod-b,dev-c", "prod%", SQL_FALSE), + ElementsAre("prod-a", "prod-b")); +} + +TEST(FilterAllowedProjects, UnderscoreMatchesSingleCharacter) { + EXPECT_THAT(FilterAllowedProjects("pa,pb,pcc", "p_", SQL_FALSE), + ElementsAre("pa", "pb")); +} + +TEST(FilterAllowedProjects, NonMatchingPatternReturnsNothing) { + EXPECT_THAT(FilterAllowedProjects("prod-a,prod-b", "staging%", SQL_FALSE), + IsEmpty()); +} + +TEST(FilterAllowedProjects, MetadataIdMatchesExactIdCaseInsensitively) { + EXPECT_THAT( + FilterAllowedProjects("Project-1,project-2", "PROJECT-1", SQL_TRUE), + ElementsAre("Project-1")); +} + +TEST(FilterAllowedProjects, MetadataIdTreatsWildcardAsLiteral) { + // With SQL_ATTR_METADATA_ID set the catalog argument is an identifier, not a + // pattern, so "%" matches only a project literally named "%". + EXPECT_THAT(FilterAllowedProjects("project-1,project-2", "%", SQL_TRUE), + IsEmpty()); +} + TEST(CreateResultSetForProjects, CreateResultSetForProjects) { std::vector project_ids = {"id-1", "id-2"}; diff --git a/google/cloud/odbc/bq_driver/internal/utils.cc b/google/cloud/odbc/bq_driver/internal/utils.cc index b9fb984502..c539151c93 100644 --- a/google/cloud/odbc/bq_driver/internal/utils.cc +++ b/google/cloud/odbc/bq_driver/internal/utils.cc @@ -26,6 +26,7 @@ #include #include #ifdef _WIN32 +#include // Required for WC_LISTVIEW and SetWindowSubclass #include #include // Required for SetWindowTheme #pragma comment(lib, "UxTheme.lib") // Link UxTheme.lib @@ -361,6 +362,29 @@ HWND CreateScrollableEditBox(HWND parent, int x, int y, int width, int height, return hwndEdit; } +HWND CreateListView(HWND parent, int x, int y, int width, int height, int id) { + HWND hwndListView = CreateWindowEx( + 0, WC_LISTVIEW, "", + WS_TABSTOP | WS_VISIBLE | WS_CHILD | WS_BORDER | LVS_REPORT | + LVS_NOCOLUMNHEADER | LVS_SINGLESEL, + x, y, width, height, parent, (HMENU)id, g_hDllInstance, NULL); + if (!hwndListView) { + return hwndListView; + } + ListView_SetExtendedListViewStyle(hwndListView, LVS_EX_CHECKBOXES); + + // A report-mode list view renders nothing until it has at least one column. + // One column spanning the control is all a pick list needs; leave room for + // the vertical scrollbar so long values are not hidden behind it. + LVCOLUMN column = {}; + column.mask = LVCF_WIDTH | LVCF_SUBITEM; + column.iSubItem = 0; + column.cx = width - GetSystemMetrics(SM_CXVSCROLL) - 4; + ListView_InsertColumn(hwndListView, 0, &column); + + return hwndListView; +} + // Helper function to create a combo box (dropdown) HWND CreateComboBox(HWND parent, int x, int y, int width, int height, int id) { HWND hwndCombo = CreateWindowEx( diff --git a/google/cloud/odbc/bq_driver/internal/utils.h b/google/cloud/odbc/bq_driver/internal/utils.h index a557ce6725..7e1dd502d4 100644 --- a/google/cloud/odbc/bq_driver/internal/utils.h +++ b/google/cloud/odbc/bq_driver/internal/utils.h @@ -284,6 +284,11 @@ HWND CreateCheckBox(HWND parent, char const* text, int x, int y, int width, HWND CreateScrollableEditBox(HWND parent, int x, int y, int width, int height, int id); +// Creates a single-column, header-less list view whose rows carry checkboxes, +// for picking several values out of a list. Unlike the other controls here, +// WC_LISTVIEW requires comctl32 to have been initialized with +// ICC_LISTVIEW_CLASSES before this is called. +HWND CreateListView(HWND parent, int x, int y, int width, int height, int id); HWND CreateGroupBox(HWND parent, char const* text, int x, int y, int width, int height, int id); HWND CreateNumericEditBox(HWND parent, char const* text, int x, int y, diff --git a/google/cloud/odbc/bq_driver/odbc_connection.cc b/google/cloud/odbc/bq_driver/odbc_connection.cc index 96646b9efa..d0eeb04935 100644 --- a/google/cloud/odbc/bq_driver/odbc_connection.cc +++ b/google/cloud/odbc/bq_driver/odbc_connection.cc @@ -210,6 +210,7 @@ SQLRETURN HandleDriverPrompt(std::string& conn_string, SQLHWND window_handle, {"SESSIONLOCATION", form.GetAdvanceOptions()->GetSessionLocation()}, {"ENABLESESSION", form.GetAdvanceOptions()->GetEnableSession()}, {"ADDITIONALPROJECTS", form.GetAdvanceOptions()->GetAdditionalProjects()}, + {"ALLOWEDPROJECTS", form.GetAdvanceOptions()->GetAllowedProjects()}, }; handle_ref->SetUp(dsn_section, form.GetDSN()); diff --git a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc index 5d17ce2085..112af2d21d 100644 --- a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc +++ b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc @@ -479,12 +479,14 @@ SQLRETURN SQLTablesInternal(SQLHSTMT stmt_handle, SQLCHAR* catalog_name, if (!metadata_id && project_filter == SQL_ALL_CATALOGS && dataset_filter.empty() && table_filter.empty()) { result_set_status = GetResultSetForProjects( - bq_client, metadata_id, conn_handle.GetDsn().additional_projects); + bq_client, metadata_id, conn_handle.GetDsn().additional_projects, + conn_handle.GetDsn().allowed_projects); } else if (!metadata_id && project_filter.empty() && dataset_filter == SQL_ALL_SCHEMAS && table_filter.empty()) { result_set_status = GetResultSetForDatasets(bq_client, metadata_id, kMatchAll, - conn_handle.GetDsn().additional_projects); + conn_handle.GetDsn().additional_projects, + conn_handle.GetDsn().allowed_projects); } else if (!metadata_id && project_filter.empty() && dataset_filter.empty() && table_filter.empty() && table_type_filter == SQL_ALL_TABLE_TYPES) { result_set_status = CreateResultSetForTableTypes(); diff --git a/google/cloud/odbc/bq_driver/odbc_windows.cc b/google/cloud/odbc/bq_driver/odbc_windows.cc index 03bc6ed784..093efe6658 100644 --- a/google/cloud/odbc/bq_driver/odbc_windows.cc +++ b/google/cloud/odbc/bq_driver/odbc_windows.cc @@ -145,6 +145,8 @@ bool ConfigDSNInternal(HWND hwnd_parent, WORD f_request, LPCSTR lpsz_driver, GetValueOrDefault(section, max_retries_key, kDefaultMaxRetries); std::string additional_projects = GetValueOrDefault(section, additional_projects_key); + std::string allowed_projects = + GetValueOrDefault(section, allowed_projects_key); std::string query_properties = GetValueOrDefault(section, query_properties_key); // std::string use_wchar = GetValueOrDefault(section, use_wchar_key); @@ -194,6 +196,7 @@ bool ConfigDSNInternal(HWND hwnd_parent, WORD f_request, LPCSTR lpsz_driver, {temp_expiration_key, temp_expiration}, {session_location_key, session_location}, {additional_projects_key, additional_projects}, + {allowed_projects_key, allowed_projects}, {query_properties_key, query_properties}, // {use_wchar_key, use_wchar}, {enable_session_key, enable_session}, @@ -257,6 +260,7 @@ bool ConfigDSNInternal(HWND hwnd_parent, WORD f_request, LPCSTR lpsz_driver, temp_expiration = advance_form.GetTempTableExpiration(); session_location = advance_form.GetSessionLocation(); additional_projects = advance_form.GetAdditionalProjects(); + allowed_projects = advance_form.GetAllowedProjects(); query_properties = advance_form.GetQueryProperties(); // use_wchar = advance_form.GetUseWchar(); enable_session = advance_form.GetEnableSession(); diff --git a/google/cloud/odbc/bq_driver/odbc_windows.h b/google/cloud/odbc/bq_driver/odbc_windows.h index 16a565904c..38db42bf47 100644 --- a/google/cloud/odbc/bq_driver/odbc_windows.h +++ b/google/cloud/odbc/bq_driver/odbc_windows.h @@ -44,6 +44,7 @@ std::string const default_string_length_key = "DefaultStringColumnLength"; std::string const temp_expiration_key = "LargeResultsTempTableExpirationTime"; std::string const session_location_key = "SessionLocation"; std::string const additional_projects_key = "AdditionalProjects"; +std::string const allowed_projects_key = "AllowedProjects"; std::string const query_properties_key = "QueryProperties"; std::string const max_threads_key = "MaxThreads"; std::string const max_retries_key = "MaxRetries"; diff --git a/google/cloud/odbc/integration_tests/odbc_driver_tests/catalog_test.cc b/google/cloud/odbc/integration_tests/odbc_driver_tests/catalog_test.cc index 3a32ffe9e3..9e0eb1cdb0 100644 --- a/google/cloud/odbc/integration_tests/odbc_driver_tests/catalog_test.cc +++ b/google/cloud/odbc/integration_tests/odbc_driver_tests/catalog_test.cc @@ -422,6 +422,63 @@ TEST(CatalogTest, SQLTables_AllProjects) { EXPECT_EQ(Disconnect(conn), SQL_SUCCESS); } +TEST(CatalogTest, SQLTables_AllowedProjects) { + // First learn every project the test principal can reach, so the allow-listed + // run below can be compared against it. + std::set all_catalogs; + { + auto conn = std::make_shared(); + EXPECT_EQ(Connect(kDefaultConnectionString, conn), SQL_SUCCESS); + auto status = SQLSetStmtAttr(conn->hstmt, SQL_ATTR_METADATA_ID, + (SQLPOINTER)SQL_FALSE, 0); + CheckError(status, "SQLSetStmtAttr", conn); + for (auto const& result : + Catalog::GetTables(conn, SQL_ALL_CATALOGS, "", "")) { + if (result.project_name.has_value()) { + all_catalogs.insert(result.project_name.value()); + } + } + EXPECT_EQ(Disconnect(conn), SQL_SUCCESS); + } + ASSERT_TRUE(all_catalogs.find(kCatalogName) != all_catalogs.end()) + << "Default project/catalog not found without an allowlist."; + if (all_catalogs.size() < 2) { + GTEST_LOG_(WARNING) + << "Test principal can reach only " << all_catalogs.size() + << " project(s), so this run does not exercise exclusion."; + } + + auto conn = std::make_shared(); + std::string connection_string = + kDefaultConnectionString + ";AllowedProjects=" + kCatalogName; + EXPECT_EQ(Connect(connection_string, conn), SQL_SUCCESS); + + auto status = SQLSetStmtAttr(conn->hstmt, SQL_ATTR_METADATA_ID, + (SQLPOINTER)SQL_FALSE, 0); + CheckError(status, "SQLSetStmtAttr", conn); + + std::vector results = + Catalog::GetTables(conn, SQL_ALL_CATALOGS, "", ""); + + std::set catalogs; + for (auto const& result : results) { + if (result.project_name.has_value()) { + catalogs.insert(result.project_name.value()); + } + + EXPECT_FALSE(result.dataset_name.has_value()); + EXPECT_FALSE(result.table_name.has_value()); + EXPECT_FALSE(result.table_type.has_value()); + EXPECT_FALSE(result.description.has_value()); + } + + // Exactly the allowlist: any other project the principal can reach must be + // absent, which is only possible if projects.list was not consulted. + EXPECT_EQ(catalogs, std::set{kCatalogName}); + + EXPECT_EQ(Disconnect(conn), SQL_SUCCESS); +} + TEST(CatalogTest, SQLTables_AllDatasets) { auto conn = std::make_shared(); EXPECT_EQ(Connect(kDefaultConnectionString, conn), SQL_SUCCESS); From 704eeb0d3be00c9e2d67c2f0b2d88572fcd7018b Mon Sep 17 00:00:00 2001 From: Joshua McHenry Date: Tue, 28 Jul 2026 10:08:29 -0400 Subject: [PATCH 2/3] fix(bq_driver): keep the Advanced Options dialog within the screen The Advanced Options dialog asked for a fixed 720x525 window and centred it on the raw screen metrics. On a display whose usable height is smaller than that -- a 1280x720 panel at 150% scaling leaves 672px of work area -- the lower part of the dialog, including the OK and Cancel buttons, fell below the screen edge with no way to reach it. Adding the allowed-projects pick list made this worse by growing the layout, but the dialog already did not fit such a display before that. Clamp the window height to the desktop work area and give the window a vertical scrollbar, so a layout taller than the available space stays fully reachable. Scrolling moves the child controls via ScrollWindow, which keeps the existing absolute control positions untouched. The scroll range is derived from the laid-out content height and refreshed on WM_SIZE; the scrollbar, mouse wheel, page keys and thumb dragging are all handled. Widen the frame by the scrollbar width. The scrollbar is carved out of the client area, so without this every right-aligned control loses those pixels and the edit boxes are clipped. Accumulate mouse wheel movement rather than dividing each message by WHEEL_DELTA. High-resolution wheels and precision trackpads report fractions of a notch, which integer division rounded away to nothing, so those devices could not scroll the dialog at all. Also honour the system SPI_GETWHEELSCROLLLINES setting instead of assuming three lines. Also centre the window inside the work area rather than the full screen, so it is not positioned partly behind the taskbar. --- .../bq_driver/internal/driver_adv_opt_form.cc | 197 ++++++++++++++++-- .../bq_driver/internal/driver_adv_opt_form.h | 17 ++ 2 files changed, 198 insertions(+), 16 deletions(-) diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc index eb0eec0a32..874331205c 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc @@ -68,6 +68,8 @@ std::string AdvanceOptions::max_retries_ = std::to_string(kDefaultMaxRetries); std::string AdvanceOptions::private_service_connect_uris_; std::string AdvanceOptions::enable_gcd_; std::string AdvanceOptions::universe_domain_; +int AdvanceOptions::scroll_pos_ = 0; +int AdvanceOptions::wheel_remainder_ = 0; std::string const kLanguageDialect = "SQLDialect"; std::string const kLargeResultsDatasetId = "LargeResultsDatasetId"; @@ -112,6 +114,14 @@ int const kEditBoxHeight = 17; int const kinputComboBoxXAxis = 237; int const KComboBoxHeight = 100; +// Full height of the laid-out controls. The window is clamped to the desktop +// work area, so on a short or DPI-scaled display this is larger than the client +// area and the difference is what scrolls. +int const kContentHeight = kButtonY + 44 + kButtonHeight + 10; + +// Pixels scrolled per scrollbar arrow click. +int const kScrollLine = 20; + HWND AdvanceOptions::GetHwnd() const { return adv_hwnd; } AdvanceOptions::AdvanceOptions() : adv_hwnd(NULL) {} AdvanceOptions::~AdvanceOptions() { @@ -470,6 +480,57 @@ void AdvanceOptions::CreateAdditionalControls(HFONT h_font) { // SendMessage(h_hyperlink, WM_SETFONT, (WPARAM)h_font, TRUE); } +void AdvanceOptions::UpdateScrollInfo(HWND hwnd) { + RECT client = {}; + GetClientRect(hwnd, &client); + int const page = client.bottom - client.top; + + SCROLLINFO si = {}; + si.cbSize = sizeof(si); + si.fMask = SIF_RANGE | SIF_PAGE | SIF_POS; + si.nMin = 0; + si.nMax = kContentHeight - 1; + si.nPage = page; + si.nPos = scroll_pos_; + SetScrollInfo(hwnd, SB_VERT, &si, TRUE); + + // Growing the window can leave us scrolled past the end; pull the content + // back so there is never blank space below the last control. + int const max_pos = (kContentHeight > page) ? kContentHeight - page : 0; + if (scroll_pos_ > max_pos) { + ScrollWindow(hwnd, 0, scroll_pos_ - max_pos, NULL, NULL); + scroll_pos_ = max_pos; + si.fMask = SIF_POS; + si.nPos = scroll_pos_; + SetScrollInfo(hwnd, SB_VERT, &si, TRUE); + } +} + +void AdvanceOptions::ScrollTo(HWND hwnd, int new_pos) { + RECT client = {}; + GetClientRect(hwnd, &client); + int const page = client.bottom - client.top; + int const max_pos = (kContentHeight > page) ? kContentHeight - page : 0; + new_pos = std::max(0, std::min(new_pos, max_pos)); + if (new_pos == scroll_pos_) { + return; + } + + int const delta = scroll_pos_ - new_pos; + scroll_pos_ = new_pos; + // ScrollWindow shifts the child controls along with the client area, which is + // what makes absolutely-positioned controls scroll without repositioning each + // one by hand. + ScrollWindow(hwnd, 0, delta, NULL, NULL); + + SCROLLINFO si = {}; + si.cbSize = sizeof(si); + si.fMask = SIF_POS; + si.nPos = scroll_pos_; + SetScrollInfo(hwnd, SB_VERT, &si, TRUE); + UpdateWindow(hwnd); +} + void AdvanceOptions::PopulateAllowedProjectsListView( HWND h_list_view, std::vector const& project_ids) { if (!h_list_view) { @@ -875,17 +936,26 @@ LRESULT CALLBACK AdvanceOptions::AdvanceOptProc(HWND hwnd, UINT u_msg, if (HIWORD(w_param) != BN_CLICKED) { break; } - // The credentials live on the parent DSN dialog; this dialog holds no - // copy of them. - HWND h_parent = GetParent(hwnd); + // The credentials live on the main DSN dialog; this dialog holds no + // copy of them. This is a top-level owned window rather than a child, + // and GetParent only reports the owner for WS_POPUP windows, so ask + // for the owner explicitly. + HWND h_parent = GetWindow(hwnd, GW_OWNER); + if (h_parent == NULL) { + h_parent = GetParent(hwnd); + } + if (h_parent == NULL) { + ShowErrorWindow(hwnd, + "Internal error: cannot locate the main dialog to " + "read the connection settings from."); + break; + } char key_file_buffer[1024] = {0}; char auth_buffer[256] = {0}; - if (h_parent) { - GetWindowText(GetDlgItem(h_parent, kIdcKeyfileEdit), - key_file_buffer, sizeof(key_file_buffer)); - GetWindowText(GetDlgItem(h_parent, kIdcAuthBox), auth_buffer, - sizeof(auth_buffer)); - } + GetWindowText(GetDlgItem(h_parent, kIdcKeyfileEdit), key_file_buffer, + sizeof(key_file_buffer)); + GetWindowText(GetDlgItem(h_parent, kIdcAuthBox), auth_buffer, + sizeof(auth_buffer)); if (auth_buffer[0] == '\0') { ShowErrorWindow(hwnd, "Select an OAuth mechanism on the main dialog " @@ -930,6 +1000,76 @@ LRESULT CALLBACK AdvanceOptions::AdvanceOptProc(HWND hwnd, UINT u_msg, } break; } + case WM_VSCROLL: { + RECT client = {}; + GetClientRect(hwnd, &client); + int const page = client.bottom - client.top; + int pos = scroll_pos_; + switch (LOWORD(w_param)) { + case SB_TOP: + pos = 0; + break; + case SB_BOTTOM: + pos = kContentHeight; + break; + case SB_LINEUP: + pos -= kScrollLine; + break; + case SB_LINEDOWN: + pos += kScrollLine; + break; + case SB_PAGEUP: + pos -= page; + break; + case SB_PAGEDOWN: + pos += page; + break; + case SB_THUMBTRACK: + case SB_THUMBPOSITION: { + SCROLLINFO si = {}; + si.cbSize = sizeof(si); + si.fMask = SIF_TRACKPOS; + if (GetScrollInfo(hwnd, SB_VERT, &si)) { + pos = si.nTrackPos; + } + break; + } + default: + break; + } + ScrollTo(hwnd, pos); + return 0; + } + case WM_MOUSEWHEEL: { + // Accumulate: precision trackpads and high-resolution wheels send deltas + // smaller than WHEEL_DELTA, which would round to zero on their own. + wheel_remainder_ += GET_WHEEL_DELTA_WPARAM(w_param); + int const notches = wheel_remainder_ / WHEEL_DELTA; + if (notches == 0) { + return 0; + } + wheel_remainder_ -= notches * WHEEL_DELTA; + + // Honour the system "roll the mouse wheel to scroll" setting. + UINT lines_per_notch = 3; + if (!SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0, &lines_per_notch, + 0)) { + lines_per_notch = 3; + } + RECT client = {}; + GetClientRect(hwnd, &client); + int step = 0; + if (lines_per_notch == WHEEL_PAGESCROLL) { + step = client.bottom - client.top; + } else { + step = static_cast(lines_per_notch) * kScrollLine; + } + ScrollTo(hwnd, scroll_pos_ - notches * step); + return 0; + } + case WM_SIZE: + UpdateScrollInfo(hwnd); + break; case WM_KEYDOWN: // Capture global key presses if (w_param == VK_ESCAPE) { if (p_current_window) { @@ -1047,17 +1187,40 @@ void AdvanceOptions::Show(HWND hwnd) { RegisterClass(&wc_adv); - int window_width = 525; + scroll_pos_ = 0; + wheel_remainder_ = 0; + + // The vertical scrollbar is carved out of the client area, so widen the frame + // by its width; otherwise every right-aligned control loses that many pixels + // and the edit boxes are clipped. + int window_width = 525 + GetSystemMetrics(SM_CXVSCROLL); int window_height = 860; - int screen_width = GetSystemMetrics(SM_CXSCREEN); - int screen_height = GetSystemMetrics(SM_CYSCREEN); - int x_pos = (screen_width - window_width) / 2; - int y_pos = (screen_height - window_height) / 2; + + // Never open taller than the desktop work area: on a short or DPI-scaled + // display the full control layout does not fit, and a window whose OK button + // sits below the screen edge cannot be dismissed. Whatever does not fit is + // reachable through the vertical scrollbar instead. + RECT work_area = {}; + int work_left = 0; + int work_top = 0; + int work_width = GetSystemMetrics(SM_CXSCREEN); + int work_height = GetSystemMetrics(SM_CYSCREEN); + if (SystemParametersInfo(SPI_GETWORKAREA, 0, &work_area, 0)) { + work_left = work_area.left; + work_top = work_area.top; + work_width = work_area.right - work_area.left; + work_height = work_area.bottom - work_area.top; + } + if (window_height > work_height) { + window_height = work_height; + } + int x_pos = work_left + (work_width - window_width) / 2; + int y_pos = work_top + (work_height - window_height) / 2; adv_hwnd = CreateWindowEx( WS_EX_TOPMOST, CLASS_NAME, "Advanced Options", - WS_OVERLAPPED | WS_CAPTION | WS_SYSMENU | WS_DLGFRAME, x_pos, y_pos, - window_width, window_height, hwnd, NULL, g_hDllInstance, this); + WS_OVERLAPPED | WS_CAPTION | WS_SYSMENU | WS_DLGFRAME | WS_VSCROLL, x_pos, + y_pos, window_width, window_height, hwnd, NULL, g_hDllInstance, this); if (adv_hwnd) { HFONT h_font = CreateFont(-10, 0, 0, 0, FW_NORMAL, FALSE, FALSE, FALSE, @@ -1073,6 +1236,8 @@ void AdvanceOptions::Show(HWND hwnd) { CreateAdditionalControls(h_font); CreateButtons(h_font); + UpdateScrollInfo(adv_hwnd); + ShowWindow(adv_hwnd, SW_SHOW); UpdateWindow(adv_hwnd); diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h index 842a5c0518..31b38e0d12 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.h @@ -143,6 +143,23 @@ class AdvanceOptions { static std::string enable_gcd_; static std::string universe_domain_; + // Current vertical scroll offset, in pixels, of the control area. The dialog + // is taller than the work area on small or scaled displays, so it scrolls. + static int scroll_pos_; + + // Leftover wheel movement smaller than one notch. High-resolution wheels and + // precision trackpads report fractions of WHEEL_DELTA, which would otherwise + // be rounded away to nothing. + static int wheel_remainder_; + + // Recompute the scrollbar range/page from the current client height. Call + // after the controls are created and whenever the window is resized. + static void UpdateScrollInfo(HWND hwnd); + + // Scroll the control area to 'new_pos' pixels, clamped to the scrollable + // range. Moves the child controls with it. + static void ScrollTo(HWND hwnd, int new_pos); + // Fill the allowed-projects pick list with 'project_ids', keeping ticked any // id that is currently ticked as well as any id already in // 'allowed_projects_'. Used both when the dialog opens and when the user From 90e0976b34a55416e72de591718b84c97a4ac07e Mon Sep 17 00:00:00 2001 From: Joshua McHenry Date: Tue, 28 Jul 2026 10:57:41 -0400 Subject: [PATCH 3/3] fix(bq_driver): stop empty dialog areas acting as a documentation link Clicking a blank part of the DSN setup dialog or the Advanced Options dialog could open a browser at the BigQuery ODBC/JDBC drivers page. Both dialogs hit-test WM_LBUTTONDOWN against the documentation hyperlink's rectangle, but the CreateHyperlinkLabel calls that would create that control are currently commented out. GetDlgItem therefore returns NULL, GetClientRect(NULL, &rect) fails and leaves 'rect' holding whatever was on the stack, and PtInRect matches that uninitialised rectangle -- so clicks on unrelated parts of the dialog launch the browser. Which clicks match varies between runs, since it depends on stack contents. Bail out when the control does not exist or its rectangle cannot be read, and zero-initialise the rectangle so it can never hold garbage. --- .../bq_driver/internal/driver_adv_opt_form.cc | 14 +++++++++++--- .../cloud/odbc/bq_driver/internal/driver_form.cc | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc index 874331205c..1829aefa0e 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_adv_opt_form.cc @@ -639,12 +639,20 @@ LRESULT CALLBACK AdvanceOptions::AdvanceOptProc(HWND hwnd, UINT u_msg, return 1; // Indicate we handled the background redraw } case WM_LBUTTONDOWN: { + // Same guard as in driver_form.cc: the documentation hyperlink is not + // created at present, so without checking for a NULL control the + // uninitialised rect can swallow clicks anywhere on the dialog. + HWND h_hyperlink = GetDlgItem(hwnd, kIdcHyperlink2); + if (h_hyperlink == NULL) { + break; + } + RECT rect = {}; + if (!GetClientRect(h_hyperlink, &rect)) { + break; + } POINT pt; GetCursorPos(&pt); ScreenToClient(hwnd, &pt); - HWND h_hyperlink = GetDlgItem(hwnd, kIdcHyperlink2); - RECT rect; - GetClientRect(h_hyperlink, &rect); MapWindowPoints(h_hyperlink, hwnd, (LPPOINT)&rect, 2); if (PtInRect(&rect, pt)) { ShellExecute(NULL, "open", kBigQueryDocsURL, NULL, NULL, SW_SHOWNORMAL); diff --git a/google/cloud/odbc/bq_driver/internal/driver_form.cc b/google/cloud/odbc/bq_driver/internal/driver_form.cc index 930c179326..a32544e371 100644 --- a/google/cloud/odbc/bq_driver/internal/driver_form.cc +++ b/google/cloud/odbc/bq_driver/internal/driver_form.cc @@ -908,12 +908,22 @@ LRESULT CALLBACK DriverForm::WindowProc(HWND hwnd, UINT u_msg, WPARAM w_param, return 1; // Indicate we handled the background redraw } case WM_LBUTTONDOWN: { + // The documentation hyperlink is currently not created (see the + // commented-out CreateHyperlinkLabel call), so GetDlgItem returns NULL. + // Without these guards GetClientRect fails and leaves 'rect' + // uninitialised, and clicks on empty parts of the dialog can fall inside + // that garbage rectangle and launch a browser. + HWND h_hyperlink = GetDlgItem(hwnd, kIdcHyperlink3); + if (h_hyperlink == NULL) { + break; + } + RECT rect = {}; + if (!GetClientRect(h_hyperlink, &rect)) { + break; + } POINT pt; GetCursorPos(&pt); ScreenToClient(hwnd, &pt); - HWND h_hyperlink = GetDlgItem(hwnd, kIdcHyperlink3); - RECT rect; - GetClientRect(h_hyperlink, &rect); MapWindowPoints(h_hyperlink, hwnd, (LPPOINT)&rect, 2); if (PtInRect(&rect, pt)) { ShellExecute(NULL, "open", kBigQueryDocsURL, NULL, NULL, SW_SHOWNORMAL);