Skip to content

fix(auth): improve OAuth token lifecycle and storage reliability - #128

Open
jpsiddharth2008 wants to merge 4 commits into
nitrocloudofficial:developfrom
jpsiddharth2008:feature/issue-115-oauth-followups
Open

jpsiddharth2008 wants to merge 4 commits into
nitrocloudofficial:developfrom
jpsiddharth2008:feature/issue-115-oauth-followups

Conversation

@jpsiddharth2008

@jpsiddharth2008 jpsiddharth2008 commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Closes #115.

Make the OAuth client easier to use with real providers: accept string expiry values, retrieve usable access tokens automatically, and support local logout when remote revocation is unavailable. Reduce encrypted-store and lock-registry overhead while preserving existing storage compatibility and default revocation behavior.

Changes

  • Accept finite, nonnegative numeric strings in provider expires_in responses. Preserve the one-hour missing/null fallback, immediate zero expiry, and strict numeric helper APIs.
  • Add revoke_token(..., local_fallback=True) to permit local deletion after a missing endpoint or remote failure. The default still retains credentials on failure. Invalid hints, storage failures and cancellation are not hidden.
  • Derive encrypted-store keys once per instance, retaining the original salt, PBKDF2 parameters and AES-GCM file format. Writes still use fresh IVs.
  • Accept SecretValue in typed and mapping-based client configuration; unwrap only when constructing authentication.
  • Add get_access_token(key) -> str with expiry checks, shared refresh tasks, cancellation isolation, rotation preservation, persistence before success, and a locked recheck to prevent duplicate refreshes. Explicit refresh requests still force refresh even when joining an automatic check.
  • Replace the growing file-path lock registry with 256 fixed process-local locks. Document that refresh coordination requires a shared client and does not protect rotation across processes.
  • Update OAuth usage and implementation documentation, remove the historical verification report from Git, and remove its references.

File-length follow-up

Split the OAuth client types and follow-up tests into cohesive modules so this PR no longer depends on merging before #126:

File Physical lines
nitrostack/auth/client.py 418
nitrostack/auth/_client_types.py 98
tests/test_oauth_client.py 418
tests/test_oauth_client_followups.py 242
tests/support/oauth_client.py 39

PR #126's checker passes these files without baseline exemptions. Existing top-level and nitrostack.auth.client type imports retain object identity. AST comparisons confirm all production class/function bodies and every original test and assertion are unchanged. Shared test helpers live in a non-test support module.

Validation after the file split (b1f4dac)

  • Complete collection: 1,373 tests; focused OAuth/storage/secret regressions: 156 passed.
  • Full Linux suites: 1,373 passed on each of Python 3.10, 3.11 and 3.12; coverage 87.69%, 87.67% and 87.67%, respectively.
  • Windows Python 3.12: 1,356 passed, 17 existing skips, no failures; coverage 85.64%. The earlier Windows failures below remain historical results and were not hidden or excluded.
  • Hosted CI on the pushed head: Python 3.10, 3.11 and 3.12 all passed.
  • Public import identity, legacy pickle type lookup, unchanged production/test ASTs, PR ci: enforce 500-line Python file-length ratchet (#111) #126's file-length checker, and staged whitespace checks pass.

Validation before the file split

  • Full collection: 1,373 tests.
  • Focused OAuth client, transport, token-store and SecretValue tests: 156 passed.
  • Full Linux suites on Python 3.10, 3.11 and 3.12: 1,373 passed per version; 87.67% coverage each; MCP 2.3.0 and native QuickJS 1.19.4.
  • Windows Python 3.10: 1,354 passed, 17 existing skips, 2 OAuth discovery socket failures; 85.65% coverage.
  • Windows Python 3.12: 1,354 passed, 17 existing skips, 1 OAuth discovery socket failure and 1 installed-wheel pip timeout; 85.63% coverage.
  • Windows Python 3.11: 1,355 passed, 17 existing skips, 1 OAuth discovery socket failure; 85.63% coverage.
  • Existing encrypted-format fixture, atomic-write failure, permission, symlink and concurrency regressions remain enabled.
  • No new dependencies, skips, exclusions, or xfail markers. Diff whitespace checks pass.

The initial Windows 3.12 run reached the 60-second per-test limit in the existing installed-SDK-wheel template test. A targeted run on unchanged develop reproduced a timeout in the same test. The complete Windows matrix uses a 180-second per-test limit; initial failures remain in local verification records. A matching baseline run reproduced the installed-wheel pip timeout on Python 3.12. Baseline OAuth module runs also reproduce WinError 10053: the first discovery test on Python 3.10 and the second discovery test on Python 3.12. Both OAuth discovery tests failed with WinError 10053 on the unchanged Python 3.11 baseline, including the exact test failing in the feature run. The exact first-test Python 3.12 baseline probe passed, so that individual failure was not reproduced under identical conditions. These results do not constitute an all-green Windows suite.

Compatibility

Existing callers and plaintext stores continue to work. Remote revocation remains required unless callers explicitly enable local fallback. Local deletion does not establish provider-side invalidation. Cross-process refresh-token rotation remains unsupported, as permitted by the issue.

@manish-wekan

Copy link
Copy Markdown
Collaborator

Reviewed with develop (12e65b1) merged in: 1373 tests pass and all 7 items in #115 are covered. Looks good to merge. One merge-order note: once #126's file-length check lands, nitrostack/auth/client.py (462 → 503 lines) and tests/test_oauth_client.py (447 → 677) will fail it, so either this merges first and #126 updates its baseline, or these two files need a split.

~sent from claude-code

This branch has not been deployed

No deployments
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.

2 participants