-
Notifications
You must be signed in to change notification settings - Fork 731
GUACAMOLE-1969: Implement recording-include-clipboard connection parameter #530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
necouchman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A handful of places where I think some copy-pasta slipped in and got replicated across all of the files you modified. Other than that, I think this looks good.
| IDX_RECORDING_INCLUDE_KEYS, | ||
|
|
||
| /** | ||
| * Whether clipboard paste data should be included in the session recording. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it only "paste" data, or would it also include copy events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, it only reports paste data -> those within the user-originating clipboard blobs. I aimed this to give better insight into what is the actual data transfered in, since currently if somebody pastes some command via clipboard, it's not really visible in the recordings (merely the UI changes are rendered, whatever happens based on that).
If also server-originating clipboard changes should be logged, from brief check it should probably be easy to hook it up to __send_user_clipboard (or probably the respective method that calls this for each connected user), but that recording could get quite big in such cases (so if this would be desired, I would probably aim for two separate args, like with the disable-copy/paste options, so users can choose which one to log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I wonder if it would be worth changing the overall parameter name to better reflect what we're capturing, in case we do want to add something different in the future. Your point about the size of the recording capturing certain clipboard data is well-taken, and I'm not sure it makes sense to add that (now), I just think we want to be very clear about what data we're capturing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, what would you suggest as an appropriate parameter name? recording-include-clipboard-paste / recording-include-user-clipboard or something else? I'm not sure how long these args can be in practice and I would like to avoid overly long parameter names, but then it should be fairly descriptive as you mention. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either of those options - they tend to line up with the existing ones (e.g. recording-include-keys).
8180dc0 to
06bb2f7
Compare
|
Hi, thanks for putting together this PR! I've been working on a similar implementation but when I went to see if an issue for clipboard recordings already existed I found this PR. I'd love to merge your PR instead as it's already looking great. I just have one change I'd suggest: in addition to recording clipboard data sent to the RDP server (via the Currently, this PR captures clipboard content when a user pastes into the remote session (client → server), but it doesn't capture when the clipboard is modified on the remote desktop itself (server → client). Adding recording support in |
This PR introduces new
recording-include-clipboardinto all supported protocols that dumps theclipboardinstruction, along with the relevant stream blobs, into recording.Few key notes:
recording-include-keysand administrators need to explicitly set the parameter totrueguac_recording_createincludes the new parameter (i've opted for making it the last argument, but let me know if I should switch it to be next to theinclude_keys)Once this PR is approved from technical perspective, I will create respective PRs for guacamole-client and guacamole-manual to accomodate for both documentation change as well the client side option