kaxil commented on code in PR #66575:
URL: https://github.com/apache/airflow/pull/66575#discussion_r3262698231
##########
task-sdk/tests/task_sdk/execution_time/test_secrets.py:
##########
@@ -120,6 +120,70 @@ def test_get_conn_value_not_implemented(self):
with pytest.raises(NotImplementedError, match="Use get_connection
instead"):
backend.get_conn_value("test_conn")
+ def test_get_connection_raises_on_permission_denied(self,
mock_supervisor_comms):
+ """An explicit deny from the Execution API must raise, not fall
through.
+
+ Returning None on a 401/403 would let the secrets-backend dispatcher
+ fall through to a less-restrictive backend (e.g.
EnvironmentVariablesBackend).
+ """
+ from airflow.sdk.exceptions import ErrorType
+ from airflow.sdk.execution_time.comms import ErrorResponse
Review Comment:
Same for all tests below
##########
task-sdk/tests/task_sdk/execution_time/test_secrets.py:
##########
@@ -120,6 +120,70 @@ def test_get_conn_value_not_implemented(self):
with pytest.raises(NotImplementedError, match="Use get_connection
instead"):
backend.get_conn_value("test_conn")
+ def test_get_connection_raises_on_permission_denied(self,
mock_supervisor_comms):
Review Comment:
These backend-level tests pass even when the end-to-end behaviour is still
broken (see comment on `_raise_if_authz_denied`). Worth adding a
dispatcher-level test, e.g. one that calls `_get_connection("denied")` (or
`Connection.get`) with a mocked `SUPERVISOR_COMMS.send` returning
`ErrorResponse(PERMISSION_DENIED)` and asserts a `PermissionError` (or
`AirflowAuthError`) actually reaches the caller, not `AirflowNotFoundException`
and not a value from a downstream backend.
A test at that layer would have caught the outer `except Exception:`
swallowing the deny.
--
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]
