Skip to content

desktop access: soften the disconnect error#33941

Merged
zmb3 merged 2 commits intomasterfrom
zmb3/soften-desktop-disconnect
Nov 9, 2023
Merged

desktop access: soften the disconnect error#33941
zmb3 merged 2 commits intomasterfrom
zmb3/soften-desktop-disconnect

Conversation

@zmb3
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 commented Oct 26, 2023

If the Windows host (RDP server) decides to terminate the connection, it often sends a message indicating why the disconnect is coming. One example of a server-initiated disconnect would be when the user clicks "log off" in the RDP session or when they restart the remote machine.

We forward this message to the UI, but we put it in a big red ERROR box that makes it look like something went wrong.

Errors should be reserved for unexpected disconnections, so covert the dialog to "info" level and soften the messaging.

Before:

image

After:

image

@zmb3 zmb3 added backport/branch/v12 no-changelog Indicates that a PR does not require a changelog entry labels Oct 26, 2023
@github-actions github-actions Bot requested review from rudream and ryanclark October 26, 2023 18:39
@zmb3 zmb3 force-pushed the zmb3/soften-desktop-disconnect branch from d9a209d to 053fd95 Compare October 26, 2023 19:41
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from rudream October 31, 2023 19:54
@zmb3
Copy link
Copy Markdown
Collaborator Author

zmb3 commented Nov 5, 2023

Note to self that we should do the same when playing back a session recording.

If the Windows host (RDP server) decides to terminate the connection,
it often sends a message indicating why the disconnect is coming.
One example of a server-initiated disconnect would be when the user
clicks "log off" in the RDP session or when they restart the remote
machine.

We forward this message to the UI, but we put it in a big red ERROR
box that makes it look like something went wrong.

Errors should be reserved for unexpected disconnections, so covert
the dialog to "info" level and soften the messaging.
@zmb3 zmb3 force-pushed the zmb3/soften-desktop-disconnect branch from 053fd95 to 014653d Compare November 7, 2023 00:36
@zmb3
Copy link
Copy Markdown
Collaborator Author

zmb3 commented Nov 7, 2023

@ibeckermayer take a look at the latest commit and let me know what you think.

Instead of changing all cases of this error from Danger to Info, we continue to use danger if it was an unexpected disconnect and we didn't receive a reason from the server.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This has been a bug for a loooong time. We ignore the length that the server sends and just assume the rest of the message is one giant string. (Which is why we often see unprintable characters at the end of the error message).

Comment thread web/packages/teleport/src/DesktopSession/DesktopSession.tsx Outdated
With the prior commit, we successfully made server-side disconnects
look less like errors, but we also made some real errors (like
connection time outs) look less like errors.

Now, we render an error indicator when we have an unexpected
disconnect. We also add a special check for "RDP connection failed"
which is used to indicate that the connection was never established
succesfully.

This also fixes an ancient bug where our strings had an extra
unprintable byte at the end. This is because the TDP codec was
interpreting the "severity" byte as part of the string, rather
than reading only the length that was specified in the message.
@zmb3 zmb3 force-pushed the zmb3/soften-desktop-disconnect branch from 014653d to c82328b Compare November 8, 2023 19:51
@zmb3 zmb3 added this pull request to the merge queue Nov 9, 2023
Merged via the queue into master with commit 5014118 Nov 9, 2023
@zmb3 zmb3 deleted the zmb3/soften-desktop-disconnect branch November 9, 2023 20:06
@public-teleport-github-review-bot
Copy link
Copy Markdown

@zmb3 See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User logging off Desktop via windows treated like error

4 participants