-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes error propagation and resizes during MFA #41570
Fixes error propagation and resizes during MFA #41570
Conversation
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this will work fine but I wonder if there's a better way.
Also, it would be great to start to get some test coverage in here, especially as we fix bugs like this one.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please make sure to test with and without MFA required, and with a resizing desktop as well as a fixed-size one.
Let's try to get the backport merged by EOD Monday so it makes it into the Tuesday release. |
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.
|
* 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.
* 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.
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