fix(server): Persist auto increase storage disable on DB servers#6691
Conversation
`configure_auto_add_storage` is always dispatched on the app server from the dashboard ($appServer), with the real target passed as the `server` argument. The enable branch routed to the right doc via `server`, but the disable branch hardcoded `self`, so disabling on a DB/replication server cleared the flag on the app server instead. The target server kept `auto_increase_storage = True` and the checkbox reverted on reload. Resolve the target doc once and use it in both branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Confidence Score: 5/5Safe to merge — the targeted bug is correctly fixed, both branches now write to the right document, and a permission guard is in place. The change is a minimal, well-scoped refactor of a single method. No files require special attention;
|
| Filename | Overview |
|---|---|
| press/press/doctype/server/server.py | Resolves server_doc once at the top of configure_auto_add_storage and adds check_permission("write") before both the enable and disable branches — correctly fixing the bug where the disable path wrote to self instead of the resolved target. |
| press/press/doctype/server/test_server.py | Adds three focused tests: the original bug (disable on DB server), the happy path (disable on app server), and a cross-team permission guard. Only the enabled=False path is covered in the security test. |
Sequence Diagram
sequenceDiagram
participant Dashboard
participant AppServer as App Server (self)
participant DBServer as Database Server (server_doc)
Dashboard->>AppServer: "configure_auto_add_storage(server=db_server, enabled)"
AppServer->>AppServer: resolve server_doc
AppServer->>DBServer: check_permission("write")
alt "enabled=False"
AppServer->>DBServer: frappe.db.set_value(..., "auto_increase_storage", False)
else "enabled=True"
AppServer->>DBServer: "server_doc.auto_increase_storage = True"
AppServer->>DBServer: server_doc.save()
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
press/press/doctype/server/test_server.py:408
**Local import inside test body**
`create_test_user` is imported inside the method rather than at the module level. If the import raises (e.g., `ui_test_helpers` is unavailable in the test environment), the failure happens inside the test rather than at collection time, making it harder to diagnose. Move it to the top-level imports alongside `create_test_team`.
### Issue 2 of 2
press/press/doctype/server/test_server.py:406-435
**Cross-team `enabled=True` path is not exercised**
The security test only calls `configure_auto_add_storage(..., enabled=False)`. The `enabled=True` path now also goes through `server_doc.check_permission("write")`, but there's no corresponding test asserting that an attacker can't enable auto-storage on another team's Database Server. The enable path calls `server_doc.save()` (which runs permission hooks), so the risk is lower, but a parallel test case would close the gap and guard against future refactors that replace `save()` with `set_value()`.
Reviews (3): Last reviewed commit: "fix(server): Authorize target before tog..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6691 +/- ##
===========================================
+ Coverage 50.61% 56.70% +6.09%
===========================================
Files 993 993
Lines 83653 83690 +37
Branches 526 678 +152
===========================================
+ Hits 42343 47460 +5117
+ Misses 41278 36195 -5083
- Partials 32 35 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`configure_auto_add_storage` resolves the target via `frappe.get_doc` from
the attacker-controllable `server` argument, but the dashboard API only
team-checks `self` (the app server). The disable path writes via
`frappe.db.set_value`, which skips permission hooks, so any Press User could
flip `auto_increase_storage` off on another team's Database Server by passing
its name. (The enable path was already covered by `save()`'s write hook.)
Add an explicit `server_doc.check_permission("write")` after resolving the
target — guards both branches without giving up `set_value`'s cheap,
side-effect-free single-field write on the disable path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@greptileai rereview |
|
@Mergifyio backport master |
✅ Backports have been createdDetails
|
fix(server): Persist auto increase storage disable on DB servers (backport #6691)
configure_auto_add_storageis always dispatched on the app server from the dashboard ($appServer), with the real target passed as theserverargument. The enable branch routed to the right doc viaserver, but the disable branch hardcodedself, so disabling on a DB/replication server cleared the flag on the app server instead. The target server keptauto_increase_storage = Trueand the checkbox reverted on reload.Resolve the target doc once and use it in both branches.