fix(compute): correctly specify scopes when fetching token #735
Conversation
googlebot
added
the
cla: yes
stephenplusplus
force-pushed
the
spp--scopes-fix
branch
from
aeecb42
to
8ad0e05
Compare
stephenplusplus
changed the title
Codecov Report
@@ Coverage Diff @@
## master #735 +/- ##
==========================================
+ Coverage 84.37% 84.42% +0.04%
==========================================
Files 18 18
Lines 947 950 +3
Branches 209 210 +1
==========================================
+ Hits 799 802 +3
Misses 88 88
Partials 60 60
Continue to review full report at Codecov.
|
src/auth/computeclient.ts
Outdated
| instanceOptions.params = { | ||
| scopes: this.scopes.join(','), | ||
| }; | ||
| // TODO: clean up before submit, fix upstream type bug |
There was a problem hiding this comment.
do we need this TODO comment? don't quite follow it.
There was a problem hiding this comment.
Carried over from @JustinBeckwith, who might have intended to fix the problem this PR solves before it shipped, but forgot 🤷♂. Interested to see if he has a better fix in mind!
There was a problem hiding this comment.
You're change resolves the TODO (modulo the 'upstream bug' part). It can be removed.
There was a problem hiding this comment.
What upstream type bug, though? Did Justin want to change the way params were parsed upstream? I guess I would like to make sure before removing the comment or merging this PR, as the clarity could nullify either idea.
There was a problem hiding this comment.
At one point there was a cast here, so that can be removed 🙃
| @@ -29,7 +29,7 @@ const tokenPath = `${BASE_PATH}/instance/service-accounts/default/token`; | |||
| function mockToken(statusCode = 200, scopes?: string[]) { | |||
| let path = tokenPath; | |||
| if (scopes && scopes.length > 0) { | |||
| path += '?' + qs.stringify({scopes}); | |||
| path += `?scopes=${encodeURIComponent(scopes.join(','))}`; | |||
There was a problem hiding this comment.
interesting, so the root of the problem is that storage required multiple scopes?
There was a problem hiding this comment.
Essentially, yes. Storage does, and I believe many other APIs do as well. The function that I modified is only active in GCE/GCF environments.
src/auth/computeclient.ts
Outdated
| instanceOptions.params = { | ||
| scopes: this.scopes.join(','), | ||
| }; | ||
| // TODO: clean up before submit, fix upstream type bug |
There was a problem hiding this comment.
You're change resolves the TODO (modulo the 'upstream bug' part). It can be removed.
stephenplusplus
force-pushed
the
spp--scopes-fix
branch
from
60a6620
to
40570de
Compare
@JustinBeckwith are there tests that run on GC* environments? @ofrobots I think the unit tests are an adequate way to cover this former leak. I just made a change to the unit test to make sure the intended behavior is described and covered better. |
Fixes googleapis/nodejs-storage#748
According to the requirements of the metadata server, this array of values was being parsed incorrectly by gaxios, which uses
querystring.stringify(). The scopes query string parameter is expected as a comma-separated string value, as opposed to the?scope=scope1&scopes=scope2style thatquerystring.stringify()uses: https://cloud.google.com/functions/docs/securing/function-identity#access_tokens