Fixes error propagation and resizes during MFA #41570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…een displayed to the user. This way the first error message (which is usually the most informative as to what's gone awry) is displayed to the user in the case of an error cascade. (E.g. tdp error is sent back, the server to closes the websocket, and that close causes a client websocket error. In that case, we want to display the tdp error that was sent back, not the websocket is closed error.)
1. A new mechanism is added to withhold all non-MFA TDP messages during the MFA ceremony within lib/web/desktop.go 2. A new mechanism is added to withhold all resizes when the rdpclient/client.go is not yet ready to receive messages. (Non-resize messages are still just dropped). This fixes #41515
zmb3
reviewed
May 15, 2024
to use a single function to handle checking if MFA is required, and refactors the desktop handler to make the TLS configuration creation more clear.
zmb3
approved these changes
May 17, 2024
|
Let's try to get the backport merged by EOD Monday so it makes it into the Tuesday release. |
probakowski
approved these changes
May 20, 2024
The only time I know this happens is when the user tries to resize on when an "Active Session" warning modal is shown, in which case the session was failing. In reality we can just ignore this scenario. If the websocket isn't yet open, we don't want to send messages. If it closes, then the onclose handler will take care of cleaning up the session, we don't need to error if any straggling messages are attempted to be sent.
|
@ibeckermayer See the table below for backport results.
|
ibeckermayer
added a commit
that referenced
this pull request
May 21, 2024
ibeckermayer
added a commit
that referenced
this pull request
May 21, 2024
* Update 'failed' statusText only if a previous error has not already been displayed to the user. This way the first error message (which is usually the most informative as to what's gone awry) is displayed to the user in the case of an error cascade. (E.g. tdp error is sent back, the server to closes the websocket, and that close causes a client websocket error. In that case, we want to display the tdp error that was sent back, not the websocket is closed error.) * Adds mechanisms for withholding resizes 1. A new mechanism is added to withhold all non-MFA TDP messages during the MFA ceremony within lib/web/desktop.go 2. A new mechanism is added to withhold all resizes when the rdpclient/client.go is not yet ready to receive messages. (Non-resize messages are still just dropped). This fixes #41515 * Remove superfluous comment, rename handleMessage to handleTDPInput * Refactors mfa and desktop handlers to use a single function to handle checking if MFA is required, and refactors the desktop handler to make the TLS configuration creation more clear. * use idiomatic TypeAttr * Only log a warning when we try to send TDP message on a closed ws The only time I know this happens is when the user tries to resize on when an "Active Session" warning modal is shown, in which case the session was failing. In reality we can just ignore this scenario. If the websocket isn't yet open, we don't want to send messages. If it closes, then the onclose handler will take care of cleaning up the session, we don't need to error if any straggling messages are attempted to be sent.
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 21, 2024
* Backports the piece of mfa.go changed in #40077 required for backporting #41570 * Fixes error propagation and resizes during MFA (#41570) * Update 'failed' statusText only if a previous error has not already been displayed to the user. This way the first error message (which is usually the most informative as to what's gone awry) is displayed to the user in the case of an error cascade. (E.g. tdp error is sent back, the server to closes the websocket, and that close causes a client websocket error. In that case, we want to display the tdp error that was sent back, not the websocket is closed error.) * Adds mechanisms for withholding resizes 1. A new mechanism is added to withhold all non-MFA TDP messages during the MFA ceremony within lib/web/desktop.go 2. A new mechanism is added to withhold all resizes when the rdpclient/client.go is not yet ready to receive messages. (Non-resize messages are still just dropped). This fixes #41515 * Remove superfluous comment, rename handleMessage to handleTDPInput * Refactors mfa and desktop handlers to use a single function to handle checking if MFA is required, and refactors the desktop handler to make the TLS configuration creation more clear. * use idiomatic TypeAttr * Only log a warning when we try to send TDP message on a closed ws The only time I know this happens is when the user tries to resize on when an "Active Session" warning modal is shown, in which case the session was failing. In reality we can just ignore this scenario. If the websocket isn't yet open, we don't want to send messages. If it closes, then the onclose handler will take care of cleaning up the session, we don't need to error if any straggling messages are attempted to be sent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds mechanisms for withholding resizes
This fixes #41515
Also fixes the error propagation issue evident in #41515, see point 1 in #41515 (comment)
changelog: Solved a bug that was causing Windows Desktop sessions to fail when the screen was resized during an MFA ceremony