Changeset - b2634df81a11
[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:
0 comments (0 inline, 0 general)
kallithea/lib/auth.py
Show inline comments
 
@@ -538,49 +538,49 @@ class AuthUser(object):
 
            'admin': ['group.admin'],
 
        }[level]
 
        actual_perm = self.permissions['repositories_groups'].get(repo_group_name)
 
        ok = actual_perm in required_perms
 
        log.debug('Checking if user %r can %r repo group %r (%s): %s (has %r)',
 
            self.username, level, repo_group_name, purpose, ok, actual_perm)
 
        return ok
 

	
 
    def has_user_group_permission_level(self, user_group_name, level, purpose=None):
 
        required_perms = {
 
            'read': ['usergroup.read', 'usergroup.write', 'usergroup.admin'],
 
            'write': ['usergroup.write', 'usergroup.admin'],
 
            '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
 
        user_is_admin = user.is_admin
 

	
 
        log.debug('Getting PERMISSION tree')
 
        compute = conditional_cache('short_term', 'cache_desc',
 
                                    condition=cache, func=_cached_perms_data)
 
        return compute(user_id, user_is_admin, explicit)
 

	
 
    def _get_api_keys(self):
 
        api_keys = [self.api_key]
 
        for api_key in UserApiKeys.query() \
 
                .filter_by(user_id=self.user_id, is_expired=False):
 
            api_keys.append(api_key.api_key)
kallithea/tests/api/api_base.py
Show inline comments
 
@@ -86,48 +86,49 @@ def make_user_group(name=TEST_USER_GROUP
 
def make_repo_group(name=TEST_REPO_GROUP):
 
    gr = fixture.create_repo_group(name, cur_user=TEST_USER_ADMIN_LOGIN)
 
    Session().commit()
 
    return gr
 

	
 

	
 
class _BaseTestApi(object):
 
    REPO = None
 
    REPO_TYPE = None
 

	
 
    @classmethod
 
    def setup_class(cls):
 
        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()
 
        fixture.destroy_repo_group(TEST_REPO_GROUP)
 

	
 
    def _compare_ok(self, id_, expected, given):
 
        expected = jsonify({
 
            'id': id_,
 
            'error': None,
 
            'result': expected
 
        })
 
        given = json.loads(given)
 
        assert expected == given, (expected, given)
 

	
 
    def _compare_error(self, id_, expected, given):
 
@@ -685,57 +686,65 @@ class _BaseTestApi(object):
 
                         'permission': perm}
 
            members.append(user_data)
 

	
 
        for user_group in repo.users_group_to_perm:
 
            perm = user_group.permission.permission_name
 
            user_group_obj = user_group.users_group
 
            user_group_data = {'name': user_group_obj.users_group_name,
 
                               'type': "user_group", 'permission': perm}
 
            members.append(user_group_data)
 

	
 
        for user in repo.followers:
 
            followers.append(user.user.get_api_data())
 

	
 
        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')
 
        response = api_call(self, params)
 

	
 
        expected = jsonify([
 
            repo.get_api_data()
 
            for repo in Repository.query()
 
        ])
 

	
 
        self._compare_ok(id_, expected, given=response.body)
 

	
 
    def test_api_get_repos_non_admin(self):
 
        id_, params = _build_data(self.apikey_regular, 'get_repos')
 
        response = api_call(self, params)
 
@@ -1334,59 +1343,64 @@ class _BaseTestApi(object):
 
        ret = {
 
            'msg': 'Created fork of `%s` as `%s`' % (self.REPO,
 
                                                     fork_name),
 
            'success': True,
 
            'task': None,
 
        }
 
        expected = ret
 
        self._compare_ok(id_, expected, given=response.body)
 
        fixture.destroy_repo(fork_name)
 

	
 
    def test_api_fork_repo_non_admin_specify_owner(self):
 
        fork_name = u'api-repo-fork'
 
        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,
 
                                          perm=perm)
 
        UserModel().revoke_perm('default', 'hg.create.repository')
 
        UserModel().grant_perm('default', 'hg.create.none')
 
        id_, params = _build_data(self.apikey_regular, 'fork_repo',
 
                                  repoid=self.REPO,
 
                                  fork_name=fork_name,
 
        )
 
        response = api_call(self, params)
 
        expected = 'no permission to create repositories'
 
        self._compare_error(id_, expected, given=response.body)
 
        fixture.destroy_repo(fork_name)
 

	
kallithea/tests/functional/test_forks.py
Show inline comments
 
@@ -228,52 +228,57 @@ class _BaseTestCase(TestController):
 
        response = self.app.get(url('summary_home', repo_name=fork_name))
 
        response.mustcontain(fork_name)
 
        response.mustcontain(self.REPO_TYPE)
 
        response.mustcontain('Fork of "<a href="/%s">%s</a>"' % (repo_name, repo_name))
 

	
 
        usr = self.log_user(self.username, self.password)['user_id']
 

	
 
        forks = Repository.query() \
 
            .filter(Repository.repo_type == self.REPO_TYPE) \
 
            .filter(Repository.fork_id != None).all()
 
        assert 1 == len(forks)
 

	
 
        # 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
 
    NEW_REPO = NEW_HG_REPO
 
    REPO_TYPE = 'hg'
 
    REPO_FORK = HG_FORK
kallithea/tests/models/test_permissions.py
Show inline comments
 
@@ -111,71 +111,62 @@ class TestPermissions(TestController):
 
            'repositories': {HG_REPO: 'repository.read'}
 
        }
 
        assert u1_auth.permissions['repositories'][HG_REPO] == perms['repositories'][HG_REPO]
 
        assert u1_auth.permissions['repositories_groups'] == perms['repositories_groups']
 
        assert u1_auth.permissions['global'] == perms['global']
 

	
 
    def test_default_admin_group_perms(self):
 
        self.g1 = fixture.create_repo_group(u'test1', skip_if_exists=True)
 
        self.g2 = fixture.create_repo_group(u'test2', skip_if_exists=True)
 
        a1_auth = AuthUser(user_id=self.a1.user_id)
 
        perms = {
 
            'repositories_groups': {u'test1': 'group.admin', u'test2': 'group.admin'},
 
            '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
 
        u3_auth = AuthUser(user_id=self.u3.user_id)
 
        perms = {
 
            'repositories_groups': {},
 
            'global': set(['hg.create.repository', 'repository.read',
 
                           'hg.register.manual_activate']),
 
            'repositories': {HG_REPO: 'repository.read'}
 
        }
 
        assert u3_auth.permissions['repositories'][HG_REPO] == new_perm_gr
 
        assert u3_auth.permissions['repositories_groups'] == perms['repositories_groups']
 

	
 
    def test_propagated_permission_from_users_group_lower_weight(self):
 
        # make group
0 comments (0 inline, 0 general)