feat(rdpdr): Notify RdpdrBackend of 'User Logged On' Messages#1211
feat(rdpdr): Notify RdpdrBackend of 'User Logged On' Messages#1211rhammonds-teleport wants to merge 3 commits intoDevolutions:masterfrom
Conversation
…ves the backend an opportunity to modify the device list with the context that logon is complete. Additionally, do not throw a fatal error when handling an IO request for an unkown device identifier. Per the spec, these SHOULD be ignored.
rhammonds-teleport
force-pushed
the
rhammonds/rdpdr-handle-user-logged-on
branch
from
32a3c7f to
b86822b
Compare
… reference to the base Rdpdr instance so that it may call add/remove methods that construct the appropriate device announce/remove messages.
rhammonds-teleport
force-pushed
the
rhammonds/rdpdr-handle-user-logged-on
branch
from
b86822b to
88693d6
Compare
There was a problem hiding this comment.
Pull request overview
Adds a User Logged On hook to the RDPDR backend so consumers can react to DR_CORE_USER_LOGGEDON (e.g., re-announce/remove devices at the spec-recommended time), and adjusts device I/O handling to ignore unknown device IDs per MS-RDPEFS.
Changes:
- Add
handle_user_logged_on(&mut self, rdpdr: &mut Rdpdr)toRdpdrBackendand invoke it onRdpdrPdu::UserLoggedon. - Change
Rdpdrto store the backend as anOption<Box<dyn RdpdrBackend>>to allow temporarily moving it while calling the new hook. - Update device I/O request handling to ignore requests for unknown/removed device IDs instead of treating them as fatal.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ironrdp-rdpdr/src/lib.rs | Dispatch UserLoggedon to backend hook; ignore unknown DeviceId I/O requests; make backend optional for temporary move-out. |
| crates/ironrdp-rdpdr/src/backend/mod.rs | Extend RdpdrBackend trait with handle_user_logged_on. |
| crates/ironrdp-rdpdr/src/backend/noop.rs | Implement new trait method for the noop backend. |
| crates/ironrdp-rdpdr-native/src/nix/backend.rs | Implement new trait method for the nix backend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut backend = self.backend.take().expect("missing rdpdr backend"); | ||
| let res = backend.handle_user_logged_on(self)?; | ||
| self.backend = Some(backend); |
There was a problem hiding this comment.
handle_user_logged_on temporarily take()s the backend, but if backend.handle_user_logged_on(self) returns Err, the ? early-returns before self.backend is restored. That leaves self.backend = None permanently and can cause later processing to fail/panic. Restore the backend before propagating the error (e.g., capture the result first, put the backend back, then ?), and avoid expect() panics by returning a PduError like the other handlers.
| let mut backend = self.backend.take().expect("missing rdpdr backend"); | |
| let res = backend.handle_user_logged_on(self)?; | |
| self.backend = Some(backend); | |
| let mut backend = self | |
| .backend | |
| .take() | |
| .ok_or_else(|| pdu_other_err!("missing rdpdr backend"))?; | |
| let res = backend.handle_user_logged_on(self); | |
| self.backend = Some(backend); | |
| let res = res?; |
There was a problem hiding this comment.
Fixed in latest commit!
| fn handle_server_device_announce_response(&mut self, pdu: ServerDeviceAnnounceResponse) -> PduResult<()>; | ||
| fn handle_scard_call(&mut self, req: DeviceControlRequest<ScardIoCtlCode>, call: ScardCall) -> PduResult<()>; | ||
| fn handle_drive_io_request(&mut self, req: ServerDriveIoRequest) -> PduResult<Vec<SvcMessage>>; | ||
| fn handle_user_logged_on(&mut self, rdpdr: &mut Rdpdr) -> PduResult<Vec<SvcMessage>>; |
There was a problem hiding this comment.
Adding a new required method to the public RdpdrBackend trait is a breaking change for downstream crates implementing the trait. Consider providing a default implementation for handle_user_logged_on (e.g., returning Ok(Vec::new())) so existing external backends keep compiling unless they need custom behavior.
| fn handle_user_logged_on(&mut self, rdpdr: &mut Rdpdr) -> PduResult<Vec<SvcMessage>>; | |
| fn handle_user_logged_on(&mut self, _rdpdr: &mut Rdpdr) -> PduResult<Vec<SvcMessage>> { | |
| Ok(Vec::new()) | |
| } |
There was a problem hiding this comment.
Breaking changes are fine for now, although it may make sense to have a default implementation nonetheless, if that’s not something that is necessary for all backends.
There was a problem hiding this comment.
I went ahead and added a default implementation. No good reason not to.
…d after take. Add default implementation of 'handle_user_logged_on' to avoid breaking existing implementations.
|
Hi Benoît Cortier (@CBenoit), do you have any other feedback or concerns with this PR? |
At Teleport, we've noticed a regression with filesystem device redirection with newer versions of Windows (Windows Server 2025 in particular) where our device list announcements are ignored by the server. I wasn't able to find any case where Teleport and/or IronRDP are violating spec, but after a lot of experimentation I was able to find two workarounds:
Client ID Confirmmessage from the server.User Logged Onmessage by removing our smart card device. This seems to trigger some state change on the server where it will now acknowledge subsequent device announce/remove messages.The first isn't a great option for us, since we've always allowed ad-hoc directory sharing mid-session. We're also in the midst of adding support for sharing multiple directories as well as shared directory removal without restarting the session.
This PR adds a new method to the
RdpdrBackendtrait that is called when aDR_CORE_USER_LOGGEDONmessage is received. According to MS-RDPEFS, this is an appropriate time to announce additional devices. This also provides an ergonomic way for the Teleport backend implementation to remove its smart card device after log on.It also includes a fix for
handle_device_io_request, which previously returned a fatal error when it encounters an unknown device ID. According to the spec these should be ignored.