-
Notifications
You must be signed in to change notification settings - Fork 715
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
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
8180dc0 to
06bb2f7
Compare
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