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

Add permission checking to all RainMachine services #22399

Merged
merged 7 commits into from
Apr 1, 2019

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Mar 25, 2019

Description:

This PR implements user permission checking on all RainMachine services (per the "Can I Have User Permissions?" blog post).

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

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 self-assigned this Mar 25, 2019
@ghost ghost added the in progress label Mar 25, 2019
@bachya bachya force-pushed the rainmachine-service-permissions branch from b4ee99b to 390cac3 Compare March 26, 2019 21:26
@bachya
Copy link
Contributor Author

bachya commented Mar 28, 2019

@balloob, I still need to write a test, but in the meantime, could you check b97c594 and comment on whether I'm on the correct track?

@bachya bachya force-pushed the rainmachine-service-permissions branch from b97c594 to e8c0fe2 Compare March 31, 2019 22:56
@ghost ghost assigned balloob Apr 1, 2019
@balloob balloob merged commit 3d8efd4 into home-assistant:dev Apr 1, 2019
@ghost ghost removed the in progress label Apr 1, 2019
@@ -128,6 +131,44 @@
}, extra=vol.ALLOW_EXTRA)


def _check_valid_user(hass):
Copy link
Member

@MartinHjelmare MartinHjelmare Apr 1, 2019

Choose a reason for hiding this comment

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

Shouldn't we make this a helper decorator? There's nothing here that is specific to rainmachine. It could be used on any domain that has entity registry support that needs to do the same check, if we pass in domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, love that idea. I'll submit a PR.

Copy link
Member

Choose a reason for hiding this comment

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

@bachya and then maybe also update your implementation here so that you do something like this:

check_valid_user = GEN_FUNC(hass, DOMAIN)

@check_valid_user
async def handle_bla(call):

That way the decorator is re-used instead of re-generated each time.

@bachya bachya deleted the rainmachine-service-permissions branch April 1, 2019 15:59
unibeck pushed a commit to unibeck/home-assistant that referenced this pull request Apr 7, 2019
…2399)

* Add permission checking to all RainMachine services

* Linting

* Some initial work

* Owner comments

* Test in place (I think)

* Linting

* Update conftest.py
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