feat(auth): Regional access boundaries main merge#8769
Conversation
* RAB endpoints changed from staging to prod; Removed RAB env variable gate; updated tests. * removed sinon.createSandbox from nested beforeEach blocks in test.compute.ts, test.jwt.ts, and test.impersonated.ts.
There was a problem hiding this comment.
Code Review
This pull request introduces Regional Access Boundary (RAB) support to the Google Auth Library for Node.js, adding a RegionalAccessBoundaryManager to fetch, cache, and apply regional access boundary rules (such as the x-allowed-locations header) across various auth clients. Feedback on the changes includes correcting swapped comments for workload and workforce identity pools in baseexternalclient.ts, and replacing flaky polling loops with direct awaits on the internal refresh promise in the test suite.
| // Check if the audience corresponds to a workload identity pool. | ||
| const wfPoolId = getWorkforcePoolIdFromAudience(this.audience); | ||
| if (wfPoolId) { | ||
| return WORKFORCE_LOOKUP_ENDPOINT.replace( | ||
| '{pool_id}', | ||
| encodeURIComponent(wfPoolId), | ||
| ); | ||
| } | ||
|
|
||
| // Check if the audience corresponds to a workforce identity pool. | ||
| const wlPoolId = getWorkloadPoolIdFromAudience(this.audience); | ||
| const projectNumber = this.getProjectNumber(this.audience); | ||
| if (wlPoolId && projectNumber) { | ||
| return WORKLOAD_LOOKUP_ENDPOINT.replace( | ||
| '{project_id}', | ||
| projectNumber, | ||
| ).replace('{pool_id}', wlPoolId); | ||
| } |
There was a problem hiding this comment.
The comments for workload and workforce identity pools are swapped. The comment on line 750 refers to a workload identity pool but the code checks for a workforce pool, and vice versa on line 759. Swapping them back will improve code readability and maintainability.
| // Check if the audience corresponds to a workload identity pool. | |
| const wfPoolId = getWorkforcePoolIdFromAudience(this.audience); | |
| if (wfPoolId) { | |
| return WORKFORCE_LOOKUP_ENDPOINT.replace( | |
| '{pool_id}', | |
| encodeURIComponent(wfPoolId), | |
| ); | |
| } | |
| // Check if the audience corresponds to a workforce identity pool. | |
| const wlPoolId = getWorkloadPoolIdFromAudience(this.audience); | |
| const projectNumber = this.getProjectNumber(this.audience); | |
| if (wlPoolId && projectNumber) { | |
| return WORKLOAD_LOOKUP_ENDPOINT.replace( | |
| '{project_id}', | |
| projectNumber, | |
| ).replace('{pool_id}', wlPoolId); | |
| } | |
| // Check if the audience corresponds to a workforce identity pool. | |
| const wfPoolId = getWorkforcePoolIdFromAudience(this.audience); | |
| if (wfPoolId) { | |
| return WORKFORCE_LOOKUP_ENDPOINT.replace( | |
| '{pool_id}', | |
| encodeURIComponent(wfPoolId), | |
| ); | |
| } | |
| // Check if the audience corresponds to a workload identity pool. | |
| const wlPoolId = getWorkloadPoolIdFromAudience(this.audience); | |
| const projectNumber = this.getProjectNumber(this.audience); | |
| if (wlPoolId && projectNumber) { | |
| return WORKLOAD_LOOKUP_ENDPOINT.replace( | |
| '{project_id}', | |
| projectNumber, | |
| ).replace('{pool_id}', wlPoolId); | |
| } |
| // Wait for the background task to complete (not ideal but necessary for testing side effect) | ||
| // In a real scenario we'd use a better way to wait for the internal promise | ||
| let attempts = 0; | ||
| while (!rabLookupCalled && attempts < 10) { | ||
| await new Promise(r => setTimeout(r, 50)); | ||
| attempts++; | ||
| } |
There was a problem hiding this comment.
Instead of using a polling loop with setTimeout to wait for the background task to complete, you can directly await the internal regionalAccessBoundaryRefreshPromise from the manager. This is cleaner, faster, and avoids potential test flakiness in slow CI environments.
| // Wait for the background task to complete (not ideal but necessary for testing side effect) | |
| // In a real scenario we'd use a better way to wait for the internal promise | |
| let attempts = 0; | |
| while (!rabLookupCalled && attempts < 10) { | |
| await new Promise(r => setTimeout(r, 50)); | |
| attempts++; | |
| } | |
| await (compute as any).regionalAccessBoundaryManager | |
| .regionalAccessBoundaryRefreshPromise; |
The Regional Access Boundaries PR to main. Contains all the changes merged to the feature branch "regional-access-boundaries" rebased on top of main.