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 permissions foundation #16890

Merged
merged 8 commits into from Oct 11, 2018

Conversation

Projects
None yet
6 participants
@balloob
Member

balloob commented Sep 26, 2018

# First need to merge groups PR

Description:

This PR adds the foundation for permissions to Home Assistant. It's the goal to make permissions available. It's not the goal of this PR to enforce permissions.

It is based on top off the groups PR. Until that is merged, that code will be included in the diff of this PR.

TO DO:

  • Create groups that a user can be a part of (separate PR)
  • Attach policies to a group
  • Docs

Not going to do in this PR:

  • Filter Rest API output for states (here)
  • Filter WebSocket API output (here)
  • Check policy when calling services (here)

Related issue (if applicable): fixes home-assistant/architecture#67

Pull request in home-assistant.io with documentation (if applicable): home-assistant/developers.home-assistant#118

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

@wafflebot wafflebot bot added the in progress label Sep 26, 2018

@balloob balloob referenced this pull request Sep 26, 2018

Closed

Permissions #67

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 26, 2018

Regarding groups:

I think we can start with

  • change user.is_owner, system_generated to owner group and system group
  • user should be in a default user group upon creation

@balloob balloob referenced this pull request Sep 28, 2018

Merged

Add group foundation #16935

3 of 3 tasks complete

@balloob balloob requested a review from home-assistant/core as a code owner Oct 2, 2018

@awarecan

I think one user should be able to belong multiple groups, owner should be a group, likes administrators group in Windows.

@attr.s(slots=True)
class User:
"""A user."""
name = attr.ib(type=str) # type: Optional[str]
group = attr.ib(type=Group)

This comment has been minimized.

@awarecan

awarecan Oct 2, 2018

Contributor

User only belongs to one group?

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

Yep. Since policies are attached to groups, having multiple groups becomes madness

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

And with madness I mean it becomes very expensive to verify each entity access.

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

I'll open an architecture issue.

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

btw, the group code is in this PR but that's PR #16935. It was just needed for this PR to progress.

@@ -0,0 +1,171 @@
"""Permissions for Home Assistant."""
from typing import (

This comment has been minimized.

@houndci-bot

houndci-bot Oct 3, 2018

'typing.Dict' imported but unused
'typing.cast' imported but unused

@@ -0,0 +1,226 @@
"""Permissions for Home Assistant."""
from typing import (

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

'typing.Dict' imported but unused
'typing.Set' imported but unused

@balloob balloob changed the title from WIP: Add permissions foundation to Add permissions foundation Oct 8, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 8, 2018

This is ready for review.

@balloob

This comment has been minimized.

Member

balloob commented Oct 8, 2018

Just like with the auth system, we're starting slow and will merge it in while it is not 100% finished. This is ok as long as it does not impact the normal running of the system.

@MartinHjelmare

I got lost in merging policies 🙈.

Show resolved Hide resolved homeassistant/auth/auth_store.py Outdated
Show resolved Hide resolved homeassistant/auth/models.py Outdated
Show resolved Hide resolved homeassistant/auth/permissions.py Outdated
Show resolved Hide resolved homeassistant/auth/permissions.py Outdated
@awarecan

I think the permission merge should be
False > True > Dict > None

Show resolved Hide resolved homeassistant/auth/permissions.py Outdated
Show resolved Hide resolved homeassistant/auth/permissions.py Outdated
Show resolved Hide resolved tests/auth/test_permissions.py Outdated

balloob added some commits Oct 9, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

Addressed comments by Martin

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 9, 2018

I think we would benefit from docs that explains the implementation. The policy merging was really hard to follow for me.

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

Updated so that False > True.

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

Will work on docs now!

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

@awarecan

Better change several string literals to const, only comment on the first appears in the code

# Default policy if group has no policy applied.
DEFAULT_POLICY = {
"entities": True

This comment has been minimized.

@awarecan

awarecan Oct 9, 2018

Contributor

change entities to a const

} # type: PolicyType
ENTITY_POLICY_SCHEMA = vol.Any(bool, vol.Schema({
vol.Optional('domains'): vol.Any(bool, vol.Schema({

This comment has been minimized.

@awarecan

awarecan Oct 9, 2018

Contributor

change domains to a const

vol.Optional('domains'): vol.Any(bool, vol.Schema({
str: bool
})),
vol.Optional('entity_ids'): vol.Any(bool, vol.Schema({

This comment has been minimized.

@awarecan

awarecan Oct 9, 2018

Contributor

change entity_ids to a const

def filter_states(self, states: List[State]) -> List[State]:
"""Filter a list of states for what the user is allowed to see."""
func = self._policy_func('entities', _compile_entities)
keys = ('read',)

This comment has been minimized.

@awarecan

awarecan Oct 9, 2018

Contributor

change read to a const

@balloob

This comment has been minimized.

Member

balloob commented Oct 10, 2018

I think that I am going to drop support for False completely as it is incompatible with merging policies and makes things overly complicated. For example, we will be unable to merge the following two examples:

{
  "entities": {
    "entity_ids": {
      "light.kitchen": False
    }
  }
}
{
  "entities": {
    "entity_ids": True
  }
}

balloob added some commits Oct 10, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 10, 2018

We can always consider to add support for False in the future if we can come up with way that makes merging work.

@balloob

This comment has been minimized.

Member

balloob commented Oct 10, 2018

Docs updated to represent latest state too.

@awarecan

This comment has been minimized.

Contributor

awarecan commented Oct 10, 2018

It could be a solution if we support "*"

{
  "entities": {
    "entity_ids": {
      "light.kitchen": False
    }
  }
}
{
  "entities": {
    "entity_ids": True
  }
}

result can be

{
  "entities": {
    "entity_ids": {
      "light.kitchen": False,
      "*": True,
    }
  }
}
@balloob

This comment has been minimized.

Member

balloob commented Oct 10, 2018

I think that we should leave that for a future extension. I think that making a UI that supports * will be difficult.

@balloob

This comment has been minimized.

Member

balloob commented Oct 11, 2018

Sooooooo, let's do this? It will unblock other permission related work.

@pvizeli

Looks not bad. Some things need to be grow with the time.

Not sure if the compiled policy result enough optimize and don't slow down to match on slow CPUs.
But that will be seen in future.

@balloob

This comment has been minimized.

Member

balloob commented Oct 11, 2018

I've already tried to optimize it as much as possible.

@balloob balloob merged commit 61f7a39 into dev Oct 11, 2018

6 checks passed

Hound No violations found. Woof!
WIP ready for review
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 permissions at 93.492%
Details

@wafflebot wafflebot bot removed the in progress label Oct 11, 2018

@balloob balloob deleted the permissions branch Oct 11, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment