Repository navigation
fix(token-vault): allow requests to resource server endpoints similar to AM endpoints - #598
Conversation
🦋 Changeset detectedLatest commit: de96cf6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit de96cf6
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #598 +/- ##
============================================
- Coverage 56.58% 26.99% -29.60%
============================================
Files 105 11 -94
Lines 28534 1582 -26952
Branches 1644 49 -1595
============================================
- Hits 16145 427 -15718
+ Misses 12389 1155 -11234 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (!tokens) { | ||
| const errorResponse = createErrorResponse( | ||
| 'no_tokens', | ||
| new Error('No OAuth/OIDC tokens found'), | ||
| ); | ||
| responseChannel.postMessage(errorResponse); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you share why you added this?
There was a problem hiding this comment.
Previously this check was done right before proxying allowed AM requests that needed a token. That token check is still done but is now wrapped up inside of the parent if (requestOrigin === amOrigin) conditional. So the check is no longer applied before making a fall-through proxied resource request. We need to do the same token check now when we reach this fallback because the request requires the token be sent in the auth header.
There was a problem hiding this comment.
On closer inspection, this is needed to check both tokens.tokenExpiry and tokens.refreshToken downstream. It seems at this point we are still expecting tokens to exist so I think this is valid but lmk otherwise.
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
I don't have anything to add Justin commented on the one block i was looking at as well
878f18b to
64312db
Compare
64312db to
de96cf6
Compare
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Looks great!
Good to have a check for non-AM URLs to they are not tangled by our endpoints!
And the !tokens check makes sense too outside the if (requestOrigin === amOrigin) check!
5 participants
JIRA Ticket
Description
Ensure that requests to AM for supported endpoints are only proxied when the request origin is an AM origin. Requests to the resource server with endpoints similar in name to supported AM endpoints should still be allowed with the access token sent only in the authorization header.