Skip to content
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

Use ID token from metadata server when sending tasks for extensions #1812

Merged
merged 4 commits into from Jul 18, 2022

Conversation

joehan
Copy link
Collaborator

@joehan joehan commented Jul 14, 2022

Use an ID token from the compute metadata server when sending tasks from an extension.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

*/
function requestIDToken(client: HttpClient, request: HttpRequestConfig): Promise<string> {
return client.send(request).then((resp) => {
return resp.text || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we explicitly handle the cases where resp.text is undefined, or is it safe to just return an empty token here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - we probably should throw in this case (since if were requesting an id token & get an empty string, we'll almost certainly get an error later).

: await this.getUrl(resources, FIREBASE_FUNCTION_URL_FORMAT);
task.httpRequest.url = functionUrl;
// When run from a deployed extension, we should be using ComputeEngineCredentials
if (extensionId && this.app.options.credential instanceof ComputeEngineCredential) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use validator.isNonEmptyString(extensionId) here?

Copy link
Contributor

@ifielker ifielker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joehan joehan merged commit 1dc314c into master Jul 18, 2022
@joehan joehan deleted the jh-tasks-auth-refactor branch July 18, 2022 15:46
joehan added a commit that referenced this pull request Jul 21, 2022
@joehan joehan mentioned this pull request Jul 21, 2022
joehan added a commit that referenced this pull request Jul 26, 2022
* PR fixes from #1812

* Remove unused dep
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants