Convert dialog to authorize web session to regular tab and handle expired certs #49754
Conversation
web/packages/teleterm/src/ui/DocumentAuthorizeWebSession/DocumentAuthorizeWebSession.tsx
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAuthorizeWebSession/DocumentAuthorizeWebSession.tsx
Outdated
Show resolved
Hide resolved
| rootCluster: Cluster, | ||
| webSessionRequest: WebSessionRequest | ||
| ): string { | ||
| const processedRedirectUri = processRedirectUri( |
There was a problem hiding this comment.
Why is processRedirectUri called only by buildUnauthorizedSessionUrl?
There was a problem hiding this comment.
It's because in buildUnauthorizedSessionUrl we don't return redirect_uri but instead a full path that we open directly.
This was originally done this way in #47221, I didn't change it.
web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAuthorizeWebSession/DocumentAuthorizeWebSession.tsx
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAuthorizeWebSession/DocumentAuthorizeWebSession.tsx
Outdated
Show resolved
Hide resolved
| const documentsToPersist = ( | ||
| workspace.previous?.documents || workspace.documents | ||
| ).filter(d => d.kind !== 'doc.authorize_web_session'); | ||
|
|
||
| stateToSave.workspaces[w] = { | ||
| localClusterUri: workspace.localClusterUri, | ||
| location: workspace.previous?.location || workspace.location, |
There was a problem hiding this comment.
location might point to a document which has been removed. Instead of handling this each time a workspace state is saved, I think we should just check it once when opening the app. When reopening previous session, we could check if the doc that location points to exists, if not then we should use the first doc as the location.
There was a problem hiding this comment.
Ah, good point with the location! The main reason I handled filtering out the documents before saving was this comment:
On top of that, if we don't take any extra precautions, converting this modal into a tab means that data passed from the Web UI to Connect through the custom scheme URL would get saved on disk. Connect automatically saves the state of all open tabs on disk so that it's able to reopen tabs on the next launch.
IIRC, the security model of Device Trust already accounts for this kind of data exfiltration. But it'd probably be a good idea to not persist this particular tab on disk in the first place.
The only kind of attack I can think of is authorizing a session with the user's confirmation. But you could only do it for a valid web request for a valid cluster, so I'm not sure if there's any benefit of that for an attacker.
I agree that it's better to filter out the document when opening the app. However, maybe we should still clear the token and id from the document before saving to disk?
There was a problem hiding this comment.
Filtering the documents before saving is fine. I meant that instead of adjusting persistState so that it checks location, we could check if location is valid only when restoring the previous session.
There was a problem hiding this comment.
I see, I will modify the fix I pushed.
There was a problem hiding this comment.
Restored previous code and added a test.
| documents: Document[], | ||
| location: DocumentUri | ||
| ): DocumentUri { | ||
| return documents.find(d => d.uri === location) ? location : documents[0]!.uri; |
There was a problem hiding this comment.
Nit, but I think we should avoid TypeScript's ! where possible. It's not much different than using as any. It's fine in rare cases where a 3rd-party library makes some guarantees that for some reason are not expressed by types.
But here in our code, if our heart was really in it, we'd use a non-empty array to guarantee that documents always have at least one element. I don't think people do it that often in TS because the ergonomics of it are much worse compared to doing the same in Elm or Haskell.
So in this particular scenario, I think it might be better to do documents[0]?.uri rather than keep our fingers crossed that this won't ever be empty. On line 425, we already made so that an empty array is used if persistedWorkspace?.documents doesn't return anything.
There was a problem hiding this comment.
Yeah, it's not really needed actually, it's fine for location to be undefined.
| > | ||
| enroll your device | ||
| </a> | ||
| . Then log out of the app, log back in, and try again. |
There was a problem hiding this comment.
Forgot to mention it last time, but this should be more specific – to a regular user, "the app" might mean Connect or Web UI. So perhaps "Then log out of Teleport Connect"?
…ired certs (#49754) * Add `AddMetadataToRetryableError` to `AuthenticateWebDevice` * Add a document for web session authorization * Replace `AuthenticateWebDevice` modal with the document * Remove dot from anchor * Move more logic to `authorizeAndCloseDocument` * Add a comment about `canAuthorize` * Filter out documents before restoring session * Add a comment about `processedRedirectUri` * Restore filtering out documents before saving them on disk * Fix test * Refer to Teleport Connect instead of 'the app' (cherry picked from commit e2f3daf)
…le expired certs (#49932) * Convert dialog to authorize web session to regular tab and handle expired certs (#49754) * Add `AddMetadataToRetryableError` to `AuthenticateWebDevice` * Add a document for web session authorization * Replace `AuthenticateWebDevice` modal with the document * Remove dot from anchor * Move more logic to `authorizeAndCloseDocument` * Add a comment about `canAuthorize` * Filter out documents before restoring session * Add a comment about `processedRedirectUri` * Restore filtering out documents before saving them on disk * Fix test * Refer to Teleport Connect instead of 'the app' (cherry picked from commit e2f3daf) * Adjust alerts to the old `Alert` component
Closes #43328
To make handling device trust errors easier, the dialog to authorize a web session has been converted to a regular tab. Previously, we couldn't handle "expired certs" errors because a relogin dialog would overwrite the dialog to authorize the session. There are ways to overcome it, but the regular tab provides better UX.
changelog: The web session authorization dialog in Teleport Connect is now a dedicated tab, which properly shows a re-login dialog when the local session is expired.
Note to reviewers: when testing session expiration, you may want to use
max_session_ttllonger than 3m1s.