Skip to content

[py][bidi]: implement bidi permissions module #15830

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 4, 2025
Merged

[py][bidi]: implement bidi permissions module #15830

merged 6 commits into from
Jun 4, 2025

Conversation

navin772
Copy link
Member

@navin772 navin772 commented May 30, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

💥 What does this PR do?

Adds support for the permissions module in python - https://www.w3.org/TR/permissions/#automation-webdriver-bidi

🔧 Implementation Notes

Currently, I am supporting the permission name input in 2 ways:

  1. Direct string input: 
    driver.permissions.set_permission("geolocation", PermissionState.DENIED, origin)
  2. Via PermissionDescriptor object: 
    descriptor = PermissionDescriptor("geolocation")
driver.permissions.set_permission(descriptor, PermissionState.DENIED, origin)

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests

Description

  • Implements BiDi permissions module in Python bindings

  • Adds Permissions API to WebDriver for BiDi commands

  • Provides comprehensive tests for permissions functionality

  • Supports both string and descriptor-based permission setting

Changes walkthrough 📝

Relevant files
Enhancement
permissions.py
Add BiDi permissions module implementation                             

py/selenium/webdriver/common/bidi/permissions.py

  • Introduces PermissionState and PermissionDescriptor classes
  • Implements Permissions class for BiDi permission commands
  • Adds input validation and command execution logic
    		• +88/-0   
    webdriver.py
    Integrate BiDi permissions API into WebDriver                       

    py/selenium/webdriver/remote/webdriver.py

  • Imports and initializes the new Permissions module
  • Adds permissions property to WebDriver for BiDi access
  • Documents usage with examples in property docstring
    		• +24/-0   
    Tests
    bidi_permissions_tests.py
    Add tests for BiDi permissions module                                       

    py/test/selenium/webdriver/common/bidi_permissions_tests.py

  • Adds tests for all major permissions module features
  • Covers permission state setting, user context, and error handling
  • Validates permission constants and integration with driver
    		• +142/-0 
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels May 30, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating Chrome Driver multiple times

    Requires further human verification:

    • Issue occurs on Ubuntu 16.04.4 with Chrome 65.0.3325.181 and ChromeDriver 2.35
    • First instance works fine, but subsequent instances fail with connection errors

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()

    Requires further human verification:

    • Problem occurs in Firefox 42.0 32bit on 64bit machine
    • Alert is triggered in 2.47.1 but not in 2.48.0 or 2.48.2

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The code validates permission states but doesn't validate permission names or origins. Consider adding validation for descriptor names and origin format to prevent invalid inputs.

     
    if state not in [PermissionState.GRANTED, PermissionState.DENIED, PermissionState.PROMPT]:
    valid_states = f"{PermissionState.GRANTED}, {PermissionState.DENIED}, {PermissionState.PROMPT}"
    raise ValueError(f"Invalid permission state. Must be one of: {valid_states}")
    Test Coverage

    Tests focus on geolocation permission only. Consider adding tests for other common permission types (camera, microphone, notifications) to ensure broader functionality coverage.

     
    def test_can_set_permission_to_granted(driver, pages):
    """Test setting permission to granted state."""
    pages.load("blank.html")

    origin = get_origin(driver)

    # Set geolocation permission to granted
    driver.permissions.set_permission("geolocation", PermissionState.GRANTED, origin)

    result = get_geolocation_permission(driver)
    assert result == PermissionState.GRANTED


def test_can_set_permission_to_denied(driver, pages):
    """Test setting permission to denied state."""
    pages.load("blank.html")

    origin = get_origin(driver)

    # Set geolocation permission to denied
    driver.permissions.set_permission("geolocation", PermissionState.DENIED, origin)

    result = get_geolocation_permission(driver)
    assert result == PermissionState.DENIED


def test_can_set_permission_to_prompt(driver, pages):
    """Test setting permission to prompt state."""
    pages.load("blank.html")

    origin = get_origin(driver)

    # First set to denied, then to prompt since most of the time the default state is prompt
    driver.permissions.set_permission("geolocation", PermissionState.DENIED, origin)
    driver.permissions.set_permission("geolocation", PermissionState.PROMPT, origin)

    result = get_geolocation_permission(driver)
    assert result == PermissionState.PROMPT

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 30, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add error handling

    The test doesn't handle potential failures in the BiDi connection setup. Add
    error handling to ensure the test fails gracefully if BiDi features aren't
    available in the current browser or driver configuration.

    py/test/selenium/webdriver/common/bidi_permissions_tests.py [87-92]

     def test_can_set_permission_for_user_context(driver, pages):
     """Test setting permission for a specific user context."""
+    # Skip if BiDi is not available
+    if not hasattr(driver, 'browser') or not hasattr(driver, 'browsing_context'):
+        pytest.skip("BiDi support not available")
+        
     # Create a user context
     user_context = driver.browser.create_user_context()
 
     context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: This is a reasonable error handling suggestion that adds BiDi availability checks to prevent test failures. However, it's an error handling improvement that provides defensive programming rather than fixing a critical issue.

    		Low
    • Update

    @navin772 navin772 requested review from shbenzer and cgoldberg May 30, 2025 16:14
    @navin772 navin772 mentioned this pull request May 30, 2025
    Merged
    shbenzer
    shbenzer approved these changes Jun 3, 2025
    View reviewed changes
    Copy link
    Contributor

    @shbenzer shbenzer left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This looks solid @navin772, great work!

    @shbenzer
    Copy link
    Contributor

    shbenzer commented Jun 3, 2025

    These failures appear to be unrelated:

    //py:common-firefox-bidi-test/selenium/webdriver/common/driver_element_finding_tests.py FLAKY, failed in 1 out of 2 in 600.3s
  Stats over 2 runs: max = 600.3s, min = 571.2s, avg = 585.8s, dev = 14.6s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-firefox-bidi-test/selenium/webdriver/common/driver_element_finding_tests.py/test_attempts/attempt_1.log
//py:common-chrome-bidi-test/selenium/webdriver/common/devtools_tests.py FAILED in 2 out of 2 in 17.6s
  Stats over 2 runs: max = 17.6s, min = 17.2s, avg = 17.4s, dev = 0.2s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-chrome-bidi-test/selenium/webdriver/common/devtools_tests.py/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-chrome-bidi-test/selenium/webdriver/common/devtools_tests.py/test_attempts/attempt_1.log
//py:common-edge-bidi-test/selenium/webdriver/common/devtools_tests.py   FAILED in 2 out of 2 in 17.4s
  Stats over 2 runs: max = 17.4s, min = 17.1s, avg = 17.3s, dev = 0.2s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-edge-bidi-test/selenium/webdriver/common/devtools_tests.py/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-edge-bidi-test/selenium/webdriver/common/devtools_tests.py/test_attempts/attempt_1.log
//py:common-firefox-bidi-test/selenium/webdriver/common/webdriverwait_tests.py FAILED in 2 out of 2 in 302.5s
  Stats over 2 runs: max = 302.5s, min = 301.1s, avg = 301.8s, dev = 0.7s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-firefox-bidi-test/selenium/webdriver/common/webdriverwait_tests.py/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-firefox-bidi-test/selenium/webdriver/common/webdriverwait_tests.py/test_attempts/attempt_1.log

    @navin772
    Copy link
    Member Author

    navin772 commented Jun 4, 2025

    @shbenzer yes, those are unrelated tests. The recent CI run passes for all tests.

    @shbenzer shbenzer merged commit 42c06ec into SeleniumHQ:trunk Jun 4, 2025
    16 checks passed
    @navin772 navin772 deleted the py-bidi-permission branch June 4, 2025 14:29
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @shbenzer shbenzer shbenzer approved these changes

    @cgoldberg cgoldberg Awaiting requested review from cgoldberg

    Assignees
    No one assigned
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @navin772 @shbenzer @selenium-ci