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

Take additional SB Config from FE #52

Conversation

devang1281
Copy link
Collaborator

@devang1281 devang1281 commented Sep 18, 2024

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

  • 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):

  • 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

    Copy link

    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.

    @devang1281 devang1281 linked an issue Sep 18, 2024 that may be closed by this pull request
    3 tasks
    Copy link

    qodo-merge-pro bot commented Sep 18, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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]

     '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
    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]

    -// 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]

    -$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
    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]

    -<script src="https://cdn.ckeditor.com/4.25.0-lts/standard/ckeditor.js"></script>
    +<script>
    +var CKEDITOR_VERSION = '4.25.0-lts';
    +document.write('<script src="https://cdn.ckeditor.com/' + CKEDITOR_VERSION + '/standard/ckeditor.js"><\/script>');
    +</script>
     
    • 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

    - Renamed and added new fields for blacklisted software and secure browser settings.
    - Updated database schema and upgrade script to include new fields.
    - Modified form elements and validation rules to accommodate new settings.
    - Adjusted language strings for new field names and help texts.
    - Updated observer and settings classes to handle new configurations.
    - Incremented plugin version to 1.5.0.
    @rohansharmasitoula rohansharmasitoula merged commit 046d8de into release/v1.5.0 Oct 15, 2024
    1 check passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [Quizaccess Proctor] [Moodle] Accept all configurations for secure browser
    2 participants