fix: resolve GCP service-account vs oauth credentials correctly (#3328)#4026
Open
Pawansingh3889 wants to merge 1 commit into
Open
fix: resolve GCP service-account vs oauth credentials correctly (#3328)#4026Pawansingh3889 wants to merge 1 commit into
Pawansingh3889 wants to merge 1 commit into
Conversation
…hub#3328) The GCS/filesystem credentials are a Union[GcpServiceAccountCredentials, GcpOAuthCredentials], but neither spec checked that the supplied JSON matched its own type. A service-account JSON was therefore happily ingested by the oauth spec, so resolution could pick oauth and the union surfaced the oauth parse error ("GcpOAuthCredentials cannot parse...") instead of using the service account. Each spec now rejects the other's JSON (raising the existing NativeValueError subclasses) so a credentials union falls through to the correct spec. Also read the credentials from disk when the value is a path to a json file.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #3328.
What's going on
GCS / filesystem credentials are typed as
Union[GcpServiceAccountCredentials, GcpOAuthCredentials]. When dlt resolves a union it only falls through to the next member if a spec raisesNativeValueError. The problem: neither GCP spec checked that the JSON actually matched its own type.GcpOAuthCredentials.parse_native_representationhappilyjson.loads-es a service-account JSON (a service-account file even has aclient_id), so resolution can land on oauth.GcpOAuthCredentials cannot parse the configuration value provided...even though they supplied a service account. That's both the error in dlt tries OAuth in GCP Instead of Service Account Credentials when given Service Account Credentials via credential file #3328 and the "redirects to the OAuth consent screen instead of using the service account" symptom from the google_sheets source.Changes
GcpOAuthCredentialsrejects a JSON that looks like a service account (type == "service_account"or aprivate_keypresent) by raisingInvalidGoogleOauth2Json.GcpServiceAccountCredentialsrejects oauth client-secret / user info (client_secret/installed/web/type == "authorized_user") by raisingInvalidGoogleServicesJson.NativeValueErrorsubclasses, so a credentials union now falls through to the correct spec instead of mis-parsing.credentialsmay now also be a path to a json file — if the value is an existing file it's read from disk, socredentials = "/path/to/service_account.json"works (the literal case in the issue).Tests
Added to
tests/common/configuration/test_credentials.py:Union[...]resolves a service-account JSON to the service-account spec and oauth info to the oauth spec,The GCP/oauth credential tests pass locally (9 passed, including the existing ones),
blackandflake8are clean, and no existing credential test changed behaviour.