fix: disable RAB lookup for Domain-Wide Delegation#17763
Conversation
Updates credentials classes to skip building the Regional Access Boundary lookup URL when Domain-Wide Delegation (DWD) is active, as RAB does not apply to Workspace User Accounts via DWD.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to bypass Regional Access Boundary (RAB) lookup when a subject is populated for Domain-wide Delegation across several credential classes. However, the reviewer pointed out that ImpersonatedCredentials does not support Domain-Wide Delegation or possess a _subject attribute, which will result in a runtime AttributeError. Consequently, the check in impersonated_credentials.py and its corresponding unit test should be removed.
| if self._subject: | ||
| # RAB does not apply to Workspace User Accounts via Domain-wide Delegation. | ||
| return None | ||
|
|
There was a problem hiding this comment.
Accessing self._subject on ImpersonatedCredentials will raise an AttributeError at runtime because _subject is not an attribute of this class. Furthermore, Domain-Wide Delegation (DWD) is not supported or applicable for impersonated credentials, so this check is unnecessary and should be removed.
There was a problem hiding this comment.
Interesting that Gemini thinks this doesn't exist as an attribute of this class - I think it is just flat out wrong here, right?
There was a problem hiding this comment.
Yup, it's hallucinating. Unfortunately it seems to be an existing issue with the gemini reviewer on this PR that it checks the code that was modified and if it doesn't see something being imported or initialized in the new block, it doesn't check if it already existed in the file.
| def test_build_regional_access_boundary_lookup_url_with_subject(self): | ||
| credentials = self.make_credentials(subject="user@example.com") | ||
| assert credentials._build_regional_access_boundary_lookup_url() is None | ||
|
|
There was a problem hiding this comment.
Since ImpersonatedCredentials does not support Domain-Wide Delegation or have a _subject attribute, this test is testing an invalid scenario and should be removed.
There was a problem hiding this comment.
This is based on the same incorrect assumption as the previous comment. _subject is supported and this test correctly verifies the expected behavior when Domain-Wide Delegation is used. Keeping this test as-is.
| if self._subject: | ||
| # RAB does not apply to Workspace User Accounts via Domain-wide Delegation. | ||
| return None | ||
|
|
There was a problem hiding this comment.
Interesting that Gemini thinks this doesn't exist as an attribute of this class - I think it is just flat out wrong here, right?
|
|
||
| Returns None if the subject is populated. | ||
| """ | ||
| if self._subject and self._subject != self._issuer: |
There was a problem hiding this comment.
Just double checking since this is a bit different than the impersonated credentials case where we just look at _subject being set - is this the canonical way for detecting "DwD" here?
There was a problem hiding this comment.
Yes, it is intentional. In google.auth.jwt.Credentials (unlike service_account.Credentials or ImpersonatedCredentials), subject defaults to client_email (which is the issuer) if not provided. Therefore, self._subject is always populated, and we have to check self._subject != self._issuer to determine if DwD is actually being used. I'll add an inline comment here for our future reference.
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
4 participants
Updates credentials classes to skip building the Regional Access Boundary lookup URL when Domain-Wide Delegation (DWD) is active, as RAB does not apply to Workspace User Accounts via DWD.
Fixes #17703