[Feat] Storage Providers: Edit the connection - #1224
Merged
RichardAnderson merged 5 commits intoAug 2, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughStorage providers now support dynamic edit fields, partial credential updates, connection re-verification, cache invalidation, and authorised non-secret data exposure. The frontend renders provider-specific edit forms and validation errors. ChangesStorage provider editing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EditDialog
participant EditStorageProvider
participant ProviderHandler
participant StorageProvider
User->>EditDialog: Submit provider fields
EditDialog->>EditStorageProvider: Send edited credentials
EditStorageProvider->>StorageProvider: Validate and merge credentials
StorageProvider-->>EditStorageProvider: Return credentials and reconnect flag
EditStorageProvider->>ProviderHandler: Verify connection with credentials
ProviderHandler-->>EditStorageProvider: Return connection result
EditStorageProvider-->>EditDialog: Return updated provider or validation error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/StorageProviders/FTP.php (1)
89-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the FTP connection even when login fails.
If
FTP::connect()succeeds butFTP::login()fails,$isConnectedisfalse, andFTP::close($connection)is skipped. The open connection resource is never closed. Sinceconnect()now runs on every edit verification in addition to creation, this leak occurs more often. Close the connection whenever$connectionis truthy, not only when$isConnectedis true.🔧 Proposed fix
public function connect(array $credentials): bool { $connection = $this->connection($credentials); $isConnected = $connection && $this->login($connection, $credentials); - if ($isConnected) { + if ($connection) { \App\Facades\FTP::close($connection); } return $isConnected; }🤖 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 `@app/StorageProviders/FTP.php` around lines 89 - 100, Update StorageProvider’s connect method to close the FTP connection whenever the connection resource is truthy, regardless of whether login succeeds. Keep returning the existing $isConnected result and preserve the current connection and login flow.public/api-docs/openapi/user-storage-providers.yaml (1)
171-182: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMark
nameas required in the update request schema.
EditStorageProvider::edit()merges the rule['name' => ['required']]and then reads$input['name']unconditionally. A request withoutnamereturns 422. The deprecated project-scoped endpoint documentsrequired: [name], but this schema does not. Add therequiredlist so the two documents agree with the backend.📘 Proposed fix
schema: type: object + required: + - name properties: name: type: stringAs per path instructions: "Keep OpenAPI schemas in sync with API Resources and backend enums."
🤖 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 `@public/api-docs/openapi/user-storage-providers.yaml` around lines 171 - 182, Update the request schema containing the name and global properties to declare name in its required list, matching EditStorageProvider::edit() validation and the deprecated project-scoped endpoint documentation; leave the existing property definitions unchanged.Source: Path instructions
🤖 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 `@app/Actions/StorageProvider/EditStorageProvider.php`:
- Around line 44-49: Add a no-op forgetCachedState(): void hook to
StorageProvider and AbstractStorageProvider, override it in Dropbox to call
forgetAccessToken(), and invoke the hook unconditionally in the edit action
before save so changed credentials invalidate the cached token without
provider-specific logic.
In `@app/Http/Resources/DNSProviderResource.php`:
- Line 26: Update DNSProviderResource’s editable_data construction to check
whether the configured provider handler is available before invoking
provider()->editableData(), using the model-level handler check already
established for storage providers. When the handler is unavailable, return an
empty object; otherwise preserve the existing revealCredentials authorization
and editable-data behavior.
In `@app/Models/StorageProvider.php`:
- Around line 56-61: Update hasProviderHandler() to require the configured
handler to implement or extend App\StorageProviders\StorageProvider by combining
the existing string validation with is_a(..., true), rather than relying only on
class_exists(). Preserve the false result for missing or unrelated handler
classes.
In `@app/Policies/DNSProviderPolicy.php`:
- Around line 48-57: Update app/Policies/DNSProviderPolicy.php lines 48-57 and
app/Policies/StorageProviderPolicy.php lines 39-48 to use the HasRolePolicies
trait, and replace inline token or owner checks in update() and
revealCredentials() with the applicable hasWriteAccess() check against the
provider’s project. Ensure both policies consistently use the project-bound role
policy model.
In `@composer.json.bak`:
- Around line 1-131: Delete composer.json.bak and composer.lock.bak from the
repository, and add *.bak to .gitignore to prevent future Composer backup
artefacts; make no changes to the real Composer manifests unless an intentional
lock update is required.
In `@composer.lock.bak`:
- Around line 11122-11132: Regenerate composer.lock and composer.lock.bak using
Composer so the phpstan/phpstan 2.2.2 entry contains the complete package
metadata, including its source object. Do not manually remove or add fields;
ensure both lock files are synchronized and no longer contain the malformed
entry.
---
Outside diff comments:
In `@app/StorageProviders/FTP.php`:
- Around line 89-100: Update StorageProvider’s connect method to close the FTP
connection whenever the connection resource is truthy, regardless of whether
login succeeds. Keep returning the existing $isConnected result and preserve the
current connection and login flow.
In `@public/api-docs/openapi/user-storage-providers.yaml`:
- Around line 171-182: Update the request schema containing the name and global
properties to declare name in its required list, matching
EditStorageProvider::edit() validation and the deprecated project-scoped
endpoint documentation; leave the existing property definitions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db16db45-37b7-44da-9fc1-5babdf0b1c87
📒 Files selected for processing (32)
app/Actions/StorageProvider/CreateStorageProvider.phpapp/Actions/StorageProvider/EditStorageProvider.phpapp/Http/Resources/DNSProviderResource.phpapp/Http/Resources/StorageProviderResource.phpapp/Models/StorageProvider.phpapp/Plugins/RegisterStorageProvider.phpapp/Policies/DNSProviderPolicy.phpapp/Policies/SitePolicy.phpapp/Policies/StorageProviderPolicy.phpapp/StorageProviders/AbstractStorageProvider.phpapp/StorageProviders/Dropbox.phpapp/StorageProviders/FTP.phpapp/StorageProviders/Local.phpapp/StorageProviders/S3.phpapp/StorageProviders/SFTP.phpapp/StorageProviders/StorageProvider.phpapp/Tables/StorageProviderTable.phpcomposer.json.bakcomposer.lock.bakpublic/api-docs/openapi/schemas/DNSProvider.yamlpublic/api-docs/openapi/schemas/StorageProvider.yamlpublic/api-docs/openapi/storage-providers.yamlpublic/api-docs/openapi/user-storage-providers.yamlresources/js/components/ui/dynamic-field.tsxresources/js/pages/storage-providers/components/edit-dialog.tsxresources/js/pages/storage-providers/index.tsxresources/js/types/dynamic-field-config.d.tsresources/js/types/index.d.tsresources/js/types/storage-provider.d.tstests/Feature/API/StorageProvidersTest.phptests/Feature/StorageProvidersTest.phptests/Unit/StorageProviders/S3Test.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/StorageProviders/Dropbox.php (1)
67-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd Dropbox edit-field definitions.
Dropboxinherits empty edit-field definitions. Therefore,mergeEditData()ignoresapp_key,app_secret, andrefresh_token. The Action does not reconnect or save submitted Dropbox credentials.Define the dynamic fields and classify
app_keyas editable. Classifyapp_secretandrefresh_tokenas secret fields.🤖 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 `@app/StorageProviders/Dropbox.php` around lines 67 - 69, Update the Dropbox provider class to define its dynamic edit fields, marking app_key as editable and app_secret and refresh_token as secret fields, so mergeEditData() preserves and processes submitted credentials. Use the existing edit-field definition structure and classification symbols used by other storage providers.app/Actions/StorageProvider/EditStorageProvider.php (1)
63-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not convert unexpected provider failures into validation errors.
catch (Throwable $e)also catchesErrorandTypeError. Theverify()method then reports provider defects as invalid credentials instead of letting them reach error handling and monitoring. Catch only expected connection exceptions and rethrow unexpected throwables.🤖 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 `@app/Actions/StorageProvider/EditStorageProvider.php` around lines 63 - 72, Update the exception handling around provider->connect in verify() to catch only the expected connection exception type, allowing Error, TypeError, and other unexpected throwables to propagate to error handling and monitoring. Preserve the existing logging and connected = false behavior for expected connection failures.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@app/Actions/StorageProvider/EditStorageProvider.php`:
- Around line 63-72: Update the exception handling around provider->connect in
verify() to catch only the expected connection exception type, allowing Error,
TypeError, and other unexpected throwables to propagate to error handling and
monitoring. Preserve the existing logging and connected = false behavior for
expected connection failures.
In `@app/StorageProviders/Dropbox.php`:
- Around line 67-69: Update the Dropbox provider class to define its dynamic
edit fields, marking app_key as editable and app_secret and refresh_token as
secret fields, so mergeEditData() preserves and processes submitted credentials.
Use the existing edit-field definition structure and classification symbols used
by other storage providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 857526eb-d6d3-4cca-95a8-e2bdc0942d76
📒 Files selected for processing (10)
app/Actions/StorageProvider/EditStorageProvider.phpapp/Http/Resources/DNSProviderResource.phpapp/Models/DNSProvider.phpapp/Models/StorageProvider.phpapp/StorageProviders/AbstractStorageProvider.phpapp/StorageProviders/Dropbox.phpapp/StorageProviders/FTP.phpapp/StorageProviders/StorageProvider.phppublic/api-docs/openapi/user-storage-providers.yamltests/Feature/StorageProvidersTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/Feature/StorageProvidersTest.php`:
- Around line 559-562: Add an Http::assertSent() check in the test around the
Dropbox connect() flow to verify a POST request to
https://api.dropboxapi.com/2/check/user includes the fresh-access token returned
by the fake OAuth response, placing the assertion before the provider refresh
operation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 855fa8a1-1a03-4c79-b3b5-4d46eed90017
📒 Files selected for processing (2)
app/StorageProviders/Dropbox.phptests/Feature/StorageProvidersTest.php
saeedvaziry
approved these changes
Aug 2, 2026
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.
Storage providers can now have their connection details edited in place - each provider class declares its own edit fields, non-secret values are prefilled while secrets stay blank and are only written when replaced, and changed credentials are re-verified against the provider before being saved.
Summary by CodeRabbit
New Features
Bug Fixes
false,0, and empty strings.Documentation