Add application credentials platform#69148
Conversation
homeassistant/components/developer_credentials/translations/en.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We prevent the router from getting frozen so this is not an issue to work around.
There was a problem hiding this comment.
I am having trouble reconciling your comment with the error message I see:
homeassistant/data_entry_flow.py:205: in async_init
flow, result = await task
homeassistant/data_entry_flow.py:232: in _async_init
result = await self._async_handle_step(flow, flow.init_step, data, init_done)
homeassistant/data_entry_flow.py:335: in _async_handle_step
result: FlowResult = await getattr(flow, method)(user_input)
homeassistant/helpers/config_entry_oauth2_flow.py:319: in async_step_user
return await self.async_step_pick_implementation(user_input)
homeassistant/helpers/config_entry_oauth2_flow.py:236: in async_step_pick_implementation
implementations = await async_get_implementations(self.hass, self.DOMAIN)
homeassistant/helpers/config_entry_oauth2_flow.py:370: in async_get_implementations
async_register_local_apis(hass)
homeassistant/helpers/config_entry_oauth2_flow.py:349: in async_register_local_apis
hass.http.register_view(OAuth2AuthorizeCallbackView())
homeassistant/components/http/__init__.py:295: in register_view
view.register(self.app, self.app.router)
homeassistant/components/http/view.py:95: in register
routes.append(router.add_route(method, url, handler))
venv/lib/python3.9/site-packages/aiohttp/web_urldispatcher.py:1100: in add_route
resource = self.add_resource(path, name=name)
venv/lib/python3.9/site-packages/aiohttp/web_urldispatcher.py:1085: in add_resource
self.register_resource(resource)
...
resource = <PlainResource /auth/external/callback>
def register_resource(self, resource: AbstractResource) -> None:
assert isinstance(
resource, AbstractResource
), f"Instance of AbstractResource class is required, got {resource!r}"
if self.frozen:
> raise RuntimeError("Cannot register a resource into frozen router.")
E RuntimeError: Cannot register a resource into frozen router.
My impression is that we are in fact freezing the router and this registration must be done at startup time, rather than just in time when the first local oauth implementation is registered. Should I interpret your comment to mean the test harness is incorrect?
There was a problem hiding this comment.
The above error happens if i do the API registration lazily, like how the non-provider does it for local oauth implementations. An alternative framing is that the integration just needs to call this to have the APIs exposed at all when using the provider interface, so i removed the comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Well .. that nice "creative" :) solution to avoid freezing does not run during tests. I am not going to dig into this, as moving the registration to the a component seems like the best fix. I'll revert changes to this file in this PR.
There was a problem hiding this comment.
Yeah this is one of those ticking time bombs that will one aiohttp update explode in our face. I think that we can workaround it using a resource, like we did for the frontend index, but never dug in to solve it.
Move the local oauth callback http endpoint registration into the auth compoent rather than registering as a side effect of invoking the config flow registration. Today the scaffold script adds http as a dependency which is a little non-obvious, and so now it is more explicit as auth. Breaking change: config_entry_oauth2_flow no longer registers the local OAuth callback endpoint, and this is now done by adding a depednency on the auth component. Pulled out as a pre-factor step of home-assistant#69148
3dd18cd to
79239d5
Compare
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Add additional structure needed for an MVP, including a target component Xbox
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
e543eb1 to
ee3aefd
Compare
MartinHjelmare
left a comment
There was a problem hiding this comment.
Great! Can be merged when frontend PR is happy with this.
Thanks for all the review time on this. home-assistant/frontend#12344 (comment) :) I'll poke and see if frontend is close. |
Resolve an issue with compatibility of exisiting config entries when importing client credentials
|
Frontend PR is good to go. I updated how importing works to register imported credentials with the specified auth domain, rather than using the unique id auth domain based on the client id. This is needed for compatibility with existing config entries and was found when i was testing with other integrations besides xbox (future PRs). Have another look. |
Proposed change
Initial implementation of a OAuth application credentials platform, from architecture discussion, initially supporting:
Upcoming considerations:
Type of change
Additional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: