potiuk commented on code in PR #70869:
URL: https://github.com/apache/airflow/pull/70869#discussion_r3694168634
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -37,6 +38,10 @@
SECRET_ID_PATTERN = r"^[a-zA-Z0-9-_]*$"
+# Separator between the team name and the secret id in a team scoped secret
name.
+# Matches the convention the other secrets backends use.
+TEAM_SEP = "--"
Review Comment:
Agreed. There's no shared constant in core to import yet -- Azure and Amazon
each define their own `TEAM_SEP`, and Yandex derives it from `sep` -- so this
stays local for now. Worth consolidating the moment one lands in core.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str,
secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before
the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
Review Comment:
Fixed -- the `.replace()` is gone and the secret id is used verbatim.
Both halves of this were right. I reproduced the collision exactly as you
wrote it, and the naming inconsistency was the other side of the same coin.
`_build_team_secret_name` now builds `<prefix><sep><team>--<secret_id>` with
the id untouched, so the team separator can only ever originate from the team
name, and underscores survive in the team-scoped name just as they do in the
team-agnostic one.
Two tests cover it: `test_team_secret_name_is_built_literally` pins the
literal string rather than deriving it from the method under test, and
`test_underscores_cannot_manufacture_a_team_namespace` asserts your
`prod__conn` case returns `None`. The latter fails against the revision you
reviewed.
For the record, the same normalisation is still live in the Azure backend.
#70876 closes it there -- but at the entry point rather than in the builder,
since Azure normalises everywhere by design and the team-scoped name has to
match how the secret is actually stored.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str,
secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before
the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret
id>``, so an id
+ that already contains the team separator makes the team agnostic
lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such
an id.
+
+ The id is never parsed to work out *which* team it names, because it
cannot be: a
+ team name may itself contain the separator, so nothing in the string
distinguishes
+ team ``a`` with id ``b--c`` from team ``a--b`` with id ``c``.
Comparing the id against
+ the prefix the caller's own team builds looks equivalent and is not --
a caller in
+ team ``a`` would match ``a--b``'s namespace on the prefix and read its
secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
Review Comment:
Fixed -- the guard matches the raw id now (`TEAM_SEP in secret_id`) and
doesn't call `build_path` at all.
Your read of the inheritance is correct: this backend uses the base
implementation, so `build_path('', 'smtp_default')` yields `-smtp_default` -- a
stray leading separator and no normalisation, which is why the guard never saw
the `prod__conn` form. With the id no longer normalised when the team-scoped
name is built, there's nothing left for the guard to normalise either, so
matching the raw id is both correct and simpler. I left a note in the docstring
on why routing through `build_path` is wrong here, so it doesn't get copied
back from Azure later.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self,
mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored
for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's
namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards
against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full
secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id,
**kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX,
variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
Review Comment:
Fixed. `test_team_secret_name_is_built_literally` now asserts the literal
name:
```
test-connections-team_a--test_postgres
```
plus a second assertion that `smtp_default` keeps its underscore.
Your negative test went in as
`test_underscores_cannot_manufacture_a_team_namespace`. I confirmed it fails
against the revision you reviewed and passes now -- and re-confirmed after the
later refactor by reintroducing the `.replace()` and watching it fail again, so
it isn't passing vacuously.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str,
secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before
the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret
id>``, so an id
+ that already contains the team separator makes the team agnostic
lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such
an id.
+
+ The id is never parsed to work out *which* team it names, because it
cannot be: a
+ team name may itself contain the separator, so nothing in the string
distinguishes
+ team ``a`` with id ``b--c`` from team ``a--b`` with id ``c``.
Comparing the id against
+ the prefix the caller's own team builds looks equivalent and is not --
a caller in
+ team ``a`` would match ``a--b``'s namespace on the prefix and read its
secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
+ return bool(re.fullmatch(rf".+{re.escape(TEAM_SEP)}.+",
normalized_secret_id))
+
+ def _get_secret(self, path_prefix: str, secret_id: str, team_name: str |
None = None) -> str | None:
"""
Get secret value from the SecretManager based on prefix.
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param team_name: Team the lookup is scoped to (if any)
"""
+ # The team scoped name is tried first and is safe by construction: it
can only ever
+ # build the caller's own namespace.
+ if team_name:
+ team_secret = self.client.get_secret(
Review Comment:
Fixed -- `client = self.client` is bound once at the top of `_get_secret`
and used for both lookups, with a comment recording why, since the cost isn't
visible at the call site.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self,
mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored
for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's
namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards
against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full
secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id,
**kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX,
variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self,
mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
Review Comment:
Fixed, using your second suggestion -- `_backend` returns `(backend, store)`
and the store is populated after the backend exists, since the lookup closes
over it by reference. The discarded backend is gone from every test in the
class.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self,
mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored
for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's
namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards
against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full
secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id,
**kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX,
variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self,
mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=CONN_ID, team_name=self.TEAM) ==
CONN_URI
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_for_another_team(self,
mock_client, mock_get_creds):
+ """The whole point: team B must not reach team A's secret by spelling
out its name."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded,
team_name=self.OTHER_TEAM) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_without_a_team_scope(self,
mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_whose_name_extends_the_callers_is_not_readable(self,
mock_client, mock_get_creds):
Review Comment:
Covered, though as a separate test rather than folded into this one --
`test_underscores_cannot_manufacture_a_team_namespace` asserts exactly your
line:
```python
assert backend.get_conn_value(conn_id=f"prod__{CONN_ID}",
team_name=self.TEAM) is None
```
They stay apart because this test is about a team whose *name* extends the
caller's, and that one is about the *id* manufacturing the separator -- two
different mechanisms that happen to reach the same namespace, each worth
failing independently.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
