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 a service require_admin wrapper #21953

Merged
merged 6 commits into from Mar 13, 2019

Conversation

Projects
None yet
6 participants
@balloob
Copy link
Member

commented Mar 11, 2019

Description:

Add a helper method to register a service that is limited to admin users.

hass.helpers.service.async_register_admin_service(
    'zha', 'pair', pair_service, vol.Schema({})
)

home-assistant/developers.home-assistant#197

Old description, no longer relevant

Add a function to wrap around admin only services to ensure called by an admin user.

Because the admin check requires a hass object, decorating a service handler with @service.require_admin, will make it a factory function requiring hass to be passed in.

# Usage 1
@service.require_admin
async def mock_service(call):

hass.services.async_register('test', 'test', mock_service(hass))

# Usage 2
async def mock_service(call):

hass.services.async_register('test', 'test', service.require_admin(mock_service, hass))

Pull request in home-assistant.io with documentation (if applicable): TBD

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@balloob balloob requested a review from home-assistant/core as a code owner Mar 11, 2019

@ghost ghost assigned balloob Mar 11, 2019

@ghost ghost added in progress and removed cla-signed labels Mar 11, 2019

@awarecan

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

The usage1 syntax looks hacky.

Show resolved Hide resolved tests/helpers/test_service.py Outdated
@andrewsayre

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Should there be a corresponding schema change to ‘services.yaml’ to add a flag for admin?

@pvizeli

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Yeah, I like the idea of services yaml instead to add this all into code base

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

So services.yaml is just describing the call interface. We should not use it to verify if we require admin access.

I also don't want to put it inside async_register, because it doesn't belong there.

I'll rewrite it to be a helper instead of decorator.

balloob added some commits Mar 11, 2019

@balloob balloob force-pushed the service-perm-checker branch from fad149f to 378c004 Mar 12, 2019

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Updated PR, this is a lot easier to understand.

@awarecan

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Can we still have @admin_service annotation, but just use as a note, if user try to async_register_admin_service a service without @admin_service annotation, throw an error. And if user try to async_register a service with @admin_service annotation throw an error as well.

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I don't want to add decorators that don't do anything.

@balloob balloob merged commit c15f433 into dev Mar 13, 2019

10 checks passed

Hound No violations found. Woof!
Python 3.5 - lints Python 3.5 - lints
Details
Python 3.5 - tests Python 3.5 - tests
Details
Python 3.6 - tests Python 3.6 - tests
Details
Python 3.7 - tests Python 3.7 - tests
Details
Pyton 3.5 - typing Pyton 3.5 - typing
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on service-perm-checker at 92.825%
Details

@delete-merged-branch delete-merged-branch bot deleted the service-perm-checker branch Mar 13, 2019

@ghost ghost removed the in progress label Mar 13, 2019

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.