Skip to content

Commit

Permalink
Fixes part of oppia#5002: Remove CSRF token from GLOBALS (oppia#6951)
Browse files Browse the repository at this point in the history 
* remove csrf global

* refactor tests

* lint issues

* lint issues

* work on fe tests

* working on failing tests

* work on e2e tests

* minor issue in admin

* address some review comments

* work on failing tests

* erroring tests

* still on tests

* address more review comments

* still on the test issue

* fix error

* still on tests

* work on erroring tests

* still on failing tests

* address review comments

* address review comment

* address review comment and working on tests

* address review comment

* work on failing tests

* remove csrf from other pages

* fix lint issue

* work on failing tests

* use promises in csrf token

* refactor to use jQuery

* Address review comments.

* remove unnecessary imports from base.ts

* work on failing FE test

* fix failing tests

* address review comments

* work on FE tests

* work on failing test

* refactor based on test

* address review comments

* address review comments and fix failing Travis test

* work on failing tests

* work on fe tests
  • Loading branch information
@jameesjohn @seanlip
jameesjohn authored and seanlip committed Jul 2, 2019
1 parent b8280c3 commit 521a4b0
Show file tree
Hide file tree
Showing 66 changed files with 762 additions and 853 deletions.
1 change: 1 addition & 0 deletions .github/CODEOWNERS
View file
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@
/core/templates/dev/head/pages/footer_js_libs.html @vojtechjelinek @seanlip
/core/templates/dev/head/pages/header_css_libs.html @vojtechjelinek @seanlip
/core/templates/dev/head/pages/header_js_libs.html @vojtechjelinek @seanlip
/core/templates/dev/head/services/CsrfTokenService*.ts @jamesjay4199 @vojtechjelinek @seanlip
/gulpfile.js @vojtechjelinek @seanlip
/jinja_utils*.py @vojtechjelinek @seanlip
/webpack.config.ts @vojtechjelinek @seanlip
Expand Down
25 changes: 9 additions & 16 deletions core/controllers/admin_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ def test_change_configuration_property(self):
"""Test that configuration properties can be changed."""

self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response('/admin')
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
new_config_value = True

response_dict = self.get_json('/adminhandler')
Expand Down Expand Up @@ -92,8 +91,7 @@ class GenerateDummyExplorationsTest(test_utils.GenericTestBase):
def test_generate_count_greater_than_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response('/admin')
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
self.post_json(
'/adminhandler', {
'action': 'generate_dummy_explorations',
Expand All @@ -108,8 +106,7 @@ def test_generate_count_greater_than_publish_count(self):
def test_generate_count_equal_to_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response('/admin')
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
self.post_json(
'/adminhandler', {
'action': 'generate_dummy_explorations',
Expand All @@ -124,8 +121,7 @@ def test_generate_count_equal_to_publish_count(self):
def test_generate_count_less_than_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response('/admin')
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
generated_exps_response = self.post_json(
'/adminhandler', {
'action': 'generate_dummy_explorations',
Expand Down Expand Up @@ -164,8 +160,7 @@ def test_view_and_update_role(self):
response_dict, {'user1': feconf.ROLE_ID_EXPLORATION_EDITOR})

# Check role correctly gets updated.
response = self.get_html_response(feconf.ADMIN_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
response_dict = self.post_json(
feconf.ADMIN_ROLE_HANDLER_URL,
{'role': feconf.ROLE_ID_MODERATOR, 'username': username},
Expand All @@ -186,15 +181,14 @@ def test_invalid_username_in_view_and_update_role(self):
self.login(self.ADMIN_EMAIL, is_super_admin=True)

# Trying to view role of non-existent user.
response = self.get_json(
self.get_json(
feconf.ADMIN_ROLE_HANDLER_URL,
params={'method': 'username', 'username': username},
expected_status_int=400)

# Trying to update role of non-existent user.
response = self.get_html_response(feconf.ADMIN_URL)
csrf_token = self.get_csrf_token_from_response(response)
response = self.post_json(
csrf_token = self.get_new_csrf_token()
self.post_json(
feconf.ADMIN_ROLE_HANDLER_URL,
{'role': feconf.ROLE_ID_MODERATOR, 'username': username},
csrf_token=csrf_token,
Expand Down Expand Up @@ -294,8 +288,7 @@ def test_clear_search_index(self):
self.assertEqual(result_collections, ['0'])
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response('/admin')
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
generated_exps_response = self.post_json(
'/adminhandler', {
'action': 'clear_search_index'
Expand Down
25 changes: 17 additions & 8 deletions core/controllers/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,11 +325,6 @@ def render_template(self, filepath, iframe_restriction='DENY'):
# Create a new csrf token for inclusion in HTML responses. This assumes
# that tokens generated in one handler will be sent back to a handler
# with the same page name.
values['csrf_token'] = ''

if self.REQUIRE_PAYLOAD_CSRF_CHECK:
values['csrf_token'] = CsrfTokenManager.create_csrf_token(
self.user_id)

self.response.cache_control.no_cache = True
self.response.cache_control.must_revalidate = True
Expand Down Expand Up @@ -502,7 +497,7 @@ def _create_token(cls, user_id, issued_on):
"""Creates a new CSRF token.
Args:
user_id: str. The user_id for which the token is generated.
user_id: str|None. The user_id for which the token is generated.
issued_on: float. The timestamp at which the token was issued.
Returns:
Expand Down Expand Up @@ -543,7 +538,7 @@ def create_csrf_token(cls, user_id):
"""Creates a CSRF token for the given user_id.
Args:
user_id: str. The user_id for whom the token is generated.
user_id: str|None. The user_id for whom the token is generated.
Returns:
str. The generated CSRF token.
Expand All @@ -555,7 +550,7 @@ def is_csrf_token_valid(cls, user_id, token):
"""Validates a given CSRF token.
Args:
user_id: str. The user_id to validate the CSRF token against.
user_id: str|None. The user_id to validate the CSRF token against.
token: str. The CSRF token to validate.
Returns:
Expand All @@ -578,3 +573,17 @@ def is_csrf_token_valid(cls, user_id, token):
return False
except Exception:
return False


class CsrfTokenHandler(BaseHandler):
"""Handles sending CSRF tokens to the frontend."""

GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
REDIRECT_UNFINISHED_SIGNUPS = False

def get(self):
csrf_token = CsrfTokenManager.create_csrf_token(
self.user_id)
self.render_json({
'token': csrf_token,
})
25 changes: 18 additions & 7 deletions core/controllers/base_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,8 +670,8 @@ def test_every_method_has_decorator(self):

# Following handler are present in base.py where acl_decorators
# cannot be imported.
if (handler.__name__ == 'LogoutPage' or
handler.__name__ == 'Error404Handler'):
if (handler.__name__ in (
('CsrfTokenHandler', 'Error404Handler', 'LogoutPage'))):
continue

if handler.get != base.BaseHandler.get:
Expand Down Expand Up @@ -856,8 +856,7 @@ def test_error_is_raised_on_opening_new_tab_during_signup(self):
during signup.
"""
self.login('abc@example.com')
response = self.get_html_response(feconf.SIGNUP_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

response = self.get_html_response('/about', expected_status_int=302)
self.assertIn('Logout', response.location)
Expand All @@ -877,9 +876,7 @@ def test_no_error_is_raised_on_opening_new_tab_after_signup(self):
after signup.
"""
self.login('abc@example.com')
response = self.get_html_response(feconf.SIGNUP_URL)
csrf_token = self.get_csrf_token_from_response(response)

csrf_token = self.get_new_csrf_token()
self.post_json(
feconf.SIGNUP_DATA_URL, {
'username': 'abc',
Expand All @@ -888,3 +885,17 @@ def test_no_error_is_raised_on_opening_new_tab_after_signup(self):
)

self.get_html_response('/about')


class CsrfTokenHandlerTests(test_utils.GenericTestBase):

def test_valid_token_is_returned(self):
"""Test that a valid CSRF token is returned by
the handler.
"""

response = self.get_json('/csrfhandler')
csrf_token = response['token']

self.assertTrue(base.CsrfTokenManager.is_csrf_token_valid(
None, csrf_token))
42 changes: 8 additions & 34 deletions core/controllers/collection_editor_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,7 @@ def test_editable_collection_handler_put_with_invalid_payload_version(self):
self.login(self.EDITOR_EMAIL)

# Call get handler to return the csrf token.
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX,
self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

# Raises error as version is None.
json_response = self.put_json(
Expand Down Expand Up @@ -180,11 +176,7 @@ def test_editable_collection_handler_put_cannot_access(self):
self.login(self.VIEWER_EMAIL)

# Call get handler to return the csrf token.
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX,
self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

# Ensure viewers do not have access to the PUT Handler.
self.put_json(
Expand All @@ -211,12 +203,7 @@ def test_editable_collection_handler_put_can_access(self):
self.login(self.EDITOR_EMAIL)

# Call get handler to return the csrf token.
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX,
self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)

csrf_token = self.get_new_csrf_token()
json_response = self.put_json(
'%s/%s' % (
feconf.COLLECTION_EDITOR_DATA_URL_PREFIX,
Expand Down Expand Up @@ -293,10 +280,7 @@ def test_can_not_publish_collection_with_invalid_payload_version(self):
self.save_new_valid_collection(
collection_id, self.owner_id, exploration_id=exploration_id)
rights_manager.publish_exploration(self.owner, exploration_id)
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

# Raises error as version is None.
response_dict = self.put_json(
Expand Down Expand Up @@ -332,10 +316,7 @@ def test_can_not_unpublish_collection_with_invalid_payload_version(self):
collection_id, self.owner_id, exploration_id=exploration_id)
rights_manager.publish_exploration(self.owner, exploration_id)
collection = collection_services.get_collection_by_id(collection_id)
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
response_dict = self.put_json(
'/collection_editor_handler/publish/%s' % collection_id,
{'version': collection.version},
Expand All @@ -345,9 +326,7 @@ def test_can_not_unpublish_collection_with_invalid_payload_version(self):

# Login as admin and try to unpublish the collection.
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

# Raises error as version is None.
response_dict = self.put_json(
Expand Down Expand Up @@ -384,10 +363,7 @@ def test_publish_unpublish_collection(self):
collection_id, self.owner_id, exploration_id=exploration_id)
rights_manager.publish_exploration(self.owner, exploration_id)
collection = collection_services.get_collection_by_id(collection_id)
response = self.get_html_response(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
response_dict = self.put_json(
'/collection_editor_handler/publish/%s' % collection_id,
{'version': collection.version},
Expand All @@ -397,9 +373,7 @@ def test_publish_unpublish_collection(self):

# Login as admin and unpublish the collection.
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
response_dict = self.put_json(
'/collection_editor_handler/unpublish/%s' % collection_id,
{'version': collection.version},
Expand Down
18 changes: 6 additions & 12 deletions core/controllers/creator_dashboard_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -726,8 +726,7 @@ def test_can_update_display_preference(self):
display_preference = self.get_json(
feconf.CREATOR_DASHBOARD_DATA_URL)['display_preference']
self.assertEqual(display_preference, 'card')
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
self.post_json(
feconf.CREATOR_DASHBOARD_DATA_URL,
{'display_preference': 'list'},
Expand All @@ -740,8 +739,7 @@ def test_can_update_display_preference(self):
def test_can_create_collections(self):
self.set_admins([self.OWNER_USERNAME])
self.login(self.OWNER_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
collection_id = self.post_json(
feconf.NEW_COLLECTION_URL, {}, csrf_token=csrf_token)[
creator_dashboard.COLLECTION_ID_KEY]
Expand Down Expand Up @@ -886,8 +884,7 @@ def test_new_exploration_ids(self):
"""Test generation of exploration ids."""
self.login(self.EDITOR_EMAIL)

response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
exp_a_id = self.post_json(
feconf.NEW_EXPLORATION_URL, {}, csrf_token=csrf_token
)[creator_dashboard.EXPLORATION_ID_KEY]
Expand All @@ -897,8 +894,7 @@ def test_new_exploration_ids(self):

def test_can_non_admins_can_not_upload_exploration(self):
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()

response = self.post_json(
feconf.UPLOAD_EXPLORATION_URL, {}, csrf_token=csrf_token,
Expand All @@ -914,8 +910,7 @@ def test_can_upload_exploration(self):
self.set_admins([self.ADMIN_USERNAME])
self.login(self.ADMIN_EMAIL, is_super_admin=True)

response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
explorations_list = self.get_json(
feconf.CREATOR_DASHBOARD_DATA_URL)['explorations_list']
self.assertEqual(explorations_list, [])
Expand All @@ -935,8 +930,7 @@ def test_can_not_upload_exploration_when_server_does_not_allow_file_upload(
self):
self.set_admins([self.ADMIN_USERNAME])
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
csrf_token = self.get_new_csrf_token()
self.post_json(
'%s?yaml_file=%s' % (
feconf.UPLOAD_EXPLORATION_URL,
Expand Down
Loading

0 comments on commit 521a4b0

Please sign in to comment.