Skip to content
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.

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

Create decorator to check service permissions #22667

Merged
merged 17 commits into from
Apr 13, 2019

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Apr 2, 2019

Description:

Following up on @MartinHjelmare's and @balloob's suggestion, this PR creates a generic decorator to check user permissions on a service call belonging to a specific domain. The decorator checks the user's permissions against all entities owned by the passed-in domain.

Since this logic originally existed in the RainMachine component, this PR also replaces that logic with this decorator.

Related issue (if applicable): N/A

Pull request in home-assistant.io with documentation (if applicable): N/A

Example entry for configuration.yaml (if applicable):

N/A

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@bachya bachya requested a review from a team as a code owner April 2, 2019 22:04
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good! I think we need a test. But let's hear what core thinks first.

@awarecan
Copy link
Contributor

awarecan commented Apr 3, 2019

Haven't looked at too details, several thought:

  1. I think better put the decorator to helpers/service.py
  2. If entity included in the call, it should be an entity_service_call, the permission check already covered in there.

@bachya
Copy link
Contributor Author

bachya commented Apr 3, 2019

@awarecan Thanks for your thoughts. RE: #2, do you mean to say that I don't need to cover the case of passing specific entity IDs into my decorator?

EDIT: assuming so. Updated with comments and added tests.

@bachya bachya force-pushed the permissions-helper branch from b2e35e0 to a1acda5 Compare April 3, 2019 12:11
@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #22667 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22667      +/-   ##
==========================================
+ Coverage   94.13%   94.14%   +<.01%     
==========================================
  Files         448      448              
  Lines       36720    36741      +21     
==========================================
+ Hits        34567    34589      +22     
+ Misses       2153     2152       -1
Impacted Files Coverage Δ
homeassistant/helpers/service.py 93.92% <100%> (+0.66%) ⬆️
homeassistant/components/websocket_api/http.py 89.34% <0%> (ø) ⬆️
homeassistant/components/uk_transport/sensor.py 94.16% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2cfc4a...bc5f3d2. Read the comment docs.

homeassistant/helpers/service.py Outdated Show resolved Hide resolved
homeassistant/helpers/service.py Show resolved Hide resolved
@bachya bachya force-pushed the permissions-helper branch from 1905b05 to 0de9785 Compare April 10, 2019 23:49
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge when final comment addressed

Co-Authored-By: bachya <bachya1208@gmail.com>
@bachya bachya merged commit fc48113 into home-assistant:dev Apr 13, 2019
@ghost ghost removed the in progress label Apr 13, 2019
@bachya bachya deleted the permissions-helper branch April 13, 2019 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants