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 user group #21832

Merged
merged 2 commits into from Mar 10, 2019

Conversation

Projects
None yet
4 participants
@balloob
Copy link
Member

commented Mar 8, 2019

Description:

This add a "user" group. User group has access to all entities, but is not an admin, hence cannot administrate things.

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.

@ghost ghost assigned balloob Mar 8, 2019

@ghost ghost added the in progress label Mar 8, 2019

@awarecan
Copy link
Contributor

left a comment

We need have a follow up PR to add "user management" police and apply it to admin group. Otherwise, admin group no different than user group so far.

Show resolved Hide resolved homeassistant/auth/auth_store.py
Show resolved Hide resolved homeassistant/auth/const.py Outdated
@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

The admin group is different from the user group not based on the policy, but based on the group. All admin actions should check this property:

@property
def is_admin(self) -> bool:
"""Return if user is part of the admin group."""
if self.is_owner:
return True
return self.is_active and any(
gr.id == GROUP_ID_ADMIN for gr in self.groups)

Once we can launch that, we can slowly start breaking policies out of the admin group while making sure the admin system group remains keeping those policies. That way we can improve it backwards compatible.

@Kane610

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

I'd call it operator instead, too many users with this naming

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

@Kane610 I don't understand. Which one would you rename ? I think that having a dropdown with system groups, being able to pick "Administrator", "Users", "Read Only" seems like it makes sense ?

@Kane610

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Sorry, I'm talking about all things user management. Which is a separate concept from these roles, something like administrator, operator and viewer/read only is better as explanatory in a role purpose, everyone is still a user. But I guess it might mainly affect us people affected by a specific type of detail obsession :)

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

Yes 😛

@balloob balloob merged commit 5b2c664 into dev Mar 10, 2019

9 of 10 checks passed

Python 3.7 - tests Python 3.7 - tests
Details
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
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 Coverage increased (+0.002%) to 92.733%
Details

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

@delete-merged-branch delete-merged-branch bot deleted the add-user-group branch Mar 10, 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.