kallithea Changeset - b2634df81a11

Changeset - b2634df81a11

Parent rev.

Child rev.

[Not reviewed]

default

0 4 0

Mads Kiilerich - 7 years ago 2018-12-29 17:48:07
mads@kiilerich.com

auth: explicit user permission should not blindly overrule permissions through user groups

Before, explicit permissions of a user could shadow higher permissions that
would otherwise be obtained through a group the user is member of.
That was confusing and fragile: *removing* a permission could then suddenly
give a user *more* permissions.

Instead, change the flag for controlling internal permission computation to
*not* use "explicit". Permissions will then add up, no matter if they are
explicit or through groups.

The change in auth.py is small, but read the body of __get_perms to see the
actual impact ... and also the clean-up changeset that will come next.

This might in some cases be a behaviour change and give users more access ...
but it will probably only give the user that was intended. This change can thus
be seen as a bugfix.

Some tests assumed the old behaviour. Not for good reasons, but just because
that is how they were written. These tests are updated to expect the new
behaviour, and it has been reviewed that it makes sense.

Note that this 'explicit' flag mostly is for repo permissions and independent
of the 'user_inherit_default_permissions' that just was removed and is about
global permissions.

4 files changed with 44 insertions and 34 deletions:

kallithea/lib/auth.py

kallithea/tests/api/api_base.py

kallithea/tests/functional/test_forks.py

kallithea/tests/models/test_permissions.py

0 comments (0 inline, 0 general)

kallithea/lib/auth.py

➞

Show inline comments

@@ @@ -550,25 +550,25 @@ class AuthUser(object): @@
             'admin': ['usergroup.admin'],
         }[level]
         actual_perm = self.permissions['user_groups'].get(user_group_name)
         ok = actual_perm in required_perms
         log.debug('Checking if user %r can %r user group %r (%s): %s (has %r)',
             self.username, level, user_group_name, purpose, ok, actual_perm)
         return ok
     @property
     def api_keys(self):
         return self._get_api_keys()
-    def __get_perms(self, user, explicit=True, cache=False):
+    def __get_perms(self, user, explicit=False, cache=False):
         """
         Fills user permission attribute with permissions taken from database
         works for permissions given for repositories, and for permissions that
         are granted to groups
         :param user: `AuthUser` instance
         :param explicit: In case there are permissions both for user and a group
             that user is part of, explicit flag will define if user will
             explicitly override permissions from group, if it's False it will
             compute the decision
         """
         user_id = user.user_id

kallithea/tests/api/api_base.py

➞

Show inline comments

@@ @@ -98,24 +98,25 @@ class _BaseTestApi(object): @@
         cls.usr = User.get_by_username(TEST_USER_ADMIN_LOGIN)
         cls.apikey = cls.usr.api_key
         cls.test_user = UserModel().create_or_update(
             username='test-api',
             password='test',
             email='test@example.com',
             firstname=u'first',
             lastname=u'last'
+        )
         Session().commit()
         cls.TEST_USER_LOGIN = cls.test_user.username
         cls.apikey_regular = cls.test_user.api_key
         cls.default_user_username = User.get_default_user().username
     @classmethod
     def teardown_class(cls):
         pass
     def setup_method(self, method):
         make_user_group()
         make_repo_group()
     def teardown_method(self, method):
         fixture.destroy_user_group(TEST_USER_GROUP)
         fixture.destroy_gists()
@@ @@ -697,33 +698,41 @@ class _BaseTestApi(object): @@
         ret['members'] = members
         ret['followers'] = followers
         expected = ret
         try:
             self._compare_ok(id_, expected, given=response.body)
         finally:
             RepoModel().revoke_user_permission(self.REPO, self.TEST_USER_LOGIN)
     def test_api_get_repo_by_non_admin_no_permission_to_repo(self):
         RepoModel().grant_user_permission(repo=self.REPO,
-                                          user=self.TEST_USER_LOGIN,
+                                          user=self.default_user_username,
                                           perm='repository.none')
         try:
             RepoModel().grant_user_permission(repo=self.REPO,
                                               user=self.TEST_USER_LOGIN,
                                               perm='repository.none')
         id_, params = _build_data(self.apikey_regular, 'get_repo',
                                   repoid=self.REPO)
         response = api_call(self, params)
             id_, params = _build_data(self.apikey_regular, 'get_repo',
                                       repoid=self.REPO)
             response = api_call(self, params)
         expected = 'repository `%s` does not exist' % (self.REPO)
         self._compare_error(id_, expected, given=response.body)
             expected = 'repository `%s` does not exist' % (self.REPO)
             self._compare_error(id_, expected, given=response.body)
         finally:
             RepoModel().grant_user_permission(repo=self.REPO,
                                               user=self.default_user_username,
                                               perm='repository.read')
     def test_api_get_repo_that_doesn_not_exist(self):
         id_, params = _build_data(self.apikey, 'get_repo',
                                   repoid='no-such-repo')
         response = api_call(self, params)
         ret = 'repository `%s` does not exist' % 'no-such-repo'
         expected = ret
         self._compare_error(id_, expected, given=response.body)
     def test_api_get_repos(self):
         id_, params = _build_data(self.apikey, 'get_repos')
@@ @@ -1346,35 +1355,40 @@ class _BaseTestApi(object): @@
         id_, params = _build_data(self.apikey_regular, 'fork_repo',
                                   repoid=self.REPO,
                                   fork_name=fork_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
+        )
         response = api_call(self, params)
         expected = 'Only Kallithea admin can specify `owner` param'
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(fork_name)
     def test_api_fork_repo_non_admin_no_permission_to_fork(self):
         RepoModel().grant_user_permission(repo=self.REPO,
-                                          user=self.TEST_USER_LOGIN,
+                                          user=self.default_user_username,
                                           perm='repository.none')
         fork_name = u'api-repo-fork'
         id_, params = _build_data(self.apikey_regular, 'fork_repo',
                                   repoid=self.REPO,
                                   fork_name=fork_name,
+        )
         response = api_call(self, params)
         expected = 'repository `%s` does not exist' % (self.REPO)
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(fork_name)
         try:
             fork_name = u'api-repo-fork'
             id_, params = _build_data(self.apikey_regular, 'fork_repo',
                                       repoid=self.REPO,
                                       fork_name=fork_name,
+            )
             response = api_call(self, params)
             expected = 'repository `%s` does not exist' % (self.REPO)
             self._compare_error(id_, expected, given=response.body)
         finally:
             RepoModel().grant_user_permission(repo=self.REPO,
                                               user=self.default_user_username,
                                               perm='repository.read')
             fixture.destroy_repo(fork_name)
     @parametrize('name,perm', [
         ('read', 'repository.read'),
         ('write', 'repository.write'),
         ('admin', 'repository.admin'),
     ])
     def test_api_fork_repo_non_admin_no_create_repo_permission(self, name, perm):
         fork_name = u'api-repo-fork'
         # regardless of base repository permission, forking is disallowed
         # when repository creation is disabled
         RepoModel().grant_user_permission(repo=self.REPO,
                                           user=self.TEST_USER_LOGIN,

kallithea/tests/functional/test_forks.py

➞

Show inline comments

@@ @@ -240,37 +240,42 @@ class _BaseTestCase(TestController): @@
         # set read permissions for this
         RepoModel().grant_user_permission(repo=forks[0],
                                           user=usr,
                                           perm='repository.read')
         Session().commit()
         response = self.app.get(url(controller='forks', action='forks',
                                     repo_name=repo_name))
         response.mustcontain('<div>fork of vcs test</div>')
         # remove permissions
         default_user = User.get_default_user()
         try:
             RepoModel().grant_user_permission(repo=forks[0],
                                               user=usr, perm='repository.none')
             RepoModel().grant_user_permission(repo=forks[0],
                                               user=default_user, perm='repository.none')
             Session().commit()
             # fork shouldn't be visible
             response = self.app.get(url(controller='forks', action='forks',
                                         repo_name=repo_name))
             response.mustcontain('There are no forks yet')
         finally:
             RepoModel().grant_user_permission(repo=forks[0],
                                               user=usr, perm='repository.read')
             RepoModel().grant_user_permission(repo=forks[0],
                                               user=default_user, perm='repository.read')
             RepoModel().delete(repo=forks[0])
 class TestGIT(_BaseTestCase):
     REPO = GIT_REPO
     NEW_REPO = NEW_GIT_REPO
     REPO_TYPE = 'git'
     REPO_FORK = GIT_FORK
 class TestHG(_BaseTestCase):
     REPO = HG_REPO

kallithea/tests/models/test_permissions.py

➞

Show inline comments

@@ @@ -123,47 +123,38 @@ class TestPermissions(TestController): @@
             'global': set(['hg.admin', 'hg.create.write_on_repogroup.true']),
             'repositories': {HG_REPO: 'repository.admin'}
+        }
         assert a1_auth.permissions['repositories'][HG_REPO] == perms['repositories'][HG_REPO]
         assert a1_auth.permissions['repositories_groups'] == perms['repositories_groups']
     def test_propagated_permission_from_users_group_by_explicit_perms_exist(self):
         # make group
         self.ug1 = fixture.create_user_group(u'G1')
         UserGroupModel().add_user_to_group(self.ug1, self.u1)
         # set permission to lower
         new_perm = 'repository.none'
         RepoModel().grant_user_permission(repo=HG_REPO, user=self.u1, perm=new_perm)
         # set user permission none
         RepoModel().grant_user_permission(repo=HG_REPO, user=self.u1, perm='repository.none')
         Session().commit()
         u1_auth = AuthUser(user_id=self.u1.user_id)
-        assert u1_auth.permissions['repositories'][HG_REPO] == new_perm
+        assert u1_auth.permissions['repositories'][HG_REPO] == 'repository.read' # inherit from default user
         # grant perm for group this should not override permission from user
         # since it has explicitly set
         new_perm_gr = 'repository.write'
         # grant perm for group this should override permission from user
         RepoModel().grant_user_group_permission(repo=HG_REPO,
                                                  group_name=self.ug1,
                                                  perm=new_perm_gr)
         # check perms
                                                  perm='repository.write')
         # verify that user group permissions win
         u1_auth = AuthUser(user_id=self.u1.user_id)
         perms = {
             'repositories_groups': {},
             'global': set(['hg.create.repository', 'repository.read',
                            'hg.register.manual_activate']),
             'repositories': {HG_REPO: 'repository.read'}
+        }
         assert u1_auth.permissions['repositories'][HG_REPO] == new_perm
         assert u1_auth.permissions['repositories_groups'] == perms['repositories_groups']
         assert u1_auth.permissions['repositories'][HG_REPO] == 'repository.write'
     def test_propagated_permission_from_users_group(self):
         # make group
         self.ug1 = fixture.create_user_group(u'G1')
         UserGroupModel().add_user_to_group(self.ug1, self.u3)
         # grant perm for group this should override default permission from user
         new_perm_gr = 'repository.write'
         RepoModel().grant_user_group_permission(repo=HG_REPO,
                                                  group_name=self.ug1,
                                                  perm=new_perm_gr)
         # check perms

0 comments (0 inline, 0 general)