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

fix(run/authentication): url encode query parameters #1665

Closed
wants to merge 3 commits into from
Closed

fix(run/authentication): url encode query parameters #1665

wants to merge 3 commits into from

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Aug 20, 2020

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 20, 2020
@codyoss codyoss requested review from tbpg and grayside August 20, 2020 20:37
@product-auto-label product-auto-label bot added api: run Issues related to the Cloud Run API. samples Issues that are directly related to samples. labels Aug 21, 2020
run/authentication/auth.go Outdated Show resolved Hide resolved
v := url.Values{}
v.Set("audience", serviceURL)
// query the id_token with audience as the serviceURL
tokenURL := "/instance/service-accounts/default/identity?" + v.Encode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the API we want for users? Should they be expected to URL encode? I think so...

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the API we want for users?

It feels like it could be better, but without passing an explicit url.Values I am not sure how.

Should they be expected to URL encode?

I think so too. Perhaps we should call this out in the godoc explicitly for Get. "All query parameters are expected to to url encoded.". Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, with our learnings from the linked issue I don't think we need to do this. Closing.

codyoss and others added 2 commits August 21, 2020 08:20
Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
@codyoss codyoss closed this Aug 21, 2020
@grayside
Copy link
Contributor

This sample is slated for deletion in September, in favor of the planned reuse of the Cloud Functions sample. Action pending docs changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: run Issues related to the Cloud Run API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants