Fix daemon deauthorize and cancel transfer requests to match Wings API - #2576
lancepioch wants to merge 1 commit into
Conversation
RevokeSftpAccessJob always failed on the node path because deauthorize() read the uninitialized $server property, so SFTP and websocket sessions were never revoked on password change or user deletion (#2566). Both deauthorize() and cancelTransfer() also wrapped their payloads in a 'json' key that Laravel's HTTP client nests into the request body, so Wings bound an empty payload, and the destination cancel used the stale DELETE /api/transfer route that current Wings no longer registers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe repository now uses the daemon’s transfer-specific cancellation endpoint and sends flattened deauthorization fields. Integration tests verify deauthorization for nodes and servers, plus transfer cancellation across old and new nodes. ChangesDaemon request corrections
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The corrected daemon requests match supported Wings transfer routes, with no remaining concrete merge risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The linked issue is
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 |
1 participant
Fixes #2566.
RevokeSftpAccessJobalways failed when dispatched with a node becausedeauthorize()read the uninitialized$servertyped property, so the job died before sending anything and open SFTP/websocket sessions were never revoked on password change or user deletion. The check is nowisset($this->server), which is the only line that treats the property as nullable; the other methods that read$this->serverdirectly are unchanged so an unset server still fails loudly.While writing tests against the real payload it turned out both
deauthorize()andcancelTransfer()wrapped their bodies in ajsonkey, which Laravel's HTTP client nests into the request body, so Wings bound an empty struct. The payloads are now flat, and the destination-node cancel now callsDELETE /api/transfers/{uuid}since the oldDELETE /api/transferroute no longer exists in Wings and returned 404.The existing job tests mocked
DaemonServerRepository, which is why none of this was caught; the new tests run the real repository againstHttp::fake()and assert the wire format Wings binds (postDeauthorizeUseranddeleteTransferinrouter_system.go/router_transfer.go).Verified against a real Wings 1.0.0-beta29 instance registered to a local panel: node-path and server-path deauthorize both return 204 (the node path previously threw before any request), and the source-node cancel returns the expected 409 when no transfer is active. Note the destination cancel currently 500s on the Wings side regardless of caller:
protected.DELETE("/api/transfers/:server", ...)is registered withoutmiddleware.ServerExists()but the handler callsExtractServer, so it panics; that needs a one-line middleware fix in Wings.