fix(bqjdbc): add Google Driver scope to all credential types#12847
fix(bqjdbc): add Google Driver scope to all credential types#12847
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of the RequestGoogleDriveScope property in BigQueryJdbcOAuthUtility. The property is now processed globally and applied across all credential types, including Service Account, User Account, Pre-generated tokens, Application Default Credentials, and External Account authentication. The implementation standardizes the logic for adding the Google Drive read-only scope using boolean string checks and includes corresponding test updates. Feedback was provided regarding the duplication of hardcoded scope lists across multiple methods, suggesting they be moved to a shared utility class for better maintainability.
There was a problem hiding this comment.
If generic GoogleCredentials allows to createScoped(), can we add scope modification here instead of doing it for every type of auth separately?
| static final String DRIVE_READONLY_SCOPE = "https://www.googleapis.com/auth/drive.readonly"; | ||
|
|
||
| static final List<String> DEFAULT_SCOPES = Arrays.asList(BIGQUERY_SCOPE); | ||
| static final List<String> DRIVE_SCOPES = Arrays.asList(BIGQUERY_SCOPE, DRIVE_READONLY_SCOPE); |
There was a problem hiding this comment.
nit: BIGQUERY_WITH_DRIVE_SCOPES?
| "release_level": "stable", | ||
| "transport": "both", | ||
| "language": "java", | ||
| "repo": "googleapis/google-cloud-java", |
There was a problem hiding this comment.
Why editing iam-policy files?
There was a problem hiding this comment.
It is from a git merge. Not a file change made in this PR. Not sure why the file is in the changelist.
There was a problem hiding this comment.
Reverted the file change to remove from PR.
|
|
||
| Integer reqGoogleDriveScope = ds.getRequestGoogleDriveScope(); | ||
| if (reqGoogleDriveScope != null) { | ||
| Boolean reqGoogleDriveScopeBool = |
There was a problem hiding this comment.
I also don't like using magic string & comparing it for bool property.. Should we use retrieve bool at Connection layer from ds & pass bool directly to getCredentials()?
| if ("true" | ||
| .equals( | ||
| authProperties.get( | ||
| BigQueryJdbcUrlUtility.REQUEST_GOOGLE_DRIVE_SCOPE_PROPERTY_NAME))) { | ||
| builder.setScopes(DRIVE_SCOPES); |
There was a problem hiding this comment.
Maybe we can put this logic in a private helper method and use that in all the places unless we can do what @logachev said:
If generic GoogleCredentials allows to createScoped(), can we add scope modification here instead of doing it for every type of auth separately?
There was a problem hiding this comment.
Done. As per Kirill's suggestion.
| } | ||
| } | ||
|
|
||
| GoogleCredentials credentials; |
There was a problem hiding this comment.
nit: unused variable (surprised Lint presubmit doesn't catch it)
| } | ||
|
|
||
| return getServiceAccountImpersonatedCredentials(credentials, authProperties); | ||
| if (reqGoogleDriveScopeBool) { |
There was a problem hiding this comment.
Should we reverse the order? Getting serviceImpersonatedCredentials() first & adding scope second.
In case of Impersonated credentials, drive scope is required only in final credentials.. Adding it to the impersonator can require extra IAM permissions (not sure, but I'd anticipate it). In that case can also remove adding drive scope entirely from getServiceAccountImpersonatedCredentials
4 participants
b/500748880