mirror of
https://github.com/stack-auth/stack.git
synced 2026-07-20 21:29:36 +08:00
fix: address PR review findings from bot analysis
- Fix incorrect docstring method names in get_item (set_quantity -> increase_quantity, decrease_quantity, try_decrease_quantity) - Add asyncio.Lock to AsyncJWKSFetcher._fetch_jwks to deduplicate concurrent JWKS fetches on cold/expired cache - Add threading.Lock to SyncJWKSFetcher._fetch_jwks for same reason - Add threading.Lock to _get_or_create_memory_store to prevent race condition in token store registry - Fix README clone URL to point to upstream repo - Fix E2E test SyncJWKSFetcher constructor to use correct signature (jwks_url + http_client instead of project_id + base_url)
This commit is contained in:
parent
5c09ea70c3
commit
d0550079f7
@ -192,7 +192,7 @@ app = StackServerApp(
|
||||
|
||||
```bash
|
||||
# Clone the repo
|
||||
git clone https://github.com/ejirocodes/stack-auth.git
|
||||
git clone https://github.com/stack-auth/stack-auth.git
|
||||
cd stack-auth/sdks/implementations/python
|
||||
|
||||
# Install in dev mode
|
||||
|
||||
@ -1235,7 +1235,8 @@ class StackServerApp:
|
||||
|
||||
Returns:
|
||||
A :class:`ServerItem` wrapping the :class:`Item` data and
|
||||
providing ``set_quantity`` and ``increment_quantity`` methods.
|
||||
providing ``increase_quantity``, ``decrease_quantity``, and
|
||||
``try_decrease_quantity`` methods.
|
||||
|
||||
Raises:
|
||||
ValueError: If not exactly one customer identifier is provided.
|
||||
@ -2570,7 +2571,8 @@ class AsyncStackServerApp:
|
||||
|
||||
Returns:
|
||||
An :class:`AsyncServerItem` wrapping the :class:`Item` data and
|
||||
providing ``set_quantity`` and ``increment_quantity`` coroutines.
|
||||
providing ``increase_quantity``, ``decrease_quantity``, and
|
||||
``try_decrease_quantity`` coroutines.
|
||||
|
||||
Raises:
|
||||
ValueError: If not exactly one customer identifier is provided.
|
||||
|
||||
@ -7,6 +7,8 @@ to prevent CVE-2022-29217 style algorithm confusion attacks.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import threading
|
||||
import time
|
||||
from typing import Any
|
||||
|
||||
@ -34,6 +36,7 @@ class AsyncJWKSFetcher:
|
||||
self._http_client = http_client
|
||||
self._cache: dict[str, Any] | None = None
|
||||
self._cache_time: float = 0.0
|
||||
self._fetch_lock = asyncio.Lock()
|
||||
|
||||
async def get_signing_key(self, kid: str) -> Any:
|
||||
"""Return the RSA public key for the given key ID.
|
||||
@ -55,16 +58,21 @@ class AsyncJWKSFetcher:
|
||||
return RSAAlgorithm.from_jwk(key_data)
|
||||
|
||||
async def _fetch_jwks(self, force: bool = False) -> dict[str, Any]:
|
||||
"""Fetch JWKS from the endpoint, using cache if fresh."""
|
||||
now = time.monotonic()
|
||||
if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL:
|
||||
return self._cache
|
||||
"""Fetch JWKS from the endpoint, using cache if fresh.
|
||||
|
||||
response = await self._http_client.get(self._jwks_url)
|
||||
response.raise_for_status()
|
||||
self._cache = response.json()
|
||||
self._cache_time = time.monotonic()
|
||||
return self._cache # type: ignore[return-value]
|
||||
Uses an asyncio.Lock to deduplicate concurrent fetches so only one
|
||||
HTTP request is made when multiple coroutines hit a cold or expired cache.
|
||||
"""
|
||||
async with self._fetch_lock:
|
||||
now = time.monotonic()
|
||||
if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL:
|
||||
return self._cache
|
||||
|
||||
response = await self._http_client.get(self._jwks_url)
|
||||
response.raise_for_status()
|
||||
self._cache = response.json()
|
||||
self._cache_time = time.monotonic()
|
||||
return self._cache # type: ignore[return-value]
|
||||
|
||||
|
||||
class SyncJWKSFetcher:
|
||||
@ -80,6 +88,7 @@ class SyncJWKSFetcher:
|
||||
self._http_client = http_client
|
||||
self._cache: dict[str, Any] | None = None
|
||||
self._cache_time: float = 0.0
|
||||
self._fetch_lock = threading.Lock()
|
||||
|
||||
def get_signing_key(self, kid: str) -> Any:
|
||||
"""Return the RSA public key for the given key ID.
|
||||
@ -100,16 +109,21 @@ class SyncJWKSFetcher:
|
||||
return RSAAlgorithm.from_jwk(key_data)
|
||||
|
||||
def _fetch_jwks(self, force: bool = False) -> dict[str, Any]:
|
||||
"""Fetch JWKS from the endpoint, using cache if fresh."""
|
||||
now = time.monotonic()
|
||||
if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL:
|
||||
return self._cache
|
||||
"""Fetch JWKS from the endpoint, using cache if fresh.
|
||||
|
||||
response = self._http_client.get(self._jwks_url)
|
||||
response.raise_for_status()
|
||||
self._cache = response.json()
|
||||
self._cache_time = time.monotonic()
|
||||
return self._cache # type: ignore[return-value]
|
||||
Uses a threading.Lock to deduplicate concurrent fetches so only one
|
||||
HTTP request is made when multiple threads hit a cold or expired cache.
|
||||
"""
|
||||
with self._fetch_lock:
|
||||
now = time.monotonic()
|
||||
if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL:
|
||||
return self._cache
|
||||
|
||||
response = self._http_client.get(self._jwks_url)
|
||||
response.raise_for_status()
|
||||
self._cache = response.json()
|
||||
self._cache_time = time.monotonic()
|
||||
return self._cache # type: ignore[return-value]
|
||||
|
||||
|
||||
def _find_key(jwks: dict[str, Any], kid: str) -> dict[str, Any] | None:
|
||||
|
||||
@ -177,12 +177,20 @@ ensures that invariant.
|
||||
To reset in tests: ``stack_auth._token_store._token_store_registry.clear()``
|
||||
"""
|
||||
|
||||
_registry_lock = threading.Lock()
|
||||
"""Guards _token_store_registry against concurrent check-then-set races."""
|
||||
|
||||
|
||||
def _get_or_create_memory_store(project_id: str) -> MemoryTokenStore:
|
||||
"""Return the shared MemoryTokenStore for a project, creating one if needed."""
|
||||
if project_id not in _token_store_registry:
|
||||
_token_store_registry[project_id] = MemoryTokenStore()
|
||||
return _token_store_registry[project_id]
|
||||
"""Return the shared MemoryTokenStore for a project, creating one if needed.
|
||||
|
||||
Thread-safe: uses _registry_lock to prevent two threads from creating
|
||||
duplicate stores for the same project_id.
|
||||
"""
|
||||
with _registry_lock:
|
||||
if project_id not in _token_store_registry:
|
||||
_token_store_registry[project_id] = MemoryTokenStore()
|
||||
return _token_store_registry[project_id]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@ -302,12 +302,15 @@ class TestAuthenticateRequestE2E:
|
||||
"""
|
||||
|
||||
def test_unauthenticated_request_without_token(self, app: StackServerApp) -> None:
|
||||
import httpx as httpx_client
|
||||
|
||||
from stack_auth._auth import sync_authenticate_request
|
||||
from stack_auth._jwt import SyncJWKSFetcher
|
||||
|
||||
jwks_url = f"{BASE_URL}/api/v1/projects/{PROJECT_ID}/.well-known/jwks.json"
|
||||
fetcher = SyncJWKSFetcher(
|
||||
project_id=PROJECT_ID,
|
||||
base_url=BASE_URL,
|
||||
jwks_url=jwks_url,
|
||||
http_client=httpx_client.Client(),
|
||||
)
|
||||
|
||||
class FakeRequest:
|
||||
@ -320,12 +323,15 @@ class TestAuthenticateRequestE2E:
|
||||
assert result.status == "unauthenticated"
|
||||
|
||||
def test_invalid_token_returns_unauthenticated(self, app: StackServerApp) -> None:
|
||||
import httpx as httpx_client
|
||||
|
||||
from stack_auth._auth import sync_authenticate_request
|
||||
from stack_auth._jwt import SyncJWKSFetcher
|
||||
|
||||
jwks_url = f"{BASE_URL}/api/v1/projects/{PROJECT_ID}/.well-known/jwks.json"
|
||||
fetcher = SyncJWKSFetcher(
|
||||
project_id=PROJECT_ID,
|
||||
base_url=BASE_URL,
|
||||
jwks_url=jwks_url,
|
||||
http_client=httpx_client.Client(),
|
||||
)
|
||||
|
||||
class FakeRequest:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user