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.

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Member

@navin772 navin772 commented May 30, 2025

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

    Copy link
    Contributor

    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
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants