From 78204252e03dbd714fd3ecfcb01fd50fd5a45b6e Mon Sep 17 00:00:00 2001 From: Siddharth Date: Thu, 5 Nov 2026 20:40:51 +0530 Subject: [PATCH 1/4] fix(auth): accept provider expiry strings and bound token-store overhead --- nitrostack/auth/token_store.py | 23 ++++++++------ tests/test_token_store.py | 55 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/nitrostack/auth/token_store.py b/nitrostack/auth/token_store.py index 9534466..77b5c30 100644 --- a/nitrostack/auth/token_store.py +++ b/nitrostack/auth/token_store.py @@ -86,6 +86,8 @@ def token_response_to_stored( """Use a one-hour fallback only for absent expiry, preserving refresh rotation.""" try: expires_in = response.get("expires_in") + if isinstance(expires_in, str): + expires_in = float(expires_in.strip()) return StoredToken( access_token=response["access_token"], token_type=response.get("token_type", "Bearer"), @@ -142,8 +144,8 @@ async def clear(self) -> None: self._tokens.clear() -_path_locks: dict[str, Any] = {} -_path_locks_guard = threading.Lock() +_ENCRYPTION_SALT = b"nitrostack-token-store" +_path_locks = tuple(threading.RLock() for _ in range(256)) class FileTokenStore: @@ -157,20 +159,23 @@ def __init__(self, store_path: str | Path, encryption_key: str | None = None): self._path = Path(store_path).expanduser().absolute() if encryption_key is not None and (not isinstance(encryption_key, str) or not encryption_key): raise TokenStoreError("Encryption key must be a nonempty string") - self._encryption_key = encryption_key + self._encryption_key = None + if encryption_key is not None: + try: + self._encryption_key = hashlib.pbkdf2_hmac( + "sha256", encryption_key.encode("utf-8"), _ENCRYPTION_SALT, 100000, 32, + ) + except Exception: + raise TokenStoreError("Token store key derivation failed") from None canonical = os.path.normcase(str(self._path.parent.resolve() / self._path.name)) - with _path_locks_guard: - self._lock = _path_locks.setdefault(canonical, threading.RLock()) + self._lock = _path_locks[hash(canonical) % len(_path_locks)] def _cipher_parts(self): try: from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes except ImportError: raise TokenStoreError("Install nitrostack[token-encryption] for encrypted storage") from None - key = hashlib.pbkdf2_hmac( - "sha256", self._encryption_key.encode("utf-8"), b"nitrostack-token-store", 100000, 32, - ) - return Cipher, algorithms, modes, key + return Cipher, algorithms, modes, self._encryption_key def _encrypt(self, text: str) -> str: if self._encryption_key is None: diff --git a/tests/test_token_store.py b/tests/test_token_store.py index e4a29bd..3e3dcd9 100644 --- a/tests/test_token_store.py +++ b/tests/test_token_store.py @@ -209,3 +209,58 @@ def test_default_store_helpers_are_explicit(tmp_path, monkeypatch): assert Path(get_default_store_path()) == tmp_path / ".nitrostack" / "tokens.json" assert isinstance(create_default_token_store(), FileTokenStore) assert not (tmp_path / ".nitrostack").exists() + +@pytest.mark.parametrize("value,seconds", [("3600", 3600), ("0", 0), (" 1.5 ", 1.5), ("1e2", 100)]) +def test_provider_numeric_string_expiry(monkeypatch, value, seconds): + monkeypatch.setattr("nitrostack.auth.token_store.time.time", lambda: 1000) + assert token_response_to_stored({"access_token": ACCESS, "expires_in": value}).expires_at == int((1000 + seconds) * 1000) + + +@pytest.mark.parametrize("value", ["", " ", "no", "-1", "NaN", "Infinity", "1e999", "1e308", True]) +def test_provider_invalid_expiry_is_sanitized(value): + with pytest.raises(TokenStoreError, match="Invalid OAuth token response"): + token_response_to_stored({"access_token": ACCESS, "expires_in": value}) + + +@pytest.mark.asyncio +async def test_key_is_derived_once_per_instance(tmp_path, monkeypatch): + original = hashlib.pbkdf2_hmac + calls = [] + def derive(*args): + calls.append(1) + return original(*args) + monkeypatch.setattr(hashlib, "pbkdf2_hmac", derive) + path = tmp_path / "tokens" + store = FileTokenStore(path, KEY) + assert len(calls) == 1 and KEY not in vars(store).values() + await store.save_token("a", token()) + first = path.read_text().split(":")[0] + await store.get_token("a") + await store.save_token("b", token()) + assert path.read_text().split(":")[0] != first + assert len(calls) == 1 + assert await FileTokenStore(path, KEY).get_token("a") == token() + assert len(calls) == 2 + + +@pytest.mark.asyncio +async def test_fixed_lock_pool_same_path_and_collisions(tmp_path): + from nitrostack.auth.token_store import _path_locks + stores = [FileTokenStore(tmp_path / str(i)) for i in range(600)] + assert len(_path_locks) == 256 + assert len({id(store._lock) for store in stores}) <= 256 + same = FileTokenStore(tmp_path / "." / "0") + assert same._lock is stores[0]._lock + await asyncio.gather(*(FileTokenStore(tmp_path / "shared").save_token(str(i), token()) for i in range(30))) + assert len(await FileTokenStore(tmp_path / "shared").list_keys()) == 30 + by_lock = {} + for store in stores: + if id(store._lock) in by_lock: + other = by_lock[id(store._lock)] + await asyncio.gather(store.save_token("first", token()), other.save_token("second", token())) + assert await store.list_keys() == ["first"] + assert await other.list_keys() == ["second"] + break + by_lock[id(store._lock)] = store + else: + pytest.fail("Expected a collision in the fixed pool") From 8a400907208dac237d7c323e0298d68e790a4a42 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Thu, 5 Nov 2026 20:40:52 +0530 Subject: [PATCH 2/4] feat(auth): add managed access tokens and local revocation fallback --- nitrostack/auth/client.py | 73 +++++++++--- tests/test_oauth_client.py | 230 +++++++++++++++++++++++++++++++++++++ 2 files changed, 287 insertions(+), 16 deletions(-) diff --git a/nitrostack/auth/client.py b/nitrostack/auth/client.py index 92d2663..8260d37 100644 --- a/nitrostack/auth/client.py +++ b/nitrostack/auth/client.py @@ -17,8 +17,11 @@ from typing import Any, Mapping from nitrostack.auth import cimd +from nitrostack.auth.secret import SecretValue, unwrap_secret from nitrostack.auth.pkce import generate_pkce_params, is_valid_code_verifier, validate_pkce_support -from nitrostack.auth.token_store import MemoryTokenStore, StoredToken, TokenStore, token_response_to_stored +from nitrostack.auth.token_store import ( + MemoryTokenStore, StoredToken, TokenStore, is_token_expired, token_response_to_stored, +) _OAUTH_ERRORS = frozenset({ @@ -45,7 +48,7 @@ def __init__(self, operation: str, *, status: int | None = None, error_code=None @dataclass(frozen=True) class OAuth2ClientConfig: client_id: str | None = field(default=None, repr=False) - client_secret: str | None = field(default=None, repr=False) + client_secret: str | SecretValue | None = field(default=None, repr=False) issuer: str | None = field(default=None, repr=False) redirect_uri: str | None = field(default=None, repr=False) authorization_endpoint: str | None = field(default=None, repr=False) @@ -57,8 +60,11 @@ class OAuth2ClientConfig: def __post_init__(self): if any(value is not None and (not isinstance(value, str) or not value) - for value in (getattr(self, item.name) for item in fields(self))): + for value in (getattr(self, item.name) for item in fields(self) if item.name != "client_secret")): raise OAuthClientError("configuration") + if self.client_secret is not None and not isinstance(self.client_secret, SecretValue): + if not isinstance(self.client_secret, str) or not self.client_secret: + raise OAuthClientError("configuration") @dataclass(frozen=True) @@ -156,6 +162,7 @@ def __init__( self.max_response_bytes = max_response_bytes self._metadata: dict[str, Any] = {} self._refresh_tasks: dict[str, asyncio.Task[StoredToken]] = {} + self._force_refresh: dict[str, bool] = {} self._key_locks: weakref.WeakValueDictionary[str, asyncio.Lock] = weakref.WeakValueDictionary() self._trusted_urls = { value for item in fields(self.config) @@ -350,8 +357,14 @@ def _authenticate(self, params: dict[str, Any]) -> dict[str, str]: params["client_id"] = self._client_id() headers = {"Content-Type": "application/x-www-form-urlencoded"} if self.config.client_secret is not None: + try: + secret = unwrap_secret(self.config.client_secret) + if not secret: + raise ValueError + except Exception: + raise OAuthClientError("client authentication") from None credentials = ":".join(urllib.parse.quote_plus(value, safe="") for value in - (self._client_id(), self.config.client_secret)) + (self._client_id(), secret)) headers["Authorization"] = "Basic " + base64.b64encode(credentials.encode("ascii")).decode("ascii") return headers @@ -393,15 +406,18 @@ async def get_client_credentials_token( async with self._key_lock(key): return await self._obtain_token(key, params, resource=resource) - async def _refresh(self, key: str) -> StoredToken: + async def _refresh(self, key: str, *, only_if_expired: bool = False) -> StoredToken: async with self._key_lock(key): - return await self._refresh_locked(key) + return await self._refresh_locked(key, only_if_expired=only_if_expired) - async def _refresh_locked(self, key: str) -> StoredToken: + async def _refresh_locked(self, key: str, *, only_if_expired: bool = False) -> StoredToken: try: previous = await self.store.get_token(key) except Exception: raise OAuthClientError("token retrieval") from None + if (only_if_expired and not self._force_refresh.get(key, False) + and previous is not None and not is_token_expired(previous)): + return previous if previous is None or not previous.refresh_token: raise OAuthClientError("refresh credential") params = {"grant_type": "refresh_token", "refresh_token": previous.refresh_token} @@ -409,39 +425,64 @@ async def _refresh_locked(self, key: str) -> StoredToken: params["scope"] = previous.scope return await self._obtain_token(key, params, resource=previous.resource, previous=previous) + async def get_access_token(self, key: str) -> str: + """Return a usable access token, refreshing and persisting when necessary.""" + token = await self._coalesced_refresh(key, only_if_expired=True) + if is_token_expired(token): + raise OAuthClientError("access token expiry") + return token.access_token + async def refresh_token(self, key: str) -> StoredToken: + return await self._coalesced_refresh(key) + + async def _coalesced_refresh(self, key: str, *, only_if_expired: bool = False) -> StoredToken: if not isinstance(key, str) or not key: raise OAuthClientError("token key") task = self._refresh_tasks.get(key) - if task is None: - task = asyncio.create_task(self._refresh(key)) + if task is None or task.done(): + self._force_refresh[key] = not only_if_expired + task = asyncio.create_task(self._refresh(key, only_if_expired=only_if_expired)) self._refresh_tasks[key] = task def completed(done): if self._refresh_tasks.get(key) is done: self._refresh_tasks.pop(key, None) + self._force_refresh.pop(key, None) if not done.cancelled(): done.exception() # Consume failures even if every waiter cancelled. task.add_done_callback(completed) + elif not only_if_expired: + self._force_refresh[key] = True return await asyncio.shield(task) - async def revoke_token(self, key: str, *, token_type_hint: str | None = None) -> None: + async def revoke_token( + self, key: str, *, token_type_hint: str | None = None, local_fallback: bool = False, + ) -> None: + """Revoke remotely; opt in to local deletion if remote revocation fails.""" + if type(local_fallback) is not bool: + raise OAuthClientError("revocation configuration") async with self._key_lock(key): - await self._revoke_locked(key, token_type_hint) + await self._revoke_locked(key, token_type_hint, local_fallback) - async def _revoke_locked(self, key: str, token_type_hint: str | None) -> None: + async def _revoke_locked(self, key: str, token_type_hint: str | None, local_fallback: bool) -> None: try: stored = await self.store.get_token(key) if stored is None: return - hint = token_type_hint or ("refresh_token" if stored.refresh_token else "access_token") + hint = token_type_hint if token_type_hint is not None else ( + "refresh_token" if stored.refresh_token else "access_token" + ) if hint not in ("access_token", "refresh_token") or not getattr(stored, hint): raise OAuthClientError("revocation configuration") params = {"token": getattr(stored, hint), "token_type_hint": hint} - headers = self._authenticate(params) - await self._request("revocation", "POST", self._endpoint("revocation_endpoint"), - body=urllib.parse.urlencode(params).encode("utf-8"), headers=headers) + try: + headers = self._authenticate(params) + await self._request("revocation", "POST", self._endpoint("revocation_endpoint"), + body=urllib.parse.urlencode(params).encode("utf-8"), headers=headers) + except OAuthClientError: + if not local_fallback: + raise await self.store.delete_token(key) except OAuthClientError: raise diff --git a/tests/test_oauth_client.py b/tests/test_oauth_client.py index 4622146..61d2bf1 100644 --- a/tests/test_oauth_client.py +++ b/tests/test_oauth_client.py @@ -445,3 +445,233 @@ def test_invalid_client_limits(options): def test_invalid_operator_url_is_rejected(url): with pytest.raises(OAuthClientError): client(token_endpoint=url) + +@pytest.mark.asyncio +async def test_secret_wrapper_deferred_unwrap(monkeypatch, caplog, capsys): + import pickle + from nitrostack import SecretValue + secret = SecretValue(SECRET) + original = SecretValue.unwrap + calls = [] + def unwrap(self): + calls.append(1) + return original(self) + monkeypatch.setattr(SecretValue, "unwrap", unwrap) + oauth = client(client_secret=secret) + assert oauth.config.client_secret is secret and calls == [] + send = install_transport(monkeypatch, oauth) + await oauth.get_client_credentials_token("alice") + assert len(calls) == 1 + plain = client() + plain_send = install_transport(monkeypatch, plain) + await plain.get_client_credentials_token("alice") + assert send.call_args.args[0].headers == plain_send.call_args.args[0].headers + for unusable in (pickle.loads(pickle.dumps(secret)),): + broken = client(client_secret=unusable) + transport = install_transport(monkeypatch, broken) + with pytest.raises(OAuthClientError) as exc: + await broken.get_client_credentials_token("alice") + transport.assert_not_awaited() + assert SECRET not in str(exc.value) + repr(exc.value) + assert SECRET not in repr(oauth.config) + caplog.text + str(capsys.readouterr()) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("failure", ["missing", "http", "network"]) +@pytest.mark.parametrize("fallback", [False, True]) +async def test_local_revocation_fallback(monkeypatch, failure, fallback): + oauth = client(revocation_endpoint=None) if failure == "missing" else client() + previous = await saved(oauth) + send = install_transport(monkeypatch, oauth, response({"error": "server_error"}, status=500)) + if failure == "network": + send.side_effect = OSError(SECRET) + if fallback: + await oauth.revoke_token("alice", local_fallback=True) + assert await oauth.store.get_token("alice") is None + else: + with pytest.raises(OAuthClientError): + await oauth.revoke_token("alice") + assert await oauth.store.get_token("alice") == previous + assert send.await_count == (0 if failure == "missing" else 1) + + +@pytest.mark.asyncio +async def test_fallback_does_not_hide_validation_storage_or_cancellation(monkeypatch): + oauth = client(revocation_endpoint=None) + previous = await saved(oauth) + with pytest.raises(OAuthClientError): + await oauth.revoke_token("alice", token_type_hint="bad", local_fallback=True) + assert await oauth.store.get_token("alice") == previous + monkeypatch.setattr(oauth.store, "delete_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError) as exc: + await oauth.revoke_token("alice", local_fallback=True) + assert SECRET not in str(exc.value) + oauth = client() + previous = await saved(oauth) + send = install_transport(monkeypatch, oauth) + send.side_effect = asyncio.CancelledError() + with pytest.raises(asyncio.CancelledError): + await oauth.revoke_token("alice", local_fallback=True) + assert await oauth.store.get_token("alice") == previous + + +@pytest.mark.asyncio +async def test_get_access_token_boundaries_and_explicit_refresh(monkeypatch): + monkeypatch.setattr("nitrostack.auth.token_store.time.time", lambda: 1000) + oauth = client() + send = install_transport(monkeypatch, oauth) + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030001, REFRESH)) + assert await oauth.get_access_token("alice") == ACCESS + send.assert_not_awaited() + await oauth.refresh_token("alice") + assert send.await_count == 1 + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030000, REFRESH)) + await oauth.get_access_token("alice") + assert send.await_count == 2 + with pytest.raises(OAuthClientError): + await oauth.get_access_token("missing") + await saved(oauth, refresh=None) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + + +@pytest.mark.asyncio +async def test_get_access_token_coalesces_cancellation_and_delayed_read(monkeypatch): + oauth = client() + await saved(oauth) + entered, release = asyncio.Event(), asyncio.Event() + calls = [] + async def send(request): + calls.append(request) + entered.set() + await release.wait() + return response({"access_token": "new", "refresh_token": "rotated"}) + monkeypatch.setattr(oauth, "_send", send) + tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(50)] + await asyncio.wait_for(entered.wait(), 2) + tasks[0].cancel() + with pytest.raises(asyncio.CancelledError): + await tasks[0] + release.set() + assert await asyncio.gather(*tasks[1:]) == ["new"] * 49 + assert await oauth.get_access_token("alice") == "new" + assert len(calls) == 1 + assert oauth._refresh_tasks == {} and oauth._force_refresh == {} + assert (await oauth.store.get_token("alice")).refresh_token == "rotated" + + +@pytest.mark.asyncio +async def test_auto_refresh_rechecks_after_lock_and_explicit_call_forces(monkeypatch): + oauth = client() + send = install_transport(monkeypatch, oauth) + await saved(oauth) + lock = oauth._key_lock("alice") + async with lock: + pending = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.sleep(0) + await oauth.store.save_token("alice", StoredToken("fresh", "Bearer", 10**15, REFRESH)) + assert await pending == "fresh" + send.assert_not_awaited() + async with lock: + automatic = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.sleep(0) + explicit = asyncio.create_task(oauth.refresh_token("alice")) + await asyncio.sleep(0) + await asyncio.gather(automatic, explicit) + assert send.await_count == 1 + + +@pytest.mark.asyncio +async def test_get_access_token_failure_retry_and_persistence(monkeypatch): + oauth = client() + previous = await saved(oauth) + install_transport(monkeypatch, oauth, response({"error": "invalid_grant"}, status=400)) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + assert await oauth.store.get_token("alice") == previous + send = install_transport(monkeypatch, oauth) + monkeypatch.setattr(oauth.store, "save_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + assert send.await_count == 1 and await oauth.store.get_token("alice") == previous + +@pytest.mark.asyncio +async def test_explicit_refresh_during_slow_valid_token_read(monkeypatch): + oauth = client() + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 10**15, REFRESH)) + entered, release = asyncio.Event(), asyncio.Event() + original = oauth.store.get_token + async def slow_read(key): + entered.set() + await release.wait() + return await original(key) + monkeypatch.setattr(oauth.store, "get_token", slow_read) + send = install_transport(monkeypatch, oauth) + automatic = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.wait_for(entered.wait(), 2) + explicit = asyncio.create_task(oauth.refresh_token("alice")) + await asyncio.sleep(0) + release.set() + await asyncio.gather(automatic, explicit) + assert send.await_count == 1 + + +@pytest.mark.asyncio +async def test_local_fallback_waits_for_active_refresh(monkeypatch): + oauth = client(revocation_endpoint=None) + await saved(oauth) + entered, release = asyncio.Event(), asyncio.Event() + async def send(request): + entered.set() + await release.wait() + return response({"access_token": "rotated-access", "refresh_token": "rotated-refresh"}) + monkeypatch.setattr(oauth, "_send", send) + pending = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.wait_for(entered.wait(), 2) + revoke = asyncio.create_task(oauth.revoke_token("alice", local_fallback=True)) + await asyncio.sleep(0) + assert not revoke.done() + release.set() + await asyncio.gather(pending, revoke) + assert await oauth.store.get_token("alice") is None + + +@pytest.mark.asyncio +async def test_helper_shared_failures_independent_keys_and_retry(monkeypatch): + oauth = client() + await saved(oauth) + await saved(oauth, "bob") + entered, release = asyncio.Event(), asyncio.Event() + count = 0 + async def send(request): + nonlocal count + count += 1 + if count == 2: + entered.set() + await release.wait() + return response({"error": "invalid_grant", "error_description": SECRET}, status=400) + monkeypatch.setattr(oauth, "_send", send) + tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(20)] + tasks.append(asyncio.create_task(oauth.get_access_token("bob"))) + await asyncio.wait_for(entered.wait(), 2) + release.set() + results = await asyncio.gather(*tasks, return_exceptions=True) + assert count == 2 and all(isinstance(result, OAuthClientError) for result in results) + assert all(result is results[0] for result in results[:20]) + assert SECRET not in repr(results) + assert oauth._refresh_tasks == {} and oauth._force_refresh == {} + install_transport(monkeypatch, oauth) + assert await oauth.get_access_token("alice") == ACCESS + + +@pytest.mark.asyncio +async def test_helper_rejects_immediately_expired_replacement_and_storage_failure(monkeypatch): + oauth = client() + await saved(oauth) + install_transport(monkeypatch, oauth, response({"access_token": ACCESS, "expires_in": "0"})) + with pytest.raises(OAuthClientError, match="expiry"): + await oauth.get_access_token("alice") + monkeypatch.setattr(oauth.store, "get_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError) as exc: + await oauth.get_access_token("alice") + assert SECRET not in str(exc.value) From 79c3fdf8e21a868b057c9f823217b7989f3cd833 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Thu, 5 Nov 2026 20:40:52 +0530 Subject: [PATCH 3/4] docs(auth): document OAuth follow-ups and remove historical test report --- docs/oauth-client-implementation.md | 32 ++++++++--- docs/oauth-client-verification.md | 89 ----------------------------- docs/oauth-client.md | 62 ++++++++++++++------ 3 files changed, 68 insertions(+), 115 deletions(-) delete mode 100644 docs/oauth-client-verification.md diff --git a/docs/oauth-client-implementation.md b/docs/oauth-client-implementation.md index dd12fa7..a3c6ce2 100644 --- a/docs/oauth-client-implementation.md +++ b/docs/oauth-client-implementation.md @@ -20,7 +20,8 @@ seconds to milliseconds, adds the current Unix time, and returns an integer. margin, also converting its seconds to milliseconds. Equality counts as expired. `token_response_to_stored()` is the conversion boundary for provider responses. -Only an absent or null expiry receives the one-hour fallback. Zero remains zero. +Numeric strings are normalized at this boundary before validation. Only an absent +or null expiry receives the one-hour fallback. Zero remains zero. During refresh, a missing refresh token or scope inherits the previous value; the original resource association remains attached to the replacement record. @@ -34,8 +35,9 @@ because doing so would also discard refresh credentials. `FileTokenStore` delegates blocking work to `asyncio.to_thread()`. Its lock is selected by canonical path, so different instances targeting the same file share -the same process-local lock. The complete load/modify/save operation holds that -lock, preventing a stale reader from overwriting another caller's newly saved key. +a lock from a fixed pool of 256 process-local locks. Unrelated paths can share a +lock; memory usage does not grow with paths. The complete load/modify/save +operation holds that lock, preventing a stale reader from overwriting another caller's newly saved key. The read path rejects symlinks, opens without following the final symlink where supported, requires a regular file, and tightens POSIX permissions. Missing files @@ -56,6 +58,8 @@ serialization, synchronization and replacement to verify this property. Encryption is imported only when selected. AES-GCM, key derivation and the hex `iv:tag:ciphertext` representation preserve the older implementation's format. +The static salt is named separately and PBKDF2 runs once during encrypted store +construction. Only derived bytes are retained; IVs remain random per write. `token-encryption` advertises the optional dependency, and the development extra includes it so encryption tests always execute. Neither store coordinates multiple writer processes; Windows directory privacy depends on ACLs. @@ -65,7 +69,9 @@ multiple writer processes; Windows directory privacy depends on ACLs. `auth/client.py` defines typed, redacted config, authorization-flow and registration results. Constructor mappings use the same snake_case names as the config model. The client creates a memory store unless the caller supplies one. -Request timeout and response limits are validated once at construction. +Request timeout and response limits are validated once at construction. Client +secrets can remain wrapped in `SecretValue` until authentication calls +`unwrap_secret()`; empty or unusable wrappers produce sanitized errors. The transport has two layers. `_request()` owns validation, DNS safety, status handling and sanitized errors. `_send()` is an asynchronous seam that delegates @@ -120,6 +126,12 @@ The first caller creates the task; simultaneous callers await that same task. A completion callback removes the task and retrieves its exception, including when all waiters cancelled. A failed operation can therefore be retried later. +`get_access_token()` shares the refresh task machinery and checks expiry under +the per-key lock. It returns a valid credential without HTTP, or refreshes and +persists before returning its access-token string. Explicit refresh requests +promote a pending automatic check to a forced refresh. Completed tasks are not +reused, and both coordination maps are cleared on completion. + Different keys have different locks and refresh tasks. Same-key acquisition, refresh and revocation hold a shared lock within the client instance. In particular, revocation waits for an already-running refresh and then revokes the @@ -134,8 +146,11 @@ save failure, which an application must handle by reauthorization if necessary. Revocation defaults to the stored refresh token when available and otherwise the access token. An explicit hint selects either credential. It removes the local -record only after upstream success. Client registration sends JSON and returns a -redacted typed result; it does not implicitly change client configuration. +record only after upstream success by default. Opt-in `local_fallback=True` also +deletes after missing endpoints or remote failures, without claiming remote +invalidation. Storage errors, invalid hints and cancellation still propagate. +Refresh rotation across independent clients or processes remains unsupported. +Client registration sends JSON and returns a redacted typed result; it does not implicitly change client configuration. ## Error boundaries and verification @@ -148,6 +163,5 @@ tracebacks, logs, stdout and stderr. Explicit serialization remains secret-beari The three new test modules cover storage/expiry, OAuth lifecycle/concurrency, and urllib/pinned transport behavior respectively. They use fake upstream responses and controlled coordination events instead of live providers. Existing OAuth, -PKCE, CIMD, auth and full-suite tests exercise integration with the SDK. See the -separate verification report for exact platform results and the Windows socket -limitation; local Linux runs are not a claim of hosted CI success. +PKCE, CIMD, auth and full-suite tests exercise integration with the SDK. Current +validation results belong in the PR description rather than a permanent report. diff --git a/docs/oauth-client-verification.md b/docs/oauth-client-verification.md deleted file mode 100644 index 8eb90c3..0000000 --- a/docs/oauth-client-verification.md +++ /dev/null @@ -1,89 +0,0 @@ -# Issue #68 verification - -Validation date: 2026-10-28. Feature branch: -`feature/issue-68-oauth-client`, based directly on upstream `develop` at -`b3a2e13ae9f6af024f568b10d7c689927ae8ec2a`. PR #36 and spillover commits are not -part of this branch. - -## Scope and checks - -The three new test modules add 78 cases to the unchanged 889-test baseline. -Complete collection succeeds with 967 tests. The new feature selection passes -all 78 tests on Windows Python 3.12.14. It covers token records and expiry, -plaintext/encrypted stores, atomic failure recovery, concurrent writers, -authorization and client credentials, refresh rotation and cancellation, -revocation, registration, discovery, bounded urllib responses, DNS pins, TLS -identity, ports, redirect rejection and secret-safe output. - -`git diff --check`, compilation/AST checks, public-export inspection and -`uv lock --check` pass. Runtime requirements are unchanged; cryptography is an -optional encryption extra and is also included in development dependencies. -No HTTP client dependency was added. No test exclusions, skips or xfail markers -were added. - -## Complete suites - -All runs use MCP 2.2.0 and install `.[dev,token-encryption]`. Linux runs use Ubuntu -under WSL. These are local compatibility runs, not hosted GitHub Actions results. - -| Platform | Python | Result | Total coverage | -|---|---|---|---| -| Linux | 3.10.21 | 967 passed | 86.38% | -| Linux | 3.11.16 | 967 passed | 86.36% | -| Linux | 3.12.14 | 967 passed | 86.36% | -| Windows | 3.10.21 | 967 passed | 86.37% | -| Windows | 3.11.16 | 967 passed | 86.35% | -| Windows | 3.12.14 | 966 passed, 1 failed | 86.35% | - -Commands, with an environment-specific interpreter and temporary directory: - -```shell -python -m pytest tests --collect-only -o addopts= -q -python -m pytest tests/test_token_store.py tests/test_oauth_client.py tests/test_oauth_client_transport.py -o addopts= -q --timeout=60 -python -m pytest tests --cov=nitrostack --cov-report=term-missing --timeout=60 --tb=short -git diff --check -uv lock --check -``` - -Full-run logs are retained locally in `.pytest_cache/linux-3.10.log`, -`linux-3.11.log`, `linux-3.12.log`, `windows-3.10.log`, `windows-3.11.log`, and -`windows-final.log`. The cache is ignored by Git. - -## Windows socket limitation - -The final Windows 3.12 run fails the unchanged -`tests/test_oauth.py::test_discovery_server_idempotent_start_and_dcr_http` while -reading a live localhost POST response: `ConnectionAbortedError: [WinError 10053]`. -Two earlier complete feature runs each reported 964 passed and one failure in -`test_discovery_dcr_404_when_disabled_or_missing_client_id` with the same socket -error. Those runs preceded the final two additional storage cases. - -Comparison used an isolated archive of the exact unchanged develop commit and -the same Windows 3.12.14 environment/MCP version: - -- Two complete baseline runs passed all 889 tests (85.78% coverage). -- The failing disabled-registration test passed five isolated repetitions on - both baseline and feature code. -- Running the existing OAuth/PKCE/CIMD/authentication modules together on the - unchanged baseline reproduced WinError 10053 in the disabled-registration test: - 83 passed, 1 failed. -- The corresponding feature selection reported 160 passed, 2 failed, both live - discovery socket tests. The new feature-only selection remained 78 passed. - -Baseline reproduction command: - -```shell -python -m pytest tests/test_oauth.py tests/test_pkce.py tests/test_mcp20_oauth_cimd.py tests/test_auth_modules.py tests/test_oauth_discovery.py -o addopts= -q --timeout=60 --tb=short -``` - -This demonstrates an intermittent baseline socket failure under matching local -conditions. It does not establish the operating-system-level cause, and the -complete Windows 3.12 suite is not reported as passing. No socket test, server -implementation or assertion was changed to conceal the result. - -## Publication status - -Implementation and local validation were completed before publication. Branch -publication was subsequently authorized by the user. No PR creation or reviewer -message is included. Hosted CI remains to be verified on the published head; -the local matrix results above are not hosted CI results. diff --git a/docs/oauth-client.md b/docs/oauth-client.md index 6416c41..f2f5754 100644 --- a/docs/oauth-client.md +++ b/docs/oauth-client.md @@ -11,7 +11,7 @@ it. It does not launch browsers, receive callbacks, or intercept API requests. import asyncio import os -from nitrostack import OAuth2Client, OAuth2ClientConfig, is_token_expired +from nitrostack import OAuth2Client, OAuth2ClientConfig async def main(): @@ -21,13 +21,10 @@ async def main(): token_endpoint="https://identity.example.com/oauth/token", )) key = "identity.example.com|reporting-app|service-account|reports-api" - token = await client.store.get_token(key) - if token is None or is_token_expired(token): - token = await client.get_client_credentials_token( - key, scope="reports:read", resource="https://reports.example.com", - ) - # Pass token.access_token to your HTTP client's Authorization header. - # Do not print the credential or its serialized representation. + token = await client.get_client_credentials_token( + key, scope="reports:read", resource="https://reports.example.com", + ) + # Use token.access_token without logging it. asyncio.run(main()) @@ -148,7 +145,9 @@ store = FileTokenStore( Encryption imports are lazy. The file format remains compatible with PR #4: AES-GCM with a random 16-byte IV, hex `iv:tag:ciphertext`, and its existing PBKDF2-HMAC-SHA256 key derivation (100,000 iterations, `nitrostack-token-store` -salt). Keep the encryption key separate from the file. Switching an existing +salt). Derivation runs once per encrypted store instance at construction; only +the derived key is retained. Every write uses a fresh IV. Keep the encryption key +separate from the file. Switching an existing plaintext file to encryption requires an explicit migration by the application. ## Expiry and refresh @@ -156,18 +155,19 @@ plaintext file to encryption requires an explicit migration by the application. `StoredToken` is immutable. `expires_at` is an integer Unix timestamp in **milliseconds**. `calculate_expiration(expires_in)` and `is_token_expired(token, skew=30)` take **seconds**. Missing/`None` `expires_in` uses one hour; zero expires -immediately. Negative, nonnumeric, boolean and nonfinite durations are rejected. +immediately. Provider responses also accept numeric strings such as `"3600"` +and `"1.5"`. Negative, nonnumeric, boolean and nonfinite durations are rejected. +The numeric-only expiry helpers do not accept strings. + +`get_access_token(key)` returns a string, reuses valid stored tokens, and refreshes +within the 30-second safety margin. Missing records or refresh credentials raise +`OAuthClientError`; the application must then acquire a token explicitly. Stores retain expired records so the refresh credential remains available: ```python -token = await client.store.get_token(key) -if token is not None and is_token_expired(token): - if token.refresh_token: - token = await client.refresh_token(key) - else: - # Start the appropriate acquisition flow again. - ... +access_token = await client.get_access_token(key) +# Use the returned credential without logging it. ``` Share one client instance to coalesce concurrent refresh calls for a key into one @@ -185,6 +185,18 @@ and deletes the local record only after upstream success. Pass successful revocation response is accepted. Coordination is not shared between client instances or processes. +Pass `local_fallback=True` to delete the local record when the revocation endpoint +is missing or the remote attempt fails. This does **not** guarantee the provider +invalidated the credential. Invalid hints and storage errors still raise, and +cancellation propagates. For unconditional local deletion without a request, use +`await client.store.delete_token(key)`. + +File operations share a fixed pool of 256 process-local locks selected by canonical +path. Unrelated paths may serialize when they select the same lock. This bounds +lock memory but does not coordinate refresh rotation across client instances or +processes. Use one shared client per credential owner; do not run concurrent +refresh-token rotation against the same file from separate processes. + ## Errors and secret handling Catch `OAuthClientError` and `TokenStoreError` for sanitized failures. OAuth errors @@ -196,3 +208,19 @@ Config, token, pending-flow and registration representations hide credentials. Explicit `token.to_dict()` and `registration.to_dict()` contain credentials for interoperability/persistence: **never log these mappings**. Redacted repr does not make arbitrary attribute access, application logs or memory dumps safe. + +### Wrapped client secrets + +```python +from nitrostack import OAuth2Client, OAuth2ClientConfig, SecretValue + +client = OAuth2Client(OAuth2ClientConfig( + client_id="my-client", + client_secret=SecretValue.from_env("OAUTH_CLIENT_SECRET"), + token_endpoint="https://issuer.example/token", +)) +``` + +Mappings also accept `SecretValue`. The wrapper stays in configuration and is +unwrapped only while constructing authentication. Empty or serialized/redacted +wrappers fail safely before a request. Plain-string secrets remain supported. From b1f4dace8bfd2fdb98b61c805ab4f7bbe32988ab Mon Sep 17 00:00:00 2001 From: Siddharth Date: Sat, 7 Nov 2026 10:15:26 +0530 Subject: [PATCH 4/4] refactor(auth): split OAuth client types and follow-up tests --- docs/oauth-client-implementation.md | 7 +- nitrostack/auth/_client_types.py | 98 ++++++++++ nitrostack/auth/client.py | 99 +--------- tests/support/oauth_client.py | 39 ++++ tests/test_oauth_client.py | 265 +-------------------------- tests/test_oauth_client_followups.py | 242 ++++++++++++++++++++++++ 6 files changed, 393 insertions(+), 357 deletions(-) create mode 100644 nitrostack/auth/_client_types.py create mode 100644 tests/support/oauth_client.py create mode 100644 tests/test_oauth_client_followups.py diff --git a/docs/oauth-client-implementation.md b/docs/oauth-client-implementation.md index a3c6ce2..cab0cb4 100644 --- a/docs/oauth-client-implementation.md +++ b/docs/oauth-client-implementation.md @@ -66,9 +66,10 @@ multiple writer processes; Windows directory privacy depends on ACLs. ## Client configuration and transport -`auth/client.py` defines typed, redacted config, authorization-flow and -registration results. Constructor mappings use the same snake_case names as the -config model. The client creates a memory store unless the caller supplies one. +`auth/_client_types.py` defines typed, redacted config, authorization-flow and +registration results. `auth/client.py` owns the client lifecycle and transport, +and re-exports these types to preserve existing imports. Constructor mappings +use the same snake_case names as the config model. The client creates a memory store unless the caller supplies one. Request timeout and response limits are validated once at construction. Client secrets can remain wrapped in `SecretValue` until authentication calls `unwrap_secret()`; empty or unusable wrappers produce sanitized errors. diff --git a/nitrostack/auth/_client_types.py b/nitrostack/auth/_client_types.py new file mode 100644 index 0000000..19daa5e --- /dev/null +++ b/nitrostack/auth/_client_types.py @@ -0,0 +1,98 @@ +"""Secret-safe OAuth configuration, results, errors, and transport records.""" + +from __future__ import annotations + +from dataclasses import dataclass, field, fields +from types import MappingProxyType +from typing import Any, Mapping + +from nitrostack.auth.secret import SecretValue + + +_OAUTH_ERRORS = frozenset({ + "invalid_request", "invalid_client", "invalid_grant", "unauthorized_client", + "unsupported_grant_type", "invalid_scope", "access_denied", "server_error", + "temporarily_unavailable", "invalid_token", "unsupported_token_type", + "invalid_redirect_uri", "invalid_client_metadata", +}) + + +class OAuthClientError(ValueError): + """Only operation, status, and recognized OAuth error codes are exposed.""" + + def __init__(self, operation: str, *, status: int | None = None, error_code=None): + self.operation = operation + self.status = status + self.error_code = error_code if isinstance(error_code, str) and error_code in _OAUTH_ERRORS else None + suffix = f" (HTTP {status})" if status is not None else "" + if self.error_code: + suffix += f": {self.error_code}" + super().__init__(f"OAuth {operation} failed{suffix}") + + +@dataclass(frozen=True) +class OAuth2ClientConfig: + client_id: str | None = field(default=None, repr=False) + client_secret: str | SecretValue | None = field(default=None, repr=False) + issuer: str | None = field(default=None, repr=False) + redirect_uri: str | None = field(default=None, repr=False) + authorization_endpoint: str | None = field(default=None, repr=False) + token_endpoint: str | None = field(default=None, repr=False) + revocation_endpoint: str | None = field(default=None, repr=False) + registration_endpoint: str | None = field(default=None, repr=False) + protected_resource_metadata_url: str | None = field(default=None, repr=False) + authorization_server_metadata_url: str | None = field(default=None, repr=False) + + def __post_init__(self): + if any(value is not None and (not isinstance(value, str) or not value) + for value in (getattr(self, item.name) for item in fields(self) if item.name != "client_secret")): + raise OAuthClientError("configuration") + if self.client_secret is not None and not isinstance(self.client_secret, SecretValue): + if not isinstance(self.client_secret, str) or not self.client_secret: + raise OAuthClientError("configuration") + + +@dataclass(frozen=True) +class AuthorizationFlow: + authorization_url: str = field(repr=False) + state: str = field(repr=False) + code_verifier: str = field(repr=False) + code_challenge: str = field(repr=False) + code_challenge_method: str = "S256" + + +@dataclass(frozen=True) +class ClientRegistration: + client_id: str = field(repr=False) + client_secret: str | None = field(default=None, repr=False) + metadata: Mapping[str, Any] = field(default_factory=dict, repr=False) + + def __post_init__(self): + if (not isinstance(self.client_id, str) or not self.client_id + or (self.client_secret is not None and not isinstance(self.client_secret, str))): + raise OAuthClientError("registration response") + object.__setattr__(self, "metadata", MappingProxyType(dict(self.metadata))) + + def to_dict(self) -> dict[str, Any]: + """Explicit credential-bearing output; never log this mapping.""" + result = {**self.metadata, "client_id": self.client_id} + if self.client_secret is not None: + result["client_secret"] = self.client_secret + return result + + +@dataclass(frozen=True) +class _Request: + method: str + url: str = field(repr=False) + headers: Mapping[str, str] = field(repr=False) + body: bytes | None = field(repr=False) + timeout: float + pinned_ip: str | None = field(default=None, repr=False) + + +@dataclass(frozen=True) +class _Response: + status: int + headers: Mapping[str, str] = field(repr=False) + body: bytes = field(repr=False) diff --git a/nitrostack/auth/client.py b/nitrostack/auth/client.py index 8260d37..d458f55 100644 --- a/nitrostack/auth/client.py +++ b/nitrostack/auth/client.py @@ -12,107 +12,22 @@ import urllib.parse import urllib.request import weakref -from dataclasses import dataclass, field, fields -from types import MappingProxyType +from dataclasses import fields from typing import Any, Mapping from nitrostack.auth import cimd -from nitrostack.auth.secret import SecretValue, unwrap_secret +# Re-export the original import locations for callers and transport test seams. +from nitrostack.auth._client_types import ( + AuthorizationFlow, ClientRegistration, OAuth2ClientConfig, OAuthClientError, + _Request, _Response, +) +from nitrostack.auth.secret import unwrap_secret from nitrostack.auth.pkce import generate_pkce_params, is_valid_code_verifier, validate_pkce_support from nitrostack.auth.token_store import ( MemoryTokenStore, StoredToken, TokenStore, is_token_expired, token_response_to_stored, ) -_OAUTH_ERRORS = frozenset({ - "invalid_request", "invalid_client", "invalid_grant", "unauthorized_client", - "unsupported_grant_type", "invalid_scope", "access_denied", "server_error", - "temporarily_unavailable", "invalid_token", "unsupported_token_type", - "invalid_redirect_uri", "invalid_client_metadata", -}) - - -class OAuthClientError(ValueError): - """Only operation, status, and recognized OAuth error codes are exposed.""" - - def __init__(self, operation: str, *, status: int | None = None, error_code=None): - self.operation = operation - self.status = status - self.error_code = error_code if isinstance(error_code, str) and error_code in _OAUTH_ERRORS else None - suffix = f" (HTTP {status})" if status is not None else "" - if self.error_code: - suffix += f": {self.error_code}" - super().__init__(f"OAuth {operation} failed{suffix}") - - -@dataclass(frozen=True) -class OAuth2ClientConfig: - client_id: str | None = field(default=None, repr=False) - client_secret: str | SecretValue | None = field(default=None, repr=False) - issuer: str | None = field(default=None, repr=False) - redirect_uri: str | None = field(default=None, repr=False) - authorization_endpoint: str | None = field(default=None, repr=False) - token_endpoint: str | None = field(default=None, repr=False) - revocation_endpoint: str | None = field(default=None, repr=False) - registration_endpoint: str | None = field(default=None, repr=False) - protected_resource_metadata_url: str | None = field(default=None, repr=False) - authorization_server_metadata_url: str | None = field(default=None, repr=False) - - def __post_init__(self): - if any(value is not None and (not isinstance(value, str) or not value) - for value in (getattr(self, item.name) for item in fields(self) if item.name != "client_secret")): - raise OAuthClientError("configuration") - if self.client_secret is not None and not isinstance(self.client_secret, SecretValue): - if not isinstance(self.client_secret, str) or not self.client_secret: - raise OAuthClientError("configuration") - - -@dataclass(frozen=True) -class AuthorizationFlow: - authorization_url: str = field(repr=False) - state: str = field(repr=False) - code_verifier: str = field(repr=False) - code_challenge: str = field(repr=False) - code_challenge_method: str = "S256" - - -@dataclass(frozen=True) -class ClientRegistration: - client_id: str = field(repr=False) - client_secret: str | None = field(default=None, repr=False) - metadata: Mapping[str, Any] = field(default_factory=dict, repr=False) - - def __post_init__(self): - if (not isinstance(self.client_id, str) or not self.client_id - or (self.client_secret is not None and not isinstance(self.client_secret, str))): - raise OAuthClientError("registration response") - object.__setattr__(self, "metadata", MappingProxyType(dict(self.metadata))) - - def to_dict(self) -> dict[str, Any]: - """Explicit credential-bearing output; never log this mapping.""" - result = {**self.metadata, "client_id": self.client_id} - if self.client_secret is not None: - result["client_secret"] = self.client_secret - return result - - -@dataclass(frozen=True) -class _Request: - method: str - url: str = field(repr=False) - headers: Mapping[str, str] = field(repr=False) - body: bytes | None = field(repr=False) - timeout: float - pinned_ip: str | None = field(default=None, repr=False) - - -@dataclass(frozen=True) -class _Response: - status: int - headers: Mapping[str, str] = field(repr=False) - body: bytes = field(repr=False) - - def get_well_known_metadata_uris(resource_url: str) -> list[str]: parsed = urllib.parse.urlsplit(resource_url) origin = f"{parsed.scheme}://{parsed.netloc}" diff --git a/tests/support/oauth_client.py b/tests/support/oauth_client.py new file mode 100644 index 0000000..3cd949a --- /dev/null +++ b/tests/support/oauth_client.py @@ -0,0 +1,39 @@ +"""Shared OAuth client fixtures without network access.""" + +import json +from unittest.mock import AsyncMock + +from nitrostack import OAuth2Client, StoredToken +from nitrostack.auth.client import _Response + +ACCESS = "ACCESS_SENTINEL_NEVER_LOG" +REFRESH = "REFRESH_SENTINEL_NEVER_LOG" +SECRET = "CLIENT_SENTINEL_NEVER_LOG: with spaces" +ENDPOINT = "https://issuer.example/token" + + +def response(value=None, *, status=200, headers=None, raw=None): + body = raw if raw is not None else json.dumps(value if value is not None else {"access_token": ACCESS}).encode() + return _Response(status, headers or {}, body) + + +def client(**overrides): + return OAuth2Client({"client_id": "client:id", "client_secret": SECRET, + "issuer": "https://issuer.example", "redirect_uri": "http://127.0.0.1/callback", + "authorization_endpoint": "https://issuer.example/authorize?audience=api&state=old&scope=old", + "token_endpoint": ENDPOINT, "revocation_endpoint": "https://issuer.example/revoke", + "registration_endpoint": "https://issuer.example/register", **overrides}) + + +def install_transport(monkeypatch, oauth, *responses): + send = AsyncMock(side_effect=list(responses) if responses else None) + if not responses: + send.return_value = response() + monkeypatch.setattr(oauth, "_send", send) + return send + + +async def saved(oauth, key="alice", refresh=REFRESH): + token = StoredToken(ACCESS, "Bearer", 1, refresh, "read", "https://resource.example") + await oauth.store.save_token(key, token) + return token diff --git a/tests/test_oauth_client.py b/tests/test_oauth_client.py index 61d2bf1..0cd6344 100644 --- a/tests/test_oauth_client.py +++ b/tests/test_oauth_client.py @@ -18,40 +18,11 @@ parse_www_authenticate_header, ) from nitrostack.auth import cimd -from nitrostack.auth.client import _Response from nitrostack.auth.pkce import generate_code_challenge, is_valid_code_verifier -ACCESS = "ACCESS_SENTINEL_NEVER_LOG" -REFRESH = "REFRESH_SENTINEL_NEVER_LOG" -SECRET = "CLIENT_SENTINEL_NEVER_LOG: with spaces" -ENDPOINT = "https://issuer.example/token" - - -def response(value=None, *, status=200, headers=None, raw=None): - body = raw if raw is not None else json.dumps(value if value is not None else {"access_token": ACCESS}).encode() - return _Response(status, headers or {}, body) - - -def client(**overrides): - return OAuth2Client({"client_id": "client:id", "client_secret": SECRET, - "issuer": "https://issuer.example", "redirect_uri": "http://127.0.0.1/callback", - "authorization_endpoint": "https://issuer.example/authorize?audience=api&state=old&scope=old", - "token_endpoint": ENDPOINT, "revocation_endpoint": "https://issuer.example/revoke", - "registration_endpoint": "https://issuer.example/register", **overrides}) - - -def install_transport(monkeypatch, oauth, *responses): - send = AsyncMock(side_effect=list(responses) if responses else None) - if not responses: - send.return_value = response() - monkeypatch.setattr(oauth, "_send", send) - return send - - -async def saved(oauth, key="alice", refresh=REFRESH): - token = StoredToken(ACCESS, "Bearer", 1, refresh, "read", "https://resource.example") - await oauth.store.save_token(key, token) - return token +from tests.support.oauth_client import ( + ACCESS, REFRESH, SECRET, ENDPOINT, client, install_transport, response, saved, +) @pytest.mark.asyncio @@ -445,233 +416,3 @@ def test_invalid_client_limits(options): def test_invalid_operator_url_is_rejected(url): with pytest.raises(OAuthClientError): client(token_endpoint=url) - -@pytest.mark.asyncio -async def test_secret_wrapper_deferred_unwrap(monkeypatch, caplog, capsys): - import pickle - from nitrostack import SecretValue - secret = SecretValue(SECRET) - original = SecretValue.unwrap - calls = [] - def unwrap(self): - calls.append(1) - return original(self) - monkeypatch.setattr(SecretValue, "unwrap", unwrap) - oauth = client(client_secret=secret) - assert oauth.config.client_secret is secret and calls == [] - send = install_transport(monkeypatch, oauth) - await oauth.get_client_credentials_token("alice") - assert len(calls) == 1 - plain = client() - plain_send = install_transport(monkeypatch, plain) - await plain.get_client_credentials_token("alice") - assert send.call_args.args[0].headers == plain_send.call_args.args[0].headers - for unusable in (pickle.loads(pickle.dumps(secret)),): - broken = client(client_secret=unusable) - transport = install_transport(monkeypatch, broken) - with pytest.raises(OAuthClientError) as exc: - await broken.get_client_credentials_token("alice") - transport.assert_not_awaited() - assert SECRET not in str(exc.value) + repr(exc.value) - assert SECRET not in repr(oauth.config) + caplog.text + str(capsys.readouterr()) - - -@pytest.mark.asyncio -@pytest.mark.parametrize("failure", ["missing", "http", "network"]) -@pytest.mark.parametrize("fallback", [False, True]) -async def test_local_revocation_fallback(monkeypatch, failure, fallback): - oauth = client(revocation_endpoint=None) if failure == "missing" else client() - previous = await saved(oauth) - send = install_transport(monkeypatch, oauth, response({"error": "server_error"}, status=500)) - if failure == "network": - send.side_effect = OSError(SECRET) - if fallback: - await oauth.revoke_token("alice", local_fallback=True) - assert await oauth.store.get_token("alice") is None - else: - with pytest.raises(OAuthClientError): - await oauth.revoke_token("alice") - assert await oauth.store.get_token("alice") == previous - assert send.await_count == (0 if failure == "missing" else 1) - - -@pytest.mark.asyncio -async def test_fallback_does_not_hide_validation_storage_or_cancellation(monkeypatch): - oauth = client(revocation_endpoint=None) - previous = await saved(oauth) - with pytest.raises(OAuthClientError): - await oauth.revoke_token("alice", token_type_hint="bad", local_fallback=True) - assert await oauth.store.get_token("alice") == previous - monkeypatch.setattr(oauth.store, "delete_token", AsyncMock(side_effect=OSError(SECRET))) - with pytest.raises(OAuthClientError) as exc: - await oauth.revoke_token("alice", local_fallback=True) - assert SECRET not in str(exc.value) - oauth = client() - previous = await saved(oauth) - send = install_transport(monkeypatch, oauth) - send.side_effect = asyncio.CancelledError() - with pytest.raises(asyncio.CancelledError): - await oauth.revoke_token("alice", local_fallback=True) - assert await oauth.store.get_token("alice") == previous - - -@pytest.mark.asyncio -async def test_get_access_token_boundaries_and_explicit_refresh(monkeypatch): - monkeypatch.setattr("nitrostack.auth.token_store.time.time", lambda: 1000) - oauth = client() - send = install_transport(monkeypatch, oauth) - await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030001, REFRESH)) - assert await oauth.get_access_token("alice") == ACCESS - send.assert_not_awaited() - await oauth.refresh_token("alice") - assert send.await_count == 1 - await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030000, REFRESH)) - await oauth.get_access_token("alice") - assert send.await_count == 2 - with pytest.raises(OAuthClientError): - await oauth.get_access_token("missing") - await saved(oauth, refresh=None) - with pytest.raises(OAuthClientError): - await oauth.get_access_token("alice") - - -@pytest.mark.asyncio -async def test_get_access_token_coalesces_cancellation_and_delayed_read(monkeypatch): - oauth = client() - await saved(oauth) - entered, release = asyncio.Event(), asyncio.Event() - calls = [] - async def send(request): - calls.append(request) - entered.set() - await release.wait() - return response({"access_token": "new", "refresh_token": "rotated"}) - monkeypatch.setattr(oauth, "_send", send) - tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(50)] - await asyncio.wait_for(entered.wait(), 2) - tasks[0].cancel() - with pytest.raises(asyncio.CancelledError): - await tasks[0] - release.set() - assert await asyncio.gather(*tasks[1:]) == ["new"] * 49 - assert await oauth.get_access_token("alice") == "new" - assert len(calls) == 1 - assert oauth._refresh_tasks == {} and oauth._force_refresh == {} - assert (await oauth.store.get_token("alice")).refresh_token == "rotated" - - -@pytest.mark.asyncio -async def test_auto_refresh_rechecks_after_lock_and_explicit_call_forces(monkeypatch): - oauth = client() - send = install_transport(monkeypatch, oauth) - await saved(oauth) - lock = oauth._key_lock("alice") - async with lock: - pending = asyncio.create_task(oauth.get_access_token("alice")) - await asyncio.sleep(0) - await oauth.store.save_token("alice", StoredToken("fresh", "Bearer", 10**15, REFRESH)) - assert await pending == "fresh" - send.assert_not_awaited() - async with lock: - automatic = asyncio.create_task(oauth.get_access_token("alice")) - await asyncio.sleep(0) - explicit = asyncio.create_task(oauth.refresh_token("alice")) - await asyncio.sleep(0) - await asyncio.gather(automatic, explicit) - assert send.await_count == 1 - - -@pytest.mark.asyncio -async def test_get_access_token_failure_retry_and_persistence(monkeypatch): - oauth = client() - previous = await saved(oauth) - install_transport(monkeypatch, oauth, response({"error": "invalid_grant"}, status=400)) - with pytest.raises(OAuthClientError): - await oauth.get_access_token("alice") - assert await oauth.store.get_token("alice") == previous - send = install_transport(monkeypatch, oauth) - monkeypatch.setattr(oauth.store, "save_token", AsyncMock(side_effect=OSError(SECRET))) - with pytest.raises(OAuthClientError): - await oauth.get_access_token("alice") - assert send.await_count == 1 and await oauth.store.get_token("alice") == previous - -@pytest.mark.asyncio -async def test_explicit_refresh_during_slow_valid_token_read(monkeypatch): - oauth = client() - await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 10**15, REFRESH)) - entered, release = asyncio.Event(), asyncio.Event() - original = oauth.store.get_token - async def slow_read(key): - entered.set() - await release.wait() - return await original(key) - monkeypatch.setattr(oauth.store, "get_token", slow_read) - send = install_transport(monkeypatch, oauth) - automatic = asyncio.create_task(oauth.get_access_token("alice")) - await asyncio.wait_for(entered.wait(), 2) - explicit = asyncio.create_task(oauth.refresh_token("alice")) - await asyncio.sleep(0) - release.set() - await asyncio.gather(automatic, explicit) - assert send.await_count == 1 - - -@pytest.mark.asyncio -async def test_local_fallback_waits_for_active_refresh(monkeypatch): - oauth = client(revocation_endpoint=None) - await saved(oauth) - entered, release = asyncio.Event(), asyncio.Event() - async def send(request): - entered.set() - await release.wait() - return response({"access_token": "rotated-access", "refresh_token": "rotated-refresh"}) - monkeypatch.setattr(oauth, "_send", send) - pending = asyncio.create_task(oauth.get_access_token("alice")) - await asyncio.wait_for(entered.wait(), 2) - revoke = asyncio.create_task(oauth.revoke_token("alice", local_fallback=True)) - await asyncio.sleep(0) - assert not revoke.done() - release.set() - await asyncio.gather(pending, revoke) - assert await oauth.store.get_token("alice") is None - - -@pytest.mark.asyncio -async def test_helper_shared_failures_independent_keys_and_retry(monkeypatch): - oauth = client() - await saved(oauth) - await saved(oauth, "bob") - entered, release = asyncio.Event(), asyncio.Event() - count = 0 - async def send(request): - nonlocal count - count += 1 - if count == 2: - entered.set() - await release.wait() - return response({"error": "invalid_grant", "error_description": SECRET}, status=400) - monkeypatch.setattr(oauth, "_send", send) - tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(20)] - tasks.append(asyncio.create_task(oauth.get_access_token("bob"))) - await asyncio.wait_for(entered.wait(), 2) - release.set() - results = await asyncio.gather(*tasks, return_exceptions=True) - assert count == 2 and all(isinstance(result, OAuthClientError) for result in results) - assert all(result is results[0] for result in results[:20]) - assert SECRET not in repr(results) - assert oauth._refresh_tasks == {} and oauth._force_refresh == {} - install_transport(monkeypatch, oauth) - assert await oauth.get_access_token("alice") == ACCESS - - -@pytest.mark.asyncio -async def test_helper_rejects_immediately_expired_replacement_and_storage_failure(monkeypatch): - oauth = client() - await saved(oauth) - install_transport(monkeypatch, oauth, response({"access_token": ACCESS, "expires_in": "0"})) - with pytest.raises(OAuthClientError, match="expiry"): - await oauth.get_access_token("alice") - monkeypatch.setattr(oauth.store, "get_token", AsyncMock(side_effect=OSError(SECRET))) - with pytest.raises(OAuthClientError) as exc: - await oauth.get_access_token("alice") - assert SECRET not in str(exc.value) diff --git a/tests/test_oauth_client_followups.py b/tests/test_oauth_client_followups.py new file mode 100644 index 0000000..187b7f4 --- /dev/null +++ b/tests/test_oauth_client_followups.py @@ -0,0 +1,242 @@ +"""OAuth usability, revocation fallback, and automatic refresh regressions.""" + +import asyncio +from unittest.mock import AsyncMock + +import pytest + +from nitrostack import OAuthClientError, StoredToken +from tests.support.oauth_client import ( + ACCESS, REFRESH, SECRET, client, install_transport, response, saved, +) + + +@pytest.mark.asyncio +async def test_secret_wrapper_deferred_unwrap(monkeypatch, caplog, capsys): + import pickle + from nitrostack import SecretValue + secret = SecretValue(SECRET) + original = SecretValue.unwrap + calls = [] + def unwrap(self): + calls.append(1) + return original(self) + monkeypatch.setattr(SecretValue, "unwrap", unwrap) + oauth = client(client_secret=secret) + assert oauth.config.client_secret is secret and calls == [] + send = install_transport(monkeypatch, oauth) + await oauth.get_client_credentials_token("alice") + assert len(calls) == 1 + plain = client() + plain_send = install_transport(monkeypatch, plain) + await plain.get_client_credentials_token("alice") + assert send.call_args.args[0].headers == plain_send.call_args.args[0].headers + for unusable in (pickle.loads(pickle.dumps(secret)),): + broken = client(client_secret=unusable) + transport = install_transport(monkeypatch, broken) + with pytest.raises(OAuthClientError) as exc: + await broken.get_client_credentials_token("alice") + transport.assert_not_awaited() + assert SECRET not in str(exc.value) + repr(exc.value) + assert SECRET not in repr(oauth.config) + caplog.text + str(capsys.readouterr()) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("failure", ["missing", "http", "network"]) +@pytest.mark.parametrize("fallback", [False, True]) +async def test_local_revocation_fallback(monkeypatch, failure, fallback): + oauth = client(revocation_endpoint=None) if failure == "missing" else client() + previous = await saved(oauth) + send = install_transport(monkeypatch, oauth, response({"error": "server_error"}, status=500)) + if failure == "network": + send.side_effect = OSError(SECRET) + if fallback: + await oauth.revoke_token("alice", local_fallback=True) + assert await oauth.store.get_token("alice") is None + else: + with pytest.raises(OAuthClientError): + await oauth.revoke_token("alice") + assert await oauth.store.get_token("alice") == previous + assert send.await_count == (0 if failure == "missing" else 1) + + +@pytest.mark.asyncio +async def test_fallback_does_not_hide_validation_storage_or_cancellation(monkeypatch): + oauth = client(revocation_endpoint=None) + previous = await saved(oauth) + with pytest.raises(OAuthClientError): + await oauth.revoke_token("alice", token_type_hint="bad", local_fallback=True) + assert await oauth.store.get_token("alice") == previous + monkeypatch.setattr(oauth.store, "delete_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError) as exc: + await oauth.revoke_token("alice", local_fallback=True) + assert SECRET not in str(exc.value) + oauth = client() + previous = await saved(oauth) + send = install_transport(monkeypatch, oauth) + send.side_effect = asyncio.CancelledError() + with pytest.raises(asyncio.CancelledError): + await oauth.revoke_token("alice", local_fallback=True) + assert await oauth.store.get_token("alice") == previous + + +@pytest.mark.asyncio +async def test_get_access_token_boundaries_and_explicit_refresh(monkeypatch): + monkeypatch.setattr("nitrostack.auth.token_store.time.time", lambda: 1000) + oauth = client() + send = install_transport(monkeypatch, oauth) + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030001, REFRESH)) + assert await oauth.get_access_token("alice") == ACCESS + send.assert_not_awaited() + await oauth.refresh_token("alice") + assert send.await_count == 1 + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 1030000, REFRESH)) + await oauth.get_access_token("alice") + assert send.await_count == 2 + with pytest.raises(OAuthClientError): + await oauth.get_access_token("missing") + await saved(oauth, refresh=None) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + + +@pytest.mark.asyncio +async def test_get_access_token_coalesces_cancellation_and_delayed_read(monkeypatch): + oauth = client() + await saved(oauth) + entered, release = asyncio.Event(), asyncio.Event() + calls = [] + async def send(request): + calls.append(request) + entered.set() + await release.wait() + return response({"access_token": "new", "refresh_token": "rotated"}) + monkeypatch.setattr(oauth, "_send", send) + tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(50)] + await asyncio.wait_for(entered.wait(), 2) + tasks[0].cancel() + with pytest.raises(asyncio.CancelledError): + await tasks[0] + release.set() + assert await asyncio.gather(*tasks[1:]) == ["new"] * 49 + assert await oauth.get_access_token("alice") == "new" + assert len(calls) == 1 + assert oauth._refresh_tasks == {} and oauth._force_refresh == {} + assert (await oauth.store.get_token("alice")).refresh_token == "rotated" + + +@pytest.mark.asyncio +async def test_auto_refresh_rechecks_after_lock_and_explicit_call_forces(monkeypatch): + oauth = client() + send = install_transport(monkeypatch, oauth) + await saved(oauth) + lock = oauth._key_lock("alice") + async with lock: + pending = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.sleep(0) + await oauth.store.save_token("alice", StoredToken("fresh", "Bearer", 10**15, REFRESH)) + assert await pending == "fresh" + send.assert_not_awaited() + async with lock: + automatic = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.sleep(0) + explicit = asyncio.create_task(oauth.refresh_token("alice")) + await asyncio.sleep(0) + await asyncio.gather(automatic, explicit) + assert send.await_count == 1 + + +@pytest.mark.asyncio +async def test_get_access_token_failure_retry_and_persistence(monkeypatch): + oauth = client() + previous = await saved(oauth) + install_transport(monkeypatch, oauth, response({"error": "invalid_grant"}, status=400)) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + assert await oauth.store.get_token("alice") == previous + send = install_transport(monkeypatch, oauth) + monkeypatch.setattr(oauth.store, "save_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError): + await oauth.get_access_token("alice") + assert send.await_count == 1 and await oauth.store.get_token("alice") == previous + +@pytest.mark.asyncio +async def test_explicit_refresh_during_slow_valid_token_read(monkeypatch): + oauth = client() + await oauth.store.save_token("alice", StoredToken(ACCESS, "Bearer", 10**15, REFRESH)) + entered, release = asyncio.Event(), asyncio.Event() + original = oauth.store.get_token + async def slow_read(key): + entered.set() + await release.wait() + return await original(key) + monkeypatch.setattr(oauth.store, "get_token", slow_read) + send = install_transport(monkeypatch, oauth) + automatic = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.wait_for(entered.wait(), 2) + explicit = asyncio.create_task(oauth.refresh_token("alice")) + await asyncio.sleep(0) + release.set() + await asyncio.gather(automatic, explicit) + assert send.await_count == 1 + + +@pytest.mark.asyncio +async def test_local_fallback_waits_for_active_refresh(monkeypatch): + oauth = client(revocation_endpoint=None) + await saved(oauth) + entered, release = asyncio.Event(), asyncio.Event() + async def send(request): + entered.set() + await release.wait() + return response({"access_token": "rotated-access", "refresh_token": "rotated-refresh"}) + monkeypatch.setattr(oauth, "_send", send) + pending = asyncio.create_task(oauth.get_access_token("alice")) + await asyncio.wait_for(entered.wait(), 2) + revoke = asyncio.create_task(oauth.revoke_token("alice", local_fallback=True)) + await asyncio.sleep(0) + assert not revoke.done() + release.set() + await asyncio.gather(pending, revoke) + assert await oauth.store.get_token("alice") is None + + +@pytest.mark.asyncio +async def test_helper_shared_failures_independent_keys_and_retry(monkeypatch): + oauth = client() + await saved(oauth) + await saved(oauth, "bob") + entered, release = asyncio.Event(), asyncio.Event() + count = 0 + async def send(request): + nonlocal count + count += 1 + if count == 2: + entered.set() + await release.wait() + return response({"error": "invalid_grant", "error_description": SECRET}, status=400) + monkeypatch.setattr(oauth, "_send", send) + tasks = [asyncio.create_task(oauth.get_access_token("alice")) for _ in range(20)] + tasks.append(asyncio.create_task(oauth.get_access_token("bob"))) + await asyncio.wait_for(entered.wait(), 2) + release.set() + results = await asyncio.gather(*tasks, return_exceptions=True) + assert count == 2 and all(isinstance(result, OAuthClientError) for result in results) + assert all(result is results[0] for result in results[:20]) + assert SECRET not in repr(results) + assert oauth._refresh_tasks == {} and oauth._force_refresh == {} + install_transport(monkeypatch, oauth) + assert await oauth.get_access_token("alice") == ACCESS + + +@pytest.mark.asyncio +async def test_helper_rejects_immediately_expired_replacement_and_storage_failure(monkeypatch): + oauth = client() + await saved(oauth) + install_transport(monkeypatch, oauth, response({"access_token": ACCESS, "expires_in": "0"})) + with pytest.raises(OAuthClientError, match="expiry"): + await oauth.get_access_token("alice") + monkeypatch.setattr(oauth.store, "get_token", AsyncMock(side_effect=OSError(SECRET))) + with pytest.raises(OAuthClientError) as exc: + await oauth.get_access_token("alice") + assert SECRET not in str(exc.value)