feat: reuse token#342
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe token cache now uses runtime-specific storage backends, exposes async save/load helpers with expiry checks, and updates ChangesOIDC Token Caching
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant authenticate as authenticate()
participant token_store as token_store
participant authenticate_oidc as authenticate_oidc(...)
participant Environment as environment
User->>authenticate: authenticate(force=False)
authenticate->>token_store: load_token(oidc_url)
alt cached token exists
token_store-->>authenticate: token_data
authenticate->>Environment: store_token_data(token_data)
else no cached token
authenticate->>authenticate_oidc: authenticate_oidc(...)
authenticate_oidc-->>authenticate: token_data
authenticate->>token_store: save_token(oidc_url, token_data)
authenticate->>Environment: store_token_data(token_data)
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/py/unit/test_token_store.py (2)
133-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant expiry assignment in
test_expired_token_returns_none.
td["expires_at"]is already set to an expired value beforestore.save(...)(line 135), andaugment_token_data_with_expirywon't overwrite an existingexpires_at. The manual re-open/re-write on lines 137-144 duplicates that same assignment and adds nothing to the test.♻️ Proposed simplification
def test_expired_token_returns_none(self, store): td = _make_token(expires_in=0) td["expires_at"] = time.time() - 100 # already expired store.save(MOCK_OIDC_URL, td) - # Manually force expires_at to past in the file - cache_path = store._cache_path - with open(cache_path) as f: - cache = json.load(f) - key = _make_cache_key(MOCK_OIDC_URL) - cache[key]["expires_at"] = time.time() - 100 - with open(cache_path, "w") as f: - json.dump(cache, f) - assert store.load(MOCK_OIDC_URL) is None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/py/unit/test_token_store.py` around lines 133 - 146, The test `test_expired_token_returns_none` is doing the expired-token setup twice: `td["expires_at"]` is already set before `store.save(...)`, so the manual cache file reopen and rewrite is redundant. Simplify the test by keeping only the initial expired `expires_at` assignment on the token data and remove the extra JSON cache mutation block, while still asserting `store.load(MOCK_OIDC_URL) is None`.
232-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestAuthenticateIntegrationdoesn't actually testauthenticate().The class docstring says it verifies that authenticate() reads from and writes to the token store. But
test_file_token_store_save_load_cycleonly instantiates twoFileTokenStoreobjects directly and never callsauthenticate()— it duplicates coverage already provided byTestFileTokenStore.test_save_and_load. Thepatched_envfixture setsAPI_HOST,API_PORT,API_SECURE, and deletesOIDC_ACCESS_TOKEN/OIDC_REFRESH_TOKEN, but none of these are consumed sinceauthenticate()is never invoked, making that setup dead code.Given the stack context indicates this layer depends on the "Token store factory and authentication integration" work, this test should exercise
authenticate()/authenticate_oidc()with a mocked/faked HTTP call to confirm the cache is actually consulted and populated — or, if that's out of scope for now, the class/docstring should be renamed to reflect that it only tests cross-instanceFileTokenStoresharing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/py/unit/test_token_store.py` around lines 232 - 259, The TestAuthenticateIntegration case is not exercising authenticate() at all and only duplicates FileTokenStore save/load behavior. Update test_file_token_store_save_load_cycle to call authenticate() or authenticate_oidc() with a mocked OIDC/HTTP flow so it verifies the token cache is read and written, using the existing patched_env fixture and _make_token/augment_token_data_with_expiry setup; if that integration coverage is not intended, rename the class and docstring to match the actual FileTokenStore cross-instance test.src/py/mat3ra/notebooks_utils/core/api/token_store.py (1)
104-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffRead-modify-write is not atomic across keys — concurrent kernels can drop each other's cached tokens.
save/cleardo_read_cache()→ mutate →_write_cache().os.replacekeeps the file itself intact, but two kernels persisting tokens for differentoidc_base_urlvalues concurrently will last-writer-win, silently discarding the other entry. The class docstring states implementations must be safe for concurrent multi-kernel access, so consider a file lock (e.g.fcntl.flock) around the read-modify-write. Impact is limited to an occasional re-auth, hence recommended rather than blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py` around lines 104 - 129, The TokenStore read-modify-write flow in save, load, and clear is not safe for concurrent multi-kernel access because _read_cache() and _write_cache() can race and overwrite other oidc_base_url entries. Update the TokenStore implementation to serialize cache mutations with a file lock around the read-modify-write sequence (for example in save and clear, and any prune path in load), so concurrent kernels do not drop each other’s cached tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py`:
- Around line 143-158: The temporary cache file in _write_cache is created with
default permissions and only tightened after the token is written, leaving a
brief readability window. Update _write_cache in token_store.py to create the
temp file with restrictive permissions from the start, using the existing
_cache_path/tmp_path flow but ensuring the file is opened as 0o600 before
json.dump writes any data. Keep the existing os.replace and cleanup behavior,
and preserve the restrictive directory permissions already set by os.makedirs.
---
Nitpick comments:
In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py`:
- Around line 104-129: The TokenStore read-modify-write flow in save, load, and
clear is not safe for concurrent multi-kernel access because _read_cache() and
_write_cache() can race and overwrite other oidc_base_url entries. Update the
TokenStore implementation to serialize cache mutations with a file lock around
the read-modify-write sequence (for example in save and clear, and any prune
path in load), so concurrent kernels do not drop each other’s cached tokens.
In `@tests/py/unit/test_token_store.py`:
- Around line 133-146: The test `test_expired_token_returns_none` is doing the
expired-token setup twice: `td["expires_at"]` is already set before
`store.save(...)`, so the manual cache file reopen and rewrite is redundant.
Simplify the test by keeping only the initial expired `expires_at` assignment on
the token data and remove the extra JSON cache mutation block, while still
asserting `store.load(MOCK_OIDC_URL) is None`.
- Around line 232-259: The TestAuthenticateIntegration case is not exercising
authenticate() at all and only duplicates FileTokenStore save/load behavior.
Update test_file_token_store_save_load_cycle to call authenticate() or
authenticate_oidc() with a mocked OIDC/HTTP flow so it verifies the token cache
is read and written, using the existing patched_env fixture and
_make_token/augment_token_data_with_expiry setup; if that integration coverage
is not intended, rename the class and docstring to match the actual
FileTokenStore cross-instance test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69b8eec6-b18d-4bd3-8620-eda36cdfbc1c
📒 Files selected for processing (6)
src/py/mat3ra/notebooks_utils/auth.pysrc/py/mat3ra/notebooks_utils/core/api/auth.pysrc/py/mat3ra/notebooks_utils/core/api/token_store.pysrc/py/mat3ra/notebooks_utils/pyodide/api/token_store.pysrc/py/mat3ra/notebooks_utils/token_store.pytests/py/unit/test_token_store.py
| def _write_cache(self, cache: dict) -> None: | ||
| os.makedirs(self._cache_dir, mode=0o700, exist_ok=True) | ||
| tmp_path = self._cache_path + ".tmp" | ||
| try: | ||
| with open(tmp_path, "w") as fh: | ||
| json.dump(cache, fh, indent=2) | ||
| # Set restrictive permissions before renaming into place | ||
| os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR) # 0o600 | ||
| os.replace(tmp_path, self._cache_path) | ||
| except OSError: | ||
| # Best-effort cleanup | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Token is briefly world-readable in the temp file before chmod.
open(tmp_path, "w") creates the file using the process umask (typically 0o644), and the OIDC token is written and flushed before os.chmod(..., 0o600) runs. On a multi-user host this leaves a short window where the credential file is readable by other local users. Create the file with restrictive permissions from the start instead of tightening them afterward.
🔒 Proposed fix: create temp file with 0o600 up front
def _write_cache(self, cache: dict) -> None:
os.makedirs(self._cache_dir, mode=0o700, exist_ok=True)
tmp_path = self._cache_path + ".tmp"
try:
- with open(tmp_path, "w") as fh:
+ fd = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w") as fh:
json.dump(cache, fh, indent=2)
- # Set restrictive permissions before renaming into place
- os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR) # 0o600
os.replace(tmp_path, self._cache_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _write_cache(self, cache: dict) -> None: | |
| os.makedirs(self._cache_dir, mode=0o700, exist_ok=True) | |
| tmp_path = self._cache_path + ".tmp" | |
| try: | |
| with open(tmp_path, "w") as fh: | |
| json.dump(cache, fh, indent=2) | |
| # Set restrictive permissions before renaming into place | |
| os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR) # 0o600 | |
| os.replace(tmp_path, self._cache_path) | |
| except OSError: | |
| # Best-effort cleanup | |
| try: | |
| os.unlink(tmp_path) | |
| except OSError: | |
| pass | |
| raise | |
| def _write_cache(self, cache: dict) -> None: | |
| os.makedirs(self._cache_dir, mode=0o700, exist_ok=True) | |
| tmp_path = self._cache_path + ".tmp" | |
| try: | |
| fd = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| with os.fdopen(fd, "w") as fh: | |
| json.dump(cache, fh, indent=2) | |
| os.replace(tmp_path, self._cache_path) | |
| except OSError: | |
| # Best-effort cleanup | |
| try: | |
| os.unlink(tmp_path) | |
| except OSError: | |
| pass | |
| raise |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 146-146: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(tmp_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py` around lines 143 -
158, The temporary cache file in _write_cache is created with default
permissions and only tightened after the token is written, leaving a brief
readability window. Update _write_cache in token_store.py to create the temp
file with restrictive permissions from the start, using the existing
_cache_path/tmp_path flow but ensuring the file is opened as 0o600 before
json.dump writes any data. Keep the existing os.replace and cleanup behavior,
and preserve the restrictive directory permissions already set by os.makedirs.
VsevolodX
force-pushed
the
feature/SOF-7919
branch
from
a57412a to
fd2a632
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/py/mat3ra/notebooks_utils/token_store.py (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
save_tokenmutates the caller'stoken_datadict in place.Line 19 adds
expires_atdirectly onto the dict passed in, which is a hidden side effect on the caller's object. Per theauthenticate()context snippet inauth.py,token_datais passed tosave_tokenright after being obtained fromauthenticate_oidc; mutating it silently could surprise future callers that reuse the same dict elsewhere.♻️ Proposed fix: avoid in-place mutation
def save_token(oidc_url: str, token_data: dict) -> None: - token_data["expires_at"] = time.time() + token_data.get("expires_in", 3600) + token_data = {**token_data, "expires_at": time.time() + token_data.get("expires_in", 3600)} cache = _read_cache() cache[oidc_url] = token_data _write_cache(cache)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/py/mat3ra/notebooks_utils/token_store.py` around lines 18 - 22, save_token currently mutates the caller-provided token_data by adding expires_at in place, so update the function to avoid modifying that input directly. In save_token, create a separate copied token payload, set expires_at on the copy, and persist that copy through _read_cache and _write_cache; keep the existing save_token and _write_cache flow unchanged otherwise. This will prevent hidden side effects for callers such as authenticate() that pass through the authenticate_oidc result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/token_store.py`:
- Around line 32-39: The _read_cache helper in token_store.py is catching every
Exception and silently returning an empty cache, which can discard all stored
tokens on unrelated read failures. Narrow the exception handling to only the
expected file/JSON read errors in _read_cache, and keep the fallback to {} only
for those cases while allowing unexpected errors to surface.
- Around line 42-49: The token cache write in _write_cache currently creates the
file via open(_FILE_PATH, "w") and only tightens permissions afterward, leaving
a brief non-0600 exposure window. Update the non-browser branch in _write_cache
to create the cache file atomically with restrictive permissions from the start
(for example, via os.open or an equivalent atomic create flow), then write the
JSON and preserve 0600 access on the file. Keep the existing browser
localStorage path unchanged and ensure the fix is applied where _FILE_PATH is
written.
- Around line 18-22: The save_token() path updates the shared cache without
synchronization, so concurrent writers can clobber each other’s oidc_url
entries; fix this by protecting the read-modify-write sequence in save_token()
with a file lock or by using an atomic merge/write strategy around _read_cache()
and _write_cache(). Keep the existing token_data expiration update, but ensure
only one writer can modify the cache at a time so concurrent notebook
kernels/processes do not lose entries.
---
Nitpick comments:
In `@src/py/mat3ra/notebooks_utils/token_store.py`:
- Around line 18-22: save_token currently mutates the caller-provided token_data
by adding expires_at in place, so update the function to avoid modifying that
input directly. In save_token, create a separate copied token payload, set
expires_at on the copy, and persist that copy through _read_cache and
_write_cache; keep the existing save_token and _write_cache flow unchanged
otherwise. This will prevent hidden side effects for callers such as
authenticate() that pass through the authenticate_oidc result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2c040a9-22d7-426a-836a-0cc9815dbd2d
📒 Files selected for processing (2)
src/py/mat3ra/notebooks_utils/auth.pysrc/py/mat3ra/notebooks_utils/token_store.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/py/mat3ra/notebooks_utils/auth.py
| def save_token(oidc_url: str, token_data: dict) -> None: | ||
| token_data["expires_at"] = time.time() + token_data.get("expires_in", 3600) | ||
| cache = _read_cache() | ||
| cache[oidc_url] = token_data | ||
| _write_cache(cache) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/py/mat3ra/notebooks_utils/token_store.pyRepository: mat3ra/api-examples
Length of output: 1602
🏁 Script executed:
rg -n "save_token|load_token|oidc_token_cache|mat3ra_oidc_token_cache|flock|filelock" src/pyRepository: mat3ra/api-examples
Length of output: 839
🏁 Script executed:
sed -n '1,220p' src/py/mat3ra/notebooks_utils/auth.py && printf '\n---\n' && rg -n "notebook kernels|cache|localStorage|OIDC token" src/py README.mdRepository: mat3ra/api-examples
Length of output: 2860
🏁 Script executed:
rg -n "fcntl|flock|filelock|atomic.*write|tempfile|os.replace|rename\(" src/pyRepository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
python3 - <<'PY'
import json, os, tempfile, multiprocessing as mp
def read_cache(path):
try:
with open(path) as f:
return json.load(f)
except Exception:
return {}
def write_cache(path, cache):
with open(path, "w") as f:
json.dump(cache, f)
def save_token(path, oidc_url, token_data):
token_data = dict(token_data)
token_data["expires_at"] = 0
cache = read_cache(path)
cache[oidc_url] = token_data
write_cache(path, cache)
def worker(path, key):
save_token(path, key, {"expires_in": 3600})
with tempfile.NamedTemporaryFile(delete=False) as tmp:
path = tmp.name
# Start from an empty JSON object
with open(path, "w") as f:
f.write("{}")
# Run two writers concurrently
p1 = mp.Process(target=worker, args=(path, "https://a.example"))
p2 = mp.Process(target=worker, args=(path, "https://b.example"))
p1.start(); p2.start(); p1.join(); p2.join()
with open(path) as f:
final = json.load(f)
print(final)
print("keys:", sorted(final))
os.unlink(path)
PYRepository: mat3ra/api-examples
Length of output: 327
🏁 Script executed:
python3 - <<'PY'
import json, os, tempfile, multiprocessing as mp, time
def read_cache(path):
try:
with open(path) as f:
return json.load(f)
except Exception:
return {}
def write_cache(path, cache):
with open(path, "w") as f:
json.dump(cache, f)
def interleaved_save(path, oidc_url, ready, go):
cache = read_cache(path)
cache[oidc_url] = {"expires_in": 3600}
ready.set()
go.wait()
write_cache(path, cache)
with tempfile.NamedTemporaryFile(delete=False) as tmp:
path = tmp.name
with open(path, "w") as f:
f.write("{}")
ready1 = mp.Event()
ready2 = mp.Event()
go = mp.Event()
p1 = mp.Process(target=interleaved_save, args=(path, "https://a.example", ready1, go))
p2 = mp.Process(target=interleaved_save, args=(path, "https://b.example", ready2, go))
p1.start(); p2.start()
ready1.wait(); ready2.wait()
go.set()
p1.join(); p2.join()
with open(path) as f:
final = json.load(f)
print(final)
print("keys:", sorted(final))
os.unlink(path)
PYRepository: mat3ra/api-examples
Length of output: 229
Lock the token cache write path. save_token() reads, mutates, and rewrites a shared JSON cache without synchronization, so concurrent kernels/processes can overwrite each other’s oidc_url entries. Wrap the read-modify-write cycle in a file lock or use an atomic merge strategy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/token_store.py` around lines 18 - 22, The
save_token() path updates the shared cache without synchronization, so
concurrent writers can clobber each other’s oidc_url entries; fix this by
protecting the read-modify-write sequence in save_token() with a file lock or by
using an atomic merge/write strategy around _read_cache() and _write_cache().
Keep the existing token_data expiration update, but ensure only one writer can
modify the cache at a time so concurrent notebook kernels/processes do not lose
entries.
1 participant
Summary by CodeRabbit