Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
necouchman commented on PR #530: URL: https://github.com/apache/guacamole-server/pull/530#issuecomment-3797168180 @lukasraska Any progress on this - renaming the parameter, and also @aleitner's comments? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
necouchman commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2602373910
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
Review Comment:
I'm fine with either of those options - they tend to line up with the
existing ones (e.g. `recording-include-keys`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
aleitner commented on PR #530: URL: https://github.com/apache/guacamole-server/pull/530#issuecomment-3629527559 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 `guac_rdp_clipboard_handler`, `guac_rdp_clipboard_blob_handler`, and `guac_rdp_clipboard_end_handler` flow), it would be valuable to also record clipboard data received from the RDP server by adding a call within `guac_rdp_cliprdr_format_data_response` where we call `guac_common_clipboard_send`. 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 `guac_rdp_cliprdr_format_data_response` would capture both directions, providing a complete picture of clipboard activity during the session. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
lukasraska commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2388156884
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
Yeah, tried to be overly consistent with the wording for
"recording-include-keys" and well, got too similar to it. Fixed in all
occurences.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
lukasraska commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2388191792
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
lukasraska commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2388389927
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
necouchman commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2388224112
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-1969: Implement recording-include-clipboard connection parameter [guacamole-server]
necouchman commented on code in PR #530:
URL: https://github.com/apache/guacamole-server/pull/530#discussion_r2384254145
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
Review Comment:
Is it only "paste" data, or would it also include copy events?
##
src/protocols/kubernetes/settings.h:
##
@@ -240,6 +240,16 @@ typedef struct guac_kubernetes_settings {
*/
bool recording_include_keys;
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
`clipboard paste data` or just `clipboard data`?
`key events" -> `Clipboard events`?
##
src/protocols/rdp/settings.c:
##
@@ -560,6 +561,16 @@ enum RDP_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And, again, here - with `paste data` and `Key events`...
##
src/protocols/vnc/settings.h:
##
@@ -285,6 +285,16 @@ typedef struct guac_vnc_settings {
*/
bool recording_include_keys;
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And here.
##
src/protocols/telnet/settings.h:
##
@@ -245,6 +245,16 @@ typedef struct guac_telnet_settings {
*/
bool recording_include_keys;
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And here.
##
src/protocols/kubernetes/settings.c:
##
@@ -212,6 +213,16 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
Maybe some copy-pasta, here - `Key events` -> `Clipboard events`?
##
src/protocols/telnet/settings.c:
##
@@ -195,6 +196,16 @@ enum TELNET_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And another one.
##
src/protocols/ssh/settings.c:
##
@@ -243,6 +244,16 @@ enum SSH_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And here.
##
src/protocols/ssh/settings.h:
##
@@ -257,6 +257,16 @@ typedef struct guac_ssh_settings {
*/
bool recording_include_keys;
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
Another instance.
##
src/protocols/rdp/settings.h:
##
@@ -559,6 +559,16 @@ typedef struct guac_rdp_settings {
*/
int recording_include_keys;
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
And here.
##
src/protocols/vnc/settings.c:
##
@@ -329,6 +330,16 @@ enum VNC_ARGS_IDX {
*/
IDX_RECORDING_INCLUDE_KEYS,
+/**
+ * Whether clipboard paste data should be included in the session
recording.
+ * Key events are NOT included by default within the recording,
Review Comment:
Here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
