-
Notifications
You must be signed in to change notification settings - Fork 36
FEAT: EntraID Access Token Support for BulkCopy #426
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
base: main
Are you sure you want to change the base?
Conversation
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.cursor.py: 84.7%
mssql_python.__init__.py: 84.9%🔗 Quick Links
|
97bfb90 to
f10f70d
Compare
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.
Pull request overview
Adds Entra ID (Azure AD) access-token handling to support bulkcopy() by storing the detected AAD auth type on the Connection and acquiring a fresh token at bulk-copy time, alongside related auth API adjustments and test updates.
Changes:
- Added
AADAuth.get_raw_token()and internalized token acquisition viaAADAuth._acquire_token(). - Extended
process_connection_string()to returnauth_typeand persisted it onConnectionfor later bulkcopy token acquisition. - Updated
Cursor._bulkcopy()to use a fresh AAD token (or uid/pwd) and to remove sensitive keys from the bulk-copy context after use.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
mssql_python/auth.py |
Introduces raw token acquisition and returns auth_type from process_connection_string() for later bulkcopy token refresh. |
mssql_python/connection.py |
Stores _auth_type on the connection so bulkcopy can reacquire AAD tokens later. |
mssql_python/cursor.py |
Acquires a fresh AAD token at bulkcopy time and removes sensitive fields from the pycore context in finally. |
tests/test_008_auth.py |
Updates tests for the new process_connection_string() return signature and adds coverage for get_raw_token() / _auth_type storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add extract_auth_type() fallback for Windows Interactive where process_connection_string returns auth_type=None (DDBC handles auth natively but bulkcopy needs it for Azure Identity) - Preserve auth_type in fallthrough return so bulkcopy can attempt fresh token acquisition even when connect-time token fails - Fix get_raw_token docstring to match actual credential behavior - Use 'is None' instead of '== None' in test assertion
Raises ValueError with supported types instead of KeyError when an unsupported auth_type is passed to _acquire_token.
| ) | ||
|
|
||
| try: | ||
| logger.debug( |
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.
removed unnecessary log statements, there is an info log above which captures these
…llthrough return, clean up extract_auth_type parsing
Work Item / Issue Reference
Summary
This pull request introduces significant improvements to Azure AD authentication handling for bulk copy operations, ensuring fresh token acquisition to prevent expired-token errors, and refactors related code for clarity and robustness. It also updates the connection and authentication APIs to propagate and utilize authentication type information more reliably.
Azure AD authentication enhancements:
AADAuth.get_raw_tokenand refactored token acquisition logic to ensure a fresh Azure AD token is acquired each time bulkcopy is called, preventing expired-token errors. The new method avoids credential/token caching and is used specifically for bulk copy operations. (mssql_python/auth.py, mssql_python/auth.pyL33-R51)get_raw_tokenmethod, storing the auth type on the connection and acquiring a fresh token at bulk copy time. Sensitive data is now removed from memory after use for improved security. (mssql_python/cursor.py, [1] [2]Connection and authentication API changes:
process_connection_stringto return the authentication type as a third value, and addedextract_auth_typeto reliably extract auth type from the connection string when not propagated (e.g., Windows Interactive). Connection objects now store the auth type for later use. (mssql_python/auth.py, [1] [2] [3];mssql_python/connection.py, [4] [5] [6]Testing improvements:
tests/test_008_auth.py, [1] [2] [3]Error handling and logging:
mssql_python/auth.py, [1] [2]Documentation and naming consistency:
mssql_python/auth.py, mssql_python/auth.pyL183-R197)