Skip to content

Stop re-wrapping an inherited set_password once per MRO class - #770

Open
januththedev wants to merge 1 commit into
jaraco:mainfrom
januththedev:fix/set-password-rewrap
Open

januththedev wants to merge 1 commit into
jaraco:mainfrom
januththedev:fix/set-password-rewrap

Conversation

@januththedev

Copy link
Copy Markdown

KeyringBackendMeta re-wraps an inherited set_password once per class in the MRO

Description

orig = cls.set_password
if getattr(orig, '_validates_username', False):
    # inherited from a base class that was already wrapped; wrapping
    # again would validate (and warn) once per class in the MRO.
    return

@functools.wraps(orig)
def wrapper(self, system, username, *args, **kwargs):
    self._validate_username(username)
    return orig(self, system, username, *args, **kwargs)

wrapper._validates_username = True
cls.set_password = wrapper

The metaclass runs this for every class it creates. When a subclass does not define its own set_password, cls.set_password resolves through the MRO to a base's already-wrapped function, so the wrapper is applied again — nesting once per class in the MRO. Each layer re-runs _validate_username, so one set_password() call validates N times.

Why that's wrong

The wrapper exists to validate the username exactly once per call. Two consequences:

  1. The #668 empty-username DeprecationWarning fires N times, and because each nested layer has a distinct stacklevel target, stacklevel=3 resolves to keyring's own backend.py:60 for the inner layers instead of the caller — a misattributed location, which breaks module-scoped warning filters.
  2. Wasted work proportional to inheritance depth.

Reproduction

warnings emitted for ONE set_password() call: 2
  DeprecationWarning: Empty usernames are deprecated. See #668  (reported at .../repro_wrap.py:25)
  DeprecationWarning: Empty usernames are deprecated. See #668  (reported at keyring/backend.py:60)
password actually stored: ('svc', '', 'pw')

Counting real _validate_username invocations across every registered backend, before the fix:

  2  keyring.backends.chainer.ChainerBackend
  1  keyring.backends.kwallet.DBusKeyring
  2  keyring.backends.kwallet.DBusKeyringKWallet4     <-- inherits, should be 1
  1  keyring.backends.SecretService.Keyring
  1  keyring.backends.macOS.Keyring
  ...

After, DBusKeyringKWallet4 is 1, one warning is emitted, and it is attributed to the caller's line. ChainerBackend correctly stays at 2 — it validates itself and then delegates to a chained backend, which are two distinct objects needing one validation each.

The marker is set after functools.wraps (which copies __dict__), so it can never be inherited from an unmarked original.

Tests

tests/test_backend_meta.py, two tests:

  • test_validates_username_once — the defect, failing assert 2 == 1 before the fix.
  • test_overridden_set_password_is_still_wrapped — guards against an over-broad fix: a subclass that supplies its own set_password must still be wrapped.
failed passed skipped xfailed xpassed
Baseline, unmodified HEAD 3 50 76 1 1
With fix 3 53 76 1 1

The 3 pre-existing failures are identical before and after and are environmental, not keyring bugs: test_Windows.py::test_read_utf8_password is a pywin32-ctypes incompatibility in the test helper, and test_multiprocess.py fails with ModuleNotFoundError: No module named 'tests.test_multiprocess' (Windows spawn + --import-mode importlib; upstream issue #737 / PR #761 address this). The 76 skips are the OS-keychain backends (macOS, kwallet, libsecret, SecretService), which need D-Bus, secretstorage, or the macOS Security framework. Ruff reports 10 findings in backend.py that are pre-existing at HEAD (identical set, line-shifted by 5); the new test file is clean.

Relationship to open work

Issue #717 ("KeyringBackend set_password() keyword argument renamed by wrapper") and PR #769 touch this exact function, but reading #769's diff, it only renames system to service — it does not address re-wrapping, so this is a distinct unfixed defect. A maintainer will likely want to land the two together. The news fragment is newsfragments/+set-password-wrap.bugfix.rst, using the skeleton's + form since there is no issue ID for this specific fix.

@januththedev
januththedev force-pushed the fix/set-password-rewrap branch from 7c4f8f2 to 3f381f5 Compare October 3, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant