add list:users|groups|services scopes

and govern GET /users|groups|services endpoints with these

Greatly simplifies filtering and pagination,
because these filters can be expressed in db filters,
unlike the potentially complex `read:users`.

Now the query itself will never return a model that should be excluded.

While writing the tests, I added more cleanup between tests.
We now ensure cleanup of all users and groups after each test,
which required updating some group tests which relied on this state leaking

docs/source/rbac/scopes.md

@@ -9,10 +9,14 @@ A scope has a syntax-based design that reveals which resources it provides acces
 ## Scope conventions
 
 - `<resource>` \
-  The top-level `<resource>` scopes, such as `users` or `groups`, grant read and write permissions to the resource itself as well as its sub-resources. For example, the scope `users:activity` is included in the scope `users`.
+  The top-level `<resource>` scopes, such as `users` or `groups`, grant read, write, and list permissions to the resource itself as well as its sub-resources. For example, the scope `users:activity` is included in the scope `users`.
 
 - `read:<resource>` \
-  Limits permissions to read-only operations on the resource.
+  Limits permissions to read-only operations on single resources.
+
+- `list:<resource>` \
+  Read-only access to listing endpoints.
+  Use `read:<resource>:<subresource>` to control what fields are returned.
 
 - `admin:<resource>` \
   Grants additional permissions such as create/delete on the corresponding resource in addition to read and write permissions.

jupyterhub/apihandlers/groups.py

@@ -7,6 +7,7 @@ from tornado import web
 
 from .. import orm
 from ..scopes import needs_scope
+from ..scopes import Scope
 from .base import APIHandler
 
 
@@ -34,14 +35,24 @@ class _GroupAPIHandler(APIHandler):
 
 
 class GroupListAPIHandler(_GroupAPIHandler):
-    @needs_scope('read:groups', 'read:groups:name', 'read:roles:groups')
+    @needs_scope('list:groups')
     def get(self):
         """List groups"""
         query = self.db.query(orm.Group)
+        sub_scope = self.parsed_scopes['list:groups']
+        if sub_scope != Scope.ALL:
+            if not set(sub_scope).issubset({'group'}):
+                # the only valid filter is group=...
+                # don't expand invalid !server=x to all groups!
+                self.log.warning(
+                    "Invalid filter on list:group for {self.current_user}: {sub_scope}"
+                )
+                raise web.HTTPError(403)
+            query = query.filter(orm.Group.name.in_(sub_scope['group']))
+
         offset, limit = self.get_api_pagination()
         query = query.offset(offset).limit(limit)
-        scope_filter = self.get_scope_filter('read:groups')
-        data = [self.group_model(g) for g in query if scope_filter(g, kind='group')]
+        data = [self.group_model(g) for g in query]
         self.write(json.dumps(data))
 
     @needs_scope('admin:groups')

jupyterhub/apihandlers/services.py

@@ -7,16 +7,18 @@ Currently GET-only, no actions can be taken to modify services.
 import json
 
 from ..scopes import needs_scope
+from ..scopes import Scope
 from .base import APIHandler
 
 
 class ServiceListAPIHandler(APIHandler):
-    @needs_scope('read:services', 'read:services:name', 'read:roles:services')
+    @needs_scope('list:services')
     def get(self):
         data = {}
+        service_scope = self.parsed_scopes['list:services']
         for name, service in self.services.items():
+            if service_scope == Scope.ALL or name in service_scope.get("service", {}):
                 model = self.service_model(service)
-            if model:
                 data[name] = model
         self.write(json.dumps(data))
 

jupyterhub/apihandlers/users.py

@@ -10,6 +10,7 @@ from datetime import timezone
 from async_generator import aclosing
 from dateutil.parser import parse as parse_date
 from sqlalchemy import func
+from sqlalchemy import or_
 from tornado import web
 from tornado.iostream import StreamClosedError
 
@@ -72,14 +73,7 @@ class UserListAPIHandler(APIHandler):
         user = self.users[orm_user]
         return any(spawner.ready for spawner in user.spawners.values())
 
-    @needs_scope(
-        'read:users',
-        'read:users:name',
-        'read:servers',
-        'read:users:groups',
-        'read:users:activity',
-        'read:roles:users',
-    )
+    @needs_scope('list:users')
     def get(self):
         state_filter = self.get_argument("state", None)
         offset, limit = self.get_api_pagination()
@@ -123,6 +117,29 @@ class UserListAPIHandler(APIHandler):
             # no filter, return all users
             query = self.db.query(orm.User)
 
+        sub_scope = self.parsed_scopes['list:users']
+        if sub_scope != scopes.Scope.ALL:
+            if not set(sub_scope).issubset({'group', 'user'}):
+                # don't expand invalid !server=x filter to all users!
+                self.log.warning(
+                    "Invalid filter on list:user for {self.current_user}: {sub_scope}"
+                )
+                raise web.HTTPError(403)
+            filters = []
+            if 'user' in sub_scope:
+                filters.append(orm.User.name.in_(sub_scope['user']))
+            if 'group' in sub_scope:
+                filters.append(
+                    orm.User.groups.any(
+                        orm.Group.name.in_(sub_scope['group']),
+                    )
+                )
+
+            if len(filters) == 1:
+                query = query.filter(filters[0])
+            else:
+                query = query.filter(or_(*filters))
+
         query = query.offset(offset).limit(limit)
 
         data = []

jupyterhub/roles.py

@@ -36,6 +36,7 @@ def get_default_roles():
                 'admin:servers',
                 'tokens',
                 'admin:groups',
+                'list:services',
                 'read:services',
                 'read:hub',
                 'proxy',
@@ -82,7 +83,6 @@ def expand_self_scope(name):
       expanded scopes (set): set of expanded scopes covering standard user privileges
     """
     scope_list = [
-        'users',
         'read:users',
         'read:users:name',
         'read:users:groups',

jupyterhub/scopes.py

@@ -41,7 +41,11 @@ scope_definitions = {
     'admin:auth_state': {'description': 'Read a user’s authentication state.'},
     'users': {
         'description': 'Read and write permissions to user models (excluding servers, tokens and authentication state).',
-        'subscopes': ['read:users', 'users:activity'],
+        'subscopes': ['read:users', 'list:users', 'users:activity'],
+    },
+    'list:users': {
+        'description': 'List users, including at least their names.',
+        'subscopes': ['read:users:name'],
     },
     'read:users': {
         'description': 'Read user models (excluding including servers, tokens and authentication state).',
@@ -89,13 +93,21 @@ scope_definitions = {
     },
     'groups': {
         'description': 'Read and write group information, including adding/removing users to/from groups.',
-        'subscopes': ['read:groups'],
+        'subscopes': ['read:groups', 'list:groups'],
+    },
+    'list:groups': {
+        'description': 'List groups, including at least their names.',
+        'subscopes': ['read:groups:name'],
     },
     'read:groups': {
         'description': 'Read group models.',
         'subscopes': ['read:groups:name'],
     },
     'read:groups:name': {'description': 'Read group names.'},
+    'list:services': {
+        'description': 'List services.',
+        'subscopes': ['read:services:name'],
+    },
     'read:services': {
         'description': 'Read service models.',
         'subscopes': ['read:services:name'],

jupyterhub/tests/conftest.py

@@ -182,7 +182,7 @@ def cleanup_after(request, io_loop):
         if not MockHub.initialized():
             return
         app = MockHub.instance()
-        for uid, user in app.users.items():
+        for uid, user in list(app.users.items()):
             for name, spawner in list(user.spawners.items()):
                 if spawner.active:
                     try:
@@ -190,6 +190,11 @@ def cleanup_after(request, io_loop):
                     except HTTPError:
                         pass
                     io_loop.run_sync(lambda: user.stop(name))
+            if user.name not in {'admin', 'user'}:
+                app.users.delete(uid)
+        # delete groups
+        for group in app.db.query(orm.Group):
+            app.db.delete(group)
         app.db.commit()
 
 
@@ -229,6 +234,39 @@ def admin_user(app, username):
     yield user
 
 
+_groupname_counter = 0
+
+
+def new_group_name(prefix='testgroup'):
+    """Return a new unique group name"""
+    global _groupname_counter
+    _groupname_counter += 1
+    return '{}-{}'.format(prefix, _groupname_counter)
+
+
+@fixture
+def groupname():
+    """allocate a temporary group name
+
+    unique each time the fixture is used
+    """
+    yield new_group_name()
+
+
+@fixture
+def group(app):
+    """Fixture for creating a temporary group
+
+    Each time the fixture is used, a new group is created
+
+    The group is deleted after the test
+    """
+    group = orm.Group(name=new_group_name())
+    app.db.add(group)
+    app.db.commit()
+    yield group
+
+
 class MockServiceSpawner(jupyterhub.services.service._ServiceSpawner):
     """mock services for testing.
 

jupyterhub/tests/test_api.py

@@ -187,9 +187,7 @@ async def test_get_users(app):
         fill_user(user_model),
     ]
     r = await api_request(app, 'users', headers=auth_header(db, 'user'))
-    assert r.status_code == 200
-    r_user_model = r.json()[0]
-    assert r_user_model['name'] == user_model['name']
+    assert r.status_code == 403
 
     # Tests offset for pagination
     r = await api_request(app, 'users?offset=1')
@@ -1512,6 +1510,9 @@ async def test_add_multi_group(app):
 
 @mark.group
 async def test_group_get(app):
+    group = orm.Group(name='alphaflight')
+    app.db.add(group)
+    app.db.commit()
     group = orm.Group.find(app.db, name='alphaflight')
     user = add_user(app.db, app=app, name='sasquatch')
     group.users.append(user)
@@ -1534,6 +1535,7 @@ async def test_group_get(app):
 @mark.group
 async def test_group_create_delete(app):
     db = app.db
+    user = add_user(app.db, app=app, name='sasquatch')
     r = await api_request(app, 'groups/runaways', method='delete')
     assert r.status_code == 404
 
@@ -1571,16 +1573,17 @@ async def test_group_create_delete(app):
 
 
 @mark.group
-async def test_group_add_users(app):
+async def test_group_add_delete_users(app):
     db = app.db
+    group = orm.Group(name='alphaflight')
+    app.db.add(group)
+    app.db.commit()
     # must specify users
     r = await api_request(app, 'groups/alphaflight/users', method='post', data='{}')
     assert r.status_code == 400
 
     names = ['aurora', 'guardian', 'northstar', 'sasquatch', 'shaman', 'snowbird']
-    users = [
-        find_user(db, name=name) or add_user(db, app=app, name=name) for name in names
-    ]
+    users = [add_user(db, app=app, name=name) for name in names]
     r = await api_request(
         app,
         'groups/alphaflight/users',
@@ -1596,16 +1599,6 @@ async def test_group_add_users(app):
     group = orm.Group.find(db, name='alphaflight')
     assert sorted([u.name for u in group.users]) == sorted(names)
 
-
-@mark.group
-async def test_group_delete_users(app):
-    db = app.db
-    # must specify users
-    r = await api_request(app, 'groups/alphaflight/users', method='delete', data='{}')
-    assert r.status_code == 400
-
-    names = ['aurora', 'guardian', 'northstar', 'sasquatch', 'shaman', 'snowbird']
-    users = [find_user(db, name=name) for name in names]
     r = await api_request(
         app,
         'groups/alphaflight/users',

jupyterhub/tests/test_roles.py

@@ -182,6 +182,7 @@ def test_orm_roles_delete_cascade(db):
                 'admin:users',
                 'admin:auth_state',
                 'users',
+                'list:users',
                 'read:users',
                 'users:activity',
                 'read:users:name',
@@ -194,6 +195,7 @@ def test_orm_roles_delete_cascade(db):
             ['users'],
             {
                 'users',
+                'list:users',
                 'read:users',
                 'users:activity',
                 'read:users:name',
@@ -216,6 +218,7 @@ def test_orm_roles_delete_cascade(db):
             {
                 'admin:groups',
                 'groups',
+                'list:groups',
                 'read:groups',
                 'read:roles:groups',
                 'read:groups:name',
@@ -226,6 +229,7 @@ def test_orm_roles_delete_cascade(db):
             {
                 'admin:groups',
                 'groups',
+                'list:groups',
                 'read:groups',
                 'read:roles:groups',
                 'read:groups:name',
@@ -852,7 +856,7 @@ async def test_server_token_role(app):
     [
         ('server', 'post', 'activity', 'same_user', 200),
         ('server', 'post', 'activity', 'other_user', 404),
-        ('server', 'get', 'users', 'same_user', 200),
+        ('server', 'get', 'users', 'same_user', 403),
         ('token', 'post', 'activity', 'same_user', 200),
         ('no_role', 'post', 'activity', 'same_user', 403),
     ],
@@ -890,17 +894,6 @@ async def test_server_role_api_calls(
     )
     assert r.status_code == response
 
-    if api_endpoint == 'users' and token_role == 'server':
-        reply = r.json()
-        assert len(reply) == 1
-
-        user_model = reply[0]
-        assert user_model['name'] == username
-        assert 'last_activity' in user_model.keys()
-        assert (
-            all(key for key in ['groups', 'roles', 'servers']) not in user_model.keys()
-        )
-
 
 async def test_oauth_allowed_roles(app, create_temp_role):
     allowed_roles = ['oracle', 'goose']
@@ -938,7 +931,7 @@ async def test_user_group_roles(app, create_temp_role):
 
     group_role = orm.Role.find(app.db, 'student-a')
     if not group_role:
-        create_temp_role(['read:groups!group=A'], 'student-a')
+        create_temp_role(['read:groups!group=A', 'list:groups!group=A'], 'student-a')
         roles.grant_role(app.db, group, rolename='student-a')
         group_role = orm.Role.find(app.db, 'student-a')
 
@@ -954,16 +947,16 @@ async def test_user_group_roles(app, create_temp_role):
     token = user.new_api_token()
 
     headers = {'Authorization': 'token %s' % token}
-    r = await api_request(app, 'users', method='get', headers=headers)
+    r = await api_request(app, f'users/{user.name}', method='get', headers=headers)
     assert r.status_code == 200
     r.raise_for_status()
     reply = r.json()
 
     print(reply)
 
-    assert len(reply[0]['roles']) == 1
-    assert reply[0]['name'] == 'jack'
-    assert group_role.name not in reply[0]['roles']
+    assert reply['name'] == 'jack'
+    assert len(reply['roles']) == 1
+    assert group_role.name not in reply['roles']
 
     headers = {'Authorization': 'token %s' % token}
     r = await api_request(app, 'groups', method='get', headers=headers)
@@ -972,18 +965,20 @@ async def test_user_group_roles(app, create_temp_role):
     reply = r.json()
 
     print(reply)
+    assert len(reply) == 1
+    assert reply[0]['name'] == 'A'
 
     headers = {'Authorization': 'token %s' % token}
-    r = await api_request(app, 'users', method='get', headers=headers)
+    r = await api_request(app, f'users/{user.name}', method='get', headers=headers)
     assert r.status_code == 200
     r.raise_for_status()
     reply = r.json()
 
     print(reply)
 
-    assert len(reply[0]['roles']) == 1
-    assert reply[0]['name'] == 'jack'
-    assert group_role.name not in reply[0]['roles']
+    assert reply['name'] == 'jack'
+    assert len(reply['roles']) == 1
+    assert group_role.name not in reply['roles']
 
 
 async def test_config_role_list():

jupyterhub/tests/test_scopes.py

@@ -1,4 +1,5 @@
 """Test scopes for API handlers"""
+from operator import itemgetter
 from unittest import mock
 
 import pytest
@@ -284,10 +285,10 @@ async def test_refuse_exceeding_token_permissions(
 async def test_exceeding_user_permissions(
     app, create_user_with_scopes, create_temp_role
 ):
-    user = create_user_with_scopes('read:users:groups')
+    user = create_user_with_scopes('list:users', 'read:users:groups')
     api_token = user.new_api_token()
     orm_api_token = orm.APIToken.find(app.db, token=api_token)
-    create_temp_role(['read:users'], 'reader_role')
+    create_temp_role(['list:users', 'read:users'], 'reader_role')
     roles.grant_role(app.db, orm_api_token, rolename='reader_role')
     headers = {'Authorization': 'token %s' % api_token}
     r = await api_request(app, 'users', headers=headers)
@@ -301,8 +302,8 @@ async def test_user_service_separation(app, mockservice_url, create_temp_role):
     name = mockservice_url.name
     user = add_user(app.db, name=name)
 
-    create_temp_role(['read:users'], 'reader_role')
-    create_temp_role(['read:users:groups'], 'subreader_role')
+    create_temp_role(['read:users', 'list:users'], 'reader_role')
+    create_temp_role(['read:users:groups', 'list:users'], 'subreader_role')
     roles.update_roles(app.db, user, roles=['subreader_role'])
     roles.update_roles(app.db, mockservice_url.orm, roles=['reader_role'])
     user.roles.remove(orm.Role.find(app.db, name='user'))
@@ -328,10 +329,10 @@ async def test_request_user_outside_group(app, create_user_with_scopes):
 
 
 async def test_user_filter(app, create_user_with_scopes):
-    user = create_user_with_scopes(
-        'read:users!user=lindsay', 'read:users!user=gob', 'read:users!user=oscar'
-    )
     name_in_scope = {'lindsay', 'oscar', 'gob'}
+    user = create_user_with_scopes(
+        *[f'list:users!user={name}' for name in name_in_scope]
+    )
     outside_scope = {'maeby', 'marta'}
     group_name = 'bluth'
     group = orm.Group.find(app.db, name=group_name)
@@ -359,7 +360,7 @@ async def test_service_filter(app, create_user_with_scopes):
     for service in services:
         app.services.append(service)
     app.init_services()
-    user = create_user_with_scopes('read:services!service=cull_idle')
+    user = create_user_with_scopes('list:services!service=cull_idle')
     r = await api_request(app, 'services', headers=auth_header(app.db, user.name))
     assert r.status_code == 200
     service_names = set(r.json().keys())
@@ -368,7 +369,7 @@ async def test_service_filter(app, create_user_with_scopes):
 
 async def test_user_filter_with_group(app, create_user_with_scopes):
     group_name = 'sitwell'
-    user1 = create_user_with_scopes(f'read:users!group={group_name}')
+    user1 = create_user_with_scopes(f'list:users!group={group_name}')
     user2 = create_user_with_scopes('self')
     external_user = create_user_with_scopes('self')
     name_set = {user1.name, user2.name}
@@ -393,7 +394,7 @@ async def test_group_scope_filter(app, create_user_with_scopes):
     in_groups = {'sitwell', 'bluth'}
     out_groups = {'austero'}
     user = create_user_with_scopes(
-        *(f'read:groups!group={group}' for group in in_groups)
+        *(f'list:groups!group={group}' for group in in_groups)
     )
     for group_name in in_groups | out_groups:
         group = orm.Group.find(app.db, name=group_name)
@@ -412,7 +413,7 @@ async def test_group_scope_filter(app, create_user_with_scopes):
 
 
 async def test_vertical_filter(app, create_user_with_scopes):
-    user = create_user_with_scopes('read:users:name')
+    user = create_user_with_scopes('list:users')
     r = await api_request(app, 'users', headers=auth_header(app.db, user.name))
     assert r.status_code == 200
     allowed_keys = {'name', 'kind', 'admin'}
@@ -420,23 +421,26 @@ async def test_vertical_filter(app, create_user_with_scopes):
 
 
 async def test_stacked_vertical_filter(app, create_user_with_scopes):
-    user = create_user_with_scopes('read:users:activity', 'read:users:groups')
+    user = create_user_with_scopes(
+        'list:users', 'read:users:activity', 'read:users:groups'
+    )
     r = await api_request(app, 'users', headers=auth_header(app.db, user.name))
     assert r.status_code == 200
-    allowed_keys = {'name', 'kind', 'groups', 'last_activity'}
-    result_model = set([key for user in r.json() for key in user.keys()])
+    allowed_keys = {'admin', 'name', 'kind', 'groups', 'last_activity'}
+    for user in r.json():
+        result_model = set(user)
         assert result_model == allowed_keys
 
 
 async def test_cross_filter(app, create_user_with_scopes):
-    user = create_user_with_scopes('read:users:activity', 'self')
+    user = create_user_with_scopes('read:users:activity', 'self', 'list:users')
     new_users = {'britta', 'jeff', 'annie'}
     for new_user_name in new_users:
         add_user(app.db, name=new_user_name)
     app.db.commit()
     r = await api_request(app, 'users', headers=auth_header(app.db, user.name))
     assert r.status_code == 200
-    restricted_keys = {'name', 'kind', 'last_activity'}
+    restricted_keys = {'admin', 'name', 'kind', 'last_activity'}
     key_in_full_model = 'created'
     for model_user in r.json():
         if model_user['name'] == user.name:
@@ -924,3 +928,117 @@ def test_intersect_groups(request, db, left, right, expected, groups):
     for a, b in [(left, right), (right, left)]:
         intersection = _intersect_expanded_scopes(set(left), set(right), db)
         assert intersection == set(expected)
+
+
+@mark.user
+@mark.parametrize(
+    "scopes, expected",
+    [
+        ("list:users", ['in-1', 'in-2', 'out-1', 'out-2', 'admin', 'user']),
+        ("read:users", 403),
+        ("list:users!server=irrelevant", 403),
+        ("list:users!user=nosuchuser", []),
+        ("list:users!group=nosuchgroup", []),
+        ("list:users!user=out-2", ['out-2']),
+        ("list:users!group=GROUP", ['in-1', 'in-2']),
+        (
+            ["list:users!group=GROUP", "list:users!user=out-2"],
+            ['in-1', 'in-2', 'out-2'],
+        ),
+    ],
+)
+async def test_list_users_filter(
+    app, group, create_service_with_scopes, scopes, expected
+):
+
+    # create users:
+    for i in (1, 2):
+        user = add_user(app.db, app, name=f'in-{i}')
+        group.users.append(user)
+        add_user(app.db, app, name=f'out-{i}')
+    app.db.commit()
+
+    if isinstance(scopes, str):
+        scopes = [scopes]
+
+    # in-group are in the group
+    # out-group are not in the group
+    scopes = [s.replace("GROUP", group.name).replace("IN", "ingroup") for s in scopes]
+
+    orm_service = create_service_with_scopes(*scopes)
+    token = orm_service.new_api_token()
+    r = await api_request(app, 'users', headers={"Authorization": f"token {token}"})
+    if isinstance(expected, int):
+        assert r.status_code == expected
+        return
+    r.raise_for_status()
+
+    expected_models = [
+        {
+            'name': name,
+            'admin': name == 'admin',
+            'kind': 'user',
+        }
+        for name in sorted(expected)
+    ]
+    assert sorted(r.json(), key=itemgetter('name')) == expected_models
+
+
+@mark.group
+@mark.parametrize(
+    "scopes, expected",
+    [
+        ("list:groups", ['group1', 'group2', 'group3']),
+        ("read:groups", 403),
+        ("list:groups!user=irrelevant", 403),
+        ("list:groups!group=nosuchgroup", []),
+        ("list:groups!group=group1", ['group1']),
+        (
+            ["list:groups!group=group1", "list:groups!group=group2"],
+            ['group1', 'group2'],
+        ),
+        (
+            # prefix match shouldn't match!
+            "list:groups!group=group",
+            [],
+        ),
+    ],
+)
+async def test_list_groups_filter(
+    request, app, create_service_with_scopes, scopes, expected
+):
+
+    # create groups:
+    groups = []
+    for i in (1, 2, 3):
+        group = orm.Group(name=f'group{i}')
+        groups.append(group)
+        app.db.add(group)
+    app.db.commit()
+
+    def cleanup_groups():
+        for g in groups:
+            app.db.delete(g)
+        app.db.commit()
+
+    request.addfinalizer(cleanup_groups)
+
+    if isinstance(scopes, str):
+        scopes = [scopes]
+
+    orm_service = create_service_with_scopes(*scopes)
+    token = orm_service.new_api_token()
+    r = await api_request(app, 'groups', headers={"Authorization": f"token {token}"})
+    if isinstance(expected, int):
+        assert r.status_code == expected
+        return
+    r.raise_for_status()
+
+    expected_models = [
+        {
+            'name': name,
+            'kind': 'group',
+        }
+        for name in sorted(expected)
+    ]
+    assert sorted(r.json(), key=itemgetter('name')) == expected_models