Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Port dspace-7_x] Ensures CSRF token is initialized prior to first modifying (non-GET) request #3063

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented May 17, 2024

@tdonohue
Copy link
Member Author

NOTE: The e2e test failures in this PR are the result of the Backend PR (DSpace/DSpace#9599) not yet being merged. I've verified locally (via yarn e2e) that all e2e tests succeed when run against the Backend PR.

@tdonohue
Copy link
Member Author

tdonohue commented May 17, 2024

👍 I've tested this locally alongside DSpace/DSpace#9599 and verified that this functionality is successfully backported. Namely:

  • On first request to the homepage, the POST /server/api/statistics/viewevents call always succeeds now. Previously it would often fail because the CSRF token would not yet be initialized.
  • When that first request occurs, I can verify (from the backend dspace.log) that the new /api/security/csrf endpoint was called to obtain the initial token. In that scenario, you'll see this in the logs:
    2024-05-17 16:47:45,267 INFO  unknown 373bb807-3346-4813-8120-bc6942b629cc org.dspace.app.rest.utils.DSpaceAPIRequestLoggingFilter @ Before request [GET /server/api/security/csrf] originated from /
    

Copy link

Hi @tdonohue,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@tdonohue
Copy link
Member Author

I rebased this on dspace-7_x after a very minor merge conflict with latest dspace-7_x (in a edit-relationship-list.component.spec.ts).

@tdonohue tdonohue requested a review from kshepherd June 27, 2024 14:14
@tdonohue
Copy link
Member Author

tdonohue commented Jul 1, 2024

Briefly closing & reopening to trigger tests to run again. They should now succeed since the backend PR (DSpace/DSpace#9599) was merged

@tdonohue tdonohue closed this Jul 1, 2024
@tdonohue tdonohue reopened this Jul 1, 2024
@tdonohue
Copy link
Member Author

tdonohue commented Jul 1, 2024

Merging immediately as all tests pass, and was tested by @kshepherd as described in DSpace/DSpace#9599 (review)

@tdonohue tdonohue merged commit 28bd6a5 into DSpace:dspace-7_x Jul 1, 2024
20 of 22 checks passed
@tdonohue tdonohue deleted the port_new_csrf_fixes_to_7x branch July 1, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug component: statistics high priority
Projects
Development

Successfully merging this pull request may close these issues.

3 participants