From d22dc12982a779008f254930825f61a867fd49c6 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 5 Dec 2024 17:01:55 +0100 Subject: [PATCH] Restore filtering out documents before saving them on disk --- .../workspacesService.test.ts | 90 ++++++++++++++++--- .../workspacesService/workspacesService.ts | 36 +++----- 2 files changed, 87 insertions(+), 39 deletions(-) diff --git a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts index 6af55fd4fc1ab..50647439e781a 100644 --- a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts +++ b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.test.ts @@ -33,7 +33,11 @@ import { NotificationsService } from '../notifications'; import { ModalsService } from '../modals'; import { getEmptyPendingAccessRequest } from './accessRequestsService'; -import { Workspace, WorkspacesService } from './workspacesService'; +import { + Workspace, + WorkspacesService, + WorkspacesState, +} from './workspacesService'; import { DocumentCluster, DocumentsService } from './documentsService'; import type * as tshd from 'teleterm/services/tshd/types'; @@ -138,7 +142,7 @@ describe('restoring workspace', () => { }); }); - it('does not restore doc.authorize_web_session documents', async () => { + it('location is set to first document if it points to non-existing document', async () => { const cluster = makeRootCluster(); const testWorkspace: Workspace = { accessRequests: { @@ -149,22 +153,16 @@ describe('restoring workspace', () => { documents: [ { kind: 'doc.terminal_shell', - uri: '/docs/terminal_shell_uri', + uri: '/docs/terminal_shell_uri_1', title: '/Users/alice/Documents', }, { - kind: 'doc.authorize_web_session', - rootClusterUri: cluster.uri, - uri: '/docs/authorize_web_session_uri', - webSessionRequest: { - id: '', - token: '', - redirectUri: '', - }, - title: 'Authorize Web Session', + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri_2', + title: '/Users/alice/Documents', }, ], - location: '/docs/authorize_web_session_uri', + location: '/docs/non-existing-doc', }; const { workspacesService } = getTestSetup({ @@ -182,7 +180,70 @@ describe('restoring workspace', () => { title: '/Users/alice/Documents', }, ], - location: '/docs/terminal_shell_uri', // location no longer points to the document that was omitted + location: '/docs/terminal_shell_uri', + }); + }); +}); + +describe('state persistence', () => { + it('doc.authorize_web_session is not stored to disk', () => { + const cluster = makeRootCluster(); + const workspacesState: WorkspacesState = { + rootClusterUri: cluster.uri, + isInitialized: true, + workspaces: { + [cluster.uri]: { + accessRequests: { + isBarCollapsed: true, + pending: getEmptyPendingAccessRequest(), + }, + localClusterUri: cluster.uri, + documents: [ + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri', + title: '/Users/alice/Documents', + }, + { + kind: 'doc.authorize_web_session', + uri: '/docs/authorize_web_session', + rootClusterUri: cluster.uri, + title: 'Authorize Web Session', + webSessionRequest: { + id: '', + token: '', + redirectUri: '', + }, + }, + ], + location: '/docs/authorize_web_session', + }, + }, + }; + const { workspacesService, statePersistenceService } = getTestSetup({ + cluster, + persistedWorkspaces: {}, + }); + + workspacesService.setState(() => workspacesState); + + expect(statePersistenceService.saveWorkspacesState).toHaveBeenCalledTimes( + 1 + ); + expect(statePersistenceService.saveWorkspacesState).toHaveBeenCalledWith({ + rootClusterUri: cluster.uri, + workspaces: { + [cluster.uri]: expect.objectContaining({ + documents: [ + { + kind: 'doc.terminal_shell', + uri: '/docs/terminal_shell_uri', + title: '/Users/alice/Documents', + }, + ], + location: '/docs/authorize_web_session', + }), + }, }); }); }); @@ -349,5 +410,6 @@ function getTestSetup(options: { clusterDocument, modalsService, notificationsService, + statePersistenceService, }; } diff --git a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts index b130775e24b0c..e3f38745bef83 100644 --- a/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts +++ b/web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts @@ -421,22 +421,20 @@ export class WorkspacesService extends ImmutableStore { const workspaceDefaultState = this.getWorkspaceDefaultState( persistedWorkspace?.localClusterUri || cluster.uri ); - const restorableDocuments = getRestorableDocuments( - persistedWorkspace?.documents || [] - ); + const persistedWorkspaceDocuments = persistedWorkspace?.documents; workspaces[cluster.uri] = { ...workspaceDefaultState, previous: this.canReopenPreviousDocuments({ - previousDocuments: restorableDocuments, + previousDocuments: persistedWorkspaceDocuments, currentDocuments: workspaceDefaultState.documents, }) ? { location: getLocationToRestore( - restorableDocuments, + persistedWorkspaceDocuments, persistedWorkspace.location ), - documents: restorableDocuments, + documents: persistedWorkspaceDocuments, } : undefined, connectMyComputer: persistedWorkspace?.connectMyComputer, @@ -575,20 +573,8 @@ export class WorkspacesService extends ImmutableStore { }; for (let w in this.state.workspaces) { const workspace = this.state.workspaces[w]; - const documentsToPersist = ( + const documentsToPersist = getDocumentsToPersist( workspace.previous?.documents || workspace.documents - ).map(d => - d.kind === 'doc.authorize_web_session' - ? { - ...d, - // Do not store potentially sensitive properties. - webSessionRequest: { - id: '', - token: '', - redirectUri: '', - }, - } - : d ); stateToSave.workspaces[w] = { @@ -650,19 +636,19 @@ export const useWorkspaceServiceState = () => { return useStoreSelector('workspacesService', identitySelector); }; -function getRestorableDocuments(documents: Document[]): Document[] { +function getDocumentsToPersist(documents: Document[]): Document[] { return ( documents - // We don't restore 'doc.authorize_web_session' because the user would not - // be able to authorize a session at a later time anyway. + // We don't persist 'doc.authorize_web_session' because we don't want to store + // a session token and id on disk. + // Moreover, the user would not be able to authorize a session at a later time anyway. .filter(d => d.kind !== 'doc.authorize_web_session') ); } -/** Assumes that there is at least one document to restore. */ function getLocationToRestore( documents: Document[], location: DocumentUri -): DocumentUri { - return documents.find(d => d.uri === location) ? location : documents[0]!.uri; +): DocumentUri | undefined { + return documents.find(d => d.uri === location) ? location : documents[0]?.uri; }