Connect: Implement tshd event handlers for db cert renewal#1383
Connect: Implement tshd event handlers for db cert renewal#1383
Conversation
When tshd is sending the relogin event, it needs to be able to know when the Electron app has finished relogging the user. I wanted to implement this by simply waiting for the response from the RPC. I'm so glad we did not use gRPC streams for tshd events as this would be much harder to implement with streams.
With the introduction of important and regular modals, this will help us close the specific dialog if need arises.
This will be useful in the upcoming commits. Basically, tshd is going to ask the Electron app to relogin the user, with a 1 minute timeout. The Electron app will show a login modal but if the user doesn't submit it within 1 minute, tshd is going to cancel the request. In that situation, we need to be able to close the modal.
This will let us show the relogin modal on expired cert, even if the user was using some other modal at that moment.
The user can read more by expanding the notification. The title attribute persisted even after expanding the notification, making reading it harder.
The next commit is going to add a private method to AppContext. IAppContext is an interface which enables us to pass a mocked version of AppContext in tests. That mock is not going have that private method, so any place accepting AppContext wouldn't be able to accept the mocked AppContext. Instead, classes & functions should accept IAppContext rather than AppContext.
tshd needs to be able to do two things: * Ask the user to log in again. * Forward errors from goroutines running gateways to the Electron app in form of a notification. Otherwise those error would be visible only in the logs.
Restarting the gateways on login was a workaround from times where gateways didn't manage their own certs. In the new flow, a gateway takes care of refreshing the certs itself through the middleware passed to alpnproxy.LocalProxy. --- syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two functions: * syncRootClusterAndCatchErrors * restartClusterGatewaysAndCatchErrors The second function is no longer necessary, so we can make any place that was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors call just syncRootClusterAndCatchErrors instead.
| * @param {Object} eventData.request - The request payload converted to a simple JS object. | ||
| * @param {subscribeToTshdEventListenerOnCancelled} eventData.onCancelled - A function which lets | ||
| * you register a callback to be called when the call gets cancelled by the client. | ||
| */ |
There was a problem hiding this comment.
idk, apparently this is the way to use JSDoc in cases like this?
https://jsdoc.app/tags-callback.html
It looks very messy and doesn't actually show the additional docs on symbol lookup.
Maybe I'm doing something wrong, but I think I might move all docs into a single comment… somehow.
There was a problem hiding this comment.
TBH I would only document params in SubscribeToTshdEvent (in src/types), however I still don't see any additional docs 🤔.
There was a problem hiding this comment.
TBH I would only document params in
SubscribeToTshdEvent(insrc/types), however I still don't see any additional docs 🤔.
You mean that even if you move all docs to a single comment, you still don't see docs if you look up an individual argument such as onCancelled?
I wonder if this is something that would work correctly if we actually generated HTML docs and the reason it's not shown here is simply a limitation of the language server.
Anyway, if the docs for individual arguments don't work, then having all docs in one comment would be much easier to read.
There was a problem hiding this comment.
Ah, the docs were visible when all of this was still a proof of concept and almost everything was inline within AppContext. The docs were shown because I added them to the subscribeToTshdEvent property of ElectronGlobals. Now that it's a property of AppContext, the docs are lost. I'll see what I can do about this.
There was a problem hiding this comment.
I suppose this is related to this issue: microsoft/TypeScript#51121
There doesn't even seem to be a way to reuse a single typedef across multiple sites, e.g. having the same jsdoc for appContext.subscribeToTshdEvent and ElectronGlobals.subscribeToTshdEvent. I'm going to move the comment to appContext.subscribeToTshdEvent since this is where the function will be actually used so the comment will be most helpful there.
There was a problem hiding this comment.
You mean that even if you move all docs to a single comment, you still don't see docs if you look up an individual argument such as onCancelled?
Hmm, I think so.
I'm unsure about documenting eventName and listener in AppContext. I would do it in the place where they are defined (src/types). I bet that after changing them, no one would think about updating the comment in AppContext.
There was a problem hiding this comment.
I'm unsure about documenting
eventNameandlistenerinAppContext. I would do it in the place where they are defined (src/types). I bet that after changing them, no one would think about updating the comment inAppContext.
Good point, I'm going to move them to src/types.
ravicious
force-pushed
the
ravicious/expired-db-certs
branch
from
388842d to
5fc77df
Compare
gzdunek
left a comment
gzdunek
left a comment
There was a problem hiding this comment.
First pass, looks really good :)
| * @param {Object} eventData.request - The request payload converted to a simple JS object. | ||
| * @param {subscribeToTshdEventListenerOnCancelled} eventData.onCancelled - A function which lets | ||
| * you register a callback to be called when the call gets cancelled by the client. | ||
| */ |
There was a problem hiding this comment.
TBH I would only document params in SubscribeToTshdEvent (in src/types), however I still don't see any additional docs 🤔.
gzdunek
left a comment
gzdunek
left a comment
There was a problem hiding this comment.
Great work, it is really cool feature!
I'm only wondering if one minute timeout is not too strict? I would consider increasing it to maybe 3-5 minutes.
I think increasing it wouldn't help much here. pgAdmin's default connection timeout is 10 seconds. SSMS has a timeout of 15 seconds. Of course the user is able to change the timeout (at least pgAdmin lets you do that). But in general I'd say the user is supposed to see the modal soon after the db client tries to make the connection and then they should relogin. If they don't manage to do so, then that's too bad but we'll show display a notification. Also, unfortunately right now the modal will not disappear if the db client drops the connection, see gravitational/teleport#17950 (comment). |
avatus
left a comment
avatus
left a comment
There was a problem hiding this comment.
I spent a good two hours testing against teleport-11-ent.asteroid.earth and I wasn't getting any popup in Connect. Only errors in pgAdmin saying the connection had been terminated. I had a detailed list of reproduction steps, a video and everything only to then realize I hadn't built the tsh counterpart to this PR 🤦
This works great and is REALLY dope!!
* tshd events: Wait for listeners before responding When tshd is sending the relogin event, it needs to be able to know when the Electron app has finished relogging the user. I wanted to implement this by simply waiting for the response from the RPC. I'm so glad we did not use gRPC streams for tshd events as this would be much harder to implement with streams. * Return a function from ModalsService.openDialog which closes dialog With the introduction of important and regular modals, this will help us close the specific dialog if need arises. * Make tshd events listeners aware of request cancellation This will be useful in the upcoming commits. Basically, tshd is going to ask the Electron app to relogin the user, with a 1 minute timeout. The Electron app will show a login modal but if the user doesn't submit it within 1 minute, tshd is going to cancel the request. In that situation, we need to be able to close the modal. * Add support for passing reason in DialogClusterConnect * Add support for important modals This will let us show the relogin modal on expired cert, even if the user was using some other modal at that moment. * Remove title attr from notification text The user can read more by expanding the notification. The title attribute persisted even after expanding the notification, making reading it harder. * Add WindowsManager.forceFocusWindow * Use IAppContext instead of AppContext The next commit is going to add a private method to AppContext. IAppContext is an interface which enables us to pass a mocked version of AppContext in tests. That mock is not going have that private method, so any place accepting AppContext wouldn't be able to accept the mocked AppContext. Instead, classes & functions should accept IAppContext rather than AppContext. * Implement handlers for new tshd events tshd needs to be able to do two things: - Ask the user to log in again. - Forward errors from goroutines running gateways to the Electron app in form of a notification. Otherwise those error would be visible only in the logs. * Don't restart gateways after logging in Restarting the gateways on login was a workaround from times where gateways didn't manage their own certs. In the new flow, a gateway takes care of refreshing the certs itself through the middleware passed to alpnproxy.LocalProxy. syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two functions: - syncRootClusterAndCatchErrors - restartClusterGatewaysAndCatchErrors The second function is no longer necessary, so we can make any place that was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors call just syncRootClusterAndCatchErrors instead.
…1416) * tshd events: Wait for listeners before responding When tshd is sending the relogin event, it needs to be able to know when the Electron app has finished relogging the user. I wanted to implement this by simply waiting for the response from the RPC. I'm so glad we did not use gRPC streams for tshd events as this would be much harder to implement with streams. * Return a function from ModalsService.openDialog which closes dialog With the introduction of important and regular modals, this will help us close the specific dialog if need arises. * Make tshd events listeners aware of request cancellation This will be useful in the upcoming commits. Basically, tshd is going to ask the Electron app to relogin the user, with a 1 minute timeout. The Electron app will show a login modal but if the user doesn't submit it within 1 minute, tshd is going to cancel the request. In that situation, we need to be able to close the modal. * Add support for passing reason in DialogClusterConnect * Add support for important modals This will let us show the relogin modal on expired cert, even if the user was using some other modal at that moment. * Remove title attr from notification text The user can read more by expanding the notification. The title attribute persisted even after expanding the notification, making reading it harder. * Add WindowsManager.forceFocusWindow * Use IAppContext instead of AppContext The next commit is going to add a private method to AppContext. IAppContext is an interface which enables us to pass a mocked version of AppContext in tests. That mock is not going have that private method, so any place accepting AppContext wouldn't be able to accept the mocked AppContext. Instead, classes & functions should accept IAppContext rather than AppContext. * Implement handlers for new tshd events tshd needs to be able to do two things: - Ask the user to log in again. - Forward errors from goroutines running gateways to the Electron app in form of a notification. Otherwise those error would be visible only in the logs. * Don't restart gateways after logging in Restarting the gateways on login was a workaround from times where gateways didn't manage their own certs. In the new flow, a gateway takes care of refreshing the certs itself through the middleware passed to alpnproxy.LocalProxy. syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two functions: - syncRootClusterAndCatchErrors - restartClusterGatewaysAndCatchErrors The second function is no longer necessary, so we can make any place that was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors call just syncRootClusterAndCatchErrors instead.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
) The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
The Electron app no longer asks tshd to restart gateways. This was changed in gravitational/webapps#1383. This PR merely removes the tshd implementation of that RPC.
3 participants
teleport counterpart: gravitational/teleport#17950
Why
If the user attempts to make a connection through a local db proxy but the db & user certs have expired, we want Connect to bring focus to the Connect app, let the user relogin and then proxy the connection.
This is how the end result looks like:
connect-cert-renewal.mov
How
The backend flow is described in gravitational/teleport#17950. When it comes to the Electron app, tshd needs to be able to ask the Electron app for two things:
This is done through tshd events sent from tshd.
Important & regular modals
It's possible that while a db client attempts to make a connection, the user has a modal open in the app, for example to log in to a new cluster or whatever modal we might add in the future.
To avoid discarding the state from such modal, this PR introduces the concept of important modals. The basic idea is that we can have one regular and one important modal being rendered at the same time. The important modal is rendered over the regular modal, so if the user finishes the relogin process they can continue interacting with the old modal.
Opening an important modal should be reserved for situations where the interaction with the app happens outside of its UI and requires us to interrupt the user and show them a modal.
Structured object vs strings
Both tshd event handlers need to show a specific message to the user. Instead of passing a string from tshd to the Electron app, we pass a structured RPC message. This way the Electron app can decide how exactly the message should look like.