Skip to content

Invalidate PATs; fixes #598#646

Open
andrewmusselman wants to merge 1 commit intomainfrom
invalidate-pats-598
Open

Invalidate PATs; fixes #598#646
andrewmusselman wants to merge 1 commit intomainfrom
invalidate-pats-598

Conversation

@andrewmusselman
Copy link
Contributor

Invalidate PATs when user account is disabled

Fixes #598

Problem

When an LDAP account is banned or deleted, the user's Personal Access Tokens remain in the database. While the existing LDAP check in issue_jwt() prevents banned users from exchanging PATs for JWTs, the stale tokens persist for up to 180 days, violating ASVS 7.4.2.

Solution

Three layers of defense, two of which are new:

  1. LDAP check at JWT exchange (already existed) — issue_jwt() rejects banned users immediately
  2. Automated background cleanup (new) — polls LDAP ~hourly and revokes PATs for banned/deleted accounts
  3. Manual admin page (new) — allows admins to revoke all PATs for a user instantly

Changes

New files

  • atr/token_cleanup.py — background cleanup loop with cleanup_loop() and revoke_pats_for_banned_users()
  • atr/admin/templates/revoke-user-tokens.html — admin page template
  • tests/unit/test_token_cleanup.py — 7 unit tests covering active accounts, banned accounts, deleted accounts, LDAP failures, race conditions, and multi-user cleanup
  • tests/e2e/admin/ — e2e tests for the admin revoke tokens page (page rendering, form validation, revocation flow, nav link)

Modified files

  • atr/storage/writers/tokens.py — adds FoundationAdmin class with revoke_all_user_tokens()
  • atr/storage/__init__.py — wires tokens into WriteAsFoundationAdmin
  • atr/server.py — starts/stops the cleanup loop task
  • atr/admin/__init__.py — adds RevokeUserTokensForm, GET/POST route handlers
  • atr/templates/includes/topnav.html — adds "Revoke user tokens" nav link under Admin
  • atr/docs/authentication-security.md — documents the three-layer defense and automated cleanup
  • atr/docs/authorization-security.md — documents admin bulk revocation and automated cleanup access control

Design decisions

  • Separate module rather than adding to cache.pycache.py is solely about LDAP admin caching; token cleanup is active security enforcement. A separate module also avoids circular imports (cachestorageusercache).
  • Per-UID error handling in the cleanup loop — one LDAP failure doesn't block revocation of other users' tokens.
  • Poll interval offset (3617s vs 3631s for admin cache) — avoids simultaneous LDAP request spikes.
  • revoke_pats_for_banned_users() is public — directly callable from tests and potentially a future "run now" button.
  • REVOKE confirmation on the admin form — prevents accidental bulk revocation.
  • All revocations are audit logged — both automated (via storage.audit()) and manual (via append_to_audit_log()).

Required acknowledgements

  • I have read and followed CONTRIBUTING.md
  • I have read DEVELOPMENT.md
  • I have run the required tests and checks locally
  • All required checks are currently passing
  • This branch is rebased on the current main branch

Rebase confirmation details (optional but encouraged)

$ git fetch upstream main
From github.com:apache/tooling-trusted-releases
 * branch              main       -> FETCH_HEAD
$ git rebase upstream/main
Current branch invalidate-pats-598 is up to date.

Additional notes

  • Tested locally, requires REVOKE, lists all users' PATs below
image

@andrewmusselman andrewmusselman requested a review from sbp February 12, 2026 23:45
Copy link
Member

@dave2wave dave2wave left a comment

Choose a reason for hiding this comment

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

A trivial change. A cursory review shows what looks like a comprehensive PR!

@andrewmusselman
Copy link
Contributor Author

Worth noting @sbp @dave2wave I did not test with actual changes in LDAP or a ban, just revoking manually with the admin menu item.

@alitheg alitheg force-pushed the main branch 2 times, most recently from 133ab83 to 929a8c3 Compare February 16, 2026 14:27
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.

Invalidate all PATs when user account is disabled

2 participants