Fixes part of #5002: Remove CSRF token from GLOBALS #6951

jameesjohn · 2019-06-17T09:20:28Z

Explanation

Fixes part of #5002: Remove csrf from GLOBALS

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

vojtechjelinek

Thanks! Few comments.

core/controllers/base_test.py

core/controllers/profile.py

core/controllers/skill_editor_test.py

core/templates/dev/head/services/CsrfService.ts

core/templates/dev/head/services/CsrfServiceSpec.ts

vojtechjelinek · 2019-06-17T17:31:20Z

core/templates/dev/head/App.ts

        return {
          request: function(config) {
            if (config.data) {
              config.data = $.param({
-                csrf_token: GLOBALS.csrf_token,
+                csrf_token: CsrfService.getToken(),


What if this is called before we the request for CSRF finishes? We should return a promise from .getToken() that will resolve only if the token is already available.

@Jamesjay4199 When you mark a comment as resolved, please always include an explanation (for the benefit of future developers/reviewers). Could you respond to @vojtechjelinek's comment?

I think this will never be the case because the csrf token is only used for post requests and a post request is only sent based on the user interaction with the page, while the request gets sent much earlier

It could theoretically happen though (e.g. if the server is overloaded for some reason). What will the user experience in this case, and what should they experience ideally?

I've refactored it to use a promise.
@vojtechjelinek PTAL

Why do you need to use jQuery? AngularJS has $q, try to use that for promises if possible.

…obal-removal

vojtechjelinek · 2019-06-28T17:05:54Z

@seanlip PTAL

seanlip

One more comment.

seanlip · 2019-06-28T17:33:11Z

core/templates/dev/head/services/CsrfTokenService.ts

-      // creates a circular dependency.
-      return new Promise(function(resolve, reject) {
-        if (token) {
+      if (token) {


Should we throw an error if no token is set (i.e. initialization has not happened yet)? If you think it's possible for this to be called before initialization, then change the name to getTokenAsync(), and add a test that calls getToken() before initializeToken() and make sure that works correctly. Otherwise, throw an error if token is null and just return the token otherwise; no need to return a promise.

@seanlip I think we'll still need to return a promise because it is possible that the initialization is not complete when a POST or PUT request is sent so the request will have to wait for the initialization to be complete.

OK, can you add a test that calls getTokenAsync() before initializeToken() so that we can be sure this works correctly in the case where token initialization has some latency?

I've done that already here

jameesjohn · 2019-06-28T18:11:41Z

core/templates/dev/head/services/CsrfTokenServiceSpec.ts

+    }
+  );
+
+  it('should error if getTokenAsync is called before initialization',


@seanlip It is tested here.

I called getTokenAsync without calling initialization

Wait, I'm confused, sorry. You said earlier:

@seanlip I think we'll still need to return a promise because it is possible that the initialization is not complete when a POST or PUT request is sent so the request will have to wait for the initialization to be complete.

But this test is checking that it throws an error, which seems different from your description.

Also, where does "Token needs to be initialized" come from? I do not see it in core/templates/dev/head/services/CsrfTokenService.ts

The test checks if getTokenAsync is called without already calling initializeToken.
It throws the error if a call to getTokenAsync was made before initializing the token

jameesjohn · 2019-06-28T20:13:38Z

core/templates/dev/head/services/CsrfTokenServiceSpec.ts

+    }
+  );
+
+  it('should error if getTokenAsync is called before initialization',


The test checks if getTokenAsync is called without already calling initializeToken.
It throws the error if a call to getTokenAsync was made before initializing the token

jameesjohn · 2019-06-28T20:14:44Z

core/templates/dev/head/services/CsrfTokenService.ts

+      if (tokenPromise) {
+        return tokenPromise;
+      } else {
+        throw new Error('Token needs to be initialized');


Also, where does "Token needs to be initialized" come from? I do not see it in core/templates/dev/head/services/CsrfTokenService.ts

@seanlip, Token needs to be initialized comes from here. PTAL

oppiabot · 2019-06-28T20:25:05Z

Hi @Jamesjay4199. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

seanlip

Ah ok, thank you for the explanation @Jamesjay4199. Sorry for being slow! I see what you're doing now and I think it makes sense.

Two trivial comments, then feel free to merge. Thanks!

seanlip · 2019-06-29T01:06:38Z

core/templates/dev/head/services/CsrfTokenService.ts

+    },
+
+    getTokenAsync: function() {
+      if (token) {


if (token !== null)

seanlip · 2019-06-29T01:07:52Z

core/templates/dev/head/services/CsrfTokenService.ts

+          resolve(token);
+        });
+      }
+      if (tokenPromise) {


if (tokenPromise !== null)

And, include a line comment just below this line saying "The token initialization request is still pending."

seanlip · 2019-06-29T01:08:38Z

Also please note merge conflict and failing Travis tests.

…obal-removal

jameesjohn · 2019-07-01T10:09:15Z

@aks681 PTAL as code owner

oppiabot · 2019-07-01T13:26:17Z

Hi @Jamesjay4199. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

aks681

Note, changes to package-lock.json

…obal-removal

jameesjohn · 2019-07-02T17:39:28Z

@seanlip, I think this is ready to be merged.

seanlip · 2019-07-02T17:49:20Z

Thanks, merging!

jameesjohn added 4 commits June 16, 2019 21:04

remove csrf global

f117d8a

refactor tests

6bbc862

lint issues

809e4b3

lint issues

3a917c3

jameesjohn requested a review from vojtechjelinek June 17, 2019 09:20

jameesjohn requested review from aks681, ankita240796, apb7, DubeySandeep, kevinlee12, nithusha21 and seanlip as code owners June 17, 2019 09:20

work on fe tests

b7ff267

jameesjohn added the CHANGELOG: Code health label Jun 17, 2019

working on failing tests

efe0d3c

vojtechjelinek suggested changes Jun 17, 2019

View reviewed changes

jameesjohn added 9 commits June 18, 2019 12:26

work on e2e tests

5bd0884

minor issue in admin

9e5a5e4

address some review comments

427ea8e

work on failing tests

bf83726

erroring tests

dfac34f

still on tests

f964002

address more review comments

bca6d72

still on the test issue

828e486

fix error

f9daa2c

jameesjohn requested a review from vojtechjelinek June 19, 2019 12:25

jameesjohn added 4 commits June 19, 2019 14:25

still on tests

b975bfd

Merge branch 'develop' of https://github.com/oppia/oppia into csrf-gl…

a198edf

…obal-removal

work on erroring tests

6510b39

still on failing tests

261a101

vojtechjelinek assigned seanlip Jun 28, 2019

seanlip requested changes Jun 28, 2019

View reviewed changes

address review comments

f1ec691

jameesjohn commented Jun 28, 2019

View reviewed changes

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jun 28, 2019

seanlip approved these changes Jun 29, 2019

View reviewed changes

seanlip assigned jameesjohn and unassigned seanlip Jun 29, 2019

jameesjohn added 2 commits June 29, 2019 12:30

Merge branch 'develop' of https://github.com/oppia/oppia into csrf-gl…

b143909

…obal-removal

address review comments and fix failing Travis test

ffc26b1

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jun 29, 2019

work on failing tests

493a049

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jul 1, 2019

aks681 approved these changes Jul 1, 2019

View reviewed changes

Merge branch 'develop' of https://github.com/oppia/oppia into csrf-gl…

cee7068

…obal-removal

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jul 1, 2019

jameesjohn closed this Jul 1, 2019

jameesjohn reopened this Jul 1, 2019

work on fe tests

79bc7a0

seanlip merged commit 521a4b0 into oppia:develop Jul 2, 2019

lilithxxx mentioned this pull request Jul 3, 2019

Fix backend tests on develop #7078

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes part of #5002: Remove CSRF token from GLOBALS #6951

Fixes part of #5002: Remove CSRF token from GLOBALS #6951

jameesjohn commented Jun 17, 2019

vojtechjelinek left a comment

vojtechjelinek Jun 17, 2019

seanlip Jun 21, 2019

jameesjohn Jun 21, 2019

seanlip Jun 22, 2019

jameesjohn Jun 26, 2019

seanlip Jun 26, 2019

vojtechjelinek commented Jun 28, 2019

seanlip left a comment

seanlip Jun 28, 2019

jameesjohn Jun 28, 2019 •

edited

Loading

jameesjohn Jun 28, 2019

seanlip Jun 28, 2019 •

edited

Loading

jameesjohn Jun 28, 2019

jameesjohn Jun 28, 2019

jameesjohn Jun 28, 2019

seanlip Jun 28, 2019

jameesjohn Jun 28, 2019

jameesjohn Jun 28, 2019

jameesjohn Jun 28, 2019

oppiabot bot commented Jun 28, 2019

seanlip left a comment

seanlip Jun 29, 2019

jameesjohn Jun 29, 2019

seanlip Jun 29, 2019

jameesjohn Jun 29, 2019

seanlip commented Jun 29, 2019

jameesjohn commented Jul 1, 2019 •

edited

Loading

oppiabot bot commented Jul 1, 2019

aks681 left a comment

jameesjohn commented Jul 2, 2019

seanlip commented Jul 2, 2019

Fixes part of #5002: Remove CSRF token from GLOBALS #6951

Fixes part of #5002: Remove CSRF token from GLOBALS #6951

Conversation

jameesjohn commented Jun 17, 2019

Explanation

Checklist

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek commented Jun 28, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameesjohn Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 28, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 29, 2019

jameesjohn commented Jul 1, 2019 • edited Loading

oppiabot bot commented Jul 1, 2019

aks681 left a comment

Choose a reason for hiding this comment

jameesjohn commented Jul 2, 2019

seanlip commented Jul 2, 2019

jameesjohn Jun 28, 2019 •

edited

Loading

seanlip Jun 28, 2019 •

edited

Loading

jameesjohn commented Jul 1, 2019 •

edited

Loading