Add application credentials platform for nest and deprecate yaml for SDM API #73050
Conversation
| @@ -239,25 +273,108 @@ async def async_step_reauth_confirm( | |||
| entry = next(iter(existing_entries)) | |||
| if "auth_implementation" in entry.data: | |||
| data = {"implementation": entry.data["auth_implementation"]} | |||
| return await self.async_step_user(data) | |||
| return await super().async_step_user(data) | |||
There was a problem hiding this comment.
I'm looking at this now and thinking maybe this isn't the best way to structure.
Maybe instead this branch of checking the auth impl needs to happen in self.user with the dispatch up to super user?
There was a problem hiding this comment.
I don't have an opinion at the moment about this. I think you know best.
| _LOGGER.warning( | ||
| "Nest has deprecated App Auth and OAuth credentials must be " | ||
| "re-created by you, following the latest integration " | ||
| "instructions. Please delete the integration, remove nest from " |
There was a problem hiding this comment.
Why can't we start a re-auth flow and tell the user to re-create credentials and authorize the config entry again?
There was a problem hiding this comment.
Interesting idea, I had not considered this as it would immediately block the user from using an existing working integration. Is the idea that it is worth it to make the experience easier and the user is forced to resolve it sooner rather than later.
I think the changes would look like:
- Stop registering auth in setup_async entirely (should still work with web auth import here)
- Add new config flow steps under re-auth that walk the user through upgrade steps
- Remove the warning here about configuration.yaml deprecation since its not safe to remove until the user has reauthed, but make sure it still happens once setup right
This also seems nice as it may make rollback simpler (e.g. user doesn't remove config until its working)
I'll give this a shot.
There was a problem hiding this comment.
Sounds good.
A big benefit of this approach is that the user doesn't need to remove the integration which would cause the user to lose all the customizations done for the config entry, devices and entities.
Helping the user to do the right thing is good too, of course.
There was a problem hiding this comment.
I have another related scenario where i'm making a decision but want confirmation before I go too far.
Scenario: A user has an existing configuration.yaml and no config entry (e.g. set they set up w/ old instructions either web or app auth and didn't complete, or they deleted the existing entry). Because they have no config entry, none of the import logic runs.
Now, say they start a new config flow. I think the options are:
- Act like the configuration.yaml doesn't exist at all it uses the
missing_credentialsflow with app credentials entered in the UI - Support signup from
configuration.yamlbut present a dialog to choose between app auth or web auth since we don't know what it is, - Support signup from web auth only via
configuration.yamland add more dialogs explaining the situation and rules, etc.
[Edit] I'm leaning towards option 1, but having second thoughts. It's simplest and is a reasonable balance between constraints of user simplicity and # of states to document, but that is quite a different world. It means no new more setups from configuration.yaml at all, and may mean the import logic has to be more defensive about when to import creds because its no longer safe to assume they were used for setup. The number of test cases drops dramatically though.
Option 2 is what the status quo would be like how other integrations work, but it kind of conflicts with what we're saying here about triggering re-auth and re-doing the flow. Option 3 is the hybrid between the two that we need to support status quo behaviors but removing the app auth support in config flow.
There was a problem hiding this comment.
Yeah, I think 1. is ok in this case.
There was a problem hiding this comment.
The config flow aborts, then the frontend takes user to the dialog, then when done the frontend restarts a new config flow for that integration.
(I also prototyped an alternative that was purely core based using config flows to accept app creds and write to storage, not sharing any code with the app creds UI dialogs, so that's a possibility. It works.)
There was a problem hiding this comment.
Could the frontend start a new flow with reauth context on missing credentials, ie always start a new flow with the same context as the aborted flow?
There was a problem hiding this comment.
It looks like it starts reauth with an existing flow id today. I'll look into starting a new flow with that same context. I imagine that needs new apis.
I am also looking at having core.abort and start a task to trigger reauth again, then the frontend can ask: have any on progress flows? However that only works if there's one without some context matching.
There was a problem hiding this comment.
OK I got something working! Though it has a lot of little steps, so next want to checkpoint here, and get feedback on ways to iterate and improve this.
We have an existing app auth credential that enters reauth state:
More on this re-auth step later:
The flow is aborted with missing_credentials triggering this page in the frontend and user is redirected to app credentials dialog. Behind the scenes, note there is a hack in core that schedules the config entry to re-entry the reauth state while replying with the aborted state:
This is the application credentials with description placeholders populated:
User enters the creds, and the frontend restarts a new config flow. The frontend can see the re-auth state and prompts the user to select it explicitly:
Second time shown this re-auth dialog (more on this later):
User is shown a new extra step after "pick implementation" but before actually redirecting asks them to upgrade their device access project and reminds them of the OAuth ID to link in the cloud console:
The main complexity and extra screens here are because the need to restart the flow. I added back the extra reauth confirm step because the config entry is placed into re-auth before the the new credentials are created. That extra step needs to check the current state of things before proceeding (hence the "Nest needs to re-authenticate your account" in both passes -- it needs to make sure you actually created credentials). The other step related to this is re-selecting the same config entry again after the creds. There may be a way to pick automatically with a frontend change, which I can look into next.
Anyway, taking a short checkpoint / pause for additional thoughts.
There was a problem hiding this comment.
Sounds ok. It would be nice if we could avoid duplicating the re-auth confirmation dialog, but not a blocking problem I think.
allenporter
force-pushed
the
nest_config_flow_v2
branch
from
9967e22
to
9482e86
Compare
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
allenporter
force-pushed
the
nest_config_flow_v2
branch
from
0534d79
to
dca9830
Compare
| if not isinstance( | ||
| self.flow_impl, config_entry_oauth2_flow.LocalOAuth2Implementation | ||
| ): | ||
| raise ValueError(f"Unexpected OAuth implementation: {self.flow_impl}") |
There was a problem hiding this comment.
This is failing code coverage, and i'm uncertain of our latest best practice here. I'm thinking this is not worth adding test coverage for. Is it preferred to cast instead of checking and throwing ValueError?
Or as a hack, i can move this check to function in api.py where there is another similar check that also isn't covered...
There was a problem hiding this comment.
I think it's ok to keep as a ValueError and not test it if we don't expect it to happen.
Breaking change
The Nest authentication method called "Desktop", "Installed App" or "OOB" auth has been deprecated by Google and scheduled to break existing users in October. As a result, the Home Assistant Nest integration setup has been streamlined to help make the transition easier for users. See the integration documentation for details.
The configuration of the OAuth application credentials for the Nest integration has migrated to configuration via the UI. Configuring Nest OAuth application credentials via YAML configuration has been deprecated and will be removed in a future Home Assistant release.
If you were already using Web Auth, your existing Nest application credentials in the YAML configuration are automatically imported on upgrade to this release; and thus can be safely removed from your YAML configuration after upgrading.
Proposed change
Update
nestto be fully config flow driven using application credentials platform. This is an updated version of#64680 with a similar series of screens, but now also integrating application credentials.
Nest has some complexity w.r.t. use of Application Credentials: The users' device access project id is part fo the authorization url. To handle this, a new method was added to configuration for to let the config flow override the authorize url. While this is an ad-hoc solution, it seemed simpler than the alternative of trying to parameterize Application Credentials with additional fields for the
AuthorizationServer.Additionally, the nest setup flow is a multi-screen flow that integrates documentation at each step (see prior PR for screenshots, which have only slightly changed in this PR). A followup PR will also add description translations for nest application credentials once #73014 is approved.
The initialization is updated to support import, however it is more complex than a standard import:
async_setuptime we don't know if the credentials inconfiguration.yamlcan be imported or not, so it is done at config entry time where we can check the auth implementationMuch of of the work in this PR also is preparation for supporting multiple config entries at a later time.
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: