feat: reuse token#342
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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: 3 (Moderate) | ~20 minutes 🚥 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.
| os.makedirs(os.path.dirname(_FILE_PATH), mode=0o700, exist_ok=True) | ||
| with open(_FILE_PATH, "w") as f: | ||
| json.dump(cache, f) | ||
| os.chmod(_FILE_PATH, 0o600) |
There was a problem hiding this comment.
Clearly the two above functions are not needed
|
|
||
| if is_pyodide_environment(): | ||
| from .core.api import token_store as _core_ts | ||
| from .pyodide.api import token_store as _pyodide_storage |
There was a problem hiding this comment.
we should not have imports in if
| import time | ||
| from typing import Optional | ||
|
|
||
| _EXPIRY_BUFFER = 60 # seconds before actual expiry to consider token stale |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/py/mat3ra/notebooks_utils/core/api/token_store.py (1)
14-17: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCache directory/file created with default permissions; write is not atomic.
os.makedirsandopen(_FILE_PATH, "w")use umask-derived permissions (no0o700/0o600enforcement), so the OIDC token cache can be readable by other local users. The write is also not atomic (no temp-file +os.replace), so a crash mid-write can corrupt the cache.🔒 Proposed fix: restrictive perms + atomic write
async def write(cache: dict) -> None: - os.makedirs(os.path.dirname(_FILE_PATH), exist_ok=True) - with open(_FILE_PATH, "w") as f: - json.dump(cache, f) + os.makedirs(os.path.dirname(_FILE_PATH), mode=0o700, exist_ok=True) + tmp_path = _FILE_PATH + ".tmp" + fd = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w") as f: + json.dump(cache, f) + os.replace(tmp_path, _FILE_PATH)This mirrors concerns already raised on this backend's earlier versions (world-readable temp file before chmod, missing restrictive dir mode) and appears to have regressed since no permission handling remains at all.
🤖 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 14 - 17, The token cache write in write() currently relies on default umask permissions and writes directly to the final path, so update this path to enforce restrictive access and atomic replacement. In token_store.write, create the cache directory with an explicit private mode and write the JSON to a temporary file with restrictive file permissions, then replace _FILE_PATH using an atomic swap so partial writes cannot corrupt the cache. Keep the fix localized to write() and the _FILE_PATH handling.src/py/mat3ra/notebooks_utils/token_store.py (1)
21-26: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUnsynchronized read-modify-write still races across concurrent kernels/processes.
This exact concern was raised previously (and marked as still outstanding, unlike sibling comments in the same review thread that show "Addressed").
save_token()reads the whole cache, mutates one key, and rewrites it with no lock — concurrent writers can still clobber each other'soidc_urlentries.🤖 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 21 - 26, The save_token() read-modify-write flow is still unsynchronized, so concurrent kernels or processes can overwrite each other’s token entries. Update the token_store.py persistence path so the cache update around _read() and _write() is performed under a shared lock or other atomic synchronization primitive, and keep the critical section covering the full read-update-write sequence. Make sure save_token() still preserves existing entries for other oidc_url keys while safely merging the new token_data for the current oidc_url.
🤖 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 9-11: `read()` in token_store.py should safely handle absent or
invalid cache files instead of crashing on startup. Update the `read()` function
to catch missing-file and bad-JSON cases from `_FILE_PATH` and return an empty
dict so `load_token()` and the `authenticate()` flow can proceed on a fresh or
corrupt cache.
---
Duplicate comments:
In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py`:
- Around line 14-17: The token cache write in write() currently relies on
default umask permissions and writes directly to the final path, so update this
path to enforce restrictive access and atomic replacement. In token_store.write,
create the cache directory with an explicit private mode and write the JSON to a
temporary file with restrictive file permissions, then replace _FILE_PATH using
an atomic swap so partial writes cannot corrupt the cache. Keep the fix
localized to write() and the _FILE_PATH handling.
In `@src/py/mat3ra/notebooks_utils/token_store.py`:
- Around line 21-26: The save_token() read-modify-write flow is still
unsynchronized, so concurrent kernels or processes can overwrite each other’s
token entries. Update the token_store.py persistence path so the cache update
around _read() and _write() is performed under a shared lock or other atomic
synchronization primitive, and keep the critical section covering the full
read-update-write sequence. Make sure save_token() still preserves existing
entries for other oidc_url keys while safely merging the new token_data for the
current oidc_url.
🪄 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: 7c4823a9-78c1-43ce-bc24-aa0935e798ad
📒 Files selected for processing (3)
src/py/mat3ra/notebooks_utils/core/api/token_store.pysrc/py/mat3ra/notebooks_utils/pyodide/api/token_store.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/pyodide/api/token_store.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/py/mat3ra/notebooks_utils/core/api/token_store.py (1)
9-14: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winStill missing
json.JSONDecodeErrorhandling — partially addressed from previous review.The
FileNotFoundErrorcase is now handled (returning{}), but a corrupt or partially written cache file will still raisejson.JSONDecodeErrorfromjson.load()and crashauthenticate(). This was flagged in a prior review as critical and remains unresolved.🔒 Proposed fix: also catch json.JSONDecodeError
async def read() -> dict: try: with open(_FILE_PATH) as f: return json.load(f) - except FileNotFoundError: + except (FileNotFoundError, json.JSONDecodeError): return {}🤖 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 9 - 14, The token cache reader still only handles missing files, so a corrupt or partially written cache can crash authentication. Update the read() function in token_store.py to also catch json.JSONDecodeError around json.load() and return an empty dict the same way FileNotFoundError is handled, keeping authenticate() resilient when the cache contents are invalid.
🧹 Nitpick comments (2)
src/py/mat3ra/notebooks_utils/auth.py (2)
28-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass the already-derived
oidc_urltoauthenticate_oidc.You compute
oidc_urlon Line 29 and use it forload_token/save_token, but theauthenticate_oidc()call on Line 36 omitsoidc_base_url, forcing it to recompute the URL viaget_oidc_base_url()again. Reusing the local value avoids the extraAPIEnv.from_env()read and guarantees the URL used for the device flow matches the key the token is cached under.♻️ Proposed change
- token_data = await authenticate_oidc(show_popup=show_device_flow_popup) + token_data = await authenticate_oidc(oidc_base_url=oidc_url, show_popup=show_device_flow_popup) await save_token(oidc_url, token_data)🤖 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/auth.py` around lines 28 - 37, The OIDC cache flow recomputes the base URL in authenticate_oidc instead of reusing the already-derived oidc_url from _authenticate_oidc_with_cache. Update the _authenticate_oidc_with_cache function so the authenticate_oidc call passes oidc_url as oidc_base_url, keeping the device flow consistent with load_token and save_token and avoiding the extra get_oidc_base_url/APIEnv.from_env lookup.
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuff S105 here is a false positive.
REFRESH_TOKEN_ENV_VARholds an environment-variable name, not a secret. If Ruff is enforced in CI, add an inline suppression to keep the pipeline green.Proposed suppression
-REFRESH_TOKEN_ENV_VAR = "OIDC_REFRESH_TOKEN" +REFRESH_TOKEN_ENV_VAR = "OIDC_REFRESH_TOKEN" # noqa: S105🤖 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/auth.py` at line 25, Ruff is incorrectly flagging REFRESH_TOKEN_ENV_VAR in auth.py as a secret even though it is only an environment-variable name; add an inline suppression on that constant so CI stays green. Use the REFRESH_TOKEN_ENV_VAR assignment in the auth module to place the suppression directly on the offending line, keeping the exemption narrowly scoped to this false positive.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@src/py/mat3ra/notebooks_utils/core/api/token_store.py`:
- Around line 9-14: The token cache reader still only handles missing files, so
a corrupt or partially written cache can crash authentication. Update the read()
function in token_store.py to also catch json.JSONDecodeError around json.load()
and return an empty dict the same way FileNotFoundError is handled, keeping
authenticate() resilient when the cache contents are invalid.
---
Nitpick comments:
In `@src/py/mat3ra/notebooks_utils/auth.py`:
- Around line 28-37: The OIDC cache flow recomputes the base URL in
authenticate_oidc instead of reusing the already-derived oidc_url from
_authenticate_oidc_with_cache. Update the _authenticate_oidc_with_cache function
so the authenticate_oidc call passes oidc_url as oidc_base_url, keeping the
device flow consistent with load_token and save_token and avoiding the extra
get_oidc_base_url/APIEnv.from_env lookup.
- Line 25: Ruff is incorrectly flagging REFRESH_TOKEN_ENV_VAR in auth.py as a
secret even though it is only an environment-variable name; add an inline
suppression on that constant so CI stays green. Use the REFRESH_TOKEN_ENV_VAR
assignment in the auth module to place the suppression directly on the offending
line, keeping the exemption narrowly scoped to this false positive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45c96910-9865-4e0a-a1b8-481b819b45eb
📒 Files selected for processing (2)
src/py/mat3ra/notebooks_utils/auth.pysrc/py/mat3ra/notebooks_utils/core/api/token_store.py
2 participants
Summary by CodeRabbit