-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Enable Authentication for Google Sheet Connector Using Delegated User Credentials #25746
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
Enable Authentication for Google Sheet Connector Using Delegated User Credentials #25746
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Raghav Modani.
|
@ConfigDescription("Delegated user email to impersonate the service account") | ||
public SheetsConfig setDelegatedUserEmail(String delegatedUserEmail) | ||
{ | ||
this.delegatedUserEmail = Optional.ofNullable(delegatedUserEmail); |
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.
nit: We don't have to set nullable.
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.
Reminder.
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Show resolved
Hide resolved
@@ -290,9 +292,12 @@ private static Credential getCredentials(SheetsConfig sheetsConfig) | |||
throw new TrinoException(SHEETS_BAD_CREDENTIALS_ERROR, "No sheets credentials were provided"); | |||
} | |||
|
|||
private static Credential credentialFromStream(InputStream inputStream) | |||
private static Credential credentialFromStream(InputStream inputStream, SheetsConfig sheetsConfig) |
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.
instead of passing config
we could pass the delegatedUserEmail
right ?
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Raghav Modani.
|
c31b73c
to
f034351
Compare
This comment was marked as resolved.
This comment was marked as resolved.
eae1cf7
to
f034351
Compare
@ebyhr @Praveen2112 Please review this PR and let me know if any changes are required. Thanks! |
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsClient.java
Outdated
Show resolved
Hide resolved
@Praveen2112 @ebyhr looks like the |
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.
Is there is any manual test or validation that this email-id is being used ? Like a manual screenshot based on the history of the sheet ? @ebyhr Any alternatives we could use ?
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Show resolved
Hide resolved
@Raghav-Modani Btw please squash the commit into 1. |
5618e39
to
70dbec2
Compare
Its done. Please review. @Praveen2112 @ebyhr |
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.
LGTM. Can we have a maual test or somesort of a screenshot to ensure the delegated user email is being used.
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Show resolved
Hide resolved
Definitely, I will attach some screenshot on the working of this config property here shortly. |
| `gsheets.data-cache-ttl` | How long to cache spreadsheet data or metadata, defaults to `5m` | | ||
| `gsheets.connection-timeout` | Timeout when connection to Google Sheets API, defaults to `20s` | | ||
| `gsheets.read-timeout` | Timeout when reading from Google Sheets API, defaults to `20s` | | ||
| `gsheets.write-timeout` | Timeout when writing to Google Sheets API, defaults to `20s` | |
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.
@Raghav-Modani Can you send follow-up PR to change these properties to list-table
?
70dbec2
to
cebfe61
Compare
Hey @Praveen2112 , I have attached the screenshots, please check. |
Hey @Praveen2112 @ebyhr , let me know if there are any issues. If not, can we please merge this PR? |
The commit title is too long. Please follow https://trino.io/development/process#pull-request-and-commit-guidelines-
|
cebfe61
to
ef5d065
Compare
Hey @ebyhr , changed the commit title and the message body to adhere to the guidelines. I hope this works now. Let me know any other issues to be taken care of? |
@Raghav-Modani Please wrap the commit body at 72 characters. https://trino.io/development/process#pull-request-and-commit-guidelines- |
ef5d065
to
184dee9
Compare
Hey @ebyhr , changed the commit message body. Please check now. |
@ebyhr @Praveen2112 can we merge it now? |
@Praveen2112 @ebyhr are there any other requirements to be fulfilled to get this PR merged? If not, can we please get this merged? Thanks! |
@ebyhr @Praveen2112 Could you please check this Pull Request, we are waiting for this. |
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Show resolved
Hide resolved
plugin/trino-google-sheets/src/main/java/io/trino/plugin/google/sheets/SheetsConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-google-sheets/src/test/java/io/trino/plugin/google/sheets/TestSheetsConfig.java
Outdated
Show resolved
Hide resolved
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.
@Raghav-Modani Can you please squash the changes into 1 ?
Enable impersonation of service accounts in Sheets connector.
204e140
to
ab7eb9d
Compare
@Praveen2112 squashed all the commits into 1. |
Thanks @ebyhr @Praveen2112 for merging this! |
Description
This change lets the Google Sheets connector to authenticate by using delegated user credentials instead of raw service-account identity (which lives outside the domain). By supplying a “delegated user” email address, the connector will:
Since the domain of google service account is different from the organization's Google workspace domain, this change allows to share the google sheets only within the Google workspace domain of the organization.
Additional context and related issues
gsheets.delegated-user-email
gsheets.delegated-user-email
is set, we load the service-account key, request the delegated credentials for that user, and build the Sheets client with those credentials.Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x ) Release notes are required, with the following suggested text: