Skip to content

feat(NEX-584): First iteration of changes to SDK handling new client secrets#339

Open
mateuszkuprowski wants to merge 3 commits intomainfrom
nex-584-ad-client-credentials-support-and-exchange
Open

feat(NEX-584): First iteration of changes to SDK handling new client secrets#339
mateuszkuprowski wants to merge 3 commits intomainfrom
nex-584-ad-client-credentials-support-and-exchange

Conversation

@mateuszkuprowski
Copy link
Copy Markdown

@mateuszkuprowski mateuszkuprowski commented Apr 24, 2026

This PR lets our SDK handle new type client secrets. Exchanges them for JWT and manages refresh cycle. This change is fully transparent for all the existing external apps using our SDK. Switching to clinet-secrets is an opt in.


Note

Medium Risk
Introduces new authentication flow that exchanges credentials for JWTs and rewrites request headers, which can affect all authenticated requests if misconfigured. Backward compatibility is intended, but failures could break access in dedicated/self-hosted deployments or when hooks mis-detect auth sources.

Overview
Adds an opt-in unstructured_client.auth module that can exchange client secrets (and transitional legacy API keys) for short-lived JWTs via /auth/token-exchange, with in-memory caching, refresh-before-expiry, concurrency guards, retries/backoff, and outage fallback.

Integrates this into request sending by registering a new AuthHeaderBeforeRequestHook that rewrites unstructured-api-key to Authorization: Bearer <jwt> only when the security source is one of the new exchange callables, while leaving plain-string api_key_auth behavior unchanged.

Updates SDK initialization to retain the original auth callable on the internal security factory for hook detection, adds extensive unit coverage plus an opt-in E2E test, and documents the new client-secret authentication usage and tuning in the README.

Reviewed by Cursor Bugbot for commit 908d418. Bugbot is set up for automated code reviews on this repo. Configure here.


with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
future = pool.submit(_run_in_new_loop)
return future.result()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Async __call__ lacks thread-safe locking unlike sync variant

Medium Severity

AsyncClientCredentials.__call__() doesn't use the inherited threading.Lock (self._lock) for thread synchronization, unlike ClientCredentials.__call__() which correctly implements double-checked locking. The asyncio.Lock used inside acquire() only serializes coroutines within a single event loop — it provides no protection across threads. When __call__ is invoked concurrently from multiple threads, each creates its own event loop via asyncio.run(), so the _async_lock is ineffective, leading to redundant token-exchange HTTP calls and a data race on self._cache.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 61612a8. Configure here.


with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
future = pool.submit(_run_in_new_loop)
return future.result()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

asyncio.Lock crashes on token refresh across event loops

High Severity

AsyncClientCredentials.__call__ uses asyncio.run() which creates a new event loop on each invocation. The self._async_lock (asyncio.Lock) binds to the first event loop via Python 3.10+'s _LoopBoundMixin. When the cached token expires and a second exchange is needed, asyncio.run() creates a different event loop, and async with self._async_lock raises RuntimeError because the lock is already bound to the now-closed first loop. This affects both the "no running loop" and "inside running loop" code paths in __call__, since both ultimately call asyncio.run(self.acquire()). The project requires Python >=3.11, so this always applies.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8dc719d. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 908d418. Configure here.


with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
future = pool.submit(_run_in_new_loop)
return future.result()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Async call blocks event loop with future.result()

Medium Severity

When AsyncClientCredentials.__call__ is invoked from within a running event loop (the normal async SDK path — _build_request_with_client calls security() synchronously), it spawns a worker thread but then calls future.result(), which is a blocking call on the event loop thread. This blocks all other coroutines from progressing during the token exchange, defeating the purpose of async concurrency. The code comment says it avoids blocking "the caller's loop on httpx IO," but future.result() itself blocks the loop.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 908d418. Configure here.

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