feat(validate): allow to pass multiple audiencies #198
Conversation
Dasio
force-pushed
the
multiple-audiencies
branch
from
97e99af
to
b13e0fc
Compare
| @@ -28,7 +28,7 @@ export async function getPubKey( | |||
|
|
|||
| export async function validateIAPToken( | |||
| iapToken: string, | |||
| expectedAudience: string, | |||
| expectedAudiences: string | string[], | |||
There was a problem hiding this comment.
2 notes:
- this is a breaking change, so please add the "BREAKING CHANGE: ..." mention in the commit for this change;
- I would simplify the signature to be
string[](and migration for existing users would be to just enclose their current expected audience into brackets).
I believe it would simplify the logic down the road.
| expectedAudiences: string | string[], | |
| expectedAudiences: string[], |
There was a problem hiding this comment.
I wanted to avoid breaking change, is it breaking change because of changing the name of the parameter? (didn't worked with TS for a few years)
There was a problem hiding this comment.
Oh right 🙈 I somehow misread this as an object parameter (e.g. function myFn({ param1: string, param2: string}))
So you're right, it wouldn't be a breaking change by just updating the name of the parameter, my bad.
However, my proposed change of using just string[] as a type (as opposed to extending it like you initially proposed) would be one.
| @@ -46,12 +46,22 @@ export async function validateIAPToken( | |||
| throw Error(`Invalid IAP issuer [${decoded.payload.iss}]`); | |||
| } | |||
|
|
|||
| const expectedAudienceArray = Array.isArray(expectedAudiences) | |||
There was a problem hiding this comment.
This would then become unnecessary.
src/validateIAPToken.ts
Outdated
| // From: https://cloud.google.com/iap/docs/signed-headers-howto | ||
| // `aud` Must be a string with the following values: | ||
| // App Engine: /projects/PROJECT_NUMBER/apps/PROJECT_ID | ||
| // Compute Engine and GKE: /projects/PROJECT_NUMBER/global/backendServices/SERVICE_ID | ||
| if (decoded.payload.aud !== expectedAudience) { | ||
| throw Error(`Invalid audience [${decoded.payload.aud}]`); | ||
| if ( |
There was a problem hiding this comment.
All of this might be simplified to:
| if ( | |
| if (!expectedAudiences.includes(decoded.payload.aud)) |
From the comment above, it seems the decoded aud should be a string containing only one audience for the given JWT token.
There was a problem hiding this comment.
I'm getting
TS2345: Argument of type string | string[] | undefined is not assignable to parameter of type string
Type undefined is not assignable to type string
There was a problem hiding this comment.
From reading the link that's mentioned in the comment, it seems it's expected from us to validate that the decoded audience is indeed a string.
So what you could do is:
| if ( | |
| if (typeof decoded.payload.aud !== "string") { | |
| throw Error(`Invalid audience: expected a string, received [${decoded.payload.aud}]`); | |
| } | |
| if (!expectedAudiences.includes(decoded.payload.aud)) { | |
| ... |
There was a problem hiding this comment.
In case of GCP IAP, it's string, but I suppose this should be more generic library, or?
Based on https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3
In the general case, the "aud" value is an array of case-
sensitive strings, each containing a StringOrURI value. In the
special case when the JWT has one audience
So I think we should allow array
There was a problem hiding this comment.
But we are validating only IAP tokens with this lib, right? Or you plan to use it for something else than IAP?
https://cloud.google.com/iap/docs/signed-headers-howto#verifying_the_jwt_payload
There was a problem hiding this comment.
No. My use case was just to be ready for migration to platform API, where the app would probably need to run for some time in 'dual mode' to allow multiple audiences from IAP.
I thought this was a more generic library and I don't know if it's used only in Kiwi or by someone else. But when I look on readme, I see it's probably just kiwi related, then I don't know why it's public, but that's out of scope of this MR :)
So it's up to you. If you want to allow only single audience from IAP, I'll change it, as we don't need this feature
There was a problem hiding this comment.
I thought this was a more generic library and I don't know if it's used only in Kiwi or by someone else. But when I look on readme, I see it's probably just kiwi related, then I don't know why it's public, but that's out of scope of this MR :)
Well, there are many things to say about this :)
Given the comment in code and the previous behaviour, I would not support multiple audiences decoded from the JWT token and instead only focus on the middleware's functionality to allow multiple audiences, i.e. validate that the JWT tokens have an audience taken from a provided allow-list.
|
One last thing: could you please add a test for multiple audiences passed to the middleware? |
Dasio
force-pushed
the
multiple-audiencies
branch
from
71d7085
to
c291235
Compare
|
🎉 This PR is included in version 2.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.