feat: define useful properties on google.auth.external_account.Credentials
#770
Conversation
…ntials` This includes the following properties: - `info`: This is the reverse of `from_info` defined on subclasses and useful to serialize external account credentials. - `service_account_email`: This is the corresponding service account email if impersonation is used. - `is_user`: This is `False` for workload identity pools and `True` for workforce pools (not yet supported). This can be mainly determined from the STS audience. While the properties will primarily facilitate integration with gcloud, they are publicly useful for other contexts.
google-cla
bot
added
the
cla: yes
| # Remove None fields in the info dictionary. | ||
| for k, v in dict(config_info).items(): | ||
| if v is None: | ||
| del config_info[k] |
There was a problem hiding this comment.
Hmm, deleting an item from a dictionary while iterating it is unsafe. It would be better to create a new one, e.g.:
return {key: value for key, value in config_info.items() if value is not None}There was a problem hiding this comment.
Thanks. I can update this in a follow up.
| if start_index != -1 and end_index != -1 and start_index < end_index: | ||
| start_index = start_index + 1 | ||
| return url[start_index:end_index] | ||
| return None |
There was a problem hiding this comment.
Although it is unusual that I would say so, this code would be clearer using a regex:
service_account_regex = re.compile(r".*/(?P<email>.*):generateAccessToken")
...
if self._service_account_impersonation_url:
match = service_account_regex.match(self._service_account_impersonation_url)
if match is not None:
return match.group("email")There was a problem hiding this comment.
I would disagree on this one. It is better/more efficient to use string methods when possible over regex. It does require a couple more steps to do so here but I provided an example for the format used. It should make it easier to understand and follow the code.
There was a problem hiding this comment.
Note that you used a regex in a very similar case below, except without pulling the value from the group. Efficiency here is likely not germane, and debatable: it would need measuring, if we thought it important; clarity (to the reader) is far more crucial.
There was a problem hiding this comment.
I appreciate your input but I think this is a very subjective situation. I would change it but I think this is a coin toss.
The other pattern I am checking for below would be much harder to achieve without a regex. In this specific case, the implementation is simpler to read and follow, especially when paired with an example.
| if start_index != -1 and end_index != -1 and start_index < end_index: | ||
| start_index = start_index + 1 | ||
| return url[start_index:end_index] | ||
| return None |
There was a problem hiding this comment.
Note that you used a regex in a very similar case below, except without pulling the value from the group. Efficiency here is likely not germane, and debatable: it would need measuring, if we thought it important; clarity (to the reader) is far more crucial.
| return False | ||
| # Workforce pools representing users have the following audience format: | ||
| # //iam.googleapis.com/locations/$location/workforcePools/$poolId/providers/$providerId | ||
| p = re.compile(r"//iam\.googleapis\.com/locations/[^/]+/workforcePools/") |
There was a problem hiding this comment.
This regex should likely be compiled at module scope, rather than on each call (yes, I know the re module interns some number of recent compilations).
This includes the following properties:
info: This is the reverse offrom_infodefined on subclasses and useful toserialize external account credentials.
service_account_email: This is the corresponding service account email if impersonation is used.is_user: This isFalsefor workload identity pools andTruefor workforce pools (not yet supported).This can be mainly determined from the STS audience.
While the properties will primarily facilitate integration with gcloud, they are publicly useful for other contexts.