Skip to content

Disambiguate Supervisor HTTPUnauthorized on user/password validation #44940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 8, 2021
Merged

Disambiguate Supervisor HTTPUnauthorized on user/password validation #44940

merged 3 commits into from
Jan 8, 2021

Conversation

zeehio
Copy link
Contributor

@zeehio zeehio commented Jan 8, 2021

Proposed change

The HA core API usually returns 401 when the request does not
have proper authentication tokens or they have expired.

However the user/password validation endpoint may also return
401 when the given user/password is invalid.

The supervisor is currently unable to distinguish both scenarios,
and it needs to.

See home-assistant/supervisor#2408 and home-assistant/supervisor#2411

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

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
  • 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:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@zeehio
Disambiguate HTTPUnauthorized on user/password validation
a71a73d 
The HA core API usually returns 401 when the request does not
have proper authentication tokens or they have expired.

However the user/password validation endpoint may also return
401 when the given user/password is invalid.

The supervisor is currently unable to distinguish both scenarios,
and it needs to.

See home-assistant/supervisor#2408
@zeehio zeehio mentioned this pull request Jan 8, 2021
Closed
12 tasks
@MartinHjelmare MartinHjelmare changed the title Disambiguate HTTPUnauthorized on user/password validation Disambiguate Supervisor HTTPUnauthorized on user/password validation Jan 8, 2021
@frenck
Copy link
Member

frenck commented Jan 8, 2021

I get the upstream change, but what does this change add to the changes made?
It still an unauthorized response... now with an added message?

pvizeli
pvizeli reviewed Jan 8, 2021
View reviewed changes
homeassistant/components/hassio/auth.py Outdated
)
except auth_ha.InvalidAuth:
raise HTTPUnauthorized() from None
raise HTTPUnauthorized(reason=_REASON_INVALID_USER_PASSWORD) from None
Copy link
Member

@pvizeli pvizeli Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise HTTPNotFound() / see comment on supervisor PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Thanks! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to update a unit test to reflect the new behaviour

@zeehio
Copy link
Contributor Author

zeehio commented Jan 8, 2021

I get the upstream change, but what does this change add to the changes made?
It still an unauthorized response... now with an added message?

The supervisor (authenticated with a bearer token) was trying to validate some user credentials (user and password).

If the token was not valid, the response was 401. If the user credentials were not valid, the response also was 401.

My initial commit added an http reason (a message) to disambiguate whether the token was not valid or the user&password credentials were not valid.

@pvizeli's solution is to use a 404 when the user/password is invalid. It is simpler and makes sense, since the request is authenticated (the token is valid) so a 401 would not apply.

I want to give a big thank you to both of you for all your work you do in general and for reviewing my recent pull requests. Thanks.

@pvizeli pvizeli added this to the 2021.1.1 milestone Jan 8, 2021
@pvizeli pvizeli merged commit 905100a into home-assistant:dev Jan 8, 2021
@zeehio zeehio mentioned this pull request Jan 8, 2021
Closed
balloob pushed a commit that referenced this pull request Jan 9, 2021
@zeehio @balloob
Disambiguate Supervisor HTTPUnauthorized on user/password validation (#…
0e4c560 
…44940)

* Disambiguate HTTPUnauthorized on user/password validation

The HA core API usually returns 401 when the request does not
have proper authentication tokens or they have expired.

However the user/password validation endpoint may also return
401 when the given user/password is invalid.

The supervisor is currently unable to distinguish both scenarios,
and it needs to.

See home-assistant/supervisor#2408

* Return 404 if user& password are not found/valid

* Fix test for invalid user/password
@balloob balloob mentioned this pull request Jan 9, 2021
Merged
KJonline pushed a commit to Pyhass/core that referenced this pull request Jan 9, 2021
Merge branch 'dev' of https://github.com/home-assistant/core into dev
735ce85 
* 'dev' of https://github.com/home-assistant/core: (98 commits)
  Bump pymyq to 2.0.13 (home-assistant#44961)
  Add zwave to ozw migration (home-assistant#39081)
  Add pressure forecast to HA weather entity model (home-assistant#44965)
  Deduplicate MQTT entity discovery code (home-assistant#44970)
  Fix parameters when toggling light (home-assistant#44950)
  Move MQTT entity helpers to separate file (home-assistant#44838)
  Helpers type hint improvements (home-assistant#44964)
  Remove script/test (home-assistant#44967)
  Add MQTT Number (non optimistic) (home-assistant#44883)
  Fix Netatmo climate boost for valves (home-assistant#44957)
  Disambiguate Supervisor HTTPUnauthorized on user/password validation (home-assistant#44940)
  Add torrent id to Transmission events (home-assistant#44187)
  Prefix versions in system health (home-assistant#44921)
  Fix media renderers without volume control (home-assistant#44874)
  Fix wait_template incorrectly matching falsey values (home-assistant#44938)
  Upgrade youtube_dl to 2021.01.03 (home-assistant#44942)
  Upgrade discord.py to 1.6.0 (home-assistant#44941)
  Fix KNX cover state return open when unknown (home-assistant#44926)
  Use parent_id to find cause of logbook events with new contexts (home-assistant#44416)
  Implement support for additional ecobee hold modes (home-assistant#40520)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pvizeli pvizeli pvizeli approved these changes

Assignees

No one assigned

Labels

bugfix cherry-picked cla-signed core integration: hassio small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

2021.1.1

Development

Successfully merging this pull request may close these issues.

5 participants

@zeehio @frenck @pvizeli @balloob @homeassistant