Show desktops in connection tracker by gzdunek · Pull Request #54668 · gravitational/teleport

gzdunek · 2025-05-09T14:34:07Z

Contributes to #20802

It's the last PR that should be merged before the release.

The code for showing desktop connection is a copy-paste of code for other connections.

I also reorganized the types slightly to make supporting non-Windows desktops easier in the future. In short, most places use now DesktopUri instead of WindowsDesktopUri. If they need to read windowsDesktopId, they should check if it's a WindowsDesktopUri. Thanks to that, if in the future someone opens a document for non-Windows session and then downgrades the app, it won't crash.

gzdunek · 2025-05-09T14:35:32Z

+  useListener(
+    client.onTransportClose,
+    useCallback(
+      error => documentsService.update(uri, { status: error ? 'error' : '' }),
+      [documentsService, uri]
+    )
+  );
+  useListener(
+    client.onError,
+    useCallback(
+      () => documentsService.update(uri, { status: 'error' }),
+      [documentsService, uri]
+    )
+  );


I should have combined these events while I was refactoring this, but well, I will keep it as is.

ACLs are fetched asynchronously.

avatus · 2025-05-09T15:23:45Z

+
+  let content = (
+    <Text m="auto" mt={10} textAlign="center">
+      Cannot open a connection to "{desktopUri}".


this always trips me up and i think its going to just print that instead of the desktopUri variable

gzdunek · 2025-05-12T13:57:06Z

+        // For example, the user can open a desktop connection in Connect v18
+        // and then downgrade to a version that doesn't support desktops.
+        // That connection should be shown as 'UNKNOWN' in the connection list.
+        if (!trackedConnection) {


Unfortunately, it turned out that we didn't properly handle unsupported connections in #37584.
It handled only app connections, which were a new gateway type but not a truly new connection kind. Now that desktop connections have been added, downgrading the app results in a crash, as we were trying to destructure undefined in the line 82.
I will backport this fix to all release branches.

ravicious · 2025-05-12T15:15:51Z

FWIW, there's a few places in the project that still use windowsDesktops:

src/services/tshd/testHelpers.ts:export const windowsDesktopUri = `${rootClusterUri}/windowsDesktops/foo`; src/ui/DocumentCluster/ActionButtons.story.tsx: uri: `${testCluster.uri}/windowsDesktops/bar`, src/ui/Search/pickers/results.story.tsx: uri: `${clusterUri}/windowsDesktops/long-name`, src/ui/Search/pickers/results.story.tsx: uri: `${clusterUri}/windowsDesktops/long-label-list`, src/ui/Search/pickers/results.story.tsx: uri: `${clusterUri}/windowsDesktops/short-label-list`,

This is fine, these are windows desktop resources, so they should have windows URIs.

In the future, we will probably have a linux desktop resource, so we will extend the URI type to DesktopUri = WindowsDesktopUri | LinuxDesktopUri. The linux desktop URI will probably look like ${clusterUri}/linuxDesktops/long-name.

What I meant is that tshd uses windows_desktops, not windowsDesktops.

ravicious · 2025-05-12T15:16:44Z

Small issue that's unrelated to this PR: whenever I switch to a tab with a desktop session, there's a flicker as the client sends "[TDPClient] requesting screen spec from client 837 x 881", despite the window size staying the same.

This happens because of a resize observer. If you hide the tab, its size goes to 0x0 (we ignore that) and after it becomes visible, it reports the size change (837 x 881 in this case). To avoid this, we could store the last reported size and compare it with the new size. While this is a simple fix, I'm not sure if I'll find time for it 😅

backport-bot-workflows · 2025-05-12T16:44:36Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v17	Failed

* Make supporting non-Windows desktops easier in the future * Add connection boilerplate for desktop connections * Show connected/disconnected status in the connection tracker * Make sure that ACLs were fetched before reading from them ACLs are fetched asynchronously. * Do not crash when reading unsupported connection from app_state.json * `gwDoc` -> `doc` * `windowsDesktops` -> `windows_desktops` (cherry picked from commit 9f49376)

…54926) * Show Windows desktops in Connect (#53955) * Show Windows desktops in Connect * Use `net.SplitHostPort` * Use underscore in URIs * Omit the default RDP port before displaying item in the UI (cherry picked from commit 81af813) * Add desktop session-related ACLs to Connect (#54031) * Add desktop session-related ACLs to Connect * Add new fields to `makeAcl` (cherry picked from commit aba0de0) * Add latency detector for desktop sessions (#52827) * wip * add ping message and latency from desktop side * version * Fix backward compatibility * godocs * formatting * fix imports * formatting * formatting * log and gci * lint * Apply tooltip on the icon directly, instead of on the Menu component This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear. * Use consistent spacing between top bar elements * updates from origin * e * e * lint * rework UI after merge * Rename fields in backend * prettier * Update web/packages/shared/components/DesktopSession/TopBar.tsx Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> * review comments * fix ui * add env var to disable windows desktop "ping" * review comment * review comment * Update lib/web/desktop.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Refactor latency monitoring * fix spelling * remove monitorSessionLatency * gci * review comment * review comment * Update RFD and version * fix gaps --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> (cherry picked from commit 1aba870) * Abstract directory sharing file system (#54545) * Extract an interface to interact with a shared directory file system Some methods were renamed to more closely follow common file system naming conventions. * Use the new interface in the TDP client * Provide a browser file system as the shared directory access implementation (cherry picked from commit d3fbeeb) * Support desktop access in Connect (#54373) * Extract reusable function for establishing connections to Windows Desktop Service * Add `ProxyWindowsDesktopSession` proto * Implement `ProxyWindowsDesktopSession` * Enable fetching desktops and desktop services in remote proxy cache * Implement dialing windows desktop * Implement client * Support Windows desktop certs in tsh * Fix incorrect `windowsDesktop` URI * Add proto for `ConnectToDesktop` * Implement `ConnectToDesktop` * Do not log requests/responses for `ConnectToDesktop` RPC * Add boilerplate for `DocumentDesktopSession` * Open a desktop connection * Relax ArrayBuffer type passed to encode methods, ignore tshd abort errors In tshd stream, the buffer is of type `ArrayBufferLike` (which is `ArrayBuffer` & `SharedArrayBuffer`). To allow assigning it to the type in our TDP code, we make it more general. * Ensure WASM IronRDP code is initialized only once * Use `utils.ShuffleVisit` * Improve stream cancellation handling * Leave a TODO about ListWindowsDesktops * Do not return empty data slice * Provide non-nil src and dest addresses to `streamutils.NewConn()` * Do not emit an empty message to indicate a successful connection * Fix test * Simplify code * Add missing `WindowsDesktopTLSCredentials` initialization * Require that the first message is only a dial request and the subsequent ones are only data * Add explicit `stop()` check * Hold cluster name and desktop name in a struct for the map key * Do not return early on non-connection problem errors * Handle io.EOF error specifically in BidiStreamingClient.Send instead of in `tlsConn.HandshakeContext` * Extract a common function to proxy TDP connections * Improve proto comments and connection setup * Add comments and logs * Lint * Explain why there's a special handling for abort error * Bring back the original `proxyWebsocketConn` behavior when it comes to error handling * Post merge fixes * Adjust proxying TDP connection to changes from master * Lint * Channels improvements * Post merge fixes (cherry picked from commit 980ce61) * Show desktops in connection tracker (#54668) * Make supporting non-Windows desktops easier in the future * Add connection boilerplate for desktop connections * Show connected/disconnected status in the connection tracker * Make sure that ACLs were fetched before reading from them ACLs are fetched asynchronously. * Do not crash when reading unsupported connection from app_state.json * `gwDoc` -> `doc` * `windowsDesktops` -> `windows_desktops` (cherry picked from commit 9f49376) * Implement shared directory file system in tsh daemon (#54662) * Implement shared directory file system in tshd * Remove `Open` method * Do not use `os.IsNotExist` * Read bytes into `[]byte` parameter * Correctly use `t.Helper()` * Add missing `trace.Wrap` * Rename receivers * Handle errors from `file.Close()` * Use named returns to return close error * Improve error handling (cherry picked from commit b3e9199) * Support directory sharing in Connect (#54663) * Register directory access when starting a desktop session * Add RPC to attach a directory to desktop session * Do not allow `attachDirectoryToDesktopSession` to be called from the renderer process * Open the directory picker and send the selected path to tshd * Intercept file system events coming from the server and handle them * Disallow file system messages to be sent from the renderer * Refactor dir sharing * `AttachDirectoryToDesktopSession` -> `SetSharedDirectoryForDesktopSession` * Improve comments * Small fixes * Add missing defer for `s.dirAccessMu.RUnlock()` * `TestOpenSharedDirectory` -> `TestNewDirectoryAccess` * Add a comment for JS file system handlers * `make grpc` --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 45428c0) * Simplify code around latency detection for desktop (#54795) * Simplify code around latency detection for desktop * cleanup * Use tdp.Conn for client and server * Add check for context cancellation * Cleanup * remove context check * remove context check * remove unused field * use sync.OnceFunc, error on unexpected ping * err message (cherry picked from commit 28f78c3) * Show `desktop access requires Teleport Proxy 17.5.0 or higher` when Proxy returns NotImplementedError * Add `wasm-unsafe-eval` to Connect CSP (#54916) (cherry picked from commit ee8be4d) * Ensure all errors thrown by `TdpClient` are instances of `Error` class (#54877) * Add a utility function to convert any input to an Error instance * Add types for TdpClient events * Always throw Error from `adaptWebSocketToTdpTransport` * Improve getting the error message in `getErrMessage` * Move `isAbortError` to error.ts * Remove unused `tshd/errors.ts` * Handle `JSON.stringify` error, add `cause` to thrown errors * Remove unnecessary `async` * Remove `error.toString` from the callsites * Handle `err` being undefined (cherry picked from commit 4d28408) --------- Co-authored-by: Przemko Robakowski <przemko.robakowski@goteleport.com>

gzdunek added 2 commits May 9, 2025 16:16

Make supporting non-Windows desktops easier in the future

d332897

Add connection boilerplate for desktop connections

9f88d44

gzdunek requested review from avatus and ravicious May 9, 2025 14:34

gzdunek added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels May 9, 2025

github-actions Bot added size/md ui labels May 9, 2025

github-actions Bot requested a review from flyinghermit May 9, 2025 14:34

gzdunek commented May 9, 2025

View reviewed changes

gzdunek removed the request for review from flyinghermit May 9, 2025 14:36

gzdunek added 2 commits May 9, 2025 16:41

Show connected/disconnected status in the connection tracker

9d2c92e

Make sure that ACLs were fetched before reading from them

6d512f9

ACLs are fetched asynchronously.

gzdunek force-pushed the gzdunek/desktops-in-connection-tracker branch from 6aca1ed to 6d512f9 Compare May 9, 2025 14:43

avatus approved these changes May 9, 2025

View reviewed changes

Do not crash when reading unsupported connection from app_state.json

2a528db

gzdunek commented May 12, 2025

View reviewed changes

ravicious approved these changes May 12, 2025

View reviewed changes

gwDoc -> doc

1e7478d

gzdunek added this pull request to the merge queue May 12, 2025

gzdunek removed this pull request from the merge queue due to a manual request May 12, 2025

windowsDesktops -> windows_desktops

22ea216

gzdunek enabled auto-merge May 12, 2025 16:17

gzdunek added this pull request to the merge queue May 12, 2025

Merged via the queue into master with commit 9f49376 May 12, 2025
40 checks passed

gzdunek deleted the gzdunek/desktops-in-connection-tracker branch May 12, 2025 16:43

This was referenced May 13, 2025

[v17] Do not crash when reading unsupported connection from app state #54740

Merged

[v16] Do not crash when reading unsupported connection from app state #54741

Merged

gzdunek mentioned this pull request May 19, 2025

[v17] Desktop access in Connect and desktop session latency detector #54926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show desktops in connection tracker#54668

Show desktops in connection tracker#54668
gzdunek merged 7 commits intomasterfrom
gzdunek/desktops-in-connection-tracker

gzdunek commented May 9, 2025

Uh oh!

gzdunek May 9, 2025

Uh oh!

avatus May 9, 2025

Uh oh!

gzdunek May 12, 2025

Uh oh!

Uh oh!

ravicious May 12, 2025

Uh oh!

gzdunek May 12, 2025

Uh oh!

ravicious May 12, 2025

Uh oh!

gzdunek May 12, 2025

Uh oh!

ravicious May 12, 2025

Uh oh!

gzdunek May 12, 2025

Uh oh!

Uh oh!

Uh oh!

backport-bot-workflows Bot commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gzdunek commented May 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

backport-bot-workflows Bot commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants