architecture
architecture copied to clipboard
home-assistant
Better management of user context
I thought I'd create an arch issue to document some thoughts I've been having:
Currently the user ID is stored in core.Context and is passed manually to various methods:
-
EventBus.async_fire -
StateMachine.async_set -
Service.async_call
The problem is that if a service calls another service, or makes any of the other above calls, and if it doesn't pass the context on, the context is lost and thus the permissions can't be checked. This isn't ideal, really it shouldn't be possible to just drop the context without meaning to.
When permissions are used
I could only find a couple of places, the most important being in helpers.service.entity_service_call. Presumably in time we'll also want to check permissions to get the state of an entity, and set entity state (although this is already managed with service call restrictions).
The ideal outcome
Ideally, rather than creating a context when a websocket request is received, and passing it to the fire/call/set method, we'd instead use a contextvar.ContextVar to store it for the current execution context.
We can then get the current user object from the ContextVar at any stage, and it will automatically get passed to any threads or aio tasks. We would not pass it to the above methods directly.
Unfortunately ContextVar is a python 3.7 feature.
Solutions
Mandatory context parameter.
Make context be a mandatory rather than optional parameter on the above function calls, and only set up the current user in limited places (like websocket_api). That way anything that doesn't pass the context through will be identified.
Enforce python 3.7
Upgrade to python 3.7 (which isn't gonna happen) and go from there.
Backport contextvars
I've had a look at this, and it is actually possible, but it will take a fair bit of messing around to achieve and we'd have to monkey patch some key aio methods. Might be more work than it's worth however.
The problem with contextvars is that we sometimes use create_task, which would cause the context to be lost.
I think that we should make context mandatory. Context will only need to be created in places where we externally trigger something in the Home Assistant ecosystem: Rest API, Websocket API, integrations firing events.
Permissions should only ever be checked when we are either exporting info to users (so inside websocket API and Rest API handlers) or when we are calling methods on entities (so inside service handlers).
The problem with contextvars is that we sometimes use create_task, which would cause the context to be lost.
Actually create_task copies the current context into the new task, so it doesn't get lost. This can be managed in a backport library by registering a task factory with the loop. It is achievable to backport this reliably, I'm reasonably happy of that, it just will take a little bit of work.
Most of the backporting can be achieved just by copying the contextvar package from 3.7, then it's just a matter of registering a task factory, and patching call_at, call_later and call_soon to copy the current context to the new task, and optionally allow context as a parameter. I say optionally here because this is only required if we find a need to pass the context into these methods, but likely it won't even be needed.
The guts of it is that you store the context in a thread local for threads outside the event loop, but if you're in the event loop thread you store it on the current task (or if you really want, in a weakref table with task keys). The task factory just copies the current context and puts it on the new task before it gets run.
You can probably see here I'm trying to talk myself into the challenge :grin:, just a question of if there's enough of a use case.
I think that we should make context mandatory.
Yep, but this does have the downside that you're going to have to pass around this parameter deep into the call stack. There's a cost involved, it might actually be more ongoing maintenance than the one-time effort of backporting context vars.
Context will only need to be created in places where we externally trigger something in the Home Assistant ecosystem.
Agreed. Additionally it shouldn't be possible to create context if there's already one "assigned" to the current task. Might be a good idea if we do this to use contextvars anyhow (where available, and if say an environment variable is set) to store the context anyhow and if a new context is created when one exists, throw an exception. This handles the situation where it gets accidentally dropped, and will be picked up in the 3.7 version of the tests.
Permissions should only ever be checked...
Agreed, however we can't entirely anticipate what permissions might be needed in the future.
Actually.... Looks like someone has already done the backporting here. That implementation does have some limitations, but they're not enormous. It basically does what I was thinking of doing.
Given this is there and available, there doesn't seem to be a lot of point in not changing the implementation to do it this way, unless you can see some issues with the backport implementation. Point three in the backport I don't think is an issue, even if using uvloops, just have to check to make sure the code does the right thing and use the asyncio methods rather than the uvloops methods (which you wouldn't normally do, and at worst we lose the context which will soon show up in testing).
I think that contextvars is overkill. Most things in Home Assistant do not use context because they are not doing user facing stuff.
The only things that do are the ones with external integration, and those are not that much:
- api
- websocket_api
- google_assistant
- alexa
- cloud
- service handlers
And I think that if we make service calls have mandatory context, (initially just print a warning if it's not passed in), we are there 80%.
OK I'll have one more crack at convincing you and then give up :grin:
I think that if we make service calls have mandatory context,
This will work, but it's going to be a lot more work in the end. Basically any service call that then calls another method, that then makes a service call or raises an event or sets a state will have to pass the context as a method parameter. It gets pretty messy quite fast and becomes a maintenance hassle, and an issue for security getting dropped too.
It only takes one custom component that doesn't do the right thing, or when we move to packaged components one packaged component, and then you've lost your security. With context vars however you don't need to deal with this great mess, it's basically what contextvars are invented for.
Google assistant uses websocket, so it doesn't need to worry about the context if websocket sets it up. Really websocket should create context and then hand it off to whatever handles the request as a method parameter (which kinda already happens, kinda).
I haven't looked at the code, but presumably it's the same with alexa and cloud. In terms of using security, there's only three locations in the code base that I could find, so they're very easily patched. Compare this with potentially the need to modify a whole heap of service/state/event calls plus any helper methods or callbacks they use just to pass the context object around, it's much more work!
Any task created from a task that already has the security context will inherit it cleanly, and this includes any external threads that have the security attached, that then create a task. You only lose it if you create a thread from an existing thread, so overall it's much more tidy.
we are there 80%.
If we use contextvars, we're there 100% :grin: with no need to modify anything much except to strip out the context parameter from those method calls because they're no longer needed. (or even just deprecate it and warn if you like).
Anyhow, my 2c. Feel free to close if you like.
You are right that this is what contextvars are created for.
I don't know if I am a big fan of the backporting, but it can work. It would have been easier if we did follow Debian Stable and jumped to Python 3.7 this year, now we have to wait till Python 3.8 is released until we do.
How will contextvars work for tasks that get put in the executor and then call something in the event loop ?
They copy the context of the creating thread or task. The only real issue is when you create a thread from a task or thread, then the var is unset. You can however push the context manually onto the threads stack if needed, which you'd do for anything submitted to say a worker thread pool that needed it.
We can code it so that any threads added to the executor copy the context across, so it's not a big deal:
task = self.loop.run_in_executor( # type: ignore
None, target, *args)
Becomes:
context = contextvars.copy_context()
task = self.loop.run_in_executor( # type: ignore
None, lamda(*args): context.run(target, args)), *args)
I can do trial run and see how it goes, if it doesn't work we don't merge it.
OK so this is going to be much more viable once we don't have to back-port context vars as per #167.
contextvars are still a 3.7 feature however, and since only 3.6 is imminent. It looks to me like this is something that we hold off on until 3.7.
@balloob Is this something worth considering? Otherwise it's very old and can be closed.
Since we're now in the future where context vars exist, and we make use of them in some cases, maybe a draft PR of this proposal is the next logical step?
Well step 1 is to make sure that we can correctly traverse contexts between executor and async land.
So we can have hass, request and context vars, that would be awesome, especially for things like voluptuous validation.
make sure that we can correctly traverse contexts between executor and async land
contextvars are thread local as I understand it
Yes, but Penny showed how it could work here: https://github.com/home-assistant/architecture/issues/181#issuecomment-475907865
This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.
For that reason, I'm going to close this issue.
../Frenck
