Skip to content

Add permission checking to all RainMachine services#22399

Merged
balloob merged 7 commits into
home-assistant:devfrom
bachya:rainmachine-service-permissions
Apr 1, 2019
Merged

Add permission checking to all RainMachine services#22399
balloob merged 7 commits into
home-assistant:devfrom
bachya:rainmachine-service-permissions

Conversation

@bachya
Copy link
Copy Markdown
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
Comment thread homeassistant/components/rainmachine/__init__.py Outdated
Comment thread homeassistant/components/rainmachine/__init__.py Outdated
Comment thread homeassistant/components/rainmachine/__init__.py
Comment thread homeassistant/components/rainmachine/__init__.py Outdated
@bachya
Copy link
Copy Markdown
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
Comment thread tests/components/rainmachine/test_service_permissions.py Outdated
@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
}, extra=vol.ALLOW_EXTRA)


def _check_valid_user(hass):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
jonbeckman pushed a commit to jonbeckman/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