From 5c50161d857bfcc38a01038d3756756db720f674 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 14 May 2018 16:27:16 -0400 Subject: [PATCH] Limit robots displayed in entity search Before, we'd load *all* the robots, which can be a huge issue in namespaces with a large number of robots. Now, we only load the top-20 robots (as per recency in login), and we also limit the information returned to the entity search to save some bandwidth. Fixes https://jira.coreos.com/browse/QUAY-927 --- data/model/user.py | 3 +- endpoints/api/robot.py | 44 ++++++++++++----- endpoints/api/robot_models_interface.py | 19 +++++--- endpoints/api/robot_models_pre_oci.py | 9 ++-- endpoints/api/test/test_robot.py | 62 +++++++++++++++++++++++- static/js/directives/ui/entity-search.js | 8 ++- static/js/services/api-service.js | 2 +- 7 files changed, 121 insertions(+), 26 deletions(-) diff --git a/data/model/user.py b/data/model/user.py index 73d3936e1..0200e39fd 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -410,7 +410,7 @@ def _list_entity_robots(entity_name, include_metadata=True): return query -def list_entity_robot_permission_teams(entity_name, include_permissions=False): +def list_entity_robot_permission_teams(entity_name, limit=None, include_permissions=False): query = (_list_entity_robots(entity_name)) fields = [User.username, User.creation_date, User.last_accessed, FederatedLogin.service_ident, @@ -427,6 +427,7 @@ def list_entity_robot_permission_teams(entity_name, include_permissions=False): fields.append(Repository.name) fields.append(Team.name) + query = query.limit(limit).order_by(User.last_accessed.desc()) return TupleSelector(query, fields) diff --git a/endpoints/api/robot.py b/endpoints/api/robot.py index 757893783..867329323 100644 --- a/endpoints/api/robot.py +++ b/endpoints/api/robot.py @@ -31,9 +31,11 @@ CREATE_ROBOT_SCHEMA = { ROBOT_MAX_SIZE = 1024 * 1024 # 1 KB. -def robots_list(prefix, include_permissions=False): - robots = model.list_entity_robot_permission_teams(prefix, include_permissions=include_permissions) - return {'robots': [robot.to_dict() for robot in robots]} +def robots_list(prefix, include_permissions=False, include_token=False, limit=None): + robots = model.list_entity_robot_permission_teams(prefix, limit=limit, + include_token=include_token, + include_permissions=include_permissions) + return {'robots': [robot.to_dict(include_token=include_token) for robot in robots]} @resource('/v1/user/robots') @@ -46,10 +48,18 @@ class UserRobotList(ApiResource): @query_param('permissions', 'Whether to include repositories and teams in which the robots have permission.', type=truthy_bool, default=False) + @query_param('token', + 'If false, the robot\'s token is not returned.', + type=truthy_bool, default=True) + @query_param('limit', + 'If specified, the number of robots to return.', + type=int, default=None) def get(self, parsed_args): """ List the available robots for the user. """ user = get_authenticated_user() - return robots_list(user.username, include_permissions=parsed_args.get('permissions', False)) + return robots_list(user.username, include_token=parsed_args.get('token', True), + include_permissions=parsed_args.get('permissions', False), + limit=parsed_args.get('limit')) @resource('/v1/user/robots/') @@ -67,7 +77,7 @@ class UserRobot(ApiResource): """ Returns the user's robot with the specified name. """ parent = get_authenticated_user() robot = model.get_user_robot(robot_shortname, parent) - return robot.to_dict(include_metadata=True) + return robot.to_dict(include_metadata=True, include_token=True) @require_user_admin @nickname('createUserRobot') @@ -84,7 +94,7 @@ class UserRobot(ApiResource): 'description': create_data.get('description'), 'unstructured_metadata': create_data.get('unstructured_metadata'), }) - return robot.to_dict(include_metadata=True), 201 + return robot.to_dict(include_metadata=True, include_token=True), 201 @require_user_admin @nickname('deleteUserRobot') @@ -108,11 +118,23 @@ class OrgRobotList(ApiResource): @query_param('permissions', 'Whether to include repostories and teams in which the robots have permission.', type=truthy_bool, default=False) + @query_param('token', + 'If false, the robot\'s token is not returned.', + type=truthy_bool, default=True) + @query_param('limit', + 'If specified, the number of robots to return.', + type=int, default=None) def get(self, orgname, parsed_args): """ List the organization's robots. """ permission = OrganizationMemberPermission(orgname) if permission.can(): - return robots_list(orgname, include_permissions=parsed_args.get('permissions', False)) + include_token = (AdministerOrganizationPermission(orgname).can() and + parsed_args.get('token', True)) + include_permissions = (AdministerOrganizationPermission(orgname).can() and + parsed_args.get('permissions', False)) + return robots_list(orgname, include_permissions=include_permissions, + include_token=include_token, + limit=parsed_args.get('limit')) raise Unauthorized() @@ -135,7 +157,7 @@ class OrgRobot(ApiResource): permission = AdministerOrganizationPermission(orgname) if permission.can(): robot = model.get_org_robot(robot_shortname, orgname) - return robot.to_dict(include_metadata=True) + return robot.to_dict(include_metadata=True, include_token=True) raise Unauthorized() @@ -155,7 +177,7 @@ class OrgRobot(ApiResource): 'description': create_data.get('description'), 'unstructured_metadata': create_data.get('unstructured_metadata'), }) - return robot.to_dict(include_metadata=True), 201 + return robot.to_dict(include_metadata=True, include_token=True), 201 raise Unauthorized() @@ -228,7 +250,7 @@ class RegenerateUserRobot(ApiResource): parent = get_authenticated_user() robot = model.regenerate_user_robot_token(robot_shortname, parent) log_action('regenerate_robot_token', parent.username, {'robot': robot_shortname}) - return robot.to_dict() + return robot.to_dict(include_token=True) @resource('/v1/organization//robots//regenerate') @@ -247,6 +269,6 @@ class RegenerateOrgRobot(ApiResource): if permission.can(): robot = model.regenerate_org_robot_token(robot_shortname, orgname) log_action('regenerate_robot_token', orgname, {'robot': robot_shortname}) - return robot.to_dict() + return robot.to_dict(include_token=True) raise Unauthorized() diff --git a/endpoints/api/robot_models_interface.py b/endpoints/api/robot_models_interface.py index 707fc976f..c4a07d304 100644 --- a/endpoints/api/robot_models_interface.py +++ b/endpoints/api/robot_models_interface.py @@ -55,10 +55,9 @@ class RobotWithPermissions( :type description: string """ - def to_dict(self): - return { + def to_dict(self, include_token=False): + data = { 'name': self.name, - 'token': self.password, 'created': format_date(self.created) if self.created is not None else None, 'last_accessed': format_date(self.last_accessed) if self.last_accessed is not None else None, 'teams': [team.to_dict() for team in self.teams], @@ -66,6 +65,11 @@ class RobotWithPermissions( 'description': self.description, } + if include_token: + data['token'] = self.password + + return data + class Robot( namedtuple('Robot', [ @@ -86,15 +90,17 @@ class Robot( :type unstructured_metadata: dict """ - def to_dict(self, include_metadata=False): + def to_dict(self, include_metadata=False, include_token=False): data = { 'name': self.name, - 'token': self.password, 'created': format_date(self.created) if self.created is not None else None, 'last_accessed': format_date(self.last_accessed) if self.last_accessed is not None else None, 'description': self.description, } + if include_token: + data['token'] = self.password + if include_metadata: data['unstructured_metadata'] = self.unstructured_metadata @@ -171,7 +177,8 @@ class RobotInterface(object): """ @abstractmethod - def list_entity_robot_permission_teams(self, prefix, include_permissions=False): + def list_entity_robot_permission_teams(self, prefix, include_permissions=False, + include_token=False, limit=None): """ Returns: diff --git a/endpoints/api/robot_models_pre_oci.py b/endpoints/api/robot_models_pre_oci.py index 492455e81..6f5396440 100644 --- a/endpoints/api/robot_models_pre_oci.py +++ b/endpoints/api/robot_models_pre_oci.py @@ -11,8 +11,9 @@ class RobotPreOCIModel(RobotInterface): return [Permission(permission.repository.name, permission.repository.visibility.name, permission.role.name) for permission in permissions] - def list_entity_robot_permission_teams(self, prefix, include_permissions=False): - tuples = model.user.list_entity_robot_permission_teams(prefix, + def list_entity_robot_permission_teams(self, prefix, include_token=False, + include_permissions=False, limit=None): + tuples = model.user.list_entity_robot_permission_teams(prefix, limit=limit, include_permissions=include_permissions) robots = {} robot_teams = set() @@ -22,7 +23,7 @@ class RobotPreOCIModel(RobotInterface): if robot_name not in robots: robot_dict = { 'name': robot_name, - 'token': robot_tuple.get(FederatedLogin.service_ident), + 'token': robot_tuple.get(FederatedLogin.service_ident) if include_token else None, 'created': robot_tuple.get(User.creation_date), 'last_accessed': robot_tuple.get(User.last_accessed), 'description': robot_tuple.get(RobotAccountMetadata.description), @@ -56,7 +57,7 @@ class RobotPreOCIModel(RobotInterface): if repository_name not in robot_dict['repositories']: robot_dict['repositories'].append(repository_name) robots[robot_name] = RobotWithPermissions(robot_dict['name'], robot_dict['token'], - robot_dict['created'], + robot_dict['created'], robot_dict['last_accessed'], robot_dict['teams'], robot_dict['repositories'], diff --git a/endpoints/api/test/test_robot.py b/endpoints/api/test/test_robot.py index 2e3821176..0ab0183f2 100644 --- a/endpoints/api/test/test_robot.py +++ b/endpoints/api/test/test_robot.py @@ -4,7 +4,7 @@ import json from data import model from endpoints.api import api from endpoints.api.test.shared import conduct_api_call -from endpoints.api.robot import UserRobot, OrgRobot +from endpoints.api.robot import UserRobot, OrgRobot, UserRobotList, OrgRobotList from endpoints.test.shared import client_with_identity from test.test_ldap import mock_ldap @@ -36,3 +36,63 @@ def test_create_robot_with_metadata(endpoint, body, client): body = body or {} assert resp.json['description'] == (body.get('description') or '') assert resp.json['unstructured_metadata'] == (body.get('unstructured_metadata') or {}) + + +@pytest.mark.parametrize('endpoint, params', [ + (UserRobot, {'robot_shortname': 'dtrobot'}), + (OrgRobot, {'orgname': 'buynlarge', 'robot_shortname': 'coolrobot'}), +]) +def test_retrieve_robot(endpoint, params, app, client): + with client_with_identity('devtable', client) as cl: + result = conduct_api_call(cl, endpoint, 'GET', params, None) + assert result.json['token'] is not None + + +@pytest.mark.parametrize('endpoint, params', [ + (UserRobotList, {}), + (OrgRobotList, {'orgname': 'buynlarge'}), +]) +@pytest.mark.parametrize('include_token', [ + True, + False, +]) +@pytest.mark.parametrize('limit', [ + None, + 1, + 5, +]) +def test_retrieve_robots(endpoint, params, include_token, limit, app, client): + params['token'] = 'true' if include_token else 'false' + + if limit is not None: + params['limit'] = limit + + with client_with_identity('devtable', client) as cl: + result = conduct_api_call(cl, endpoint, 'GET', params, None) + + if limit is not None: + assert len(result.json['robots']) <= limit + + for robot in result.json['robots']: + assert (robot.get('token') is not None) == include_token + + +@pytest.mark.parametrize('username, is_admin', [ + ('devtable', True), + ('reader', False), +]) +@pytest.mark.parametrize('with_permissions', [ + True, + False, +]) +def test_retrieve_robots_token_permission(username, is_admin, with_permissions, app, client): + with client_with_identity(username, client) as cl: + params = {'orgname': 'buynlarge', 'token': 'true'} + if with_permissions: + params['permissions'] = 'true' + + result = conduct_api_call(cl, OrgRobotList, 'GET', params, None) + assert result.json['robots'] + for robot in result.json['robots']: + assert (robot.get('token') is not None) == is_admin + assert (robot.get('repositories') is not None) == (is_admin and with_permissions) diff --git a/static/js/directives/ui/entity-search.js b/static/js/directives/ui/entity-search.js index dc1f602e9..c95b7ca76 100644 --- a/static/js/directives/ui/entity-search.js +++ b/static/js/directives/ui/entity-search.js @@ -121,8 +121,12 @@ angular.module('quay').directive('entitySearch', function () { // Load the user/organization's robots (if applicable). if ($scope.isAdmin && isSupported('robot')) { requiredOperations++; - - ApiService.getRobots($scope.isOrganization ? $scope.namespace : null).then(function(resp) { + var params = { + 'token': false, + 'limit': 20 + }; + + ApiService.getRobots($scope.isOrganization ? $scope.namespace : null, null, params).then(function(resp) { $scope.page.robots = resp.robots; operationComplete(); }, operationComplete); diff --git a/static/js/services/api-service.js b/static/js/services/api-service.js index 1b1bd275b..9031e558f 100644 --- a/static/js/services/api-service.js +++ b/static/js/services/api-service.js @@ -71,7 +71,7 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService' if (used[paramName]) { continue; } var value = parameters[paramName]; - if (value) { + if (value != null) { url += isFirst ? '?' : '&'; url += paramName + '=' + encodeURIComponent(value) isFirst = false;