Skip to content

Include an optional screen size in the WindowsDesktop spec#34580

Merged
zmb3 merged 1 commit intomasterfrom
zmb3/desktop-specify-screen-size
Feb 15, 2024
Merged

Include an optional screen size in the WindowsDesktop spec#34580
zmb3 merged 1 commit intomasterfrom
zmb3/desktop-specify-screen-size

Conversation

@zmb3
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 commented Nov 14, 2023

By default this should remain unspecified, which preserves the behavior we have today (Teleport uses the size of the user's browser window).

These new fields allow cluster administrators to configure desktops in such a way that they always use a particular screen size, ignoring the size passed from the browser client. This is necessary to accommodate some applications which require a very specific screen resolution in order to render correctly.

changelog: Desktops can now be configured to use the same screen resolution for all sessions.

@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch from dd62feb to ad0c126 Compare November 15, 2023 11:06
@zmb3
Copy link
Copy Markdown
Collaborator Author

zmb3 commented Nov 15, 2023

Not the prettiest - but seems to work as you'd expect:

image

@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch 2 times, most recently from ca1399c to 00fa79b Compare November 15, 2023 13:05
@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch 2 times, most recently from 8fd9ffc to b144d30 Compare February 9, 2024 21:04
@zmb3 zmb3 marked this pull request as ready for review February 9, 2024 21:05
@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch from b144d30 to 2da9d6d Compare February 9, 2024 21:05
zmb3 added a commit that referenced this pull request Feb 9, 2024
@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch from 2da9d6d to b76dfdd Compare February 9, 2024 21:20
Comment thread lib/srv/desktop/windows_server.go Outdated
Copy link
Copy Markdown
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ultimate screen size is negotiated and then returned to us in handleRDPConnectionInitialized, so consider adding these changes there in order to alert the user in case what they requested was different from what was negotiated:

diff --git a/lib/srv/desktop/rdp/rdpclient/client.go b/lib/srv/desktop/rdp/rdpclient/client.go
index 38035c64c5..970761742e 100644
--- a/lib/srv/desktop/rdp/rdpclient/client.go
+++ b/lib/srv/desktop/rdp/rdpclient/client.go
@@ -72,6 +72,7 @@ import "C"
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"runtime/cgo"
 	"sync"
@@ -123,7 +124,12 @@ func init() {
 type Client struct {
 	cfg Config
 
-	// Parameters read from the TDP stream.
+	// `clientWidth` and `clientHeight` are initially set to either
+	// the values provided by the browser or the values provided by
+	// the config file (config file values take precedence). They are
+	// then used to request a screen size from the RDP server. Finally,
+	// they are set to the negotiated screen size returned from the RDP
+	// server, which is the actual screen size that will be used.
 	clientWidth, clientHeight uint16
 	username                  string
 
@@ -666,12 +672,36 @@ func cgo_handle_rdp_connection_initialized(
 func (c *Client) handleRDPConnectionInitialized(ioChannelID, userChannelID, screenWidth, screenHeight C.uint16_t) C.CGOErrCode {
 	c.cfg.Log.Debugf("Received RDP channel IDs: io_channel_id=%d, user_channel_id=%d", ioChannelID, userChannelID)
 	c.cfg.Log.Debugf("RDP server provided resolution of %dx%d", screenWidth, screenHeight)
+	negotiatedScreenWidth, negotiatedScreenHeight := uint16(screenWidth), uint16(screenHeight)
 
+	// If the server provided a different resolution than requested
+	if c.clientWidth != negotiatedScreenWidth || c.clientHeight != negotiatedScreenHeight {
+		changedResMsg := fmt.Sprintf("RDP negotiated a different resolution than requested: %dx%d -> %dx%d",
+			c.clientWidth, c.clientHeight, negotiatedScreenWidth, negotiatedScreenHeight)
+
+		// Update the client's bookkeeping variables.
+		c.clientWidth, c.clientHeight = negotiatedScreenWidth, negotiatedScreenHeight
+
+		// If the user requested a specific resolution via the config,
+		// warn the user about the resolution change.
+		if c.cfg.Width != 0 && c.cfg.Height != 0 {
+			c.cfg.Log.Warn(changedResMsg)
+			if err := c.cfg.Conn.WriteMessage(tdp.Notification{Message: changedResMsg, Severity: tdp.SeverityWarning}); err != nil {
+				c.cfg.Log.Errorf("failed handling RDPChannelIDs: %v", err)
+				return C.ErrCodeFailure
+			}
+		} else {
+			// Otherwise just log the resolution change at debug level.
+			c.cfg.Log.Debug(changedResMsg)
+		}
+	}
+
+	// Send the negotiated resolution and necessary channel ID's to the browser.
 	if err := c.cfg.Conn.WriteMessage(tdp.ConnectionInitialized{
 		IOChannelID:   uint16(ioChannelID),
 		UserChannelID: uint16(userChannelID),
-		ScreenWidth:   uint16(screenWidth),
-		ScreenHeight:  uint16(screenHeight),
+		ScreenWidth:   negotiatedScreenWidth,
+		ScreenHeight:  negotiatedScreenHeight,
 	}); err != nil {
 		c.cfg.Log.Errorf("failed handling RDPChannelIDs: %v", err)
 		return C.ErrCodeFailure

By default this should remain unspecified, which preserves the
behavior we have today (Teleport uses the size of the user's
browser window).

These new fields allow cluster administrators to configure desktops
in such a way that they always use a particular screen size, ignoring
the size passed from the browser client. This is necessary to
accommodate some applications which require a very specific screen resolution in
order to render correctly.
@zmb3 zmb3 force-pushed the zmb3/desktop-specify-screen-size branch from b76dfdd to d9bbafd Compare February 15, 2024 19:51
@zmb3 zmb3 added this pull request to the merge queue Feb 15, 2024
Merged via the queue into master with commit ccdd399 Feb 15, 2024
@zmb3 zmb3 deleted the zmb3/desktop-specify-screen-size branch February 15, 2024 21:11
@public-teleport-github-review-bot
Copy link
Copy Markdown

@zmb3 See the table below for backport results.

Branch Result
branch/v15 Failed

zmb3 added a commit that referenced this pull request Feb 20, 2024
github-merge-queue Bot pushed a commit that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants