[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232526633
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1264,6 +1281,14 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 #endif
 
+/* Timezone redirection */
+if (guac_settings->timezone) {
+if(setenv("TZ", guac_settings->timezone, 1)) {
+guac_client_log(client, GUAC_LOG_WARNING,
+"Unable to set TZ variable, error %i", errno);
--- End diff --

Error messages should be structured such that they will be useful to the 
user viewing the logs. In this case:

* "Unable to set TZ variable" will not provide useful information to users 
that aren't familiar with this implementation-specific detail of how guac 
forwards the timezone for consumption by FreeRDP.
* Text like "error 123" is going to cause more confusion than help. You 
_can_ turn this into a human-readable message though. See: 
http://man7.org/linux/man-pages/man3/strerror.3.html

I suggest something like:

```c
guac_client_log(client, GUAC_LOG_WARNING,
"Unable to forward timezone: TZ environment variable "
"could not be set: %s", strerror(errno));
```

or similar. Something which provides context from the perspective of the 
administrator maintaining the connection and reading the logs, plus something 
which provides context for the error that occurred, plus the error itself in 
human-readable form.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232525849
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1264,6 +1281,14 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 #endif
 
+/* Timezone redirection */
+if (guac_settings->timezone) {
+if(setenv("TZ", guac_settings->timezone, 1)) {
--- End diff --

`if` is not a function. ;)


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232507335
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1265,6 +1281,9 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 
 /* Device redirection */
--- End diff --

Fixed.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232507326
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1265,6 +1277,9 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 
 /* Device redirection */
+if (guac_settings->timezone)
+setenv("TZ", guac_settings->timezone, 1);
--- End diff --

Okay, should be good, now.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232506936
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1265,6 +1277,9 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 
 /* Device redirection */
+if (guac_settings->timezone)
+setenv("TZ", guac_settings->timezone, 1);
--- End diff --

A pointer to it, yeah.


---

[GitHub] guacamole-server pull request #201: GUACAMOLE-547: Add support for SSH NONE ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/201#discussion_r232504541
  
--- Diff: src/common-ssh/ssh.c ---
@@ -321,6 +321,10 @@ static int 
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
 guac_client_log(client, GUAC_LOG_DEBUG,
 "Supported authentication methods: %s", user_authlist);
 
+/* If auth list is NULL, then authentication has succeeded with NONE */
+if (user_authlist == NULL)
--- End diff --

Relocated and logged the NONE success.


---

[GitHub] guacamole-server pull request #201: GUACAMOLE-547: Add support for SSH NONE ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/201#discussion_r232504438
  
--- Diff: src/common-ssh/ssh.c ---
@@ -321,6 +321,10 @@ static int 
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
 guac_client_log(client, GUAC_LOG_DEBUG,
 "Supported authentication methods: %s", user_authlist);
 
+/* If auth list is NULL, then authentication has succeeded with NONE */
+if (user_authlist == NULL)
--- End diff --

Good point.  Perhaps that NULL check should be located elsewhere...


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232504386
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -356,6 +357,11 @@ enum RDP_ARGS_IDX {
  */
 IDX_PRECONNECTION_BLOB,
 
+/**
+ * The timezone to pass through to the RDP connection.
--- End diff --

Added reference to Wikipedia page.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232504348
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1265,6 +1277,9 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 
 /* Device redirection */
+if (guac_settings->timezone)
+setenv("TZ", guac_settings->timezone, 1);
--- End diff --

It appears that, in order to do this, I would need to pass the entire 
`client` object through to this `guac_rdp_push_settings()` function?


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232504354
  
--- Diff: src/protocols/ssh/ssh.c ---
@@ -256,6 +256,10 @@ void* ssh_client_thread(void* data) {
 return NULL;
 }
 
+/* Set the client timezone */
+if (settings->timezone != NULL)
+libssh2_channel_setenv(ssh_client->term_channel, "TZ", 
settings->timezone);
--- End diff --

Done.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232504364
  
--- Diff: src/protocols/ssh/settings.h ---
@@ -253,6 +253,10 @@ typedef struct guac_ssh_settings {
  * environment variable.
  */
 char* locale;
+/** 
--- End diff --

Added.


---

[GitHub] guacamole-server pull request #197: GUACAMOLE-287: Added Constants in place ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/197#discussion_r232502934
  
--- Diff: src/terminal/terminal/ansi_escape_codes.h ---
@@ -0,0 +1,279 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+
+#ifndef _GUAC_TERMINAL_ANSI_ESCAPE_CODES_H
+#define _GUAC_TERMINAL_ANSI_ESCAPE_CODES_H
+
+
+/**
+ * The maximum number of parameters the CSI can have is 16.
+ */
+#define GUAC_TERMINAL_MAX_ARGUMENTS 16
+
+/**
+ * When the parameter '2' is preceded by the escape character  
 
+ * with '[' and followed by the the character 'J' the entire
+ * display is cleared. Without the '2' the line is deleted
+ * only from the cursor to the end of the display. 
+ */
+#define GUAC_TERMINAL_ERASE_DISPLAY 2
--- End diff --

Similar to the documentation for this macro, it may be worth considering 
embedding part of the context of this constant in the constant name. For 
example, if this value only has the defined meaning within a CSI sequence, then 
`GUAC_TERMINAL_CSI_ERASE_DISPLAY` might be a more sensible name. If that `J` 
you mention is yet another specific type of CSI sequence, then 
`GUAC_TERMINAL_CSI_WHATEVER_J_IS_ERASE_DISPLAY` would make yet more sense.


---

[GitHub] guacamole-server pull request #197: GUACAMOLE-287: Added Constants in place ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/197#discussion_r232502752
  
--- Diff: src/terminal/terminal/ansi_escape_codes.h ---
@@ -0,0 +1,279 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+
+#ifndef _GUAC_TERMINAL_ANSI_ESCAPE_CODES_H
--- End diff --

As mentioned previously, macros should not start with leading underscores. 
You will see this in older code, yes, but it is incorrect. Please do not use 
leading underscores in new code.

See: 
https://github.com/apache/guacamole-server/pull/197#discussion_r228667853 and 
https://github.com/apache/guacamole-server/pull/197#discussion_r229821057


---

[GitHub] guacamole-server pull request #197: GUACAMOLE-287: Added Constants in place ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/197#discussion_r232503178
  
--- Diff: src/terminal/terminal/ansi_escape_codes.h ---
@@ -0,0 +1,279 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+
+#ifndef _GUAC_TERMINAL_ANSI_ESCAPE_CODES_H
+#define _GUAC_TERMINAL_ANSI_ESCAPE_CODES_H
+
+
+/**
+ * The maximum number of parameters the CSI can have is 16.
+ */
+#define GUAC_TERMINAL_MAX_ARGUMENTS 16
+
+/**
+ * When the parameter '2' is preceded by the escape character  
 
+ * with '[' and followed by the the character 'J' the entire
--- End diff --

If the idea behind this change is to avoid the use of magic numbers, then 
the documentation shouldn't rely on magic numbers either. Ignoring that there 
are other ways to start a CSI sequence besides "`ESC` `[`", the `[` and the `J` 
have their own meanings in this context. There must be a better way to describe 
the use of this constant.


---

[GitHub] guacamole-server pull request #201: GUACAMOLE-547: Add support for SSH NONE ...

[necouchman]

GitHub user necouchman opened a pull request:

https://github.com/apache/guacamole-server/pull/201

GUACAMOLE-547: Add support for SSH NONE authentication method

I'm not 100% certain that this is all that's needed, because I don't have a 
SSH server to test this against, but, according to the libssh2 documentation, 
this may be all that's required to allow the session to continue when there's 
no authentication required.

https://libssh2.org/libssh2_userauth_list.html

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/necouchman/guacamole-server jira/547

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/guacamole-server/pull/201.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #201


commit 469defd983df7a64a2825a961f004ae8122cdf56
Author: Nick Couchman 
Date:   2018-11-11T20:16:22Z

GUACAMOLE-547: Add support for SSH NONE authentication method.




---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232502604
  
--- Diff: src/protocols/ssh/settings.c ---
@@ -246,6 +247,15 @@ enum SSH_ARGS_IDX {
  * variable to be set.
  */
 IDX_LOCALE,
+ 
+/**
+ * The timezone that is passed from the client system to the
+ * remote server, or null if not specified.  If set, and allowed
--- End diff --

"... or null if not specified" is good to know, but not actually applicable 
to the connection parameter. This is an implementation detail which goes along 
with the internal representation within the settings struct. The documentation 
of the connection parameter should be from the perspective of a user 
considering providing a value for that parameter.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232502635
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -356,6 +357,11 @@ enum RDP_ARGS_IDX {
  */
 IDX_PRECONNECTION_BLOB,
 
+/**
+ * The timezone to pass through to the RDP connection.
--- End diff --

What format are these timezones in? Is there some reference users can 
consult to determine what legal values are available?


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232502486
  
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -1265,6 +1277,9 @@ void guac_rdp_push_settings(guac_rdp_settings* 
guac_settings, freerdp* rdp) {
 #endif
 
 /* Device redirection */
+if (guac_settings->timezone)
+setenv("TZ", guac_settings->timezone, 1);
--- End diff --

A warning should be logged if this cannot be set, since an expectation of 
the behavior of the connection is going unfulfilled.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232502539
  
--- Diff: src/protocols/ssh/settings.h ---
@@ -253,6 +253,10 @@ typedef struct guac_ssh_settings {
  * environment variable.
  */
 char* locale;
+/** 
--- End diff --

Missing a blank line separating the block comment and the code above.


---

[GitHub] guacamole-server pull request #169: GUACAMOLE-422: Add support for timezone ...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/169#discussion_r232502496
  
--- Diff: src/protocols/ssh/ssh.c ---
@@ -256,6 +256,10 @@ void* ssh_client_thread(void* data) {
 return NULL;
 }
 
+/* Set the client timezone */
+if (settings->timezone != NULL)
+libssh2_channel_setenv(ssh_client->term_channel, "TZ", 
settings->timezone);
--- End diff --

Same here - a warning should be logged if this cannot be set.


---

Jenkins build is back to normal : guacamole-server-freerdp » 1.0.1,ubuntu #21

[Apache Jenkins Server]

See 

[GitHub] guacamole-server pull request #200: GUACAMOLE-630: Allow font parameters of ...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/guacamole-server/pull/200


---

[GitHub] guacamole-server pull request #200: GUACAMOLE-630: Allow font parameters of ...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/200#discussion_r232485885
  
--- Diff: src/protocols/ssh/argv.c ---
@@ -81,16 +110,51 @@ static int guac_ssh_argv_blob_handler(guac_user* user,
 static int guac_ssh_argv_end_handler(guac_user* user,
 guac_stream* stream) {
 
+int size;
+
 guac_client* client = user->client;
-guac_ssh_client* telnet_client = (guac_ssh_client*) client->data;
-guac_terminal* terminal = telnet_client->term;
--- End diff --

Wow, not sure how I missed that last time around!


---