Skip to content

[py] Add high-level BiDi permissions API#17631

Open
AutomatedTester wants to merge 3 commits into
trunkfrom
claude/permissions-api-design-MKlID
Open

[py] Add high-level BiDi permissions API#17631
AutomatedTester wants to merge 3 commits into
trunkfrom
claude/permissions-api-design-MKlID

Conversation

@AutomatedTester

@AutomatedTester AutomatedTester commented Jun 4, 2026

Copy link
Copy Markdown
Member

Introduces a PermissionsManager helper (py/private/_permissions_handlers.py)
that layers convenience methods on top of the existing permissions.setPermission
command, following the same handler-module pattern used for network and script.

Changes

  • New _permissions_handlers.py staged into the bidi package by Bazel
  • Permissions.__init__ now accepts an optional driver argument (same as Script)
  • bidi_enhancements_manifest.py wires up PermissionsManager and exposes the
    convenience methods on the generated Permissions class
  • BUILD.bazel adds _permissions_handlers.py to create-bidi-src extra_srcs
  • generate_bidi.py extends the driver-arg __init__ to the permissions module
  • Unit tests and browser tests for all new methods

New API

# Grant one or more permissions
driver.permissions.grant("geolocation", origin=origin)
driver.permissions.grant(["geolocation", "camera"], origin=origin)

# Deny a permission
driver.permissions.deny("geolocation", origin=origin)

# Reset one, several, or all tracked overrides
driver.permissions.reset("geolocation", origin=origin)
driver.permissions.reset(["geolocation", "camera"], origin=origin)
driver.permissions.reset()  # clears all overrides applied via grant/deny/override

# Temporary override — resets to prompt on exit, even if an exception is raised
with driver.permissions.override("geolocation", "granted", origin=origin):
    ...

API comparison: Selenium BiDi vs. Playwright

Feature Playwright (BrowserContext) Selenium BiDi (driver.permissions)
Grant one grant_permissions(["geolocation"], origin=…) grant("geolocation", origin=…)
Grant many grant_permissions(["geo", "camera"], origin=…) grant(["geo", "camera"], origin=…)
Deny ❌ not supported — denied is the browser default deny("geolocation", origin=…)
Reset one ❌ only bulk via clear_permissions() reset("geolocation", origin=…)
Reset many ❌ only bulk reset(["geolocation", "camera"], origin=…)
Reset all clear_permissions() reset() — clears tracked overrides
Temporary override ❌ no context manager override("geolocation", "granted", origin=…)
Origin scoping ✅ optional origin= ✅ optional origin=
User context scoping ❌ not supported user_context=
Low-level access ❌ no direct BiDi access set_permission(descriptor, state, origin)

https://claude.ai/code/session_018zDh8UVeD7rTyJQQZpAVne

@CLAassistant

CLAassistant commented Jun 4, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jun 4, 2026
@AutomatedTester AutomatedTester force-pushed the claude/permissions-api-design-MKlID branch from aa9a317 to df04f26 Compare June 5, 2026 14:28
Introduces a PermissionsManager helper (py/private/_permissions_handlers.py)
that layers grant/deny/reset/reset_all and a context manager (override) on top
of the existing permissions.setPermission command, following the same
handler-module pattern used for network and script.

Changes:
- New _permissions_handlers.py staged into the bidi package by Bazel
- Permissions.__init__ now accepts an optional driver argument (same as Script)
- bidi_enhancements_manifest.py wires up PermissionsManager and exposes the
  convenience methods on the generated Permissions class
- BUILD.bazel adds _permissions_handlers.py to create-bidi-src extra_srcs
- generate_bidi.py extends the driver-arg __init__ to the permissions module
- New unit tests cover grant/deny/reset/reset_all and the context manager
@AutomatedTester AutomatedTester force-pushed the claude/permissions-api-design-MKlID branch from df04f26 to b0687ba Compare June 8, 2026 14:08
@AutomatedTester AutomatedTester added the A-needs decision TLC needs to discuss and agree label Jun 11, 2026
Aligns the low-level set_permission command with the grant/deny/reset/
override convenience methods, where origin and user_context are already
keyword-only. Internal callers and tests updated accordingly.
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (3) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 17 rules

Grey Divider


Action required

1. set_permission breaks positional origin 📘 Rule violation ≡ Correctness
Description
Permissions.set_permission() now makes origin/user_context (and related scoping args)
keyword-only, which breaks existing callers that pass these positionally and will raise TypeError
at runtime. This is a backward-incompatible public API change that is being enforced by tests
without any prior deprecation period or migration path.
Code

py/private/bidi_enhancements_manifest.py[R1918-1934]

            '''    def set_permission(
        self,
        descriptor: "PermissionDescriptor | str",
        state: "PermissionState | str",
+        *,
        origin: str | None = None,
        user_context: str | None = None,
-        *,
        embedded_origin: str | None = None,
    ) -> None:
        """Set a browser permission.

        Args:
            descriptor: The permission descriptor or permission name as a string.
            state: The desired permission state (granted, denied, or prompt).
-            origin: The origin to scope the permission to.
-            user_context: Optional user context ID to scope the permission.
+            origin: Keyword-only. The origin to scope the permission to.
+            user_context: Keyword-only. Optional user context ID to scope the
+                permission.
Evidence
The manifest change to set_permission inserts * ahead of origin, making
origin/user_context (and embedded_origin) keyword-only and thereby changing call resolution
for existing positional callers, which violates the backward-compatibility and deprecation
requirements referenced by PR Compliance IDs 389266 (public API compatibility) and 389271
(deprecation guidance before breaking changes). The accompanying BiDi permissions browser tests
explicitly assert that passing origin positionally now raises TypeError, demonstrating the
behavior change is intentional/enforced while no deprecation mechanism or transition strategy is
added in the shown diff.

Rule 389266: Maintain backward-compatible public API and ABI
Rule 389271: Deprecate public APIs with guidance before removal
py/private/bidi_enhancements_manifest.py[1918-1934]
py/test/selenium/webdriver/common/bidi_permissions_tests.py[145-156]
py/private/bidi_enhancements_manifest.py[1918-1926]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Permissions.set_permission()` was changed so that `origin` and `user_context` (and scoping-related args such as `embedded_origin`) are keyword-only by placing `*` before them. This introduces a runtime backwards-incompatible public API change for any existing callers passing `origin`/`user_context` positionally (now raising `TypeError`) and provides no deprecation period or migration path.

## Issue Context
Tests were updated to assert that positional scoping arguments now raise `TypeError`, confirming the break is intentional and enforced. If the keyword-only style is desired for consistency with other methods (e.g., `grant/deny/reset`), it should be introduced without breaking existing callers (for example, by continuing to accept positional `origin`/`user_context` for now while emitting a deprecation warning and documenting a removal timeline), or by introducing the stricter signature under a new method name/versioned API; if the hard break is truly intended, add explicit migration guidance (release notes/changelog) and ensure all in-repo examples/docs are updated accordingly.

## Fix Focus Areas
- py/private/bidi_enhancements_manifest.py[1918-1934]
- py/test/selenium/webdriver/common/bidi_permissions_tests.py[145-156]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. override() missing return type 📘 Rule violation ✧ Quality
Description
The newly added Permissions.override() method signature lacks an explicit return type annotation.
This violates the requirement that new function/method signatures include return annotations.
Code

py/private/bidi_enhancements_manifest.py[R2025-2033]

+            '''    def override(
+        self,
+        descriptor: "PermissionDescriptor | str",
+        state: "PermissionState | str",
+        *,
+        origin: str | None = None,
+        user_context: str | None = None,
+    ):
+        """Return a context manager that applies *state* on enter and resets on exit.
Evidence
PR Compliance ID 337802 requires explicit parameter and return type annotations on newly
added/modified function and method signatures. The new override() method definition ends with ):
and provides no -> ReturnType annotation.

Rule 337802: Require type annotations on new function and method signatures
py/private/bidi_enhancements_manifest.py[2025-2033]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The generated `Permissions.override()` method is added without a return type annotation (missing `-> ...`).

## Issue Context
This method returns the context manager created by `self._manager.override(...)`, so it should be annotated (e.g., `-> PermissionOverrideContext`).

## Fix Focus Areas
- py/private/bidi_enhancements_manifest.py[2025-2049]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Test helpers lack annotations 📘 Rule violation ✧ Quality
Description
New unit-test helpers introduce new functions/methods without parameter/return type annotations
(e.g., FakeConnection.execute, make_permissions). This reduces type clarity and violates the
"new signatures must be annotated" requirement.
Code

py/test/unit/selenium/webdriver/common/bidi_permissions_tests.py[R24-55]

+class FakeConnection:
+    def __init__(self):
+        self.commands = []
+
+    def execute(self, cmd):
+        payload = next(cmd)
+        self.commands.append(payload)
+        try:
+            cmd.send({})
+        except StopIteration as exc:
+            return exc.value
+        raise AssertionError("BiDi command generator did not finish")
+
+    def commands_named(self, method):
+        return [c for c in self.commands if c["method"] == method]
+
+    def last_set_permission(self):
+        cmds = self.commands_named("permissions.setPermission")
+        return cmds[-1]["params"] if cmds else None
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def make_permissions(conn=None):
+    """Return a Permissions instance backed by *conn* (creates a new one if omitted)."""
+    if conn is None:
+        conn = FakeConnection()
+    return Permissions(conn, driver=None), conn
+
Evidence
PR Compliance ID 337802 requires explicit type annotations on new function and method signatures.
The newly added FakeConnection methods and make_permissions function are introduced without any
parameter or return annotations.

Rule 337802: Require type annotations on new function and method signatures
py/test/unit/selenium/webdriver/common/bidi_permissions_tests.py[24-55]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New helper code in unit tests defines functions/methods without type annotations (parameters and return types).

## Issue Context
Even in tests, adding minimal annotations (e.g., `-> None`, `-> dict[str, Any] | None`, and concrete types for `cmd`/`payload`) improves readability and aligns with the repository compliance rule.

## Fix Focus Areas
- py/test/unit/selenium/webdriver/common/bidi_permissions_tests.py[24-55]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. reset() leaves stale tracking 🐞 Bug ☼ Reliability
Description
PermissionsManager.reset() (no-arg form) clears _active_overrides only after looping over
overrides; if any set_permission() call raises mid-loop, tracking entries for already-reset
permissions remain, leaving client-side state inconsistent. This can cause later reset() behavior
and bookkeeping to be misleading after partial failures.
Code

py/private/_permissions_handlers.py[R200-214]

+        if descriptor is None:
+            for name, o, uc in list(self._active_overrides):
+                self._permissions.set_permission(name, "prompt", origin=o, user_context=uc)
+                logger.debug(
+                    "Permission %r reset (origin=%r, user_context=%r)",
+                    name,
+                    o,
+                    uc,
+                )
+            self._active_overrides.clear()
+        elif _is_single_descriptor(descriptor):
+            self._apply(descriptor, "prompt", origin, user_context)
+        else:
+            for d in descriptor:
+                self._apply(d, "prompt", origin, user_context)
Evidence
_apply() updates _active_overrides after a successful command, but reset(None) bypasses
_apply() and relies on a final clear() that will not execute if an exception interrupts the
loop.

py/private/_permissions_handlers.py[114-128]
py/private/_permissions_handlers.py[200-214]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In `PermissionsManager.reset(descriptor=None)`, browser state is updated one override at a time, but `_active_overrides` is only cleared at the end. If a `set_permission(..., "prompt", ...)` call raises, the final `clear()` is skipped and the manager keeps entries for overrides that may already have been reset.

### Issue Context
`_apply()` already encapsulates the “send command then update tracking dict” behavior safely on success. The no-arg reset path bypasses `_apply()`.

### Fix Focus Areas
- py/private/_permissions_handlers.py[114-128]
- py/private/_permissions_handlers.py[200-214]

### Suggested fix
Refactor the no-arg branch to update tracking incrementally:
- Iterate over a snapshot of keys (`for key in list(self._active_overrides): ...`).
- Call `_apply(name, "prompt", o, uc)` instead of calling `set_permission` directly (this will pop the entry only after a successful command).
- Remove the final unconditional `clear()` (or keep it only as a safe cleanup after the loop if you prefer).

This preserves correct tracking even if a later reset fails mid-loop.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines 1918 to +1934
''' def set_permission(
self,
descriptor: "PermissionDescriptor | str",
state: "PermissionState | str",
*,
origin: str | None = None,
user_context: str | None = None,
*,
embedded_origin: str | None = None,
) -> None:
"""Set a browser permission.

Args:
descriptor: The permission descriptor or permission name as a string.
state: The desired permission state (granted, denied, or prompt).
origin: The origin to scope the permission to.
user_context: Optional user context ID to scope the permission.
origin: Keyword-only. The origin to scope the permission to.
user_context: Keyword-only. Optional user context ID to scope the
permission.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. set_permission breaks positional origin 📘 Rule violation ≡ Correctness

Permissions.set_permission() now makes origin/user_context (and related scoping args)
keyword-only, which breaks existing callers that pass these positionally and will raise TypeError
at runtime. This is a backward-incompatible public API change that is being enforced by tests
without any prior deprecation period or migration path.
Agent Prompt
## Issue description
`Permissions.set_permission()` was changed so that `origin` and `user_context` (and scoping-related args such as `embedded_origin`) are keyword-only by placing `*` before them. This introduces a runtime backwards-incompatible public API change for any existing callers passing `origin`/`user_context` positionally (now raising `TypeError`) and provides no deprecation period or migration path.

## Issue Context
Tests were updated to assert that positional scoping arguments now raise `TypeError`, confirming the break is intentional and enforced. If the keyword-only style is desired for consistency with other methods (e.g., `grant/deny/reset`), it should be introduced without breaking existing callers (for example, by continuing to accept positional `origin`/`user_context` for now while emitting a deprecation warning and documenting a removal timeline), or by introducing the stricter signature under a new method name/versioned API; if the hard break is truly intended, add explicit migration guidance (release notes/changelog) and ensure all in-repo examples/docs are updated accordingly.

## Fix Focus Areas
- py/private/bidi_enhancements_manifest.py[1918-1934]
- py/test/selenium/webdriver/common/bidi_permissions_tests.py[145-156]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +2025 to +2033
''' def override(
self,
descriptor: "PermissionDescriptor | str",
state: "PermissionState | str",
*,
origin: str | None = None,
user_context: str | None = None,
):
"""Return a context manager that applies *state* on enter and resets on exit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

2. override() missing return type 📘 Rule violation ✧ Quality

The newly added Permissions.override() method signature lacks an explicit return type annotation.
This violates the requirement that new function/method signatures include return annotations.
Agent Prompt
## Issue description
The generated `Permissions.override()` method is added without a return type annotation (missing `-> ...`).

## Issue Context
This method returns the context manager created by `self._manager.override(...)`, so it should be annotated (e.g., `-> PermissionOverrideContext`).

## Fix Focus Areas
- py/private/bidi_enhancements_manifest.py[2025-2049]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-needs decision TLC needs to discuss and agree B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants