yeatmanlab / roar-dashboard

A dashboard to administer ROAR assessments
https://roar.education
Other
4 stars 4 forks source link

Fix consent modal race condition #832

Closed maximilianoertel closed 2 weeks ago

maximilianoertel commented 3 weeks ago

Proposed changes

This PR introduces changes to the consent logic to eliminate issues that were causing the consent forms to show even though consent had already been given. This was mostly caused by a race condition when user data was not yet fully loaded.

Additionally, this PR slightly improves error handling by adding a loading state to the consent acceptance button and adding an error toast in case of consent update failure.

Finally, this PR introduces DOMPurify to sanitize the markdown/html output rendered in the consent modal in order to reduce the possibility of an XSS attack.

Important: The introduced error handling changes make the consent fully mandatory, blocking the user in case of consent update failure whilst beforehand the user could continue within the application despite the consent not being given.

Before

https://github.com/user-attachments/assets/4a1f6dd5-920f-4fd3-895b-3d77d6a5ffcb

After

https://github.com/user-attachments/assets/9891df01-d203-45ef-b399-29beff30e89c

https://github.com/user-attachments/assets/1446f9d7-9d21-48d3-814e-65c4423c1d39

Types of changes

Checklist

Justification of missing checklist items

n/a

Further comments

n/a

Ref https://github.com/yeatmanlab/roar/issues/318

github-actions[bot] commented 3 weeks ago

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 4.56% 349 / 7647
🔵 Statements 4.5% 380 / 8427
🔵 Functions 5.6% 105 / 1872
🔵 Branches 2.32% 107 / 4593
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/components/ConsentModal.vue 0% 0% 0% 0% 31, 33, 39-40, 42-43, 45-48, 46-47, 50-97, 51, 53, 53, 55-96, 65-94, 66, 68, 68-69, 71-78, 80, 82-87, 89, 91, 93, 11, 1, 1-4
src/components/auth/RegisterParent.vue 0% 0% 0% 0% 172-175, 177, 179-181, 180, 183-185, 184, 191, 193-202, 202, 204-217, 219, 222-228, 224-227, 225-226, 231, 233, 235-243, 236-241, 238-240, 242, 245-255, 246-254, 248-251, 253, 258, 261-262, 265, 269-270, 272-273, 276-279, 278, 2, 4, 1, 15, 32, 41-42, 53, 60, 76, 108, 111, 122, 136, 3, 1-8, 1-25, 1-45, 1-65, 1-89, 1-110, 1-114, 1-144
src/composables/mutations/useUpdateConsentMutation.js 0% 0% 0% 0% 14-31, 15-16, 18-30, 21-23, 25, 28
src/composables/mutations/useUpsertAdministrationMutation.js 0% 100% 0% 0% 17-37, 18-19, 21-36, 24, 32-34
src/constants/consentTypes.js 0% 100% 100% 0% 6-24
src/constants/mutationKeys.js 100% 100% 100% 100%
src/constants/toasts.js 0% 100% 100% 0% 1-5, 7
src/pages/HomeParticipant.vue 0% 0% 0% 0% 119-123, 125, 127, 130-134, 132, 132-133, 136-137, 139-141, 140, 140, 143-145, 144, 144, 147-148, 154-156, 162-164, 166, 166, 166-167, 167, 173-175, 177-179, 178, 178, 181, 181, 181-182, 182, 188-190, 192-194, 196-198, 197, 200-202, 201, 204-207, 205, 205-206, 210-211, 213-215, 217-227, 221-225, 222-224, 226, 229-231, 233-236, 238, 240-242, 244-282, 245, 247-248, 250, 252-268, 253, 255-262, 256, 258-261, 259-260, 264-267, 265-266, 271-277, 272-276, 273-275, 278-282, 279-281, 286-290, 292-296, 299-302, 300-301, 306-348, 307-346, 308-332, 311-313, 318, 318-329, 334-343, 336-342, 337-341, 338-340, 339, 345, 347, 350-352, 351, 351, 354-356, 355, 355, 359-368, 360-367, 363, 371-373, 372, 376-378, 377, 377, 381-387, 382, 382, 384-386, 389-406, 392-394, 393, 396-397, 397, 400-403, 402, 1, 10, 12, 27, 45-46, 65, 70, 1-33, 48, 1-49, 1-58
src/pages/HomeSelector.vue 0% 0% 0% 0% 36, 36-37, 37-38, 38, 40-42, 44-45, 47, 49-55, 50-55, 53-54, 57-58, 60, 62-65, 63, 63-64, 67-69, 68, 68, 71-73, 75-77, 79, 81, 81-82, 82, 84-91, 86-90, 87, 89, 93-94, 97, 101, 101, 103-104, 106, 108-112, 109-111, 114-115, 115, 117-120, 118-119, 124-126, 129-137, 132-134, 133, 139-145, 140-143, 141-142, 144, 144, 1, 3, 5
src/store/auth.js 5% 2.43% 4.44% 5.1% 38, 41, 44, 47, 50, 53, 56, 59, 59-60, 60-61, 63, 67-68, 71-78, 72-77, 73-74, 76, 79-86, 80-85, 81-82, 84, 89-91, 90, 94, 97-105, 98-102, 100-101, 104, 108, 111-119, 112-118, 116-117, 122-127, 123-126, 125, 130-134, 131-133, 132, 137-139, 138, 142-145, 144, 148-151, 150, 154, 157-158, 161-162, 165-169, 167-168, 170-179, 171-178, 173-177, 174, 176, 182, 186-188, 191-214, 192-211, 193-202, 204-207, 209-210, 213, 217-224, 218-220, 219, 222-223, 227, 232
Generated in workflow #520 for commit 3992f39 by the Vitest Coverage Report Action
github-actions[bot] commented 3 weeks ago

Visit the preview URL for this PR (updated for commit 3992f39):

https://roar-staging--pr832-ref-318-query-compos-qrqq44z0.web.app

(expires Mon, 07 Oct 2024 21:10:02 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 2631e9c58fd0104ecbfddd72a62245ddac467460

cypress[bot] commented 3 weeks ago

roar-dashboard-e2e    Run #7550

Run Properties:  status check passed Passed #7550  •  git commit 3992f39b97: Component Tests for PR 832 "Fix consent modal race condition" from commit "3992f...
Project roar-dashboard-e2e
Branch Review ref/318/query-composables-consent-modals
Run status status check passed Passed #7550
Run duration 01m 35s
Commit git commit 3992f39b97: Component Tests for PR 832 "Fix consent modal race condition" from commit "3992f...
Committer Maximilian Oertel
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 2
View all changes introduced in this branch ↗︎
maximilianoertel commented 3 weeks ago

@richford can you please take a look at this one and confirm whether the introduced changes in regards to error handling are okay? In other words: is it correct that a user should not be able to use the dashboard until their consent was given?