Take additional SB Config from FE

devang1281 commented 2 months ago

User description

Description

Please include a summary of the change.

Github Issue

Add github issue link and title (eg: resolves #123)

Checklist before requesting a review

[x] One line description of the changes is added in the PR
[x] Issue is linked to the PR via commits (eg: resolves #123)

Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires only a documentation update

Dependencies (if any):

Mention any dependencies/issues for reviewers reference (eg: #1124 needs to be deployed before/after this PR)

References (if any):

[ ] Any reference documents to review before/after this PR review
Add the links of any related PR/documents/diagrams for reviewers reference (eg: #link to understand login flow).

PR Type

enhancement

Description

Added new secure browser configuration fields to event data in observer.php.
Defined new properties for secure browser settings in quiz_settings.php.
Updated CKEditor script version and added form elements for secure browser configurations in settings_provider.php.
Added language strings for new secure browser settings in quizaccess_proctor.php.
Updated plugin version and build number in version.php.

Changes walkthrough 📝

Relevant files

Enhancement

observer.php `Add secure browser configuration fields to event data` classes/observer.php Added new fields for blacklisted software and security settings. Enhanced event data with additional secure browser configurations.	+4/-0
quiz_settings.php `Define new secure browser properties in quiz settings` classes/quiz_settings.php Defined new properties for secure browser settings. Added default values for new properties.	+16/-1
settings_provider.php `Add secure browser configuration elements to quiz form` classes/settings_provider.php Updated CKEditor script version. Added form elements for secure browser configurations.	+40/-1
quizaccess_proctor.php `Add language strings for secure browser settings` lang/en/quizaccess_proctor.php - Added language strings for new secure browser settings.	+5/-0

Documentation

rule.php

Add comment for secure browser parameter updates

rule.php - Added comment for future secure browser parameter updates.

+1/-0

Configuration changes

version.php

Update plugin version and build number

version.php - Updated plugin version and build number.

+2/-2

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns

Potential XSS vulnerability:
The CKEditor script is loaded from an external CDN (https://cdn.ckeditor.com) without integrity checks. If the CDN is compromised, it could lead to the execution of malicious JavaScript. Consider adding integrity and crossorigin attributes to the script tag, or hosting the CKEditor script locally.

⚡ Key issues to review

Data Type Mismatch
The `blacklisted_windows_softwares` and `blacklisted_mac_softwares` are assigned directly from `$quiz_proctor_settings` without type checking or sanitization. Potential XSS Vulnerability
The CKEditor script is loaded from an external CDN without integrity checks, which could potentially lead to XSS attacks if the CDN is compromised. Validation Concern
The `validate_blacklisted_softwares` function is referenced but not shown in the diff. Ensure it properly validates the input to prevent injection attacks.

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use boolean types for boolean properties instead of integers ___ Consider using boolean values (true/false) instead of integers (0/1) for the `sb_kiosk_mode_enable` and `sb_content_protection_enable` properties to improve type consistency and readability. [classes/quiz_settings.php [93-100]](https://github.com/talview/moodle-quizaccess_proctor/pull/52/files#diff-9617ab8291ce4b5ad0e9a97e98d8df7c8da348d293a8453d2fef0d4b3b8511a2R93-R100) ```diff 'sb_kiosk_mode_enable' => [ - 'type' => PARAM_INT, - 'default' => 0, + 'type' => PARAM_BOOL, + 'default' => false, ], 'sb_content_protection_enable' => [ - 'type' => PARAM_INT, - 'default' => 1, + 'type' => PARAM_BOOL, + 'default' => true, ], ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion enhances type consistency and readability by using boolean types for properties that represent boolean values, aligning with best practices.	8
Best practice	Replace generic comment with a specific TODO for better task tracking ___ Instead of adding a comment for future updates, consider implementing a TODO with a specific task or ticket number for better tracking and follow-up. [rule.php [176]](https://github.com/talview/moodle-quizaccess_proctor/pull/52/files#diff-46982b177d48791b7d8c2bd50e54e4dcfe70642870f2e5a13de184f3ceba7e49R176-R176) ```diff -// Add remaining SB params after updating DB +// TODO: Implement remaining Secure Browser parameters (TICKET-123) ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 5 Why: Using a specific TODO comment with a task or ticket number can improve task tracking and follow-up, but it is a minor improvement in terms of code maintenance.	5
Maintainability	Use a more descriptive variable name for better code readability ___ Consider using a more descriptive variable name instead of `tsb_enabled`. For example, `secure_browser_enabled` would be clearer and more self-explanatory. [classes/observer.php [94]](https://github.com/talview/moodle-quizaccess_proctor/pull/52/files#diff-38a8807f90f0ee92d3bbe9ecaf36eb165e825541fb514b12ddca072620fc0f41R94-R94) ```diff -$eventdata->tsb_enabled = boolval($quiz_proctor_settings->tsbenabled); +$eventdata->secure_browser_enabled = boolval($quiz_proctor_settings->tsbenabled); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code readability by using a more descriptive variable name, which helps in understanding the code's purpose more clearly. However, it is not crucial for functionality.	7
Maintainability	Add version check for external script to improve maintainability ___ Consider adding a version check for the CKEditor script to ensure compatibility and easier maintenance in the future. [classes/settings_provider.php [197]](https://github.com/talview/moodle-quizaccess_proctor/pull/52/files#diff-9f1ab4680e9da9e06b8ff210ea3697c3a98d83666ea14885a893159efaf3a12aR197-R197) ```diff - + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a version check for the CKEditor script can improve maintainability by making it easier to manage script updates, but it is not essential for the current functionality.	6

💡 Need additional feedback ? start a PR chat

talview / moodle-quizaccess_proctor