Persist cloud credentials on the server side#885
Merged
mihnea-necsulescu merged 13 commits intomainfrom Apr 8, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds an enterprise API router and a new PUT /api/cloud-credentials endpoint that updates process environment variables from a JSON map and clears the S3FileSystem instance cache; includes tests and a TypeScript schema update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as "FastAPI App\n(/api/cloud-credentials)"
participant Env as "Process\nos.environ"
participant S3 as "s3fs.S3FileSystem\n(instance cache)"
Client->>API: PUT /api/cloud-credentials\n{ "AWS_ACCESS_KEY_ID": "...", ... }
API->>Env: os.environ.update(credentials)
API->>S3: import S3FileSystem\nS3FileSystem.clear_instance_cache()
API-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f09b2b87e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightly_studio/src/lightly_studio/api/routes/api/enterprise.py`:
- Around line 20-25: Extract the credential update + cache-clear logic into a
shared utility (e.g., update_cloud_credentials_and_clear_s3_cache) and replace
both usages: in the endpoint where you currently call
os.environ.update(credentials) + S3FileSystem.clear_instance_cache(), and in
enterprise.connect() where it currently calls
os.environ.update(config.cloud_credentials). The utility should accept the
credentials dict, call os.environ.update(creds), then call
S3FileSystem.clear_instance_cache() (importing S3FileSystem lazily as done in
the endpoint). Update enterprise.connect and the endpoint to call this utility
to ensure credentials and S3FileSystem cache are updated atomically and
consistently.
- Around line 12-25: The refresh_cloud_credentials endpoint (function
refresh_cloud_credentials) must be protected and hardened: require
authentication/authorization by adding the appropriate dependency to the router
or endpoint (e.g., reuse your app's auth dependency) so the endpoint is not
publicly callable; validate/whitelist incoming keys instead of blindly calling
os.environ.update — only accept a strict set of allowed credential keys (e.g.,
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN, AWS_REGION) and
reject/400 any other keys to avoid PATH/LD_PRELOAD/PYTHONPATH tampering; ensure
you do not overwrite existing critical vars unless explicitly intended (reject
or log attempts to replace existing protected names); after updating env vars,
clear both S3FileSystem instance cache (S3FileSystem.clear_instance_cache()) and
any fsspec-level caches created via fsspec.filesystem() so stale S3 instances
are not reused (call the appropriate fsspec cache invalidation function or
recreate filesystem clients).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36e1b4eb-c763-4008-9a74-ad79d3c8de45
📒 Files selected for processing (4)
lightly_studio/src/lightly_studio/api/app.pylightly_studio/src/lightly_studio/api/routes/api/enterprise.pylightly_studio/tests/api/routes/api/test_enterprise.pylightly_studio_view/src/lib/schema.d.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightly_studio/tests/api/routes/api/test_enterprise.py`:
- Around line 28-40: The test test_refresh_cloud_credentials__clears_s3_cache
mutates process environment via the API call and must restore it to avoid
cross-test leakage; update the test to snapshot os.environ (or use pytest's
monkeypatch/environment fixture) before calling test_client.put and restore the
original environment afterwards, ensuring S3FileSystem.clear_instance_cache is
still spied on and the response assertions remain the same so the test isolates
env changes and does not leak to other tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ea7e441-d8b2-4b1b-8912-e761ca744003
📒 Files selected for processing (2)
lightly_studio/src/lightly_studio/api/routes/api/enterprise.pylightly_studio/tests/api/routes/api/test_enterprise.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lightly_studio/src/lightly_studio/api/routes/api/enterprise.py
…-storage-persist-credentials-server-side' into mihnea-lig-9200-enterprise-cloud-storage-persist-credentials-server-side # Conflicts: # lightly_studio/src/lightly_studio/api/routes/api/enterprise.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lightly_studio/tests/api/routes/api/test_enterprise.py (1)
28-40:⚠️ Potential issue | 🟠 MajorIsolate environment mutation in
test_refresh_cloud_credentials__clears_s3_cache.This test still mutates
os.environvia the endpoint call without restoring it, which can make later tests order-dependent.🔧 Suggested fix
def test_refresh_cloud_credentials__clears_s3_cache( test_client: TestClient, mocker: MockerFixture, ) -> None: + mocker.patch.dict(os.environ, clear=False) spy = mocker.spy(S3FileSystem, "clear_instance_cache")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightly_studio/tests/api/routes/api/test_enterprise.py` around lines 28 - 40, The test test_refresh_cloud_credentials__clears_s3_cache mutates the real process environment when calling test_client.put("/api/cloud-credentials") which can leak state to other tests; fix it by adding the pytest monkeypatch fixture to the test signature and before making the request replace os.environ with an isolated copy (e.g. monkeypatch.setattr(os, "environ", dict(os.environ), raising=False)) so the endpoint can mutate the copy and monkeypatch will restore the original environment after the test; keep the existing assertions (spy on S3FileSystem.clear_instance_cache and status_code check) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lightly_studio/tests/api/routes/api/test_enterprise.py`:
- Around line 28-40: The test test_refresh_cloud_credentials__clears_s3_cache
mutates the real process environment when calling
test_client.put("/api/cloud-credentials") which can leak state to other tests;
fix it by adding the pytest monkeypatch fixture to the test signature and before
making the request replace os.environ with an isolated copy (e.g.
monkeypatch.setattr(os, "environ", dict(os.environ), raising=False)) so the
endpoint can mutate the copy and monkeypatch will restore the original
environment after the test; keep the existing assertions (spy on
S3FileSystem.clear_instance_cache and status_code check) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a0490c9-0114-4526-9d31-c352382035e3
📒 Files selected for processing (2)
lightly_studio/src/lightly_studio/api/routes/api/enterprise.pylightly_studio/tests/api/routes/api/test_enterprise.py
✅ Files skipped from review due to trivial changes (1)
- lightly_studio/src/lightly_studio/api/routes/api/enterprise.py
mihnea-necsulescu
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightly_studio/tests/api/routes/api/test_enterprise.py`:
- Around line 50-68: The test leaves S3FileSystem._cache populated which can
leak into other tests; modify the test to clear the global cache after it runs
(e.g., in a finally block or teardown/fixture) by resetting S3FileSystem._cache
to an empty state so subsequent calls to fsspec.filesystem("s3", anon=False)
will not return a stale instance; ensure the cleanup runs regardless of
assertions so the cached instance created by fsspec.filesystem("s3", anon=False)
is always removed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c59a617-f93f-437f-833c-3c9b32c58544
📒 Files selected for processing (1)
lightly_studio/tests/api/routes/api/test_enterprise.py
Contributor
Author
|
/review |
MalteEbner
approved these changes
Apr 7, 2026
…sist-credentials-server-side
mihnea-necsulescu
deleted the
mihnea-lig-9200-enterprise-cloud-storage-persist-credentials-server-side
branch
michal-lightly
pushed a commit
that referenced
this pull request
Apr 9, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What has changed and why?
Added a new
PUT /cloud-credentialsendpoint used to set/refresh the cloud credentials on the server side.How has it been tested?
Manually + new tests
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Tests
Documentation