Require admin for supervisor event publishing and addon options info#169325
Require admin for supervisor event publishing and addon options info#169325
Conversation
home-assistant
Bot
added
bugfix
cla-signed
core
has-tests
integration: hassio
small-pr
|
Hey there @home-assistant/supervisor, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Tightens authorization in the Hass.io websocket API to prevent non-admin clients from publishing supervisor events and from receiving potentially sensitive addon options in addon-info responses.
Changes:
- Require admin privileges for the
supervisor/eventwebsocket command. - Redact addon
optionsfrom/addons/<slug>/infosupervisor API websocket responses for non-admin users. - Add/update websocket tests to cover the new authorization and redaction behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
homeassistant/components/hassio/websocket_api.py |
Adds admin gating for supervisor event publishing and redacts addon options for non-admin addon info requests. |
tests/components/hassio/test_websocket_api.py |
Adds a non-admin authorization failure test and updates non-admin addon info expectations. |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@copilot can you address your comments? |
Done in commit
All three reviewer comments are addressed in commit |
Addressed all three review comments in commits
All three review comments have been addressed in commits
|
|
@balloob I think this should be fine. The only thing I didn't like about redacting by popping the key is because that becomes a breaking change to the model. Whereas putting an empty dictionary in still allows existing parsers/validators to work but the info is redacted. But perhaps given the circumstances breaking callers is better. |
|
I'm fine either way. It needs to pop the key or the doc string updated. I think it's fine to break, as now you know something is not working. |
mdegat01
force-pushed
the
hassio-websocket-apis-admin-required
branch
from
bdfff7d to
31b8afb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
homeassistant/components/hassio/websocket_api.py:107
- Replace the
ws_require_user(only_supervisor=True)guard with an admin check (or otherwise align the authorization + error behavior) so regular admin users can still callsupervisor/eventas described and non-admins get a standardUnauthorizedresponse.
@callback
@websocket_api.ws_require_user(only_supervisor=True)
@websocket_api.websocket_command(
{
vol.Required(WS_TYPE): WS_TYPE_EVENT,
vol.Required(ATTR_DATA): SCHEMA_WEBSOCKET_EVENT,
}
)
def websocket_supervisor_event(
hass: HomeAssistant, connection: ActiveConnection, msg: dict[str, Any]
) -> None:
"""Publish events from the Supervisor."""
connection.send_result(msg[WS_ID])
async_dispatcher_send(hass, EVENT_SUPERVISOR_EVENT, msg[ATTR_DATA])
| @@ -297,7 +330,14 @@ async def test_websocket_non_admin_user( | |||
| } | |||
| ) | |||
| msg = await websocket_client.receive_json() | |||
| assert msg["result"] == {} | |||
| assert msg["result"] == { | |||
| "name": "test", | |||
| "state": "started", | |||
| "slug": "test_addon", | |||
| "version": "2.0.0", | |||
| "ingress_url": "http://127.0.0.1/ingress/test_addon", | |||
| } | |||
| assert "options" not in msg["result"] | |||
There was a problem hiding this comment.
Add an explicit test that an admin user still receives options in the /addons/{slug}/info WS response (while non-admins do not) to prevent regressions where the field is accidentally stripped for all users.
mdegat01
force-pushed
the
hassio-websocket-apis-admin-required
branch
from
31b8afb to
3f6bee4
Compare
agners
left a comment
agners
left a comment
There was a problem hiding this comment.
Hm, yeah @websocket_api.ws_require_user(only_supervisor=True) got a bit more complicated due to tests than I thought 😰 . But I guess it make sense, we probably only restrict things more to Supervisor in the future!
| refresh_token = next(iter(hassio_user.refresh_tokens.values())) | ||
| access_token = hass.auth.async_create_access_token(refresh_token) |
There was a problem hiding this comment.
We can use the existing hass_supervisor_access_token fixture instead.
There was a problem hiding this comment.
This isn't a drop-in replacement. I'm not entirely sure why at the moment. I'm guessing it has to do with this code:
core/homeassistant/components/hassio/__init__.py
Lines 220 to 243 in d62f136
Currently this fixture has zero usage throughout the hassio tests. The only places it is used is in tests of other integrations mocking Supervisor. I'm wondering if the user it makes for the access token gets booted if the hassio integration is actually loaded by this config_store code.
I can probably fix it but this feels like a separate problem now. Given the context of the PR from the audit I think it would be better to just merge what we have that works and take a look at what's going on with the loading of the Supervisor user as a follow-up effort.
There was a problem hiding this comment.
Ah it relies on the hass_supervisor_user fixture, which uses a mock user. Your fixture uses the real Supervisor user. Ok then, let's go with what we have then.
…n-admin users Agent-Logs-Url: https://github.com/home-assistant/core/sessions/a4f8418d-42e2-4bb4-bbb3-b499007c6a64 Co-authored-by: balloob <1444314+balloob@users.noreply.github.com>
Agent-Logs-Url: https://github.com/home-assistant/core/sessions/a4f8418d-42e2-4bb4-bbb3-b499007c6a64 Co-authored-by: balloob <1444314+balloob@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Stefan Agner <stefan@agner.ch>
Update tests that send supervisor/event WebSocket messages to use a new hass_supervisor_ws_client fixture that authenticates as the Supervisor user, since the endpoint now requires only_supervisor=True instead of require_admin. Also update test_non_admin_publish_supervisor_event_failure to expect the new error message "Only allowed as Supervisor". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Stefan Agner <stefan@agner.ch>
mdegat01
force-pushed
the
hassio-websocket-apis-admin-required
branch
from
ede3aa7 to
59f4a74
Compare
| async def test_admin_non_supervisor_publish_supervisor_event_failure( | ||
| hass: HomeAssistant, hass_ws_client: WebSocketGenerator, hass_admin_user: MockUser | ||
| ) -> None: | ||
| """Test non admin user cannot publish supervisor event.""" |
There was a problem hiding this comment.
Rename this test (and its docstring) to reflect what it actually verifies (a non-admin, non-Supervisor user cannot publish Supervisor events).
The current name includes “admin” but the test explicitly removes admin privileges (hass_admin_user.groups = []), which makes the intent ambiguous.
| async def test_admin_non_supervisor_publish_supervisor_event_failure( | |
| hass: HomeAssistant, hass_ws_client: WebSocketGenerator, hass_admin_user: MockUser | |
| ) -> None: | |
| """Test non admin user cannot publish supervisor event.""" | |
| async def test_non_admin_non_supervisor_publish_supervisor_event_failure( | |
| hass: HomeAssistant, hass_ws_client: WebSocketGenerator, hass_admin_user: MockUser | |
| ) -> None: | |
| """Test non-admin, non-Supervisor user cannot publish Supervisor event.""" |
| ) | ||
| msg = await client.receive_json() | ||
| assert msg["success"] is False | ||
| assert msg["error"]["message"] == "Only allowed as Supervisor" |
There was a problem hiding this comment.
Assert the websocket error code instead of the human-readable message for the Supervisor-only restriction.
The message text is not part of a stable contract and may change, while the decorator consistently sets error.code (e.g. only_supervisor).
| assert msg["error"]["message"] == "Only allowed as Supervisor" | |
| assert msg["error"]["code"] == "only_supervisor" |
| @callback | ||
| @websocket_api.ws_require_user(only_supervisor=True) | ||
| @websocket_api.websocket_command( |
There was a problem hiding this comment.
Align the access restriction for supervisor/event with the PR’s stated intent (admin-only) or update the PR title/description to reflect that this now requires the special Supervisor user.
@ws_require_user(only_supervisor=True) blocks even regular admin users and returns only_supervisor, which is stricter than “require admin” and changes the documented breaking behavior.
| async def create_client() -> WebSocketGenerator: | ||
| hassio_user_id = hass.data[DATA_CONFIG_STORE].data.hassio_user | ||
| hassio_user = await hass.auth.async_get_user(hassio_user_id) | ||
| assert hassio_user | ||
| assert hassio_user.refresh_tokens | ||
| refresh_token = next(iter(hassio_user.refresh_tokens.values())) | ||
| access_token = hass.auth.async_create_access_token(refresh_token) | ||
| return await hass_ws_client(hass, access_token=access_token) |
There was a problem hiding this comment.
Fix the return type annotation of create_client to the actual websocket client type (not WebSocketGenerator).
WebSocketGenerator is a callable that creates a websocket client; create_client itself returns the created websocket client response, so the current annotation is incorrect and can break type checking/autocomplete.
Breaking change
supervisor/eventWS API requires an admin role to publish to as it can modify information in the Supervisor integration./addons/{slug}/infoAPI via the Supervisor API WS API in core is still allowed for non-admin users but will no longer returnoptionsinformation except to admins.Proposed change
Using the
supervisor/eventWS API requires admin access as its the way to keep data in the component in sync with Supervisor, it could trigger undesirable behaviors if used by other clients. Additionally addon options info should not be returned to non-admin users as it can contain sensitive information and the frontend doesn't need it anyway.When adding a new non-admin user, the UI tells the operator:
This PR is part of that audit.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: