diff --git a/core/controllers/base.py b/core/controllers/base.py index e987a1d8f65b..9b8aee4ddb64 100755 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -397,9 +397,11 @@ def validate_and_normalize_args(self): 'Missing schema for %s method in %s handler class.' % ( request_method, handler_class_name)) + allow_string_to_bool_conversion = request_method in ['GET', 'DELETE'] normalized_arg_values, errors = ( payload_validator.validate( - handler_args, schema_for_request_method, extra_args_are_allowed) + handler_args, schema_for_request_method, extra_args_are_allowed, + allow_string_to_bool_conversion) ) self.normalized_payload = { diff --git a/core/controllers/base_test.py b/core/controllers/base_test.py index 701eeb5282c5..b6b5fa282804 100644 --- a/core/controllers/base_test.py +++ b/core/controllers/base_test.py @@ -1442,7 +1442,11 @@ def test_default_value_in_schema_conforms_with_schema(self): default_value_schema = {arg: schema} _, errors = payload_validator.validate( - default_value, default_value_schema, True) + default_value, + default_value_schema, + allowed_extra_args=True, + allow_string_to_bool_conversion=False + ) if len(errors) == 0: continue self.log_line( @@ -1641,6 +1645,12 @@ class MockHandlerWithDefaultGetSchema(base.BaseHandler): 'type': 'basestring' }, 'default_value': 'random_exp_id' + }, + 'apply_draft': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False } } } @@ -1731,7 +1741,7 @@ def test_can_access_exploration_with_default_value_in_schema(self): self.login(self.OWNER_EMAIL) with self.swap(self, 'testapp', self.mock_testapp3): - self.get_json('/mock_play_exploration') + self.get_json('/mock_play_exploration?apply_draft=true') csrf_token = self.get_new_csrf_token() with self.swap(self, 'testapp', self.mock_testapp4): diff --git a/core/controllers/domain_objects_validator.py b/core/controllers/domain_objects_validator.py index f26be6a9c021..34be72616983 100644 --- a/core/controllers/domain_objects_validator.py +++ b/core/controllers/domain_objects_validator.py @@ -27,13 +27,13 @@ from core.domain import collection_domain from core.domain import config_domain from core.domain import exp_domain +from core.domain import state_domain import python_utils -from typing import Any, Dict # isort:skip pylint: disable=wrong-import-order, wrong-import-position, unused-import, import-only-modules +from typing import Any, Dict, Optional, Union # isort:skip -def validate_exploration_change(obj): - # type: (Dict[String, Any]) -> None +def validate_exploration_change(obj: Dict[str, Any]) -> None: """Validates exploration change. Args: @@ -41,11 +41,10 @@ def validate_exploration_change(obj): """ # No explicit call to validate_dict method is necessary, because # ExplorationChange calls validate method while initialization. - exp_domain.ExplorationChange(obj) + exp_domain.ExplorationChange(obj) # type: ignore[no-untyped-call] -def validate_new_config_property_values(obj): - # type: (Dict[String, Any]) -> None +def validate_new_config_property_values(obj: Dict[str, Any]) -> None: """Validates new config property values. Args: @@ -56,40 +55,38 @@ def validate_new_config_property_values(obj): raise Exception( 'config property name should be a string, received' ': %s' % name) - config_property = config_domain.Registry.get_config_property(name) + config_property = config_domain.Registry.get_config_property(name) # type: ignore[no-untyped-call] if config_property is None: raise Exception('%s do not have any schema.' % name) config_property.normalize(value) -def validate_change_dict_for_blog_post(change_dict): - # type: (Dict[Any, Any]) -> None +def validate_change_dict_for_blog_post(change_dict: Dict[str, Any]) -> None: """Validates change_dict required for updating values of blog post. Args: change_dict: dict. Data that needs to be validated. """ if 'title' in change_dict: - blog_domain.BlogPost.require_valid_title( + blog_domain.BlogPost.require_valid_title( # type: ignore[no-untyped-call] change_dict['title'], True) if 'thumbnail_filename' in change_dict: - blog_domain.BlogPost.require_valid_thumbnail_filename( + blog_domain.BlogPost.require_valid_thumbnail_filename( # type: ignore[no-untyped-call] change_dict['thumbnail_filename']) if 'tags' in change_dict: - blog_domain.BlogPost.require_valid_tags( + blog_domain.BlogPost.require_valid_tags( # type: ignore[no-untyped-call] change_dict['tags'], False) # Validates that the tags in the change dict are from the list of # default tags set by admin. - list_of_default_tags = config_domain.Registry.get_config_property( + list_of_default_tags = config_domain.Registry.get_config_property( # type: ignore[no-untyped-call] 'list_of_default_tags_for_blog_post').value if not all(tag in list_of_default_tags for tag in change_dict['tags']): raise Exception( 'Invalid tags provided. Tags not in default tags list.') -def validate_collection_change(obj): - # type: (Dict[String, Any]) -> None +def validate_collection_change(obj: Dict[str, Any]) -> None: """Validates collection change. Args: @@ -97,11 +94,22 @@ def validate_collection_change(obj): """ # No explicit call to validate_dict method is necessary, because # CollectionChange calls validate method while initialization. - collection_domain.CollectionChange(obj) + collection_domain.CollectionChange(obj) # type: ignore[no-untyped-call] -def validate_email_dashboard_data(data): - # type: (Dict[String, Optional[Union[bool, int]]]) -> None +def validate_state_dict(state_dict: Dict[str, Any]) -> None: + """Validates state dict. + + Args: + state_dict: dict. Data that needs to be validated. + """ + validation_class = state_domain.State.from_dict(state_dict) # type: ignore[no-untyped-call] + validation_class.validate(None, True) + + +def validate_email_dashboard_data( + data: Dict[str, Optional[Union[bool, int]]] +) -> None: """Validates email dashboard data. Args: @@ -118,8 +126,7 @@ def validate_email_dashboard_data(data): raise Exception('400 Invalid input for query.') -def validate_task_entries(task_entries): - # type: (Dict[String, Any]) -> None +def validate_task_entries(task_entries: Dict[str, Any]) -> None: """Validates the task entry dict. Args: diff --git a/core/controllers/domain_objects_validator_test.py b/core/controllers/domain_objects_validator_test.py index 5d8b495ec39e..e22b6cf14dfe 100644 --- a/core/controllers/domain_objects_validator_test.py +++ b/core/controllers/domain_objects_validator_test.py @@ -21,13 +21,13 @@ from core.tests import test_utils import utils +from typing import Any, Dict # isort:skip class ValidateExplorationChangeTests(test_utils.GenericTestBase): """Tests to validate domain objects coming from API.""" - def test_incorrect_object_raises_exception(self): - # type: () -> None + def test_incorrect_object_raises_exception(self) -> None: incorrect_change_dict = { 'old_value': '', 'property_name': 'title', @@ -38,8 +38,7 @@ def test_incorrect_object_raises_exception(self): domain_objects_validator.validate_exploration_change( incorrect_change_dict) - def test_correct_object_do_not_raises_exception(self): - # type: () -> None + def test_correct_object_do_not_raises_exception(self) -> None: correct_change_dict = { 'cmd': 'edit_exploration_property', 'new_value': 'arbitary_new_value', @@ -53,8 +52,7 @@ def test_correct_object_do_not_raises_exception(self): class ValidateCollectionChangeTests(test_utils.GenericTestBase): """Tests to validate domain objects coming from API.""" - def test_incorrect_object_raises_exception(self): - # type: () -> None + def test_incorrect_object_raises_exception(self) -> None: incorrect_change_dict = { 'old_value': '', 'property_name': 'title', @@ -65,8 +63,7 @@ def test_incorrect_object_raises_exception(self): domain_objects_validator.validate_collection_change( incorrect_change_dict) - def test_correct_object_do_not_raises_exception(self): - # type: () -> None + def test_correct_object_do_not_raises_exception(self) -> None: correct_change_dict = { 'cmd': 'edit_collection_property', 'new_value': 'arbitary_new_value', @@ -80,21 +77,21 @@ def test_correct_object_do_not_raises_exception(self): class ValidateNewConfigPropertyValuesTests(test_utils.GenericTestBase): """Tests to validate config properties dict coming from API.""" - def test_invalid_object_raises_exception(self): + def test_invalid_object_raises_exception(self) -> None: config_properties = {'some_config_property': 20, } - with self.assertRaisesRegexp( + with self.assertRaisesRegexp( # type: ignore[no-untyped-call] Exception, 'some_config_property do not have any schema.'): domain_objects_validator.validate_new_config_property_values( config_properties) - config_properties = {1234: 20, } - with self.assertRaisesRegexp( + config_properties = {1234: 20, } # type: ignore[dict-item] + with self.assertRaisesRegexp( # type: ignore[no-untyped-call] Exception, 'config property name should be a string, received' ': %s' % 1234): domain_objects_validator.validate_new_config_property_values( config_properties) - def test_valid_object_raises_no_exception(self): + def test_valid_object_raises_no_exception(self) -> None: config_properties = { 'max_number_of_tags_assigned_to_blog_post': 20, } @@ -106,22 +103,22 @@ class ValidateChangeDictForBlogPost(test_utils.GenericTestBase): """Tests to validate change_dict containing updated values for blog post object coming from API.""" - def test_invalid_title_raises_exception(self): + def test_invalid_title_raises_exception(self) -> None: blog_post_change = { 'title': 123, 'tags': ['News'], } - with self.assertRaisesRegexp( + with self.assertRaisesRegexp( # type: ignore[no-untyped-call] utils.ValidationError, 'Title should be a string'): domain_objects_validator.validate_change_dict_for_blog_post( blog_post_change) - def test_invalid_tags_raises_exception(self): + def test_invalid_tags_raises_exception(self) -> None: blog_post_change = { 'title': 'Hello Bloggers', 'tags': ['News', 'Some Tag'], } - with self.assertRaisesRegexp( + with self.assertRaisesRegexp( # type: ignore[no-untyped-call] Exception, 'Invalid tags provided. Tags not in default' ' tags list.'): domain_objects_validator.validate_change_dict_for_blog_post( @@ -129,15 +126,15 @@ def test_invalid_tags_raises_exception(self): blog_post_change = { 'title': 'Hello', - 'tags': ['News', 123], + 'tags': ['News', 123], # type: ignore[list-item] } - with self.assertRaisesRegexp( + with self.assertRaisesRegexp( # type: ignore[no-untyped-call] Exception, 'Expected each tag in \'tags\' to be a string,' ' received: \'123\''): domain_objects_validator.validate_change_dict_for_blog_post( blog_post_change) - def test_valid_dict_raises_no_exception(self): + def test_valid_dict_raises_no_exception(self) -> None: blog_post_change = { 'title': 'Hello Bloggers', 'tags': ['News', 'Learners'], @@ -152,3 +149,80 @@ def test_valid_dict_raises_no_exception(self): } domain_objects_validator.validate_change_dict_for_blog_post( blog_post_change) + + +class ValidateStateDictInStateYamlHandler(test_utils.GenericTestBase): + """Tests to validate state_dict of StateYamlHandler.""" + + def test_valid_object_raises_no_exception(self) -> None: + state_dict = { + 'content': {'content_id': 'content', 'html': ''}, + 'param_changes': [], + 'interaction': { + 'solution': None, + 'answer_groups': [], + 'default_outcome': { + 'param_changes': [], + 'feedback': { + 'content_id': 'default_outcome', + 'html': '' + }, + 'dest': 'State A', + 'refresher_exploration_id': None, + 'missing_prerequisite_skill_id': None, + 'labelled_as_correct': False + }, + 'customization_args': { + 'rows': { + 'value': 1 + }, + 'placeholder': { + 'value': { + 'unicode_str': '', + 'content_id': 'ca_placeholder_0' + } + } + }, + 'confirmed_unclassified_answers': [], + 'id': 'TextInput', + 'hints': [] + }, + 'linked_skill_id': None, + 'recorded_voiceovers': { + 'voiceovers_mapping': { + 'content': {}, + 'default_outcome': {}, + 'ca_placeholder_0': {} + } + }, + 'classifier_model_id': None, + 'written_translations': { + 'translations_mapping': { + 'content': {}, + 'default_outcome': {}, + 'ca_placeholder_0': {} + } + }, + 'next_content_id_index': 1, + 'card_is_checkpoint': False, + 'solicit_answer_details': False + } + domain_objects_validator.validate_state_dict(state_dict) + + def test_invalid_object_raises_exception(self) -> None: + invalid_state_dict: Dict[str, Any] = { + 'classifier_model_id': None, + 'written_translations': { + 'translations_mapping': { + 'content': {}, + 'default_outcome': {}, + 'ca_placeholder_0': {} + } + }, + 'next_content_id_index': 1, + 'card_is_checkpoint': False, + 'solicit_answer_details': False + } + # The error is representing the keyerror. + with self.assertRaisesRegexp(Exception, 'content'): # type: ignore[no-untyped-call] + domain_objects_validator.validate_state_dict(invalid_state_dict) diff --git a/core/controllers/editor.py b/core/controllers/editor.py index 61254f5acc4c..efe46829d557 100644 --- a/core/controllers/editor.py +++ b/core/controllers/editor.py @@ -25,6 +25,7 @@ from constants import constants from core.controllers import acl_decorators from core.controllers import base +from core.controllers import domain_objects_validator as objects_validator from core.domain import email_manager from core.domain import exp_domain from core.domain import exp_fetchers @@ -45,9 +46,6 @@ def _require_valid_version(version_from_payload, exploration_version): """Check that the payload version matches the given exploration version.""" - if version_from_payload is None: - raise base.BaseHandler.InvalidInputException( - 'Invalid POST request: a version must be specified.') if version_from_payload != exploration_version: raise base.BaseHandler.InvalidInputException( @@ -56,15 +54,42 @@ def _require_valid_version(version_from_payload, exploration_version): % (exploration_version, version_from_payload)) +# Common schemas used in this file. +SCHEMA_FOR_EXPLORATION_ID = { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_regex_matched', + 'regex_pattern': constants.ENTITY_ID_REGEX + }] +} +SCHEMA_FOR_VERSION = { + 'type': 'int', + 'validators': [{ + 'id': 'is_at_least', + 'min_value': 1 # Version must be greater than zero. + }] +} + + class EditorHandler(base.BaseHandler): """Base class for all handlers for the editor page.""" - pass + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = {} class ExplorationPage(EditorHandler): """The editor page for a single exploration.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': {} + } + @acl_decorators.can_play_exploration def get(self, unused_exploration_id): """Handles GET requests.""" @@ -76,6 +101,51 @@ class ExplorationHandler(EditorHandler): """Page with editor data for a single exploration.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'v': { + 'schema': SCHEMA_FOR_VERSION, + 'default_value': None + }, + 'apply_draft': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False + } + }, + 'PUT': { + 'version': { + 'schema': SCHEMA_FOR_VERSION + }, + 'commit_message': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'has_length_at_most', + 'max_value': constants.MAX_COMMIT_MESSAGE_LENGTH + }] + }, + 'default_value': None + }, + 'change_list': { + 'schema': { + 'type': 'list', + 'items': { + 'type': 'object_dict', + 'validation_method': ( + objects_validator.validate_exploration_change) + } + } + } + }, + 'DELETE': {} + } @acl_decorators.can_play_exploration def get(self, exploration_id): @@ -83,8 +153,8 @@ def get(self, exploration_id): # 'apply_draft' and 'v'(version) are optional parameters because the # exploration history tab also uses this handler, and these parameters # are not used by that tab. - version = self.request.get('v', default_value=None) - apply_draft = self.request.get('apply_draft', default_value=False) + version = self.normalized_request.get('v') + apply_draft = self.normalized_request.get('apply_draft') user_settings = user_services.get_user_settings(self.user_id) has_seen_editor_tutorial = False @@ -116,33 +186,21 @@ def get(self, exploration_id): def put(self, exploration_id): """Updates properties of the given exploration.""" exploration = exp_fetchers.get_exploration_by_id(exploration_id) - version = self.payload.get('version') - if version is None: - raise base.BaseHandler.InvalidInputException( - 'Invalid POST request: a version must be specified.') + version = self.normalized_payload.get('version') + if version > exploration.version: raise base.BaseHandler.InvalidInputException( 'Trying to update version %s of exploration from version %s, ' 'which is not possible. Please reload the page and try again.' % (exploration.version, version)) - commit_message = self.payload.get('commit_message') - - if (commit_message is not None and - len(commit_message) > constants.MAX_COMMIT_MESSAGE_LENGTH): - raise self.InvalidInputException( - 'Commit messages must be at most %s characters long.' - % constants.MAX_COMMIT_MESSAGE_LENGTH) - - change_list_dict = self.payload.get('change_list') + commit_message = self.normalized_payload.get('commit_message') + change_list_dict = self.normalized_payload.get('change_list') - try: - change_list = [ - exp_domain.ExplorationChange(change) - for change in change_list_dict - ] - except utils.ValidationError as e: - raise self.InvalidInputException(e) + change_list = [ + exp_domain.ExplorationChange(change) + for change in change_list_dict + ] changes_are_mergeable = exp_services.are_changes_mergeable( exploration_id, version, change_list) @@ -193,6 +251,14 @@ class UserExplorationPermissionsHandler(EditorHandler): """Handles user permissions for a particular exploration.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': {} + } @acl_decorators.can_play_exploration def get(self, exploration_id): @@ -227,17 +293,63 @@ def get(self, exploration_id): class ExplorationRightsHandler(EditorHandler): """Handles management of exploration editing rights.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'PUT': { + 'version': { + 'schema': SCHEMA_FOR_VERSION + }, + 'make_community_owned': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False + }, + 'new_member_username': { + 'schema': { + 'type': 'basestring' + }, + 'default_value': None + }, + 'new_member_role': { + 'schema': { + 'type': 'basestring', + 'choices': feconf.ALLOWED_ACTIVITY_ROLES + }, + 'default_value': None + }, + 'viewable_if_private': { + 'schema': { + 'type': 'bool' + }, + 'default_value': None + } + }, + 'DELETE': { + 'username': { + 'schema': { + 'type': 'basestring' + } + } + } + } + @acl_decorators.can_modify_exploration_roles def put(self, exploration_id): """Updates the editing rights for the given exploration.""" exploration = exp_fetchers.get_exploration_by_id(exploration_id) - version = self.payload.get('version') + version = self.normalized_payload.get('version') _require_valid_version(version, exploration.version) - make_community_owned = self.payload.get('make_community_owned') - new_member_username = self.payload.get('new_member_username') - new_member_role = self.payload.get('new_member_role') - viewable_if_private = self.payload.get('viewable_if_private') + make_community_owned = ( + self.normalized_payload.get('make_community_owned')) + new_member_username = self.normalized_payload.get('new_member_username') + new_member_role = self.normalized_payload.get('new_member_role') + viewable_if_private = self.normalized_payload.get('viewable_if_private') if new_member_username: new_member_id = user_services.get_user_id_from_username( @@ -280,7 +392,7 @@ def put(self, exploration_id): @acl_decorators.can_modify_exploration_roles def delete(self, exploration_id): """Deletes user roles from the exploration.""" - username = self.request.get('username') + username = self.normalized_request.get('username') user_id = user_services.get_user_id_from_username(username) if user_id is None: raise self.InvalidInputException( @@ -300,6 +412,22 @@ def delete(self, exploration_id): class ExplorationStatusHandler(EditorHandler): """Handles publishing of an exploration.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'PUT': { + 'make_public': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False + } + } + } + def _publish_exploration(self, exploration_id): """Publish an exploration. @@ -323,7 +451,7 @@ def _publish_exploration(self, exploration_id): def put(self, exploration_id): make_public = self.payload.get('make_public') - if make_public is not None: + if make_public: self._publish_exploration(exploration_id) self.render_json({ @@ -335,14 +463,32 @@ def put(self, exploration_id): class ExplorationModeratorRightsHandler(EditorHandler): """Handles management of exploration rights by moderators.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'PUT': { + 'email_body': { + 'schema': { + 'type': 'basestring' + } + }, + 'version': { + 'schema': SCHEMA_FOR_VERSION + } + } + } + @acl_decorators.can_access_moderator_page def put(self, exploration_id): """Unpublishes the given exploration, and sends an email to all its owners. """ exploration = exp_fetchers.get_exploration_by_id(exploration_id) - email_body = self.payload.get('email_body') - version = self.payload.get('version') + email_body = self.normalized_payload.get('email_body') + version = self.normalized_payload.get('version') _require_valid_version(version, exploration.version) # If moderator emails can be sent, check that all the prerequisites are @@ -377,6 +523,31 @@ class UserExplorationEmailsHandler(EditorHandler): exploration. """ + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'PUT': { + 'mute': { + 'schema': { + 'type': 'bool' + }, + 'default_value': None + }, + 'message_type': { + 'schema': { + 'type': 'basestring', + 'choices': [ + feconf.MESSAGE_TYPE_FEEDBACK, + feconf.MESSAGE_TYPE_SUGGESTION + ] + } + } + } + } + @acl_decorators.can_edit_exploration def put(self, exploration_id): """Updates the email notification preferences for the given exploration. @@ -388,8 +559,8 @@ def put(self, exploration_id): InvalidInputException. Invalid message type. """ - mute = self.payload.get('mute') - message_type = self.payload.get('message_type') + mute = self.normalized_payload.get('mute') + message_type = self.normalized_payload.get('message_type') if message_type == feconf.MESSAGE_TYPE_FEEDBACK: user_services.set_email_preferences_for_exploration( @@ -398,9 +569,6 @@ def put(self, exploration_id): user_services.set_email_preferences_for_exploration( self.user_id, exploration_id, mute_suggestion_notifications=mute) - else: - raise self.InvalidInputException( - 'Invalid message type.') exploration_email_preferences = ( user_services.get_email_preferences_for_exploration( @@ -416,18 +584,38 @@ class ExplorationFileDownloader(EditorHandler): """ GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_DOWNLOADABLE + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'v': { + 'schema': SCHEMA_FOR_VERSION, + 'default_value': None + }, + 'output_format': { + 'schema': { + 'type': 'basestring', + 'choices': [ + feconf.OUTPUT_FORMAT_ZIP, + feconf.OUTPUT_FORMAT_JSON + ] + }, + 'default_value': feconf.OUTPUT_FORMAT_ZIP + } + } + } @acl_decorators.can_download_exploration def get(self, exploration_id): """Handles GET requests.""" exploration = exp_fetchers.get_exploration_by_id(exploration_id) + version = self.normalized_request.get('v') + output_format = self.normalized_request.get('output_format') - version_str = self.request.get('v', default_value=exploration.version) - output_format = self.request.get('output_format', default_value='zip') - - try: - version = int(version_str) - except ValueError: + if version is None: version = exploration.version # If the title of the exploration has changed, we use the new title. @@ -446,9 +634,6 @@ def get(self, exploration_id): elif output_format == feconf.OUTPUT_FORMAT_JSON: self.render_json(exp_services.export_states_to_yaml( exploration_id, version=version)) - else: - raise self.InvalidInputException( - 'Unrecognized output format %s' % output_format) class StateYamlHandler(EditorHandler): @@ -458,14 +643,36 @@ class StateYamlHandler(EditorHandler): layer. """ + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'POST': { + 'state_dict': { + 'schema': { + 'type': 'object_dict', + 'validation_method': objects_validator.validate_state_dict + } + }, + 'width': { + 'schema': { + 'type': 'int', + 'validators': [{ + 'id': 'is_at_least', + 'min_value': 1 # Width must be greater than zero. + }] + } + } + } + } + @acl_decorators.can_play_exploration def post(self, unused_exploration_id): """Handles POST requests.""" - state_dict = self.payload.get('state_dict') - width = self.payload.get('width') - - if not width or not state_dict: - raise self.PageNotFoundException + state_dict = self.normalized_payload.get('state_dict') + width = self.normalized_payload.get('width') self.render_json({ 'yaml': state_domain.State.convert_state_dict_to_yaml( @@ -477,6 +684,12 @@ class ExplorationSnapshotsHandler(EditorHandler): """Returns the exploration snapshot history.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_play_exploration def get(self, exploration_id): @@ -501,22 +714,29 @@ def get(self, exploration_id): class ExplorationRevertHandler(EditorHandler): """Reverts an exploration to an older version.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'POST': { + 'current_version': { + 'schema': SCHEMA_FOR_VERSION + }, + 'revert_to_version': { + 'schema': SCHEMA_FOR_VERSION + } + } + } + @acl_decorators.can_edit_exploration def post(self, exploration_id): """Handles POST requests.""" - current_version = self.payload.get('current_version') - revert_to_version = self.payload.get('revert_to_version') - - if not isinstance(revert_to_version, int): - raise self.InvalidInputException( - 'Expected an integer version to revert to; received %s.' % - revert_to_version) - if not isinstance(current_version, int): - raise self.InvalidInputException( - 'Expected an integer current version; received %s.' % - current_version) + current_version = self.normalized_payload.get('current_version') + revert_to_version = self.normalized_payload.get('revert_to_version') - if revert_to_version < 1 or revert_to_version >= current_version: + if revert_to_version >= current_version: raise self.InvalidInputException( 'Cannot revert to version %s from version %s.' % (revert_to_version, current_version)) @@ -533,6 +753,12 @@ class ExplorationStatisticsHandler(EditorHandler): """ GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_view_exploration_stats def get(self, exploration_id): @@ -548,14 +774,24 @@ class StateInteractionStatsHandler(EditorHandler): """Returns detailed learner answer statistics for a state.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + }, + 'state_name': { + 'schema': { + 'type': 'basestring' + } + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_view_exploration_stats - def get(self, exploration_id, escaped_state_name): + def get(self, exploration_id, state_name): """Handles GET requests.""" current_exploration = exp_fetchers.get_exploration_by_id( exploration_id) - state_name = utils.unescape_encoded_uri_component(escaped_state_name) if state_name not in current_exploration.states: logging.exception('Could not find state: %s' % state_name) logging.exception('Available states: %s' % ( @@ -573,11 +809,23 @@ class FetchIssuesHandler(EditorHandler): """ GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'exp_version': { + 'schema': SCHEMA_FOR_VERSION + } + } + } @acl_decorators.can_view_exploration_stats def get(self, exp_id): """Handles GET requests.""" - exp_version = self.request.get('exp_version') + exp_version = self.normalized_request.get('exp_version') exp_issues = stats_services.get_exp_issues(exp_id, exp_version) if exp_issues is None: raise self.PageNotFoundException( @@ -596,6 +844,17 @@ class FetchPlaythroughHandler(EditorHandler): """Handler used for retrieving a playthrough.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + }, + 'playthrough_id': { + 'schema': { + 'type': 'basestring' + } + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_view_exploration_stats def get(self, unused_exploration_id, playthrough_id): @@ -614,16 +873,31 @@ class ResolveIssueHandler(EditorHandler): instances are deleted. """ + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'POST': { + 'exp_issue_dict': { + 'schema': { + 'type': 'object_dict', + 'object_class': stats_domain.ExplorationIssue + }, + 'default_value': None + }, + 'exp_version': { + 'schema': SCHEMA_FOR_VERSION + } + } + } + @acl_decorators.can_edit_exploration def post(self, exp_id): """Handles POST requests.""" - exp_issue_dict = self.payload.get('exp_issue_dict') - try: - stats_domain.ExplorationIssue.from_dict(exp_issue_dict) - except utils.ValidationError as e: - raise self.PageNotFoundException(e) - - exp_version = self.payload.get('exp_version') + exp_issue_dict = self.normalized_payload.get('exp_issue_dict') + exp_version = self.normalized_payload.get('exp_version') exp_issues = stats_services.get_exp_issues(exp_id, exp_version) if exp_issues is None: @@ -659,16 +933,51 @@ class ImageUploadHandler(EditorHandler): """Handles image uploads.""" _decorator = None + URL_PATH_ARGS_SCHEMAS = { + 'entity_type': { + 'schema': { + 'type': 'basestring' + } + }, + 'entity_id': { + 'schema': { + 'type': 'basestring' + } + } + } + HANDLER_ARGS_SCHEMAS = { + 'POST': { + 'image': { + 'schema': { + 'type': 'basestring' + } + }, + 'filename': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_regex_matched', + 'regex_pattern': r'\w+[.]\w+' + }] + } + }, + 'filename_prefix': { + 'schema': { + 'type': 'basestring', + 'choices': ['thumbnail', 'image'] + }, + 'default_value': constants.ASSET_TYPE_IMAGE + } + } + } @acl_decorators.can_edit_entity def post(self, entity_type, entity_id): """Saves an image uploaded by a content creator.""" - raw = self.request.get('image') - filename = self.payload.get('filename') - filename_prefix = self.payload.get('filename_prefix') - if filename_prefix is None: - filename_prefix = constants.ASSET_TYPE_IMAGE + raw = self.normalized_request.get('image') + filename = self.normalized_payload.get('filename') + filename_prefix = self.normalized_payload.get('filename_prefix') try: file_format = image_validation_services.validate_image_and_filename( @@ -698,6 +1007,15 @@ def post(self, entity_type, entity_id): class StartedTutorialEventHandler(EditorHandler): """Records that this user has started the state editor tutorial.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'POST': {} + } + @acl_decorators.can_play_exploration def post(self, unused_exploration_id): """Handles GET requests.""" @@ -708,21 +1026,57 @@ def post(self, unused_exploration_id): class EditorAutosaveHandler(ExplorationHandler): """Handles requests from the editor for draft autosave.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'PUT': { + 'version': { + 'schema': SCHEMA_FOR_VERSION + }, + 'change_list': { + 'schema': { + 'type': 'list', + 'items': { + 'type': 'object_dict', + 'validation_method': ( + objects_validator.validate_exploration_change) + } + } + } + }, + 'POST': {}, + # Below two methods are not defined in handler class but they must be + # present in schema since these two are inherited from its parent class. + 'GET': { + 'v': { + 'schema': SCHEMA_FOR_VERSION, + 'default_value': None + }, + 'apply_draft': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False + } + }, + 'DELETE': {} + } + @acl_decorators.can_save_exploration def put(self, exploration_id): """Handles PUT requests for draft updation.""" # Raise an Exception if the draft change list fails non-strict # validation. - try: - change_list_dict = self.payload.get('change_list') - change_list = [ - exp_domain.ExplorationChange(change) - for change in change_list_dict] - except utils.ValidationError as e: - # We leave any pre-existing draft changes in the datastore. - raise self.InvalidInputException(e) + change_list_dict = self.normalized_payload.get('change_list') + change_list = [ + exp_domain.ExplorationChange(change) + for change in change_list_dict + ] - version = self.payload.get('version') + version = self.normalized_payload.get('version') exploration_rights = rights_manager.get_exploration_rights( exploration_id) can_edit = rights_manager.check_can_edit_activity( @@ -766,6 +1120,12 @@ class StateAnswerStatisticsHandler(EditorHandler): """Returns basic learner answer statistics for a state.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_view_exploration_stats def get(self, unused_exploration_id): @@ -778,6 +1138,12 @@ class TopUnresolvedAnswersHandler(EditorHandler): """Returns a list of top N unresolved answers.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = {'GET': {}} @acl_decorators.can_edit_exploration def get(self, unused_exploration_id): @@ -790,6 +1156,36 @@ class LearnerAnswerInfoHandler(EditorHandler): """Handles the learner answer info for an exploration state.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'entity_type': { + 'schema': { + 'type': 'basestring', + 'choices': [ + feconf.ENTITY_TYPE_EXPLORATION, + feconf.ENTITY_TYPE_QUESTION + ] + } + }, + 'entity_id': { + 'schema': SCHEMA_FOR_EXPLORATION_ID + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': {}, + 'DELETE': { + 'state_name': { + 'schema': { + 'type': 'basestring' + }, + 'default_value': None + }, + 'learner_answer_info_id': { + 'schema': { + 'type': 'basestring' + } + } + } + } @acl_decorators.can_play_entity def get(self, entity_type, entity_id): @@ -850,7 +1246,7 @@ def delete(self, entity_type, entity_id): raise self.PageNotFoundException if entity_type == feconf.ENTITY_TYPE_EXPLORATION: - state_name = self.request.get('state_name') + state_name = self.normalized_request.get('state_name') if not state_name: raise self.InvalidInputException state_reference = ( @@ -860,9 +1256,9 @@ def delete(self, entity_type, entity_id): state_reference = ( stats_services.get_state_reference_for_question( entity_id)) - learner_answer_info_id = self.request.get('learner_answer_info_id') - if not learner_answer_info_id: - raise self.PageNotFoundException + learner_answer_info_id = ( + self.normalized_request.get('learner_answer_info_id')) + stats_services.delete_learner_answer_info( entity_type, state_reference, learner_answer_info_id) self.render_json({}) diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py index ebe97b7026b3..ad43a1501025 100644 --- a/core/controllers/editor_test.py +++ b/core/controllers/editor_test.py @@ -204,7 +204,8 @@ def _put_and_expect_400_error(payload): # A request with no version number is invalid. response_dict = _put_and_expect_400_error(_get_payload('New state')) - self.assertIn('a version must be specified', response_dict['error']) + self.assertIn( + 'Missing key in handler args: version.', response_dict['error']) # A request with the wrong version number is invalid. response_dict = _put_and_expect_400_error( @@ -622,7 +623,7 @@ def test_exploration_download_handler_for_default_exploration(self): # Download to JSON string using download handler. self.maxDiff = None download_url = ( - '/createhandler/download/%s?output_format=%s&width=50' % + '/createhandler/download/%s?output_format=%s' % (exp_id, feconf.OUTPUT_FORMAT_JSON)) response = self.get_json(download_url) @@ -636,16 +637,6 @@ def test_exploration_download_handler_for_default_exploration(self): response = self.get_json(download_url) self.assertEqual(['Introduction'], list(response.keys())) - # Check downloading an invalid version results in downloading the - # latest version. - download_url = ( - '/createhandler/download/%s?output_format=%s&v=xxx' % - (exp_id, feconf.OUTPUT_FORMAT_JSON)) - response = self.get_json(download_url) - self.assertEqual(self.SAMPLE_JSON_CONTENT, response) - self.assertEqual( - ['Introduction', 'State A', 'State B'], list(response.keys())) - self.logout() def test_exploration_download_handler_with_unicode_title(self): @@ -740,7 +731,7 @@ def test_state_yaml_handler_with_no_state_dict_raises_error(self): self.post_json( '/createhandler/state_yaml/%s' % exp_id, {}, - csrf_token=csrf_token, expected_status_int=404) + csrf_token=csrf_token, expected_status_int=400) self.logout() @@ -748,7 +739,7 @@ def test_exploration_download_handler_with_invalid_exploration_id(self): self.login(self.OWNER_EMAIL) self.get_json( - '/createhandler/download/invalid_exploration_id', + '/createhandler/download/invalid_id', expected_status_int=404) self.logout() @@ -768,9 +759,12 @@ def test_exploration_download_handler_with_invalid_output_format(self): '/createhandler/download/%s?output_format=invalid_output_format' % (exp_id), expected_status_int=400) - self.assertEqual( - response['error'], - 'Unrecognized output format invalid_output_format') + error_msg = ( + 'Schema validation for \'output_format\' failed: Received ' + 'invalid_output_format which is not in the allowed range of ' + 'choices: [\'zip\', \'json\']' + ) + self.assertEqual(response['error'], error_msg) self.logout() @@ -785,7 +779,7 @@ def test_get_with_invalid_exploration_id_raises_error(self): self.login(self.OWNER_EMAIL) self.get_json( - '/createhandler/snapshots/invalid_exploration_id', + '/createhandler/snapshots/invalid_id', expected_status_int=404) self.logout() @@ -841,7 +835,7 @@ def test_get_with_invalid_exploration_id_raises_error(self): self.login(self.OWNER_EMAIL) self.get_json( - '/createhandler/statistics/invalid_exploration_id', + '/createhandler/statistics/invalid_id', expected_status_int=404) self.logout() @@ -946,7 +940,7 @@ def test_get_with_invalid_exploration_id_raises_error(self): self.get_json( '/createhandler/state_interaction_stats/%s/%s' % ( - 'invalid_exp_id', 'state_name'), + 'invalid_id', 'state_name'), expected_status_int=404) self.logout() @@ -1002,7 +996,7 @@ class ExplorationDeletionRightsTests(BaseEditorControllerTests): def test_deletion_rights_for_unpublished_exploration(self): """Test rights management for deletion of unpublished explorations.""" - unpublished_exp_id = 'unpublished_eid' + unpublished_exp_id = 'unpub_eid' # Unpublished exploration id. exploration = exp_domain.Exploration.create_default_exploration( unpublished_exp_id) exp_services.save_new_exploration(self.owner_id, exploration) @@ -1031,7 +1025,7 @@ def test_deletion_rights_for_unpublished_exploration(self): def test_deletion_rights_for_published_exploration(self): """Test rights management for deletion of published explorations.""" - published_exp_id = 'published_eid' + published_exp_id = 'pub_eid' # Published exploration id. exploration = exp_domain.Exploration.create_default_exploration( published_exp_id, title='A title', category='A category') exp_services.save_new_exploration(self.owner_id, exploration) @@ -1090,7 +1084,7 @@ def mock_logging_function(msg, *_): with self.swap(logging, 'info', mock_logging_function), self.swap( logging, 'debug', mock_logging_function): # Checking for non-moderator/non-admin. - exp_id = 'unpublished_eid' + exp_id = 'unpub_eid' # Unpublished exploration id exploration = exp_domain.Exploration.create_default_exploration( exp_id) exp_services.save_new_exploration(self.owner_id, exploration) @@ -1109,7 +1103,7 @@ def mock_logging_function(msg, *_): # Checking for moderator. observed_log_messages = [] - exp_id = 'unpublished_eid3' + exp_id = 'unpub_eid3' # Unpublished exploration id. exploration = exp_domain.Exploration.create_default_exploration( exp_id) exp_services.save_new_exploration(self.moderator_id, exploration) @@ -1172,19 +1166,14 @@ def test_reverting_to_old_exploration(self): csrf_token = self.get_new_csrf_token() # May not revert to any version that's not 1. - for rev_version in (-1, 0, 2, 3, 4, '1', ()): + for rev_version in (2, 3, 4, '10'): response_dict = self.post_json( '/createhandler/revert/%s' % self.EXP_ID, { 'current_version': 2, 'revert_to_version': rev_version }, csrf_token=csrf_token, expected_status_int=400) - # Check error message. - if not isinstance(rev_version, int): - self.assertIn('Expected an integer', response_dict['error']) - else: - self.assertIn( - 'Cannot revert to version', response_dict['error']) + self.assertIn('Cannot revert to version', response_dict['error']) # Check that exploration is really not reverted to old version. reader_dict = self.get_json( @@ -1196,6 +1185,18 @@ def test_reverting_to_old_exploration(self): self.assertIn('ABC', init_content) self.assertNotIn('Hi, welcome to Oppia!', init_content) + # May not revert to any version that's not convertible to int. + for rev_version in ('abc', ()): + response_dict = self.post_json( + '/createhandler/revert/%s' % self.EXP_ID, { + 'current_version': 2, + 'revert_to_version': rev_version + }, csrf_token=csrf_token, expected_status_int=400) + + self.assertIn( + 'Schema validation for \'revert_to_version\' ' + 'failed:', response_dict['error']) + # Revert to version 1. rev_version = 1 response_dict = self.post_json( @@ -1261,9 +1262,11 @@ def test_revert_with_invalid_current_version_raises_error(self): 'revert_to_version': 1 }, csrf_token=csrf_token, expected_status_int=400) - self.assertEqual( - response['error'], - 'Expected an integer current version; received invalid_version.') + error_msg = ( + 'Schema validation for \'current_version\' failed: Could not ' + 'convert str to int: invalid_version' + ) + self.assertEqual(response['error'], error_msg) class ExplorationEditRightsTest(BaseEditorControllerTests): @@ -1500,7 +1503,6 @@ def test_for_deassign_editor_role(self): self.login(self.OWNER_EMAIL) self.delete_json( rights_url, params={ - 'member_role': rights_domain.ROLE_EDITOR, 'username': self.COLLABORATOR_USERNAME }) self.logout() @@ -1530,7 +1532,6 @@ def test_for_deassign_sole_owner_from_exploration(self): response = self.delete_json( rights_url, params={ - 'member_role': rights_domain.ROLE_OWNER, 'username': self.OWNER_USERNAME }, expected_status_int=400) self.assertEqual( @@ -1601,7 +1602,6 @@ def test_for_deassign_viewer_role_from_exploration(self): self.login(self.OWNER_EMAIL) self.delete_json( rights_url, params={ - 'member_role': rights_domain.ROLE_VIEWER, 'username': self.VIEWER_USERNAME }) self.logout() @@ -1618,7 +1618,7 @@ def test_for_checking_username_is_valid(self): self.save_new_valid_exploration(exp_id, self.owner_id) response = self.delete_json( '%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id), - expected_status_int=400) + params={'username': 'any_username'}, expected_status_int=400) self.assertEqual( response['error'], 'Sorry, we could not find the specified user.') self.logout() @@ -1710,7 +1710,7 @@ def test_get_with_invalid_version_raises_error(self): self.save_new_valid_exploration(exp_id, self.owner_id) self.get_json( '%s/%s' % (feconf.EXPLORATION_DATA_PREFIX, exp_id), - params={'v': 'invalid_version'}, expected_status_int=404) + params={'v': '546'}, expected_status_int=404) self.logout() def test_put_with_long_commit_message_raises_error(self): @@ -1726,6 +1726,7 @@ def test_put_with_long_commit_message_raises_error(self): exploration = exp_fetchers.get_exploration_by_id(exp_id) exploration.add_states(['State A', 'State 2', 'State 3']) + long_commit_message = 'a' * (constants.MAX_COMMIT_MESSAGE_LENGTH + 1) csrf_token = self.get_new_csrf_token() @@ -1733,8 +1734,7 @@ def test_put_with_long_commit_message_raises_error(self): '%s/%s' % (feconf.EXPLORATION_DATA_PREFIX, exp_id), { 'version': exploration.version, - 'commit_message': - 'a' * (constants.MAX_COMMIT_MESSAGE_LENGTH + 1), + 'commit_message': long_commit_message, 'change_list': [{ 'cmd': 'add_state', 'state_name': 'State 4' @@ -1748,9 +1748,13 @@ def test_put_with_long_commit_message_raises_error(self): csrf_token=csrf_token, expected_status_int=400 ) - self.assertEqual( - response_dict['error'], - 'Commit messages must be at most 375 characters long.') + + error_msg = ( + 'Schema validation for \'commit_message\' failed: Validation ' + 'failed: has_length_at_most ({\'max_value\': 375}) for object %s' + % long_commit_message + ) + self.assertEqual(response_dict['error'], error_msg) def test_put_with_invalid_new_member_raises_error(self): self.login(self.OWNER_EMAIL) @@ -1857,7 +1861,7 @@ def test_can_not_assign_roles_with_invalid_payload_version(self): self.assertEqual( response_dict['error'], - 'Invalid POST request: a version must be specified.') + 'Missing key in handler args: version.') # Raises error as version from payload does not match the exploration # version. @@ -1889,8 +1893,6 @@ def test_user_exploration_emails_handler(self): exp_id, self.owner_id, title='Title for emails handler test!', category='Category') - exploration = exp_fetchers.get_exploration_by_id(exp_id) - csrf_token = self.get_new_csrf_token() exp_email_preferences = ( user_services.get_email_preferences_for_exploration( @@ -1902,7 +1904,6 @@ def test_user_exploration_emails_handler(self): emails_url = '%s/%s' % (feconf.USER_EXPLORATION_EMAILS_PREFIX, exp_id) self.put_json( emails_url, { - 'version': exploration.version, 'mute': True, 'message_type': 'feedback' }, csrf_token=csrf_token) @@ -1915,13 +1916,11 @@ def test_user_exploration_emails_handler(self): self.put_json( emails_url, { - 'version': exploration.version, 'mute': True, 'message_type': 'suggestion' }, csrf_token=csrf_token) self.put_json( emails_url, { - 'version': exploration.version, 'mute': False, 'message_type': 'feedback' }, csrf_token=csrf_token) @@ -1945,7 +1944,12 @@ def test_put_with_invalid_message_type_raises_error(self): {'message_type': 'invalid_message_type'}, csrf_token=csrf_token, expected_status_int=400) - self.assertEqual(response['error'], 'Invalid message type.') + error_msg = ( + 'Schema validation for \'message_type\' failed: Received ' + 'invalid_message_type which is not in the allowed range ' + 'of choices: [\'feedback\', \'suggestion\']' + ) + self.assertEqual(response['error'], error_msg) self.logout() @@ -1996,7 +2000,7 @@ def test_error_cases_for_email_sending(self): 'version': 1, }, csrf_token=csrf_token, expected_status_int=400) self.assertIn( - 'Moderator actions should include an email', + 'Missing key in handler args: email_body.', response_dict['error']) response_dict = self.put_json( @@ -2004,14 +2008,15 @@ def test_error_cases_for_email_sending(self): 'email_body': '', 'version': 1, }, csrf_token=csrf_token, expected_status_int=400) - self.assertIn( - 'Moderator actions should include an email', - response_dict['error']) + + error_msg = ( + 'Moderator actions should include an email to the recipient.' + ) + self.assertIn(error_msg, response_dict['error']) # Try to unpublish the exploration even if the relevant feconf # flags are not set. This should cause a system error. valid_payload = { - 'action': feconf.MODERATOR_ACTION_UNPUBLISH_EXPLORATION, 'email_body': 'Your exploration is featured!', 'version': 1, } @@ -2043,7 +2048,6 @@ def test_email_is_sent_correctly_when_unpublishing(self): new_email_body = 'Your exploration is unpublished :(' valid_payload = { - 'action': feconf.MODERATOR_ACTION_UNPUBLISH_EXPLORATION, 'email_body': new_email_body, 'version': 1, } @@ -2104,7 +2108,6 @@ def test_email_functionality_cannot_be_used_by_non_moderators(self): new_email_body = 'Your exploration is unpublished :(' valid_payload = { - 'action': feconf.MODERATOR_ACTION_UNPUBLISH_EXPLORATION, 'email_body': new_email_body, 'version': 1, } @@ -2218,7 +2221,9 @@ def test_cannot_fetch_playthrough_with_invalid_playthrough_id(self): self.EXP_ID, 'invalid_playthrough_id'), expected_status_int=404) def test_fetch_issues_handler_with_disabled_exp_id(self): - self.get_json('/issuesdatahandler/5', expected_status_int=404) + self.get_json( + '/issuesdatahandler/5', params={'exp_version': 2}, + expected_status_int=404) def test_fetch_issues_handler(self): """Test that all issues get fetched correctly.""" @@ -2402,7 +2407,7 @@ def test_error_on_passing_invalid_exp_issue_dict(self): 'exp_issue_dict': self.exp_issue_dict, 'exp_version': 1 }, csrf_token=csrf_token, - expected_status_int=404) + expected_status_int=400) def test_error_on_passing_non_matching_exp_issue_dict(self): """Test that error is raised on passing an exploration issue dict that @@ -2548,10 +2553,12 @@ def test_exploration_not_updated_because_cmd_is_invalid(self): response = self.put_json( '/createhandler/data/%s' % self.EXP_ID3, payload, csrf_token=self.csrf_token, expected_status_int=400) - self.assertEqual( - response['error'], - 'Command edit_exploration_propert is not allowed' + + error_msg = ( + 'Schema validation for \'change_list\' failed: Command ' + 'edit_exploration_propert is not allowed' ) + self.assertEqual(response['error'], error_msg) def test_draft_not_updated_because_newer_draft_exists(self): payload = { @@ -2579,10 +2586,12 @@ def test_draft_not_updated_because_cmd_is_invalid(self): response = self.put_json( '/createhandler/autosave_draft/%s' % self.EXP_ID1, payload, csrf_token=self.csrf_token, expected_status_int=400) - self.assertEqual( - response['error'], - 'Command edit_exploration_propert is not allowed' + + error_msg = ( + 'Schema validation for \'change_list\' failed: Command ' + 'edit_exploration_propert is not allowed' ) + self.assertEqual(response['error'], error_msg) def test_draft_not_updated_validation_error(self): self.put_json( @@ -2698,7 +2707,7 @@ def test_get_invalid_exploration_id(self): self.get_json( '%s/%s' % ( feconf.EXPLORATION_STATE_ANSWER_STATS_PREFIX, illegal_id), - expected_status_int=404) + expected_status_int=400) def test_get_missing_exploration_id(self): with self.login_context(self.OWNER_EMAIL): @@ -2862,7 +2871,7 @@ def test_delete_learner_answer_info_of_exploration_states(self): '%s/%s/%s?state_name=%s' % ( feconf.LEARNER_ANSWER_INFO_HANDLER_URL, feconf.ENTITY_TYPE_EXPLORATION, self.exp_id, - self.state_name), expected_status_int=404) + self.state_name), expected_status_int=400) self.logout() def test_delete_learner_answer_info_of_question_states(self): diff --git a/core/controllers/library.py b/core/controllers/library.py index cdaeeb1a208e..9813f56eaefd 100644 --- a/core/controllers/library.py +++ b/core/controllers/library.py @@ -17,7 +17,6 @@ from __future__ import absolute_import from __future__ import unicode_literals -import json import logging import string @@ -91,6 +90,11 @@ def get_matching_activity_dicts( class OldLibraryRedirectPage(base.BaseHandler): """Redirects the old library URL to the new one.""" + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': {} + } + @acl_decorators.open_access def get(self): """Handles GET requests.""" @@ -101,6 +105,10 @@ class LibraryIndexHandler(base.BaseHandler): """Provides data for the default library index page.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': {} + } @acl_decorators.open_access def get(self): @@ -155,12 +163,26 @@ class LibraryGroupIndexHandler(base.BaseHandler): """Provides data for categories such as top rated and recently published.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'group_name': { + 'schema': { + 'type': 'basestring', + 'choices': [ + feconf.LIBRARY_GROUP_RECENTLY_PUBLISHED, + feconf.LIBRARY_GROUP_TOP_RATED + ] + } + } + } + } @acl_decorators.open_access def get(self): """Handles GET requests for group pages.""" # TODO(sll): Support index pages for other language codes. - group_name = self.request.get('group_name') + group_name = self.normalized_request.get('group_name') activity_list = [] header_i18n_id = '' @@ -181,9 +203,6 @@ def get(self): activity_list = top_rated_activity_summary_dicts header_i18n_id = feconf.LIBRARY_CATEGORY_TOP_RATED_EXPLORATIONS - else: - raise self.PageNotFoundException - preferred_language_codes = [constants.DEFAULT_LANGUAGE_CODE] if self.user_id: user_settings = user_services.get_user_settings(self.user_id) @@ -201,13 +220,53 @@ class SearchHandler(base.BaseHandler): """Provides data for activity search results.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'q': { + 'schema': { + 'type': 'basestring' + }, + 'default_value': '' + }, + 'category': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_search_query_string' + }, { + 'id': 'is_regex_matched', + 'regex_pattern': '[\\-\\w+()"\\s]*' + }] + }, + 'default_value': '' + }, + 'language_code': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_search_query_string' + }, { + 'id': 'is_regex_matched', + 'regex_pattern': '[\\-\\w+()"\\s]*' + }] + }, + 'default_value': '' + }, + 'cursor': { + 'schema': { + 'type': 'basestring' + }, + 'default_value': None + } + } + } @acl_decorators.open_access def get(self): """Handles GET requests.""" query_string = utils.unescape_encoded_uri_component( - self.request.get('q')) - + self.normalized_request.get('q')) # Remove all punctuation from the query string, and replace it with # spaces. See http://stackoverflow.com/a/266162 and # http://stackoverflow.com/a/11693937 @@ -217,11 +276,8 @@ def get(self): # If there is a category parameter, it should be in the following form: # category=("Algebra" OR "Math") - category_string = self.request.get('category', '') - if category_string and ( - not category_string.startswith('("') or - not category_string.endswith('")')): - raise self.InvalidInputException('Invalid search query.') + category_string = self.normalized_request.get('category') + # The 2 and -2 account for the '("" and '")' characters at the # beginning and end. categories = ( @@ -230,11 +286,8 @@ def get(self): # If there is a language code parameter, it should be in the following # form: # language_code=("en" OR "hi") - language_code_string = self.request.get('language_code', '') - if language_code_string and ( - not language_code_string.startswith('("') or - not language_code_string.endswith('")')): - raise self.InvalidInputException('Invalid search query.') + language_code_string = self.normalized_request.get('language_code') + # The 2 and -2 account for the '("" and '")' characters at the # beginning and end. language_codes = ( @@ -242,7 +295,7 @@ def get(self): if language_code_string else []) # TODO(#11314): Change 'cursor' to 'offset' here and in the frontend. - search_offset = self.request.get('cursor', None) + search_offset = self.normalized_request.get('cursor') activity_list, new_search_offset = get_matching_activity_dicts( query_string, categories, language_codes, search_offset) @@ -258,6 +311,11 @@ def get(self): class LibraryRedirectPage(base.BaseHandler): """An old 'gallery' page that should redirect to the library index page.""" + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': {} + } + @acl_decorators.open_access def get(self): """Handles GET requests.""" @@ -270,19 +328,30 @@ class ExplorationSummariesHandler(base.BaseHandler): """ GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'stringified_exp_ids': { + 'schema': { + 'type': 'custom', + 'obj_type': 'JsonEncodedInString' + } + }, + 'include_private_explorations': { + 'schema': { + 'type': 'bool' + }, + 'default_value': False + } + } + } @acl_decorators.open_access def get(self): """Handles GET requests.""" - try: - exp_ids = json.loads(self.request.get('stringified_exp_ids')) - except Exception: - raise self.PageNotFoundException - include_private_exps_str = self.request.get( + exp_ids = self.normalized_request.get('stringified_exp_ids') + include_private_exps = self.normalized_request.get( 'include_private_explorations') - include_private_exps = ( - include_private_exps_str.lower() == 'true' - if include_private_exps_str else False) editor_user_id = self.user_id if include_private_exps else None if not editor_user_id: @@ -311,15 +380,24 @@ class CollectionSummariesHandler(base.BaseHandler): """Returns collection summaries corresponding to collection ids.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = {} + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'stringified_collection_ids': { + 'schema': { + 'type': 'custom', + 'obj_type': 'JsonEncodedInString' + } + } + } + } @acl_decorators.open_access def get(self): """Handles GET requests.""" - try: - collection_ids = json.loads( - self.request.get('stringified_collection_ids')) - except Exception: - raise self.PageNotFoundException + collection_ids = ( + self.normalized_request.get('stringified_collection_ids')) + summaries = ( summary_services.get_displayable_collection_summary_dicts_matching_ids( # pylint: disable=line-too-long collection_ids)) diff --git a/core/controllers/library_test.py b/core/controllers/library_test.py index 0e0610fd95b3..a4a16fbc1248 100644 --- a/core/controllers/library_test.py +++ b/core/controllers/library_test.py @@ -248,24 +248,50 @@ def test_library_handler_with_invalid_category(self): 'category': 'missing-outer-parens', 'language_code': '("en")' }, expected_status_int=400) - self.assertEqual(response_1['error'], 'Invalid search query.') + + error_msg = ( + 'Schema validation for \'category\' failed: Validation ' + 'failed: is_search_query_string ({}) for ' + 'object missing-outer-parens' + ) + self.assertEqual(response_1['error'], error_msg) + response_2 = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL, params={ 'category': '(missing-inner-quotes)', 'language_code': '("en")' }, expected_status_int=400) - self.assertEqual(response_2['error'], 'Invalid search query.') + + error_msg = ( + 'Schema validation for \'category\' failed: Validation ' + 'failed: is_search_query_string ({}) for ' + 'object (missing-inner-quotes)' + ) + self.assertEqual(response_2['error'], error_msg) def test_library_handler_with_invalid_language_code(self): response_1 = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL, params={ 'category': '("A category")', 'language_code': 'missing-outer-parens' }, expected_status_int=400) - self.assertEqual(response_1['error'], 'Invalid search query.') + + error_msg = ( + 'Schema validation for \'language_code\' failed: Validation ' + 'failed: is_search_query_string ({}) for ' + 'object missing-outer-parens' + ) + self.assertEqual(response_1['error'], error_msg) + response_2 = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL, params={ 'category': '("A category")', 'language_code': '(missing-inner-quotes)' }, expected_status_int=400) - self.assertEqual(response_2['error'], 'Invalid search query.') + + error_msg = ( + 'Schema validation for \'language_code\' failed: Validation ' + 'failed: is_search_query_string ({}) for ' + 'object (missing-inner-quotes)' + ) + self.assertEqual(response_2['error'], error_msg) class LibraryIndexHandlerTests(test_utils.GenericTestBase): diff --git a/core/controllers/payload_validator.py b/core/controllers/payload_validator.py index 9af421c82574..0d25c89b1a30 100644 --- a/core/controllers/payload_validator.py +++ b/core/controllers/payload_validator.py @@ -21,13 +21,18 @@ from __future__ import absolute_import from __future__ import unicode_literals +import python_utils import schema_utils -from typing import Any, Dict, List, Text, Tuple # isort:skip pylint: disable= wrong-import-order, wrong-import-position, unused-import, import-only-modules +from typing import Any, Dict, List, Text, Tuple, Optional, Union # isort:skip pylint: disable= wrong-import-order, wrong-import-position, unused-import, import-only-modules -def validate(handler_args, handler_args_schemas, allowed_extra_args): - # type: (Any, Any, bool) -> Tuple[Dict[Any, Any], List[Text]] +def validate( + handler_args: Any, + handler_args_schemas: Any, + allowed_extra_args: bool, + allow_string_to_bool_conversion: bool = False +) -> Tuple[Dict[Text, Any], List[Text]]: """Calls schema utils for normalization of object against its schema and collects all the errors. @@ -36,6 +41,8 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): handler_args: *. Object for normalization. handler_args_schemas: dict. Schema for args. allowed_extra_args: bool. Whether extra args are allowed in handler. + allow_string_to_bool_conversion: bool. Whether to allow string to + boolean coversion. Returns: *. A two tuple, where the first element represents the normalized value @@ -58,6 +65,17 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): elif 'default_value' not in arg_schema: errors.append('Missing key in handler args: %s.' % arg_key) continue + + # Below normalization is for arguments which are expected to be boolean + # but from API request they are received as string type. + if ( + allow_string_to_bool_conversion and + arg_schema['schema']['type'] == schema_utils.SCHEMA_TYPE_BOOL + and isinstance(handler_args[arg_key], python_utils.BASESTRING) + ): + handler_args[arg_key] = ( + convert_string_to_bool(handler_args[arg_key])) + try: normalized_value[arg_key] = schema_utils.normalize_against_schema( handler_args[arg_key], arg_schema['schema']) @@ -73,6 +91,28 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): return normalized_value, errors +def convert_string_to_bool(param: Text) -> Optional[Union[bool, Text]]: + + """Converts a request param of type string into expected bool type. + + Args: + param: str. The params which needs normalization. + + Returns: + bool. Converts the string param into its expected bool type. + """ + case_insensitive_param = param.lower() + + if case_insensitive_param == 'true': + return True + elif case_insensitive_param == 'false': + return False + else: + # String values other than booleans should be returned as it is, so that + # schema validation will raise exceptions appropriately. + return param + + # Handlers which require schema validation, but currently they do # not have schema. In order to add schema incrementally this list is # maintained. Please remove the name of the handlers if they already @@ -82,7 +122,6 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'AssetDevHandler', 'AudioUploadHandler', 'BulkEmailWebhookEndpoint', - 'CollectionSummariesHandler', 'DeferredTasksHandler', 'DeleteAccountHandler', 'DeleteAccountPage', @@ -91,45 +130,24 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'EditableStoryDataHandler', 'EditableSubtopicPageDataHandler', 'EditableTopicDataHandler', - 'EditorAutosaveHandler', - 'EditorHandler', 'ExplorationActualStartEventHandler', 'ExplorationCompleteEventHandler', 'ExplorationEmbedPage', - 'ExplorationFileDownloader', - 'ExplorationHandler', 'ExplorationMaybeLeaveHandler', - 'ExplorationModeratorRightsHandler', - 'ExplorationPage', - 'ExplorationRevertHandler', - 'ExplorationRightsHandler', - 'ExplorationSnapshotsHandler', 'ExplorationStartEventHandler', - 'ExplorationStatisticsHandler', - 'ExplorationStatusHandler', - 'ExplorationSummariesHandler', 'ExportAccountHandler', 'FeedbackThreadStatusChangeEmailHandler', - 'FetchIssuesHandler', - 'FetchPlaythroughHandler', 'FetchSkillsHandler', 'FlagExplorationEmailHandler', 'FlagExplorationHandler', - 'ImageUploadHandler', 'IncomingReplyEmailHandler', 'InstantFeedbackMessageEmailHandler', 'JobOutputHandler', 'JobsHandler', 'LearnerAnswerDetailsSubmissionHandler', - 'LearnerAnswerInfoHandler', 'LearnerGoalsHandler', 'LearnerIncompleteActivityHandler', 'LeaveForRefresherExpEventHandler', - 'LibraryGroupIndexHandler', - 'LibraryGroupPage', - 'LibraryIndexHandler', - 'LibraryPage', - 'LibraryRedirectPage', 'MemoryCacheAdminHandler', 'MemoryCacheHandler', 'MergeSkillHandler', @@ -139,7 +157,6 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'NotificationsDashboardHandler', 'NotificationsDashboardPage', 'NotificationsHandler', - 'OldLibraryRedirectPage', 'OldNotificationsDashboardRedirectPage', 'PendingAccountDeletionPage', 'PreferenceHandler', @@ -162,10 +179,8 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'RecentCommitsHandler', 'RecommendationsHandler', 'ReleaseCoordinatorPage', - 'ResolveIssueHandler', 'ResubmitSuggestionHandler', 'ReviewableSuggestionsHandler', - 'SearchHandler', 'SignupHandler', 'SignupPage', 'SiteLanguageHandler', @@ -177,12 +192,8 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'SkillsDashboardPageDataHandler', 'SolutionHitEventHandler', 'StartedTranslationTutorialEventHandler', - 'StartedTutorialEventHandler', - 'StateAnswerStatisticsHandler', 'StateCompleteEventHandler', 'StateHitEventHandler', - 'StateYamlHandler', - 'StateInteractionStatsHandler', 'StatsEventsHandler', 'StorePlaythroughHandler', 'StoryEditorPage', @@ -200,7 +211,6 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'SuggestionToExplorationActionHandler', 'SuggestionToSkillActionHandler', 'SuggestionsProviderHandler', - 'TopUnresolvedAnswersHandler', 'TopicAssignmentsHandler', 'TopicEditorPage', 'TopicEditorStoryHandler', @@ -218,8 +228,6 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args): 'UpdateQuestionSuggestionHandler', 'UpdateTranslationSuggestionHandler', 'UrlHandler', - 'UserExplorationEmailsHandler', - 'UserExplorationPermissionsHandler', 'UserInfoHandler', 'UserSubmittedSuggestionsHandler', 'UsernameCheckHandler', diff --git a/core/controllers/payload_validator_test.py b/core/controllers/payload_validator_test.py index 94a530997eb9..0714ffba2290 100644 --- a/core/controllers/payload_validator_test.py +++ b/core/controllers/payload_validator_test.py @@ -23,9 +23,7 @@ class PayloadValidationUnitTests(test_utils.GenericTestBase): - def test_invalid_args_raises_exceptions(self): - # type: () -> None - + def test_invalid_args_raises_exceptions(self) -> None: # List of 3-tuples, where the first element is an invalid argument dict, # the second element is a schema dict and the third element # is a list of errors. @@ -69,14 +67,16 @@ def test_invalid_args_raises_exceptions(self): for handler_args, handler_args_schema, error_msg in ( list_of_invalid_args_with_schema_and_errors): normalized_value, errors = payload_validator.validate( - handler_args, handler_args_schema, False) + handler_args, + handler_args_schema, + allowed_extra_args=False, + allow_string_to_bool_conversion=False + ) self.assertEqual(normalized_value, {}) self.assertEqual(error_msg, errors) - def test_valid_args_do_not_raises_exception(self): - # type: () -> None - + def test_valid_args_do_not_raises_exception(self) -> None: # List of 3-tuples, where the first element is a valid argument dict, # the second element is a schema dict and the third element is the # normalized value of the corresponding argument. @@ -109,13 +109,42 @@ def test_valid_args_do_not_raises_exception(self): } }, { 'exploration_id': 'any_exp_id' + }), + ({ + 'apply_draft': 'true' + }, { + 'apply_draft': { + 'schema': { + 'type': 'bool' + } + } + }, { + 'apply_draft': True }) ] for handler_args, handler_args_schema, normalized_value_for_args in ( list_of_valid_args_with_schmea): - normalized_value_for_args, errors = payload_validator.validate( - handler_args, handler_args_schema, False) + normalized_value, errors = payload_validator.validate( + handler_args, + handler_args_schema, + allowed_extra_args=False, + allow_string_to_bool_conversion=True + ) - self.assertEqual( - normalized_value_for_args, normalized_value_for_args) + self.assertEqual(normalized_value, normalized_value_for_args) self.assertEqual(errors, []) + + +class CheckConversionOfStringToBool(test_utils.GenericTestBase): + """Test class to check behaviour of convert_string_to_bool method.""" + + def test_convert_string_to_bool(self) -> None: + """Test case to check behaviour of convert_string_to_bool method.""" + self.assertTrue( + payload_validator.convert_string_to_bool('true')) + self.assertFalse( + payload_validator.convert_string_to_bool('false')) + self.assertEqual( + payload_validator.convert_string_to_bool('any_other_value'), + 'any_other_value' + ) diff --git a/core/controllers/reader.py b/core/controllers/reader.py index 11ac15eebb12..c4103b4cc38a 100644 --- a/core/controllers/reader.py +++ b/core/controllers/reader.py @@ -111,6 +111,54 @@ def get(self, exploration_id): class ExplorationPage(base.BaseHandler): """Page describing a single exploration.""" + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_regex_matched', + 'regex_pattern': constants.ENTITY_ID_REGEX + }] + } + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'v': { + 'schema': { + 'type': 'int', + 'validators': [{ + 'id': 'is_at_least', + 'min_value': 1 # Version must be greater than zero. + }] + }, + 'default_value': None + }, + 'parent': { + 'schema': { + 'type': 'basestring' + }, + 'default_value': None + }, + 'iframed': { + 'schema': { + 'type': 'bool' + }, + 'default_value': None + }, + 'collection_id': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_regex_matched', + 'regex_pattern': constants.ENTITY_ID_REGEX + }] + }, + 'default_value': None + } + } + } + @acl_decorators.can_play_exploration def get(self, exploration_id): """Handles GET requests. @@ -118,23 +166,22 @@ def get(self, exploration_id): Args: exploration_id: str. The ID of the exploration. """ - version_str = self.request.get('v') - version = int(version_str) if version_str else None + version = self.normalized_request.get('v') - if self.request.get('iframed'): + if self.normalized_request.get('iframed'): redirect_url = '/embed/exploration/%s' % exploration_id - if version_str: - redirect_url += '?v=%s' % version_str + if version: + redirect_url += '?v=%s' % version self.redirect(redirect_url) return # Note: this is an optional argument and will be None when the # exploration is being played outside the context of a collection or if # the 'parent' parameter is present. - if self.request.get('parent'): + if self.normalized_request.get('parent'): collection_id = None else: - collection_id = self.request.get('collection_id') + collection_id = self.normalized_request.get('collection_id') if not _does_exploration_exist(exploration_id, version, collection_id): raise self.PageNotFoundException @@ -146,6 +193,31 @@ class ExplorationHandler(base.BaseHandler): """Provides the initial data for a single exploration.""" GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON + URL_PATH_ARGS_SCHEMAS = { + 'exploration_id': { + 'schema': { + 'type': 'basestring', + 'validators': [{ + 'id': 'is_regex_matched', + 'regex_pattern': constants.ENTITY_ID_REGEX + }] + } + } + } + HANDLER_ARGS_SCHEMAS = { + 'GET': { + 'v': { + 'schema': { + 'type': 'int', + 'validators': [{ + 'id': 'is_at_least', + 'min_value': 1 # Version must be greater than zero. + }] + }, + 'default_value': None + } + } + } @acl_decorators.can_play_exploration def get(self, exploration_id): @@ -154,8 +226,7 @@ def get(self, exploration_id): Args: exploration_id: str. The ID of the exploration. """ - version = self.request.get('v') - version = int(version) if version else None + version = self.normalized_request.get('v') exploration = exp_fetchers.get_exploration_by_id( exploration_id, strict=False, version=version) diff --git a/core/controllers/resources_test.py b/core/controllers/resources_test.py index 31e62f8b611c..c676630e7eda 100644 --- a/core/controllers/resources_test.py +++ b/core/controllers/resources_test.py @@ -71,7 +71,8 @@ def test_image_upload_with_no_filename_raises_error(self): upload_files=(('image', 'unused_filename', raw_image),), expected_status_int=400) - self.assertEqual(response_dict['error'], 'No filename supplied') + self.assertEqual( + response_dict['error'], 'Missing key in handler args: filename.') self.logout() @@ -101,7 +102,12 @@ def test_image_upload_with_invalid_filename_raises_error(self): upload_files=(('image', 'unused_filename', raw_image),), expected_status_int=400) - self.assertEqual(response_dict['error'], 'Invalid filename') + error_msg = ( + 'Schema validation for \'filename\' failed: Validation' + ' failed: is_regex_matched ({\'regex_pattern\': ' + '\'\\\\w+[.]\\\\w+\'}) for object .png' + ) + self.assertEqual(response_dict['error'], error_msg) self.logout() @@ -396,7 +402,12 @@ def test_bad_filenames_are_detected(self): upload_files=(('image', 'unused_filename', raw_image),), ) self.assertEqual(response_dict['status_code'], 400) - self.assertIn('Filenames should not include', response_dict['error']) + + error_msg = ( + 'Schema validation for \'filename\' failed: Validation failed: ' + 'is_regex_matched ({\'regex_pattern\': \'\\\\w+[.]\\\\w+\'}) ' + 'for object test/a.png') + self.assertIn(error_msg, response_dict['error']) self.logout() @@ -416,8 +427,12 @@ def test_missing_extensions_are_detected(self): upload_files=(('image', 'unused_filename', raw_image),), ) self.assertEqual(response_dict['status_code'], 400) - self.assertIn( - 'Image filename with no extension', response_dict['error']) + + error_msg = ( + 'Schema validation for \'filename\' failed: Validation failed: ' + 'is_regex_matched ({\'regex_pattern\': \'\\\\w+[.]\\\\w+\'}) ' + 'for object test') + self.assertIn(error_msg, response_dict['error']) self.logout() diff --git a/core/controllers/voice_artist_test.py b/core/controllers/voice_artist_test.py index abcdfd7a1dbf..bd04283e50a1 100644 --- a/core/controllers/voice_artist_test.py +++ b/core/controllers/voice_artist_test.py @@ -92,7 +92,7 @@ def setUp(self): def test_put_with_no_payload_version_raises_error(self): with self.assertRaisesRegexp( - Exception, 'Invalid POST request: a version must be specified.'): + Exception, 'Missing key in handler args: version.'): self.put_json( '%s/%s' % (feconf.EXPLORATION_DATA_PREFIX, self.EXP_ID), { 'change_list': [{ diff --git a/core/tests/protractor_desktop/embedding.js b/core/tests/protractor_desktop/embedding.js index 46f1a75e27a3..ecb4f32ded4d 100644 --- a/core/tests/protractor_desktop/embedding.js +++ b/core/tests/protractor_desktop/embedding.js @@ -112,7 +112,7 @@ describe('Embedding', function() { '\'http:\/\/localhost:9001\/\' in a frame because it set ' + '\'X-Frame-Options\' to \'deny\'.', 'chrome-error:\/\/chromewebdata\/ - Failed to load resource: the server ' + - 'responded with a status of 404 ()', + 'responded with a status of 400 ()', ]; beforeEach(function() { diff --git a/main.py b/main.py index 5b7e07a04870..ba70ce1df9b5 100644 --- a/main.py +++ b/main.py @@ -582,7 +582,7 @@ def get_redirect_route( r'/createhandler/statistics/', editor.ExplorationStatisticsHandler), get_redirect_route( - r'/createhandler/state_interaction_stats//', # pylint: disable=line-too-long + r'/createhandler/state_interaction_stats//', editor.StateInteractionStatsHandler), get_redirect_route( r'%s/' % feconf.EXPLORATION_STATE_ANSWER_STATS_PREFIX, diff --git a/schema_utils.py b/schema_utils.py index 54a031edebec..fb1405e3cadb 100644 --- a/schema_utils.py +++ b/schema_utils.py @@ -616,8 +616,7 @@ def is_supported_audio_language_code(obj): return utils.is_supported_audio_language_code(obj) @staticmethod - def is_valid_audio_language_code(obj): - # type: (Text) -> bool + def is_valid_audio_language_code(obj: str) -> bool: """Checks if the given obj (a string) represents a valid language code. Args: @@ -629,8 +628,7 @@ def is_valid_audio_language_code(obj): return utils.is_valid_language_code(obj) @staticmethod - def is_regex_matched(obj, regex_pattern): - # type: (Text, Text) -> bool + def is_regex_matched(obj: str, regex_pattern: str) -> bool: """Checks if a given string is matched with the provided regular experssion. @@ -642,3 +640,17 @@ def is_regex_matched(obj, regex_pattern): bool. Whether the given object matched with the regex pattern. """ return bool(re.match(regex_pattern, obj)) + + @staticmethod + def is_search_query_string(obj: str) -> bool: + """Checks if the given obj (a string) is a gae search query string. + + Args: + obj: str. The string to verify. + + Returns: + bool. Whether the given object is a gae search query string. + """ + if obj and (not obj.startswith('("') or not obj.endswith('")')): + return False + return True diff --git a/schema_utils_test.py b/schema_utils_test.py index 9d4f7831b030..e10fb934e834 100644 --- a/schema_utils_test.py +++ b/schema_utils_test.py @@ -835,6 +835,22 @@ def test_is_regex_matched(self): 'WzEuNjI2NTgxNDQwOTVlKzEyXQ==WzE3NThd', r'(exploration|collection)\.\w+\.\w+')) + def test_is_search_query_string(self) -> None: + """Checks whether a given string is contained within parenthesis and + double quotes. + + Returns: + bool. A boolean value representing whether a given string is + contained within parenthesis and double quotes. + """ + is_search_query_string = ( + schema_utils.get_validator('is_search_query_string')) + + self.assertTrue(is_search_query_string('("A category")')) + + self.assertFalse(is_search_query_string('(missing-inner-quotes)')) + self.assertFalse(is_search_query_string('missing-outer-parens')) + class SchemaNormalizationUnitTests(test_utils.GenericTestBase): """Test schema-based normalization of objects.""" diff --git a/scripts/run_mypy_checks.py b/scripts/run_mypy_checks.py index 2fb9dd0ac2cf..0e2eb9bb3bf5 100644 --- a/scripts/run_mypy_checks.py +++ b/scripts/run_mypy_checks.py @@ -73,8 +73,6 @@ 'core/controllers/cron_test.py', 'core/controllers/custom_landing_pages.py', 'core/controllers/custom_landing_pages_test.py', - 'core/controllers/domain_objects_validator.py', - 'core/controllers/domain_objects_validator_test.py', 'core/controllers/editor.py', 'core/controllers/editor_test.py', 'core/controllers/email_dashboard.py',