Provision secure browser without proctoring

[devang1281]

User description

• [94] [FIX] Close (X) icon is overriding the candidate id on playback view ([94] [FIX] Close (X) icon is overriding the candidate id on playback view #95)
• Generate and provide a wrapper link if only SB is enabled for a quiz

Description

• Please include a summary of the change.

Github Issue

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

Checklist before requesting a review

• One line description of the changes is added in the PR
• 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):

• https://github.com/talview/lms-connector-api/issues/196

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, bug_fix

---

Description

• Implemented a secure browser wrapper for quizzes without proctoring, including functions to create and redirect to the wrapper.
• Added logic to handle sessions without proctoring, including session ID generation and tracking injection.
• Updated the plugin version and release information to reflect new changes.
• Fixed the position of the close button in the modal to prevent it from overriding other elements.

---

Changes walkthrough 📝

	Relevant files
Enhancement	tracker.php Implement secure browser wrapper and session handling        classes/local/api/tracker.php Added functions to create and redirect to a secure browser wrapper. Implemented logic to handle sessions without proctoring. Adjusted session ID generation based on proctor type. +53/-1    injector.php Add tracking injection for non-proctored sessions                classes/local/injector.php Added condition to inject tracking for sessions without proctoring. +5/-0
Configuration changes	version.php Update plugin version and release details                                version.php Updated plugin version and release information. +2/-2
Bug fix	tracker.mustache Fix modal close button position                                                    templates/tracker.mustache Adjusted position of the close button in the modal. +1/-1

---

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

[sentry-io]

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: classes/local/api/tracker.php

Function	Unhandled Issue
local_proview\local\api\tracker::insert_tracking	ErrorException: Warning: Undefined property: stdClass::$signed_short_url /var/www/html/moodle/local/proview/classes/local/api/tracker.php in local_proview\local\api... Event Count: 21
local_proview\local\api\tracker::insert_tracking	ErrorException: Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated ... Event Count: 20
local_proview\local\api\tracker::insert_tracking	ErrorException: Warning: Trying to access array offset on value of type null /var/www/html/moodle/local/proview/classes/local/api/tracker.php in local_proview\local\ap... Event Count: 1

📄 File: classes/local/injector.php (Click to Expand)

Function	Unhandled Issue
local_proview\local\injector::inject	ErrorException: Warning: Undefined property: stdClass::$signed_short_url /var/www/html/moodle/local/proview/classes/local/api/tracker.php in local_proview\local\api... Event Count: 21
local_proview\local\injector::inject	ErrorException: Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated ... Event Count: 20
local_proview\local\injector::inject	ErrorException: Warning: Trying to access array offset on value of type null /var/www/html/moodle/local/proview/classes/local/api/tracker.php in local_proview\local\ap... Event Count: 1

---

_{Did you find this useful? React with a 👍 or 👎}

[qodo-merge-pro]

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 Security concerns Sensitive information exposure: The create_sb_wrapper function in classes/local/api/tracker.php uses var_dump($data) which could potentially expose sensitive information if not removed in production. Additionally, the hardcoded app-id in the same function could be a security risk if this value is meant to be kept secret.

⚡ Key issues to review Potential Security Issue The create_sb_wrapper function is using var_dump($data) which may expose sensitive information in production environments. Hardcoded App ID The app-id is hardcoded in the create_sb_wrapper function. This should be stored in a configuration file or environment variable for better security and flexibility. Error Handling The create_sb_wrapper function catches all exceptions but only logs them. It should also return an error response or throw a specific exception for proper error handling. Code Duplication The condition strpos($_SERVER ['HTTP_USER_AGENT'], "Proview-SB") === FALSE is repeated in multiple places. Consider extracting this into a separate function for better maintainability.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Remove debug output to prevent potential information leakage Remove the var_dump($data) statement as it may expose sensitive information in production environments. classes/local/api/tracker.php [115] -var_dump($data); +// Remove or comment out this line +// var_dump($data); Apply this suggestion Suggestion importance[1-10]: 9 Why: Removing the var_dump($data) statement is crucial for preventing sensitive information from being exposed in production environments, addressing a significant security concern.	9
Error handling	Add error handling for JSON decoding to improve robustness Handle potential errors when decoding the JSON response from the API call. classes/local/api/tracker.php [119-120] $decoded_response = json_decode($response, false); +if (json_last_error() !== JSON_ERROR_NONE) { + throw new \Exception('Failed to decode API response: ' . json_last_error_msg()); +} return $decoded_response; Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding error handling for JSON decoding enhances the robustness of the code by ensuring that any issues with the API response are caught and handled appropriately, preventing potential runtime errors.	8
	Use more specific exception handling to improve error management Consider using a more specific exception type instead of \Throwable to catch only expected exceptions. classes/local/api/tracker.php [121-123] -} catch (\Throwable $err) { +} catch (\Exception $err) { + self::capture_error($err); +} catch (\Error $err) { self::capture_error($err); } Apply this suggestion Suggestion importance[1-10]: 7 Why: Using more specific exception types improves error management by allowing for more precise handling of different error scenarios, although the current use of \\Throwable is not incorrect.	7
Maintainability	Extract complex condition into a separate method to improve code readability Consider extracting the complex condition into a separate method for better readability and maintainability. classes/local/injector.php [82-86] -if (($quizaccess_proctor_setting->tsbenabled && strpos($_SERVER ['HTTP_USER_AGENT'], "Proview-SB") === FALSE)) { +if (self::shouldInsertTracking($quizaccess_proctor_setting)) { $t = new api\tracker(); $t::insert_tracking(); return; } +// Add this method to the class +private static function shouldInsertTracking($quizaccess_proctor_setting) { + return $quizaccess_proctor_setting->tsbenabled && + strpos($_SERVER['HTTP_USER_AGENT'], "Proview-SB") === FALSE; +} + Apply this suggestion Suggestion importance[1-10]: 6 Why: Extracting the complex condition into a separate method enhances code readability and maintainability, making it easier to understand and modify in the future, though it does not address a critical issue.	6

The merge-base changed after approval.