feat: Regional Access Boundary Changes#891
feat: Regional Access Boundary Changes#891vverman wants to merge 13 commits intogoogleapis:trust_boundaryfrom
Conversation
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Outdated
Show resolved
Hide resolved
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
I made it so for ease of testing the getRegionalAccessBoundaryUrl logic. For ex: in the base external client we have scenarios where we want to test if the right url is returned or error is thrown in case of workload, workforce or a null audience.
There was a problem hiding this comment.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
There was a problem hiding this comment.
@vverman +1 to using @internal if you can verify that it's omitted from docs and transpiled classes.
There was a problem hiding this comment.
While @internal removes it from the type definitions, it doesn't remove it from the transpiled classes aka the .js files since they are needed for the logic to execute at runtime.
However if they are removed from the type definitions i.e. the d.ts type files a user cannot easily discover them as they wouldn't show up in their intellisense.
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
238aed8 to
f40a3e1
Compare
…e requestAsync level. RAB urls now have googleapis as universe domain.
e3e00da to
3427c0e
Compare
…been excluded and test suite updated.
…504 are the only retryable look up failures.
feywind
left a comment
There was a problem hiding this comment.
I don't see anything too weird in this, I guess let me know when you've got one that merges against main?
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/test/test.baseexternalclient.ts
Outdated
Show resolved
Hide resolved
| * @param url The URL to check. | ||
| */ | ||
| private isGlobalEndpoint(url: string | URL): boolean { | ||
| const hostname = url instanceof URL ? url.hostname : new URL(url).hostname; |
There was a problem hiding this comment.
new URL(url) will throws a TypeError if the url string is a relative URL, or a malformed one.
We need to gracefully handle those cases by assuming they are global.
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
@vverman +1 to using @internal if you can verify that it's omitted from docs and transpiled classes.
| async getRequestHeaders(url?: string | URL): Promise<Headers> { | ||
| const headers = (await this.getRequestMetadataAsync(url)).headers; | ||
| const {headers, isIDToken} = await this.getRequestMetadataAsync(url); | ||
| if (!isIDToken) { |
There was a problem hiding this comment.
idtoken client overrides getRequestMetadataAsync is not returning isIDToken=true.
IIUC, IdTokenClient inherits the default getRegionalAccessBoundaryUrl (which returns null), so RAB lookup happen for idtoken and we won't attach the header anyway, but I think setting this flag explicitly ensures we skip the RAB logic entirely and avoids unnecessary checks and future-proofs against changes in the base classes.
There was a problem hiding this comment.
Made the change to IdToken client now passes back isIDtoken flag.
My only concern with this was that implementing a flag to explicitly exclude a client would mean we have to include all the clients like refreshclient (for long living Authorized user workflows).
However considering we're already implementing a flag for id-token I am okay with IdTokenClient explicitly returning a flag.
…6h. Documentation changes.
Contains changes for the feature Regional Access Boundary (Previously Called Trust Boundaries).
The following are salient changes: