Improve OAuth error handling in configuration flows #103157

allenporter · 2023-11-01T05:13:39Z

Proposed change

Improve OAuth error handling in configuration flows as a follow up for the types of errors hanlded in #103131 that are currently unhandled in integrations. Today these are surfaced often as empty dialog boxes without friendly error messages.

The logic for parsing the error string comes from https://www.oauth.com/oauth2-servers/access-tokens/access-token-response/ under "Unsuccessful Response".

I have a few requests for feedback:

Naming is hard: Please suggest better reason codes!
Once we agree on the codes: we need to have strings added to every integration that uses OAuth flows + scaffold script, is that right?
I would ideally love to surface the oauth detailed error codes in the UI similar to Abort config flow if Google Tasks API is not enabled #103114 however the _token_request function raises ClientResponseError without this detail. For now i'm just logging the error, but if you have suggestions for how to raise with this let me know

The existing code oauth2_timeout does not currently have translations properly configured anywhere and needs to also be fixed when fixing these other codes.

Method for updating strings:

$ find homeassistant -name config_flow.py | xargs -i grep -l config_entry_oauth2_flow {} | sed 's/config_flow.py/strings.json/' > /tmp/oauth-strings.json
$ python3 /tmp/fix.py $(cat /tmp/oauth-strings.json)

Where fix.py is the following:

FIXES = {
      "oauth_error": "[%key:common::config_flow::abort::oauth2_error%]",
      "oauth_timeout": "[%key:common::config_flow::abort::oauth2_timeout%]",
      "oauth_unauthorized": "[%key:common::config_flow::abort::oauth2_unauthorized]",
      "oauth_failed": "[%key:common::config_flow::abort::oauth2_failed]"
}

def main():
    args = sys.argv[1:]
    for file in args:
        print(file)
        with Path(file).open(mode='r') as fd:
            contents = "".join(fd.readlines())
            print(contents)
            st = json.loads(contents)
            abt = st["config"]["abort"]
            for f in FIXES:
                if f not in abt:
                    abt[f] = FIXES[f]
        with Path(file).open(mode='w') as fd:
            fd.write(json.dumps(st))

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

MartinHjelmare

Looks good! The tests that are using the aiohttp client mocker are failing.

home-assistant · 2023-11-01T09:10:59Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

MartinHjelmare · 2023-11-01T09:12:01Z

we need to have strings added to every integration that uses OAuth flows + scaffold script, is that right?

Yes! 👍

MartinHjelmare · 2023-11-01T09:21:42Z

homeassistant/helpers/config_entry_oauth2_flow.py

+            error_code = error_response.get("error", "unknown")
+            error_description = error_response.get("error_description", "unknown error")
+            _LOGGER.error(
+                "Token request failed (%s): %s", error_code, error_description
            )
        resp.raise_for_status()


I would ideally love to surface the oauth detailed error codes in the UI

We could wrap the ClientResponseError in our own exception and add the error code and error description as additional attributes and raise the wrapped exception manually. We already check if the response status is 400 or higher above.

Ok I was considering this, these options: given existing use of this code (used in both the config flow or generally for the auth impls token refresh), I was hesitant to introduce a new exception. Re throwing an siohttp exception could work but seems rare in this code base (and one param to the constructor may be hard to get or would be faked). I considered splitting the two token request use cases here into two calls as well or slightly changing the API to return a response and handle exceptions in the caller. Thoughts?

I think it would be ok to raise a new exception since we didn't catch any exception in the helper before and we haven't documented any expectations about that yet.

We can add it to the helper and update existing integrations, update or add an example in the docs and make a dev blog for custom integrations.

The case I am worried about is where this function is called for normal token refresh and integrations catch aiohttp exceptions directly, like on async setup, outside of config flow.

We could also upgrade all those code paths with more explicit oauth exceptions if we want to fully solve this.

The token refresh is handled by the Home Assistant oauth session so we can decide what exceptions should be handled by callers of that api.

I suggest we go ahead with this PR and then improve and add the new exception separately.

allenporter

I updated all the integrations where there is a config flow that depends on the oauth flow and added the strings. (see pr description for the method used).

MartinHjelmare · 2023-11-05T09:59:16Z

The tests that are using the aiohttp client mocker are failing.

There are still some tests failing. I don't think all of them are due to daylight saving.

MartinHjelmare

Thanks!

…03157) * Improve OAuth error handling in configuration flows * Update strings for all integrations that use oauth2 config flow * Remove invalid_auth strings * Revert change to release * Revert close change in aiohttp mock

allenporter requested a review from a team as a code owner November 1, 2023 05:13

home-assistant bot added cla-signed core has-tests integration: nest by-code-owner Quality Scale: platinum labels Nov 1, 2023

allenporter mentioned this pull request Nov 1, 2023

Improve fitbit oauth token error handling in config flow #103131

Merged

20 tasks

MartinHjelmare requested changes Nov 1, 2023

View reviewed changes

home-assistant bot marked this pull request as draft November 1, 2023 09:10

MartinHjelmare reviewed Nov 1, 2023

View reviewed changes

allenporter commented Nov 5, 2023

View reviewed changes

allenporter marked this pull request as ready for review November 5, 2023 02:19

allenporter requested review from joostlek, hunterjm, frenck, Santobert, robbiet480 and bachya as code owners November 5, 2023 02:19

home-assistant bot requested a review from MartinHjelmare November 5, 2023 02:19

MartinHjelmare marked this pull request as draft November 5, 2023 09:59

allenporter added 3 commits November 5, 2023 18:17

Improve OAuth error handling in configuration flows

31e1715

Update strings for all integrations that use oauth2 config flow

519056d

Remove invalid_auth strings

ecc5bde

allenporter force-pushed the nest branch from 1c38b1c to ecc5bde Compare November 5, 2023 18:20

allenporter added 2 commits November 5, 2023 18:22

Revert change to release

9d3d449

Revert close change in aiohttp mock

e5e056a

allenporter marked this pull request as ready for review November 6, 2023 03:21

MartinHjelmare approved these changes Nov 11, 2023

View reviewed changes

MartinHjelmare merged commit 787fb3b into home-assistant:dev Nov 11, 2023
53 checks passed

github-actions bot locked and limited conversation to collaborators Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve OAuth error handling in configuration flows #103157

Improve OAuth error handling in configuration flows #103157

allenporter commented Nov 1, 2023 •

edited

MartinHjelmare left a comment

home-assistant bot commented Nov 1, 2023

MartinHjelmare commented Nov 1, 2023

MartinHjelmare Nov 1, 2023 •

edited

allenporter Nov 1, 2023

MartinHjelmare Nov 1, 2023 •

edited

allenporter Nov 1, 2023

MartinHjelmare Nov 1, 2023 •

edited

MartinHjelmare Nov 1, 2023

allenporter left a comment

MartinHjelmare commented Nov 5, 2023

MartinHjelmare left a comment

Improve OAuth error handling in configuration flows #103157

Improve OAuth error handling in configuration flows #103157

Conversation

allenporter commented Nov 1, 2023 • edited

Proposed change

Type of change

Additional information

Checklist

MartinHjelmare left a comment

Choose a reason for hiding this comment

home-assistant bot commented Nov 1, 2023

MartinHjelmare commented Nov 1, 2023

MartinHjelmare Nov 1, 2023 • edited

Choose a reason for hiding this comment

allenporter Nov 1, 2023

Choose a reason for hiding this comment

MartinHjelmare Nov 1, 2023 • edited

Choose a reason for hiding this comment

allenporter Nov 1, 2023

Choose a reason for hiding this comment

MartinHjelmare Nov 1, 2023 • edited

Choose a reason for hiding this comment

MartinHjelmare Nov 1, 2023

Choose a reason for hiding this comment

allenporter left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Nov 5, 2023

MartinHjelmare left a comment

Choose a reason for hiding this comment

allenporter commented Nov 1, 2023 •

edited

MartinHjelmare Nov 1, 2023 •

edited

MartinHjelmare Nov 1, 2023 •

edited

MartinHjelmare Nov 1, 2023 •

edited