jason810496 commented on code in PR #76458:
URL: https://github.com/apache/airflow/pull/76458#discussion_r4512687917
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##########
@@ -288,6 +288,58 @@ def validate_team_name(self) -> ConnectionBody:
ConnectionBodyPartial = make_partial_model(ConnectionBody)
+def _contains_mask(value: Any) -> bool:
+ if isinstance(value, dict):
+ return any(_contains_mask(item) for item in value.values())
+ if isinstance(value, list):
+ return any(_contains_mask(item) for item in value)
+ return value == _MASK
+
+
+class ConnectionTestBody(ConnectionBody):
+ """
+ Connection to test.
+
+ Either only ``connection_id``, to test the stored connection as it is, or
a complete
+ connection, tested exactly as given. A stored connection's secrets are
never
+ combined with a connection supplied in the request.
+ """
+
+ conn_type: str | None = None # type: ignore[assignment]
+
+ def is_stored_connection(self) -> bool:
+ """Return True when the request only names the stored connection to
test."""
+ return self.model_fields_set == {"connection_id"}
Review Comment:
Pairs with the `team_name` fallback in the route.
```suggestion
return self.model_fields_set <= {"connection_id", "team_name"}
```
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##########
@@ -288,6 +288,58 @@ def validate_team_name(self) -> ConnectionBody:
ConnectionBodyPartial = make_partial_model(ConnectionBody)
+def _contains_mask(value: Any) -> bool:
+ if isinstance(value, dict):
+ return any(_contains_mask(item) for item in value.values())
+ if isinstance(value, list):
+ return any(_contains_mask(item) for item in value)
+ return value == _MASK
+
+
+class ConnectionTestBody(ConnectionBody):
+ """
+ Connection to test.
+
+ Either only ``connection_id``, to test the stored connection as it is, or
a complete
+ connection, tested exactly as given. A stored connection's secrets are
never
+ combined with a connection supplied in the request.
+ """
+
+ conn_type: str | None = None # type: ignore[assignment]
+
+ def is_stored_connection(self) -> bool:
+ """Return True when the request only names the stored connection to
test."""
+ return self.model_fields_set == {"connection_id"}
+
+ @model_validator(mode="after")
+ def validate_complete_connection(self) -> ConnectionTestBody:
+ if self.is_stored_connection():
+ return self
+ missing = {
+ "conn_type",
+ "host",
+ "login",
+ "schema_",
+ "port",
+ "password",
+ "extra",
+ } - self.model_fields_set
Review Comment:
This duplicates `ConnectionBody`'s fields, so a field added there later is
silently optional here.
```suggestion
optional = {"connection_id", "description", "team_name"}
missing = set(ConnectionBody.model_fields) - optional -
self.model_fields_set
```
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -350,67 +319,45 @@ def test_connection(
transient_conn_id = get_random_string()
conn_env_var = f"{CONN_ENV_PREFIX}{transient_conn_id.upper()}"
try:
- # Authorize read access on the requested ``connection_id`` *before*
- # touching the secrets backends. The route-level POST dependency only
- # verifies the caller can create connections; merging the existing
- # connection's hidden fields also requires read access to that
- # specific connection. Gating the backend lookup itself (rather than
- # the post-load merge) prevents an unauthorized caller from using
- # this endpoint to enumerate protected connection ids, generate
- # access-log entries in audited backends, or impose backend load for
- # arbitrary ids. ``get_team_name`` is a metadata-only DB lookup and
- # does not touch the configured secrets backends.
- #
- # When the connection has no metadata-DB row (e.g. it lives only in
- # a team-aware secrets backend like Vault or Kubernetes), fall back
- # to the request body's validated ``team_name`` so the GET
- # authorization and the secrets lookup both run in the right team
- # scope. ``ConnectionBody.validate_team_name`` already rejects
- # ``team_name`` from clients when ``[core] multi_team`` is off, so
- # a non-None body value here is always already gated by that
- # validator.
- team_name = Connection.get_team_name(test_body.connection_id)
- if team_name is None:
- team_name = test_body.team_name
- existing_conn: Connection | None = None
- if auth_manager.is_authorized_connection(
- method="GET",
- details=ConnectionDetails(
- conn_id=test_body.connection_id,
- team_name=team_name,
- ),
- user=user,
- ):
- try:
- existing_conn = Connection.get_connection_from_secrets(
- test_body.connection_id, team_name=team_name
- )
- except AirflowNotFoundException:
- existing_conn = None
-
- if existing_conn is not None:
- # Stored credentials are only reused to test the connection's own
- # host/port; testing a different destination must supply its own.
- fields_set = test_body.model_fields_set
- host_changed = "host" in fields_set and not
_same_endpoint(test_body.host, existing_conn.host)
- port_changed = "port" in fields_set and not
_same_endpoint(test_body.port, existing_conn.port)
- if host_changed or port_changed:
- if not _supplies_own_credentials(test_body):
- raise HTTPException(
- status.HTTP_400_BAD_REQUEST,
- "The host or port to test differs from the stored
connection. "
- "Include the credentials to test in the request body.",
+ if test_body.is_stored_connection():
+ # Authorize read access on the requested ``connection_id`` *before*
+ # touching the secrets backends. The route-level POST dependency
only
+ # verifies the caller can create connections; testing the stored
+ # connection also requires read access to that specific connection.
+ # Gating the backend lookup itself (rather than the test) prevents
+ # an unauthorized caller from using this endpoint to enumerate
+ # protected connection ids, generate access-log entries in audited
+ # backends, or impose backend load for arbitrary ids.
+ # ``get_team_name`` is a metadata-only DB lookup and does not touch
+ # the configured secrets backends.
+ team_name = Connection.get_team_name(test_body.connection_id)
Review Comment:
Without the fallback to the body's `team_name`, a team connection that lives
only in a secrets backend (no metadata DB row) is looked up as global, and
sending `team_name` makes the body partial (422). So it can no longer be tested
by id. The removed `test_secrets_only_team_connection_uses_body_team_scope`
covered this.
```suggestion
team_name = Connection.get_team_name(test_body.connection_id) or
test_body.team_name
```
paired with allowing `team_name` in `is_stored_connection`.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##########
@@ -288,6 +288,58 @@ def validate_team_name(self) -> ConnectionBody:
ConnectionBodyPartial = make_partial_model(ConnectionBody)
+def _contains_mask(value: Any) -> bool:
+ if isinstance(value, dict):
+ return any(_contains_mask(item) for item in value.values())
+ if isinstance(value, list):
+ return any(_contains_mask(item) for item in value)
+ return value == _MASK
Review Comment:
Only an exact `***` counts, so a partly masked value such as `Bearer ***`
passes and is tested literally.
```suggestion
return isinstance(value, str) and _MASK in value
```
--
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]
