-
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
Open
bewithgaurav
wants to merge
8
commits into
main
Choose a base branch
from
bewithgaurav/bulkcopy-accesstoken-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+138
−31
Open
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
cb370cd
FEAT: Support Access Tokens for Bulk Copy
bewithgaurav d283b8f
Fix test_008_auth to unpack 3-tuple from process_connection_string
bewithgaurav 356392f
Fix comments: replace ODBC/Rust references with DDBC/mssql-py-core
bewithgaurav f10f70d
Add test coverage for get_raw_token and auth_type assertions
bewithgaurav bf134cb
Fix auth_type propagation for bulkcopy on all platforms
bewithgaurav 16e3a0c
Validate auth_type in _acquire_token before credential lookup
bewithgaurav cb2daea
Address review feedback: validate auth_type in _acquire_token, fix fa…
bewithgaurav f0f34fd
Add tests for extract_auth_type and _acquire_token unsupported auth_type
bewithgaurav File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,25 @@ def get_token_struct(token: str) -> bytes: | |
|
|
||
| @staticmethod | ||
| def get_token(auth_type: str) -> bytes: | ||
| """Get token using the specified authentication type""" | ||
| """Get DDBC token struct for the specified authentication type.""" | ||
| token_struct, _ = AADAuth._acquire_token(auth_type) | ||
| return token_struct | ||
|
|
||
| @staticmethod | ||
| def get_raw_token(auth_type: str) -> str: | ||
| """Acquire a fresh raw JWT for the mssql-py-core connection (bulk copy). | ||
|
|
||
| This deliberately does NOT cache the token — each call goes through | ||
| Azure Identity, which has its own internal cache on the credential | ||
| object. A fresh acquisition avoids expired-token errors when | ||
| bulkcopy() is called long after the original DDBC connect(). | ||
| """ | ||
| _, raw_token = AADAuth._acquire_token(auth_type) | ||
| return raw_token | ||
|
|
||
| @staticmethod | ||
| def _acquire_token(auth_type: str) -> Tuple[bytes, str]: | ||
| """Internal: acquire token and return (ddbc_struct, raw_jwt).""" | ||
| # Import Azure libraries inside method to support test mocking | ||
| # pylint: disable=import-outside-toplevel | ||
| try: | ||
|
|
@@ -61,22 +79,15 @@ def get_token(auth_type: str) -> bytes: | |
| ) | ||
|
|
||
| try: | ||
| logger.debug( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| "get_token: Creating credential instance - credential_class=%s", | ||
| credential_class.__name__, | ||
| ) | ||
| credential = credential_class() | ||
| logger.debug( | ||
| "get_token: Requesting token from Azure AD - scope=https://database.windows.net/.default" | ||
| ) | ||
| token = credential.get_token("https://database.windows.net/.default").token | ||
| raw_token = credential.get_token("https://database.windows.net/.default").token | ||
| logger.info( | ||
bewithgaurav marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "get_token: Azure AD token acquired successfully - token_length=%d chars", | ||
| len(token), | ||
| len(raw_token), | ||
| ) | ||
| return AADAuth.get_token_struct(token) | ||
| token_struct = AADAuth.get_token_struct(raw_token) | ||
| return token_struct, raw_token | ||
| except ClientAuthenticationError as e: | ||
| # Re-raise with more specific context about Azure AD authentication failure | ||
| logger.error( | ||
| "get_token: Azure AD authentication failed - credential_class=%s, error=%s", | ||
| credential_class.__name__, | ||
|
|
@@ -88,7 +99,6 @@ def get_token(auth_type: str) -> bytes: | |
| f"user cancellation, network issues, or unsupported configuration." | ||
| ) from e | ||
| except Exception as e: | ||
| # Catch any other unexpected exceptions | ||
| logger.error( | ||
| "get_token: Unexpected error during credential creation - credential_class=%s, error=%s", | ||
| credential_class.__name__, | ||
|
|
@@ -180,7 +190,7 @@ def remove_sensitive_params(parameters: List[str]) -> List[str]: | |
|
|
||
|
|
||
| def get_auth_token(auth_type: str) -> Optional[bytes]: | ||
| """Get authentication token based on auth type""" | ||
| """Get DDBC authentication token struct based on auth type.""" | ||
| logger.debug("get_auth_token: Starting - auth_type=%s", auth_type) | ||
| if not auth_type: | ||
| logger.debug("get_auth_token: No auth_type specified, returning None") | ||
|
|
@@ -204,15 +214,16 @@ def get_auth_token(auth_type: str) -> Optional[bytes]: | |
|
|
||
| def process_connection_string( | ||
| connection_string: str, | ||
| ) -> Tuple[str, Optional[Dict[int, bytes]]]: | ||
| ) -> Tuple[str, Optional[Dict[int, bytes]], Optional[str]]: | ||
| """ | ||
| Process connection string and handle authentication. | ||
|
|
||
| Args: | ||
| connection_string: The connection string to process | ||
|
|
||
| Returns: | ||
| Tuple[str, Optional[Dict]]: Processed connection string and attrs_before dict if needed | ||
| Tuple[str, Optional[Dict], Optional[str]]: Processed connection string, | ||
| attrs_before dict if needed, and auth_type string for bulk copy token acquisition | ||
|
|
||
| Raises: | ||
| ValueError: If the connection string is invalid or empty | ||
|
|
@@ -259,7 +270,7 @@ def process_connection_string( | |
| "process_connection_string: Token authentication configured successfully - auth_type=%s", | ||
| auth_type, | ||
| ) | ||
| return ";".join(modified_parameters) + ";", {1256: token_struct} | ||
| return ";".join(modified_parameters) + ";", {1256: token_struct}, auth_type | ||
| else: | ||
| logger.warning( | ||
| "process_connection_string: Token acquisition failed, proceeding without token" | ||
|
|
@@ -269,4 +280,4 @@ def process_connection_string( | |
| "process_connection_string: Connection string processing complete - has_auth=%s", | ||
| bool(auth_type), | ||
| ) | ||
| return ";".join(modified_parameters) + ";", None | ||
| return ";".join(modified_parameters) + ";", None, None | ||
bewithgaurav marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.